Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: streamline OpBlockExecutionError error type to replace a hard-coded BlockExecutionError #13696

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

refactor!: streamline OpBlockExecutionError error type to replace a hard-coded BlockExecutionError #13696

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3174d8e
refactor: add Eth variant to OpBlockExecutionError
chungquantin Jan 7, 2025
998c226
refactor: BlockExecutionStrategyFactory
chungquantin Jan 16, 2025
7bfd8a7
chore: init
chungquantin Jan 16, 2025
e21f7b6
Revert "chore: init"
chungquantin Jan 16, 2025
0c74b82
refactor(partial): add BlockExecutionError to BlockExecutionProvider
chungquantin Jan 16, 2025
214bd6b
refactor: add Eth variant to OpBlockExecutionError
chungquantin Jan 7, 2025
f1278f1
refactor: BlockExecutionStrategyFactory
chungquantin Jan 16, 2025
e7692dc
chore: init
chungquantin Jan 16, 2025
a36615f
Revert "chore: init"
chungquantin Jan 16, 2025
aace402
refactor(partial): add BlockExecutionError to BlockExecutionProvider
chungquantin Jan 16, 2025
d52148d
chore: rebase main
chungquantin Jan 17, 2025
9cfe592
Merge branch 'chungquantin/refactor-op_block_execution_error' into ch…
chungquantin Jan 17, 2025
5579b66
Merge pull request #2 from chungquantin/chungquantin/refactor-temp
chungquantin Jan 17, 2025
63457a4
chore: remove blockchain-tree crate
chungquantin Jan 17, 2025
9c8b898
refactor: add generic block execution error type to Stage
chungquantin Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/blockchain-tree/src/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't panic here. the return type of this method must change to
Result<(ExecutionOutcome, Option<TrieUpdates>), E::Error>.

Copy link
Contributor Author

@chungquantin chungquantin Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire crate was actually removed, I think resolving merge conflicts with main will be the most productive thing to do here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Rjected! Rebased.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

externals.consensus.validate_block_post_execution(
&block,
PostExecutionInput::new(&state.receipts, &state.requests),
Expand Down
4 changes: 3 additions & 1 deletion crates/evm/src/either.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ use revm::State;
impl<A, B> BlockExecutorProvider for Either<A, B>
where
A: BlockExecutorProvider,
B: BlockExecutorProvider<Primitives = A::Primitives>,
B: BlockExecutorProvider<Primitives = A::Primitives, Error = A::Error>,
{
type Primitives = A::Primitives;

type Error = A::Error;

type Executor<DB: Database<Error: Into<ProviderError> + Display>> =
Either<A::Executor<DB>, B::Executor<DB>>;

Expand Down
20 changes: 13 additions & 7 deletions crates/evm/src/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ pub trait BlockExecutorProvider: Send + Sync + Clone + Unpin + 'static {
/// Receipt type.
type Primitives: NodePrimitives;

/// The error type returned by the executor.
type Error;

/// An executor that can execute a single block given a database.
///
/// # Verification
Expand All @@ -153,15 +156,15 @@ pub trait BlockExecutorProvider: Send + Sync + Clone + Unpin + 'static {
DB,
Input<'a> = &'a BlockWithSenders<<Self::Primitives as NodePrimitives>::Block>,
Output = BlockExecutionOutput<<Self::Primitives as NodePrimitives>::Receipt>,
Error = BlockExecutionError,
Error = Self::Error,
>;

/// An executor that can execute a batch of blocks given a database.
type BatchExecutor<DB: Database<Error: Into<ProviderError> + Display>>: for<'a> BatchExecutor<
DB,
Input<'a> = &'a BlockWithSenders<<Self::Primitives as NodePrimitives>::Block>,
Output = ExecutionOutcome<<Self::Primitives as NodePrimitives>::Receipt>,
Error = BlockExecutionError,
Error = Self::Error,
>;

/// Creates a new executor for single block execution.
Expand Down Expand Up @@ -198,7 +201,7 @@ pub trait BlockExecutionStrategy {
type Primitives: NodePrimitives;

/// The error type returned by this strategy's methods.
type Error: From<ProviderError> + core::error::Error;
type Error;

/// Initialize the strategy with the given transaction environment overrides.
fn init(&mut self, _tx_env_overrides: Box<dyn TxEnvOverrides>) {}
Expand Down Expand Up @@ -251,7 +254,7 @@ pub trait BlockExecutionStrategy {
/// A strategy factory that can create block execution strategies.
pub trait BlockExecutionStrategyFactory: Send + Sync + Clone + Unpin + 'static {
/// The error type returned by this strategy's methods.
type Error: From<ProviderError> + core::error::Error;
type Error;

/// Primitive types used by the strategy.
type Primitives: NodePrimitives;
Expand Down Expand Up @@ -293,8 +296,10 @@ impl<F> BasicBlockExecutorProvider<F> {

impl<F> BlockExecutorProvider for BasicBlockExecutorProvider<F>
where
F: BlockExecutionStrategyFactory<Error = BlockExecutionError>,
F: BlockExecutionStrategyFactory,
{
type Error = F::Error;

type Primitives = F::Primitives;

type Executor<DB: Database<Error: Into<ProviderError> + Display>> =
Expand Down Expand Up @@ -425,12 +430,12 @@ where

impl<S, DB> BatchExecutor<DB> for BasicBatchExecutor<S>
where
S: BlockExecutionStrategy<DB = DB, Error = BlockExecutionError>,
S: BlockExecutionStrategy<DB = DB>,
DB: Database<Error: Into<ProviderError> + Display>,
{
type Input<'a> = &'a BlockWithSenders<<S::Primitives as NodePrimitives>::Block>;
type Output = ExecutionOutcome<<S::Primitives as NodePrimitives>::Receipt>;
type Error = BlockExecutionError;
type Error = S::Error;

fn execute_and_verify_one(&mut self, block: Self::Input<'_>) -> Result<(), Self::Error> {
if self.batch_record.first_block().is_none() {
Expand Down Expand Up @@ -529,6 +534,7 @@ mod tests {
struct TestExecutorProvider;

impl BlockExecutorProvider for TestExecutorProvider {
type Error = BlockExecutionError;
type Primitives = EthPrimitives;
type Executor<DB: Database<Error: Into<ProviderError> + Display>> = TestExecutor<DB>;
type BatchExecutor<DB: Database<Error: Into<ProviderError> + Display>> = TestExecutor<DB>;
Expand Down
2 changes: 2 additions & 0 deletions crates/evm/src/noop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ pub struct NoopBlockExecutorProvider<P>(core::marker::PhantomData<P>);
impl<P: NodePrimitives> BlockExecutorProvider for NoopBlockExecutorProvider<P> {
type Primitives = P;

type Error = BlockExecutionError;

type Executor<DB: Database<Error: Into<ProviderError> + Display>> = Self;

type BatchExecutor<DB: Database<Error: Into<ProviderError> + Display>> = Self;
Expand Down
2 changes: 2 additions & 0 deletions crates/evm/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ impl MockExecutorProvider {
impl BlockExecutorProvider for MockExecutorProvider {
type Primitives = EthPrimitives;

type Error = BlockExecutionError;

type Executor<DB: Database<Error: Into<ProviderError> + Display>> = Self;

type BatchExecutor<DB: Database<Error: Into<ProviderError> + Display>> = Self;
Expand Down
2 changes: 1 addition & 1 deletion crates/exex/exex/src/backfill/job.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ where
let (unsealed_header, hash) = header.split();
let block = P::Block::new(unsealed_header, body).with_senders_unchecked(senders);

executor.execute_and_verify_one(&block)?;
executor.execute_and_verify_one(&block).unwrap();
execution_duration += execute_start.elapsed();

// TODO(alexey): report gas metrics using `block.header.gas_used`
Expand Down
16 changes: 2 additions & 14 deletions crates/optimism/evm/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,6 @@ pub enum OpBlockExecutionError {
#[error("failed to load account {_0}")]
AccountLoadFailed(alloy_primitives::Address),
/// Thrown when a L1 block execution failed.
#[error("execution error on L1: {_0}")]
Eth(BlockExecutionError),
}

impl From<OpBlockExecutionError> for BlockExecutionError {
fn from(err: OpBlockExecutionError) -> Self {
Self::other(err)
}
}

impl From<ProviderError> for OpBlockExecutionError {
fn from(error: ProviderError) -> Self {
Self::Eth(BlockExecutionError::from(error))
}
#[error(transparent)]
Eth(#[from] BlockExecutionError),
}