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.

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 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
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();
chungquantin marked this conversation as resolved.
Show resolved Hide resolved
externals.consensus.validate_block_post_execution(
&block,
PostExecutionInput::new(&state.receipts, &state.requests),
Expand Down
2 changes: 2 additions & 0 deletions crates/ethereum/evm/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ where
Transaction = reth_primitives::TransactionSigned,
>,
{
type Error = BlockExecutionError;

type Primitives = EthPrimitives;

type Strategy<DB: Database<Error: Into<ProviderError> + Display>> =
Expand Down
4 changes: 3 additions & 1 deletion crates/evm/src/either.rs
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
22 changes: 16 additions & 6 deletions crates/evm/src/execute.rs
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 @@ -250,14 +253,17 @@ 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;

/// Primitive types used by the strategy.
type Primitives: NodePrimitives;

/// Associated strategy type.
type Strategy<DB: Database<Error: Into<ProviderError> + Display>>: BlockExecutionStrategy<
DB = DB,
Primitives = Self::Primitives,
Error = BlockExecutionError,
Error = Self::Error,
>;

/// Creates a strategy using the give database.
Expand Down Expand Up @@ -292,6 +298,8 @@ impl<F> BlockExecutorProvider for BasicBlockExecutorProvider<F>
where
F: BlockExecutionStrategyFactory,
{
type Error = F::Error;

type Primitives = F::Primitives;

type Executor<DB: Database<Error: Into<ProviderError> + Display>> =
Expand Down Expand Up @@ -422,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 @@ -526,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 Expand Up @@ -624,6 +633,7 @@ mod tests {
}

impl BlockExecutionStrategyFactory for TestExecutorStrategyFactory {
type Error = BlockExecutionError;
type Primitives = EthPrimitives;
type Strategy<DB: Database<Error: Into<ProviderError> + Display>> =
TestExecutorStrategy<DB, TestEvmConfig>;
Expand Down
2 changes: 2 additions & 0 deletions crates/evm/src/noop.rs
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
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
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
13 changes: 5 additions & 8 deletions crates/optimism/evm/src/error.rs
Copy link
Member

Choose a reason for hiding this comment

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

when new trait BlockExecError (see comment about rename) is implemented for the OpBlockExecutionError too, then seems like this is possibly getting closed to finish :D

Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
//! Error types for the Optimism EVM module.

use alloc::string::String;
use reth_evm::execute::BlockExecutionError;
use reth_evm::execute::{BlockExecutionError, ProviderError};

/// Optimism Block Executor Errors
#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[derive(Debug, thiserror::Error)]
pub enum OpBlockExecutionError {
/// Error when trying to parse L1 block info
#[error("could not get L1 block info from L2 block: {message}")]
Expand All @@ -21,10 +21,7 @@ pub enum OpBlockExecutionError {
/// Thrown when a database account could not be loaded.
#[error("failed to load account {_0}")]
AccountLoadFailed(alloy_primitives::Address),
}

impl From<OpBlockExecutionError> for BlockExecutionError {
fn from(err: OpBlockExecutionError) -> Self {
Self::other(err)
}
/// Thrown when a L1 block execution failed.
#[error(transparent)]
Eth(#[from] BlockExecutionError),
}
49 changes: 29 additions & 20 deletions crates/optimism/evm/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ where
+ 'static
+ ConfigureEvm<Header = alloy_consensus::Header, Transaction = OpTransactionSigned>,
{
type Error = OpBlockExecutionError;
type Primitives = OpPrimitives;
type Strategy<DB: Database<Error: Into<ProviderError> + Display>> =
OpExecutionStrategy<DB, EvmConfig>;
Expand Down Expand Up @@ -126,7 +127,7 @@ where
{
type DB = DB;
type Primitives = OpPrimitives;
type Error = BlockExecutionError;
type Error = OpBlockExecutionError;

fn init(&mut self, tx_env_overrides: Box<dyn TxEnvOverrides>) {
self.tx_env_overrides = Some(tx_env_overrides);
Expand All @@ -144,12 +145,14 @@ where
let env = self.evm_env_for_block(&block.header);
let mut evm = self.evm_config.evm_with_env(&mut self.state, env);

self.system_caller.apply_beacon_root_contract_call(
block.timestamp,
block.number,
block.parent_beacon_block_root,
&mut evm,
)?;
self.system_caller
.apply_beacon_root_contract_call(
block.timestamp,
block.number,
block.parent_beacon_block_root,
&mut evm,
)
.map_err(OpBlockExecutionError::Eth)?;

// Ensure that the create2deployer is force-deployed at the canyon transition. Optimism
// blocks will always have at least a single transaction in them (the L1 info transaction),
Expand Down Expand Up @@ -180,11 +183,12 @@ where
if transaction.gas_limit() > block_available_gas &&
(is_regolith || !transaction.is_deposit())
{
return Err(BlockValidationError::TransactionGasLimitMoreThanAvailableBlockGas {
transaction_gas_limit: transaction.gas_limit(),
block_available_gas,
}
.into())
return Err(OpBlockExecutionError::Eth(BlockExecutionError::Validation(
BlockValidationError::TransactionGasLimitMoreThanAvailableBlockGas {
transaction_gas_limit: transaction.gas_limit(),
block_available_gas,
},
)))
}

// Cache the depositor account prior to the state transition for the deposit nonce.
Expand All @@ -211,10 +215,12 @@ where
let result_and_state = evm.transact().map_err(move |err| {
let new_err = err.map_db_err(|e| e.into());
// Ensure hash is calculated for error log, if not already done
BlockValidationError::EVM {
hash: transaction.recalculate_hash(),
error: Box::new(new_err),
}
OpBlockExecutionError::Eth(BlockExecutionError::Validation(
BlockValidationError::EVM {
hash: transaction.recalculate_hash(),
error: Box::new(new_err),
},
))
})?;

trace!(
Expand Down Expand Up @@ -269,11 +275,14 @@ where
let balance_increments =
post_block_balance_increments(&self.chain_spec.clone(), &block.block);
// increment balances
self.state
.increment_balances(balance_increments.clone())
.map_err(|_| BlockValidationError::IncrementBalanceFailed)?;
self.state.increment_balances(balance_increments.clone()).map_err(|_| {
OpBlockExecutionError::Eth(BlockExecutionError::Validation(
BlockValidationError::IncrementBalanceFailed,
))
})?;
// call state hook with changes due to balance increments.
let balance_state = balance_increment_state(&balance_increments, &mut self.state)?;
let balance_state = balance_increment_state(&balance_increments, &mut self.state)
.map_err(OpBlockExecutionError::Eth)?;
self.system_caller.on_state(&balance_state);

Ok(Requests::default())
Expand Down
1 change: 1 addition & 0 deletions examples/custom-beacon-withdrawals/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ pub struct CustomExecutorStrategyFactory {
}

impl BlockExecutionStrategyFactory for CustomExecutorStrategyFactory {
type Error = BlockExecutionError;
type Primitives = EthPrimitives;
type Strategy<DB: Database<Error: Into<ProviderError> + Display>> = CustomExecutorStrategy<DB>;

Expand Down