Unify engine error to reject blocks #9085
Conversation
sorpaas
added
A0-pleasereview 🤓
5chdn
added
the
A1-onice 🌨
5chdn
removed
the
A1-onice 🌨
debris
added
A8-looksgood 🦄
ethcore/src/miner/miner.rs
Outdated
| ) | ||
| ) { | ||
| Ok(block) => block, | ||
| Err(_) => return None, |
There was a problem hiding this comment.
Should we print a warning here?
ethcore/src/miner/miner.rs
Outdated
| let block = match open_block.close() { | ||
| Ok(block) => block, | ||
| Err(_) => { | ||
| warn!(target: "miner", "Closing the block failed due to an engine error. Please check your chain specifications and consensus smart contracts."); |
There was a problem hiding this comment.
Maybe display the original error as well? Will give some additional input for debugging.
ethcore/src/miner/miner.rs
Outdated
| self.prepare_work(block, original_work_hash) | ||
| }, | ||
| None => { | ||
| prepare_new = false; |
There was a problem hiding this comment.
Isn't that a bit misleading? The function should return true if new pending block had to be prepared. And in that case it had to be prepared, but the preparation failed.
Just thinking if it won't break any other assumptions, maybe at least update the function comment.
There was a problem hiding this comment.
I added BlockPreparationStatus which distinguishes three different status.
I think previously I set prepare_new = false here is indeed not quite right. We only use the result for prepare_pending_block in prepare_and_update_sealing. If we don't have to prepare a new block, the function then call update_sealing. So if the block had to be prepared but failed, then we shouldn't continue the if statement. (update_sealing will also just call prepare_block again and early return if it finds out that the conditions does not hold.)
5chdn
removed
the
B7-releasenotes 📜
|
Removing relnotes flag as this is not relevant for users. Or is it? |
|
it is not |
Currently in our code, we have several places that deal with engine errors with different behavior. On engine error:
on_new_blockfails, blocks are rejected if it's imported externally.on_new_blockfails, the client panics if it's created internally.on_close_blockfails, the error is ignored and we print a warning.I think the rejected behavior is the most reasonable. So this PR changes all three cases to rejecting blocks.
on_close_blockto fail on a consensus smart contract. In that case, if rejected behavior is used, node operators may be able to push/replace transactions and cause the current node to create a new valid block, without bringing down the node.