refactor!: streamline OpBlockExecutionError error type to replace a hard-coded BlockExecutionError
#13696
Conversation
chungquantin
requested review from
mattsse,
Rjected,
fgimenez,
rakita and
gakonst
as code owners
chungquantin
changed the title
OpBlockExecutionError
chungquantin
requested review from
onbjerg,
shekhirin and
rkrasiuk
as code owners
This reverts commit 7bfd8a7.
crates/blockchain-tree/src/chain.rs
Outdated
| @@ -209,7 +209,7 @@ impl AppendableChain { | |||
| let block_hash = block.hash(); | |||
| let block = block.unseal(); | |||
|
|
|||
| let state = executor.execute(&block)?; | |||
| let state = executor.execute(&block).unwrap(); | |||
There was a problem hiding this comment.
we can't panic here. the return type of this method must change to
Result<(ExecutionOutcome, Option<TrieUpdates>), E::Error>.
There was a problem hiding this comment.
Thank for your suggestion. This is made on purpose for me to test my implementation. Currently I got blocked by the validate_block_post_execution that throws ConsensusError. I am thinking adding the type constraint From<ConsensusError> but it will lead to other issues.
Edit: Currently got it resolved but still have a lot of things to consider for the overall design. e.g. BlockErrorKind accepts BlockExecutionError. Hence, we would either streaming and converting the error type back to BlockExecutionError or redesign BlockErrorKind to accept generic Error type.
There was a problem hiding this comment.
great design ideas, let's choose
(i) add trait bound: From, then impl the conversion From<ConsensusError> for OpBlockExecutionError which would put the consensus error inside the OpBlockExecutionError::Eth variant
(ii) redesign BlockErrorKind to accept generic Error type
There was a problem hiding this comment.
This entire crate was actually removed, I think resolving merge conflicts with main will be the most productive thing to do here
There was a problem hiding this comment.
Thank you @Rjected! Rebased.
There was a problem hiding this comment.
great design ideas, let's choose
(i) add trait bound: From, then impl the conversion
From<ConsensusError> for OpBlockExecutionErrorwhich would put the consensus error inside theOpBlockExecutionError::Ethvariant (ii) redesignBlockErrorKindto accept generic Error type
Went further with the second option and seems like it is doable. Question for this approach would be which level of code that we want to convert back OpBlockExecutionError -> BlockExecutionError or we will streamline the OpBlockExecutionError the whole way to the Stage API level?
If the later is what we want, do you think it makes sense if Provider type can accept the Error type for block execution errors?
|
This should be rebased to resolve all the merge conflicts, it might become simpler as the legacy tree etc have been removed |
|
Sorry, will have to be rebased or merged again once #13839 is merged, since nightly clippy is broken right now |
That's ok, I don't see it as a breaking change. |
chungquantin
changed the title
OpBlockExecutionErrorOpBlockExecutionError error type to replace a hard-coded BlockExecutionError
chungquantin
changed the title
OpBlockExecutionError error type to replace a hard-coded BlockExecutionErrorOpBlockExecutionError error type to replace a hard-coded BlockExecutionError
Current issue
Adding a variant and removing the conversion brings complexities everywhere
BlockExecutionErrorhard-coded in the associated type. TakesBlockExecutionStrategyFactory,BatchExecutororBlockExecutorProvider. And the conversion betweenOpBlockExecutionError -> BlockExecutionErroris still required in some parts of the code.Requires opinions from the core devs before taking extra steps to finish this PR.
Link to the issue: #13644
cc: @emhane