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

Jump to bottom

evm: wider use of RethEvmBuilder #9944

Merged
merged 15 commits into from
Aug 22, 2024
Merged

evm: wider use of RethEvmBuilder #9944

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a5dcc3e
evm: wider use of
tcoratger Jul 31, 2024
38c4340
Update crates/evm/Cargo.toml
tcoratger Jul 31, 2024
27ab389
clean up
tcoratger Jul 31, 2024
62457e5
fix op
tcoratger Jul 31, 2024
044b4ea
fix op
tcoratger Jul 31, 2024
dff8166
fix op
tcoratger Jul 31, 2024
fb044af
handle op in reth revm builder
tcoratger Aug 5, 2024
9ddfef3
handle op in reth revm builder
tcoratger Aug 5, 2024
7a4da52
fix tests
tcoratger Aug 5, 2024
8f2ec2d
fix tests
tcoratger Aug 5, 2024
763637e
clean up
tcoratger Aug 5, 2024
9d14e9a
clean up
tcoratger Aug 5, 2024
3bd4bd4
clean up
tcoratger Aug 5, 2024
fa3626b
clean up
tcoratger Aug 5, 2024
8e268d2
clean up
tcoratger Aug 5, 2024
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
8 changes: 0 additions & 8 deletions crates/ethereum/evm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ extern crate alloc;
use reth_chainspec::{ChainSpec, Head};
use reth_evm::{ConfigureEvm, ConfigureEvmEnv};
use reth_primitives::{transaction::FillTxEnv, Address, Header, TransactionSigned, U256};
use reth_revm::{Database, EvmBuilder};
use revm_primitives::{AnalysisKind, Bytes, CfgEnvWithHandlerCfg, Env, TxEnv, TxKind};

#[cfg(not(feature = "std"))]
Expand Down Expand Up @@ -110,13 +109,6 @@ impl ConfigureEvmEnv for EthEvmConfig {
impl ConfigureEvm for EthEvmConfig {
type DefaultExternalContext<'a> = ();

fn evm<DB: Database>(
&self,
db: DB,
) -> reth_revm::Evm<'_, Self::DefaultExternalContext<'_>, DB> {
EvmBuilder::default().with_db(db).build()
}

fn default_external_context<'a>(&self) -> Self::DefaultExternalContext<'a> {}
}

Expand Down
9 changes: 4 additions & 5 deletions crates/evm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ pub trait ConfigureEvm: ConfigureEvmEnv {
/// This does not automatically configure the EVM with [`ConfigureEvmEnv`] methods. It is up to
/// the caller to call an appropriate method to fill the transaction and block environment
/// before executing any transactions using the provided EVM.
fn evm<DB: Database>(&self, db: DB) -> Evm<'_, Self::DefaultExternalContext<'_>, DB>;
fn evm<DB: Database>(&self, db: DB) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
RethEvmBuilder::new(db, self.default_external_context()).build()
}

/// Returns a new EVM with the given database configured with the given environment settings,
/// including the spec id.
Expand All @@ -55,10 +57,7 @@ pub trait ConfigureEvm: ConfigureEvmEnv {
db: DB,
env: EnvWithHandlerCfg,
) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
let mut evm = self.evm(db);
evm.modify_spec_id(env.spec_id());
evm.context.evm.env = env.env;
evm
RethEvmBuilder::new(db, self.default_external_context()).with_env(env.into()).build()
}

/// Returns a new EVM with the given database configured with the given environment settings,
Expand Down
12 changes: 12 additions & 0 deletions crates/optimism/evm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use reth_primitives::{
Address, Head, Header, TransactionSigned, U256,
};
use reth_revm::{inspector_handle_register, Database, Evm, EvmBuilder, GetInspector};
use revm_primitives::EnvWithHandlerCfg;

mod config;
pub use config::{revm_spec, revm_spec_by_timestamp_after_bedrock};
Expand Down Expand Up @@ -117,6 +118,17 @@ impl ConfigureEvm for OptimismEvmConfig {
EvmBuilder::default().with_db(db).optimism().build()
}

fn evm_with_env<DB: Database>(
&self,
db: DB,
env: EnvWithHandlerCfg,
) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
let mut evm = self.evm(db);
evm.modify_spec_id(env.spec_id());
evm.context.evm.env = env.env;
evm
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

remind me, why do we need to impl this trait function now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in the old version, evm_with_env took as first line the result of self.evm() with a .optimism() component inside for the Optimism case.

But since now we use our RethEvmBuilder, we can no longer count on the same methodology, the implementation of evm_with_env being:

/// Returns a new EVM with the given database configured with the given environment settings,
/// including the spec id.
/// /// This will preserve any handler modifications fn evm_with_env<DB: Database>( &self, db: DB, env: EnvWithHandlerCfg, ) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> { RethEvmBuilder::new(db, self.default_external_context()).with_env(env.into()).build() } ` and not taking into account `self.evm()` where the dependency on `.optimism` is present.

I wanted to tackle this in a follow up to propose something more unified or in this PR if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, tbh I'm very confused by this, not obvious to me what this pr achieves.
In general I find it hard to reason about what to invoke on evm and I don't fully get why we now need to override this function when it previously wasn't the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand, let me proposed something better in this pr in a bit :) will be clearer I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse I just proposed a very small change. I think we can merge like that. It's already a first path forward.

But to summarize for the rest, the problem I encounter here is with the .optimism() which only intervenes under the optimism cfg but which pollutes my construction with the Reth EVM builder because it intervenes before the build and in the three functions evm, evm_with_env and evm_with_inspector. This is what prevents for the moment to use the Reth EVM builder here.


fn evm_with_inspector<DB, I>(&self, db: DB, inspector: I) -> Evm<'_, I, DB>
where
DB: Database,
Expand Down
13 changes: 12 additions & 1 deletion examples/custom-evm/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use reth_node_api::{ConfigureEvm, ConfigureEvmEnv, FullNodeTypes};
use reth_node_core::{args::RpcServerArgs, node_config::NodeConfig};
use reth_node_ethereum::{node::EthereumAddOns, EthExecutorProvider, EthereumNode};
use reth_primitives::{
revm_primitives::{AnalysisKind, CfgEnvWithHandlerCfg, TxEnv},
revm_primitives::{AnalysisKind, CfgEnvWithHandlerCfg, EnvWithHandlerCfg, TxEnv},
Address, Header, TransactionSigned, U256,
};
use reth_tracing::{RethTracer, Tracer};
Expand Down Expand Up @@ -117,6 +117,17 @@ impl ConfigureEvm for MyEvmConfig {
.build()
}

fn evm_with_env<DB: Database>(
&self,
db: DB,
env: EnvWithHandlerCfg,
) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
let mut evm = self.evm(db);
evm.modify_spec_id(env.spec_id());
evm.context.evm.env = env.env;
evm
}

fn evm_with_inspector<DB, I>(&self, db: DB, inspector: I) -> Evm<'_, I, DB>
where
DB: Database,
Expand Down
13 changes: 12 additions & 1 deletion examples/stateful-precompile/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use reth_node_api::{ConfigureEvm, ConfigureEvmEnv, FullNodeTypes};
use reth_node_core::{args::RpcServerArgs, node_config::NodeConfig};
use reth_node_ethereum::{node::EthereumAddOns, EthEvmConfig, EthExecutorProvider, EthereumNode};
use reth_primitives::{
revm_primitives::{SpecId, StatefulPrecompileMut},
revm_primitives::{EnvWithHandlerCfg, SpecId, StatefulPrecompileMut},
Header, TransactionSigned,
};
use reth_tracing::{RethTracer, Tracer};
Expand Down Expand Up @@ -177,6 +177,17 @@ impl ConfigureEvm for MyEvmConfig {
.build()
}

fn evm_with_env<DB: Database>(
&self,
db: DB,
env: EnvWithHandlerCfg,
) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
let mut evm = self.evm(db);
evm.modify_spec_id(env.spec_id());
evm.context.evm.env = env.env;
evm
}

fn evm_with_inspector<DB, I>(&self, db: DB, inspector: I) -> Evm<'_, I, DB>
where
DB: Database,
Expand Down
Loading