refactor: allow chain-specific configuration of Evm #1378

Wodann · 2024-05-03T19:32:21Z

Experiment to see whether it would be possible to separate optimism and mainnet SpecIds and other types, such as HaltReason.

I'm curious to hear whether any of this is in line with #918

rakita

I have started reviewing this last weeks, but didnt have time to finish it whole and getter better grasp on it.

I general like it, change is needed but would like to minimise renames to get smaller PR.

As this is potentially a breaking change will need look at reth code and think how this impacts them as we are in the middle of devnet for Prague.

crates/precompile/Cargo.toml

crates/precompile/src/lib.rs

crates/primitives/src/chain_spec.rs

crates/primitives/src/result.rs

crates/revm/benches/bench.rs

crates/revm/src/builder.rs

Wodann · 2024-05-21T21:47:26Z

crates/revm/src/custom_upcode.rs

@@ -0,0 +1,253 @@
+use revm_interpreter::{


This file merely serves to illustrate that we can support custom opcodes. It should be removed before merging.

We can move it to examples later, fine with leaving it here for now.

Wodann · 2024-05-24T17:56:42Z

Given that the general approach seemed to be the right direction, I removed the last instances of [cfg(feature = "optimism")] from the codebase by adding new types to the trait ChainSpec for variability of transaction and block environments.

Please let me know what you think. You should be able to review those changes commit-by-commit

Wodann · 2024-05-24T17:57:45Z

I'll hold off on resolving merge conflicts until we're happy with the full design, if that's okay.

crates/revm/src/optimism/env.rs

Wodann · 2024-05-24T18:01:09Z

There are still some EVMError::Custom invocations in handler_register.rs. My understanding is that we could catch those when validating the environment.

That way we could use chain-specific errors static errors instead of strings. Alternatively, we could also add a custom error type to the trait ChainSpec.

That would allow us to remove the EVMError::Custom(String)

WDYT, @rakita ?

rakita · 2024-06-17T00:55:45Z

I really like this path. For organizational purposes, lets merge this into chain_spec branch so we can work on it and make PR there, and after we are fine with the design, we can think about rebasing it on the main.

I'll hold off on resolving merge conflicts until we're happy with the full design, if that's okay.

This is perfectly fine.

There are still some EVMError::Custom invocations in handler_register.rs. My understanding is that we could catch those when validating the environment.

That way we could use chain-specific errors static errors instead of strings. Alternatively, we could also add a custom error type to the trait ChainSpec.

That would allow us to remove the EVMError::Custom(String)

I would leave it as String for now, PR is getting big and we can add this later.

rakita · 2024-06-17T01:13:21Z

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.

I am still in investigation phase, but this looks great!

Wodann · 2024-06-17T17:44:14Z

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.

I am still in investigation phase, but this looks great!

Awesome! Is there anything you'd like me to do?

I added your commits to this PR, squashed, and rebased on main

rakita · 2024-06-18T10:24:30Z

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.
I am still in investigation phase, but this looks great!

Awesome! Is there anything you'd like me to do?

I dont have any action items rn, but I will ping you if something comes up.
PR is in good state, but it is a massive change in both the code and the design, so I want to be sure that it is okay.

I added your commits to this PR, squashed, and rebased on main

Great!

Wodann · 2024-06-19T14:31:23Z

crates/primitives/src/result.rs

@@ -16,7 +16,6 @@ pub type EVMResultGeneric<T, ChainSpecT, DBError> = core::result::Result<
 >;

 #[derive(Debug, Clone, PartialEq, Eq)]
-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]


Removing serde (de)serialisation allowed the removal of the serde-related trait bounds from the HaltReasonTrait. As the ExecutionResult and ResultAndState aren't part of the JSON-RPC of Ethereum, I think it should be safe to remove this.

WDYT?

Wodann · 2024-06-24T16:55:15Z

crates/revm/src/context/evm_context.rs

-            .field("inner", &self.inner)
-            .field("precompiles", &self.inner)


I used the derive_where procedural macro to avoid errors like this (double usage of self.inner).

Wodann · 2024-06-24T16:57:19Z

crates/revm/src/context/inner_evm_context.rs

-                let nonce = self.env.tx.nonce.unwrap_or_else(|| {
-                    let caller = self.env.tx.caller;
-                    self.load_account(caller)
-                        .map(|(a, _)| a.info.nonce)
-                        .unwrap_or_default()
-                });


I moved the configuration of disabled nonce checks to the CfgEnv such that the transaction itself always requires a nonce value to be provided. If someone wants to disable nonce checks, they can do so for the whole Evm, similar to how it works for disabling other checks.

This voids the need for the trait Transaction to return an Option<u64> and I believe is in line with the overall design philosophy for having compile-time disable for checks like these.

Wodann · 2024-06-24T16:59:03Z

crates/primitives/src/transaction.rs

+/// Trait for retrieving transaction information required for execution.
+pub trait Transaction {
+    /// Caller aka Author aka transaction signer.
+    fn caller(&self) -> &Address;


I used the philosophy that anything larger than a u64 (pointer) should use a & whereas anything smaller or equal to a u64 is returned by value. This is also the guideline for when to derive Copy or not, as it avoids unnecessarily large copies.

I'm not sure whether the presence of lifetimes in the return arguments might be a concern for some people though.

Wodann · 2024-06-24T17:50:28Z

crates/primitives/src/chain_spec.rs

+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct EthChainSpec;
+
+impl ChainSpec for EthChainSpec {


Would L1ChainSpec be a better/clearer name?

Wodann · 2024-06-24T17:50:48Z

crates/primitives/src/env.rs

    /// Configuration of the transaction that is being executed.
-    pub tx: TxEnv,


As BlockEnv and TxEnv are no needed for the EthChainSpec, should they be moved to chain_spec.rs?

Wodann · 2024-06-24T17:53:55Z

crates/revm/src/builder.rs

 };
 use core::marker::PhantomData;
 use std::boxed::Box;

 /// Evm Builder allows building or modifying EVM.
 /// Note that some of the methods that changes underlying structures
 /// will reset the registered handler to default mainnet.
-pub struct EvmBuilder<'a, BuilderStage, EXT, DB: Database> {
-    context: Context<EXT, DB>,
+pub struct EvmBuilder<'a, BuilderStage, ChainSpecT: ChainSpec, EXT, DB: Database> {


Instead of requiring Default for ChainSpec::Transaction and ChainSpec::Block, I think a nice design would be to have EvmBuilder contain transaction: Option<ChainSpecT::Transaction> and block: Option<ChainSpecT::Block>.

I feel that both should explicitly be set and when finishing the builder, a Result could be returned that constructs the Context or returns an error mentioning that no transaction and/or block was provided.

That way the user is not quietly using a default transaction or block configuration.

WDYT?

…optional

rakita · 2024-07-29T13:09:08Z

crates/primitives/src/block.rs

+pub fn get_blob_gasprice(block: &impl Block) -> Option<&u128> {
+    block.blob_excess_gas_and_price().map(|a| &a.blob_gasprice)
+}
+
+/// Return `blob_excess_gas` header field. See [EIP-4844].
+///
+/// Returns `None` if `Cancun` is not enabled. This is enforced in [`crate::Env::validate_block_env`].
+///
+/// [EIP-4844]: https://eips.ethereum.org/EIPS/eip-4844
+#[inline]
+pub fn get_blob_excess_gas(block: &impl Block) -> Option<u64> {
+    block.blob_excess_gas_and_price().map(|a| a.excess_blob_gas)
+}


nit: those two can be part of Block trait but with default impl

Wodann force-pushed the refactor/opt-spec-id branch from 0e87f0b to 45c9517 Compare May 6, 2024 21:12

Wodann mentioned this pull request May 7, 2024

Can we replay an on-chain optimism transaction using revm? NomicFoundation/edr#377

Closed

rakita reviewed May 20, 2024

View reviewed changes

Wodann commented May 21, 2024

View reviewed changes

Wodann linked an issue May 22, 2024 that may be closed by this pull request

Refactor REVM to make optimism feature flag additive NomicFoundation/edr#464

Open

Wodann commented May 24, 2024

View reviewed changes

crates/revm/src/optimism/env.rs Show resolved Hide resolved

Wodann marked this pull request as ready for review June 17, 2024 20:00

Wodann force-pushed the refactor/opt-spec-id branch 4 times, most recently from 637c088 to f9b2d03 Compare June 17, 2024 22:55

Wodann force-pushed the refactor/opt-spec-id branch 2 times, most recently from 1744639 to 4bb0f27 Compare June 18, 2024 20:33

Wodann commented Jun 19, 2024

View reviewed changes

Wodann force-pushed the refactor/opt-spec-id branch from 9cb81da to 2881cdf Compare June 24, 2024 16:53

Wodann commented Jun 24, 2024

View reviewed changes

Wodann force-pushed the refactor/opt-spec-id branch 2 times, most recently from 9e5dbea to cd9c906 Compare June 24, 2024 17:33

Wodann commented Jun 24, 2024

View reviewed changes

Wodann added 26 commits June 25, 2024 15:05

fix: clippy warnings

2a84559

fix: cargo doc

1c17cd0

refactor: limit trait bounds on HaltReason

904f0f5

refactor: allow moving of kind

8a1530c

refactor: rename Transaction::nonce to nonce_opt to signal that it's …

29f3be6

…optional

refactor: replace AccessList with alloy version

9f5e2d3

refactor: rename gas_priority_fee to max_priority_fee_per_gas

3baf771

refactor: correct trait bound on ExecutionResult::clone

4710a4b

Clone

5bf0156

refactor: only allow optional nonce check via CfgEnv

612bb40

fix: use alloy-eips with fix for serde

94d529b

build: bump to alloy-eips v0.1.2

e68f3af

build: Cargo.lock file

da97359

fix: revme

c903557

refactor: derive DummyHost

23525aa

refactor: derive Clone for ExecutionResult

729b236

refactor: remove EVMErrorForChain

a35aaa6

refactor: remove EVMErrorForChain

11a4fe6

refactor: derive Clone for CfgEnvWithChainSpec

b3b9b71

refactor: use EVMResultGeneric

1c908e1

refactor: use EVMResultGeneric

a4e49cc

refactor: use EVMResultGeneric

8e3e3be

refactor: use EVMResultGeneric

adf87c4

refactor: use EVMResultGeneric

4761941

refactor: use EVMResultGeneric

78fe3f2

refactor: add convenience EVMErrorForChain type alias

e65e829

Wodann force-pushed the refactor/opt-spec-id branch from 335551e to e65e829 Compare June 25, 2024 15:06

rakita reviewed Jul 29, 2024

View reviewed changes

Wodann mentioned this pull request Jul 29, 2024

feat: introducing EvmWiring, a chain-specific configuration #1672

Merged

Wodann closed this Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: allow chain-specific configuration of Evm #1378

refactor: allow chain-specific configuration of Evm #1378

Wodann commented May 3, 2024

rakita left a comment

Wodann May 21, 2024

rakita Jun 17, 2024

Wodann commented May 24, 2024 •

edited

Loading

Wodann commented May 24, 2024

Wodann commented May 24, 2024

rakita commented Jun 17, 2024

rakita commented Jun 17, 2024

Wodann commented Jun 17, 2024 •

edited

Loading

rakita commented Jun 18, 2024

Wodann Jun 19, 2024

Wodann Jun 24, 2024

Wodann Jun 24, 2024

Wodann Jun 24, 2024

Wodann Jun 24, 2024

Wodann Jun 24, 2024

Wodann Jun 24, 2024

rakita Jul 29, 2024

		.field("inner", &self.inner)
		.field("precompiles", &self.inner)

		/// Configuration of the transaction that is being executed.
		pub tx: TxEnv,

refactor: allow chain-specific configuration of Evm #1378

refactor: allow chain-specific configuration of Evm #1378

Conversation

Wodann commented May 3, 2024

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wodann commented May 24, 2024 • edited Loading

Wodann commented May 24, 2024

Wodann commented May 24, 2024

rakita commented Jun 17, 2024

rakita commented Jun 17, 2024

Wodann commented Jun 17, 2024 • edited Loading

rakita commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wodann commented May 24, 2024 •

edited

Loading

Wodann commented Jun 17, 2024 •

edited

Loading