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

feat: introduce evm config trait #6461

Merged
merged 21 commits into from
Feb 23, 2024
Merged

feat: introduce evm config trait #6461

merged 21 commits into from
Feb 23, 2024

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Feb 7, 2024

closes #6180

@Rjected Rjected force-pushed the matt/extend-evm-trait branch from 04d994f to cd2e600 Compare February 12, 2024 14:50
}

/// An executor capable of executing a block.
pub trait BlockExecutor {
Copy link
Member

Choose a reason for hiding this comment

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

do we dedupe with

pub trait BlockExecutor {
eventually?

Copy link
Member

@Rjected Rjected Feb 12, 2024

Choose a reason for hiding this comment

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

yep, actually I think I'll move this to a new crate that reth-provider and node-api will both depend on, instead of including this (and relevant types) in node-api. Mainly to avoid circular dependencies, and avoid making node-api gigantic

Copy link
Member

Choose a reason for hiding this comment

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

actually, hmm, need to think more about this, this might not work

Copy link
Member

Choose a reason for hiding this comment

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

Thinking we can get rid of potential reth-provider dependency issues by making the error type an associated type on BlockExecutor.

@Rjected
Copy link
Member

Rjected commented Feb 12, 2024

final challenge w.r.t. the BlockExecutor trait: BundleStateWithReceipts, part of the BlockExecutor trait, is currently in reth-provider and has a method write_to_db. This pulls in methods from reth-db (for writing) and pulls in the reth-provider types StateChanges and StateReverts. These types wrap corresponding revm types and implement a write_to_db method.

Two potential solutions:

  • We could move the non-db-related BundleStateWithReceipts methods to node-api, and wrap it, similar to StateChanges and StateReverts.
  • Create provider traits responsible for reading and writing these data structures. For example, BlockWriter was created to replace the same methods in Block: refactor: add BlockWriter and BlockExecutionWriter  #3384

Wrapping is easy but the trait seems more idiomatic, so I'm proposing we create a trait that can write BundleStateWithReceipts, StateChanges, and StateReverts

@onbjerg
Copy link
Member

onbjerg commented Feb 13, 2024

I'd lean towards proposal 2 as well since it is the approach we are doing for other things

@Rjected
Copy link
Member

Rjected commented Feb 14, 2024

Defining BlockExecutor in node-api for this purpose:

trait EvmConfig {
    /// the executor type
    type Executor: BlockExecutor;
    
    /// returns the executor with the installed db
    fn evm(&self, db: impl Database) -> Self::Executor;
}

has been scoped out for a few reasons:

  1. EvmProcessor has a lifetime, which means using BlockExecutor as a bound on an associated type would require that associated type to be a GAT.
  2. The alternative, returning EvmProcessor<'static> would require the DB to be 'static, which is basically never true.

The idea to have take_output_state consume self was proposed in #6568, but is not feasible, unless we want to run into the above problems with GATs and object safety because ExecutorFactory returns a Box<dyn PrunableBlockExecutor>, which we can't turn into an owned value

Where are we now?

Now we're focusing on a trait like this:

/// Trait for configuring the EVM for executing full blocks.
pub trait EvmConfig {
    /// Returns new EVM builder
    fn evm(&self) -> EvmBuilder<'static, SetGenericStage, (), EmptyDB> {
        Default::default()
    }
}

Because we're no longer worrying about defining BlockExecutor in node-api, the Error associated type can be removed.

@Rjected Rjected force-pushed the matt/extend-evm-trait branch from cc84e4b to 0e71257 Compare February 14, 2024 22:55
Copy link
Collaborator Author

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

we need them to be stateful, so we can capture additional state for precompiles etc.

crates/node-optimism/src/evm.rs Outdated Show resolved Hide resolved
@Rjected Rjected force-pushed the matt/extend-evm-trait branch from 11aeea4 to 5068549 Compare February 15, 2024 23:14
@Rjected Rjected marked this pull request as ready for review February 15, 2024 23:20
@Rjected Rjected requested review from onbjerg and Rjected February 15, 2024 23:24
@emhane emhane added C-enhancement New feature or request A-execution Related to the Execution and EVM labels Feb 17, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

small nits

crates/node-api/src/evm/traits.rs Show resolved Hide resolved
crates/node-api/src/evm/traits.rs Outdated Show resolved Hide resolved
crates/node-optimism/src/evm.rs Outdated Show resolved Hide resolved
@Rjected Rjected force-pushed the matt/extend-evm-trait branch from f49fd8c to 14ee329 Compare February 21, 2024 00:54
@Rjected Rjected requested a review from onbjerg February 21, 2024 19:19
Copy link
Collaborator Author

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

This should unblock us.

then we can figure out how to make the blockexecutor work.

feel free to close this PR and submit it as your own, can't approve myself.

Comment on lines +7 to +12
/// Returns new EVM with the given database
fn evm<'a, DB: Database + 'a>(&self, db: DB) -> Evm<'a, (), DB> {
EvmBuilder::default().with_db(db).build()
}

/// Returns a new EVM with the given inspector
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like a few more docs here wrt what the responsibilities of the caller are: env config

Copy link
Member

Choose a reason for hiding this comment

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

ah forgot to push these changes before merging, will add in another PR

Comment on lines +7 to +8
/// Returns new EVM with the given database
fn evm<'a, DB: Database + 'a>(&self, db: DB) -> Evm<'a, (), DB> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can add one more fn that also accepts the Handler type as arg

@Rjected Rjected force-pushed the matt/extend-evm-trait branch from fc740a6 to cd88653 Compare February 23, 2024 21:47
@Rjected
Copy link
Member

Rjected commented Feb 23, 2024

@mattsse handler method is blocked on bluealloy/revm#1124 - going to merge this, so we can work on the handler method and evm processor / executor factory refactor separately

@Rjected Rjected added this pull request to the merge queue Feb 23, 2024
Merged via the queue into main with commit a7e183d Feb 23, 2024
30 checks passed
@Rjected Rjected deleted the matt/extend-evm-trait branch February 23, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-execution Related to the Execution and EVM C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Stateful EVM component trait
4 participants