Skip to content

chore: bump revm to 21.0.0 release #10183

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

Draft
wants to merge 144 commits into
base: master
Choose a base branch
from
Draft

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Mar 26, 2025

Motivation

Opening for visibility / easier collaboration

Restructured commit history so it is easier to revert to known good changes whilst iteratively implementing the complex changes (handler, journal)

Uses #10051 as reference

CI is expected to be failing for quite some time

Solution

Depends on:

Next steps:

  • Selectively extract changes from feat: bump revm #10051
  • Continue migrating foundry-evm-core crate, preferring internal mapping types rather than direct replacement - similar to Ethers > Alloy migration process
  • Refactor / reimplement handlers:
    • Odyssey
    • CREATE2
  • Refactor JournaledState using JournalInner
  • Clarify how TxEnv + OptimismFields would work
  • Clarify how EvmContext / InnerEvmContext successor
  • Clarify interpreter.shared_memory successor
  • Investigate internal mapping approach rather than refactor cheatcodes
  • Refactor inspector
  • Support Optimism (op-revm)
  • Bump alloy-evm to 0.5.0
  • Migrate Anvil migrate anvil to revm 21 #10356
  • Fix tests
  • Implement deprecated CustomPrintTracer in Add equivalent of Revm v19 CustomPrintTracer bluealloy/revm#2422

Blocking

Currently is blocking:

Knowing breaking changes for end users are:

  • Noted by Yash: because of internal Revm fields changing Anvil state files are known to currently break

Follow ups:

Issues:

  • with_evm copies l1_block_info, this field no longer exists yielding the question whether Cheatcodes, like Anvil, expects OpEvm
  • The get_prank cheatcode expects depth to be a u64, not a usize - try_into? works
  • Pattern ccx.ecx.journaled_state.load_account(&mut ccx.ecx.db(), *from) causes cannot borrow ccx.ecx as mutable more than once at a time second mutable borrow occurs here

Closes: #10367

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@zerosnacks zerosnacks added A-dependencies Area: dependencies T-chore Type: chore labels Mar 26, 2025
@zerosnacks zerosnacks moved this to In Progress in Foundry Mar 26, 2025
@zerosnacks zerosnacks added this to the v1.1.0 milestone Mar 26, 2025
Comment on lines 324 to 326
// if let Some(factory) = &self.precompile_factory {
// inject_precompiles(&mut evm, factory.precompiles());
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly related: alloy-rs/evm#71

cc @yash-atreya

Copy link
Collaborator

Choose a reason for hiding this comment

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

should be merged soon

let offset = interpreter.stack.peek(0).expect("stack size > 1").saturating_to();
let data = interpreter.shared_memory.slice(offset, 0x40);
let data = interpreter.memory.slice_len(offset, 0x40);
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @rakita is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is equivalent

@@ -1232,8 +1238,6 @@ impl DatabaseExt for Backend {
update_env_block(env, &block);

// replay all transactions that came before
let env = env.clone();
Copy link
Member Author

Choose a reason for hiding this comment

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

This may still be necessary

Comment on lines -635 to +644
evm.context.evm.inner.journaled_state.depth = 1;
evm.journaled_state.depth = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

observation: cargo test --package forge --test cli -- cmd::gas_report_all_contracts --exact --show-output -- --nocapture appears to not run the inspector if this is set to 1. If disabled it does run but with the output as follows:

---- expected: tests/cli/cmd.rs:1627:81
++++ actual:   stdout
   1    1 | ...
   2    2 | ╭----------------------------------------+-----------------+-------+--------+-------+---------╮
   3    3 | | src/Contracts.sol:ContractOne Contract |                 |       |        |       |         |
   4    4 | +=============================================================================================+
   5    5 | | Deployment Cost                        | Deployment Size |       |        |       |         |
   6    6 | |----------------------------------------+-----------------+-------+--------+-------+---------|
   7      - | 133027                                 | 394             |       |        |       |         |
        7 + | 170470                                 | 394             |       |        |       |         |
   8    8 | |----------------------------------------+-----------------+-------+--------+-------+---------|
   9    9 | |                                        |                 |       |        |       |         |
  10   10 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  11   11 | | Function Name                          | Min             | Avg   | Median | Max   | # Calls |
  12   12 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  13      - | foo                                    | 45656           | 45656 | 45656  | 45656 | 1       |
       13 + | foo                                    | 24592           | 37706 | 37706  | 50821 | 2       |
  14   14 | ╰----------------------------------------+-----------------+-------+--------+-------+---------╯
  15   15 | 
  16   16 | ╭------------------------------------------+-----------------+--------+--------+--------+---------╮
  17   17 | | src/Contracts.sol:ContractThree Contract |                 |        |        |        |         |
  18   18 | +=================================================================================================+
  19   19 | | Deployment Cost                          | Deployment Size |        |        |        |         |
  20   20 | |------------------------------------------+-----------------+--------+--------+--------+---------|
  21      - | 133243                                   | 395             |        |        |        |         |
       21 + | 170686                                   | 395             |        |        |        |         |
  22   22 | |------------------------------------------+-----------------+--------+--------+--------+---------|
  23   23 | |                                          |                 |        |        |        |         |
  24   24 | |------------------------------------------+-----------------+--------+--------+--------+---------|
  25   25 | | Function Name                            | Min             | Avg    | Median | Max    | # Calls |
  26   26 | |------------------------------------------+-----------------+--------+--------+--------+---------|
  27      - | baz                                      | 287711          | 287711 | 287711 | 287711 | 1       |
       27 + | baz                                      | 266647          | 279772 | 279772 | 292898 | 2       |
  28   28 | ╰------------------------------------------+-----------------+--------+--------+--------+---------╯
  29   29 | 
  30   30 | ╭----------------------------------------+-----------------+-------+--------+-------+---------╮
  31   31 | | src/Contracts.sol:ContractTwo Contract |                 |       |        |       |         |
  32   32 | +=============================================================================================+
  33   33 | | Deployment Cost                        | Deployment Size |       |        |       |         |
  34   34 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  35      - | 133027                                 | 394             |       |        |       |         |
       35 + | 170470                                 | 394             |       |        |       |         |
  36   36 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  37   37 | |                                        |                 |       |        |       |         |
  38   38 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  39   39 | | Function Name                          | Min             | Avg   | Median | Max   | # Calls |
  40   40 | |----------------------------------------+-----------------+-------+--------+-------+---------|
  41      - | bar                                    | 67683           | 67683 | 67683  | 67683 | 1       |
       41 + | bar                                    | 46619           | 59733 | 59733  | 72848 | 2       |
  42   42 | ╰----------------------------------------+-----------------+-------+--------+-------+---------╯
  43   43 | 
  44   44 | 
  45   45 | Ran 3 test suites [ELAPSED]: 3 tests passed, 0 failed, 0 skipped (3 total tests)

This has been introduced in the isolate mode PR cc @klkvr, would you have an idea what could be the cause?


// Type 3, EIP-4844
env.tx.blob_hashes = blob_versioned_hashes.clone().unwrap_or_default();
env.tx.max_fee_per_blob_gas = max_fee_per_blob_gas.map(U256::from);
env.tx.max_fee_per_blob_gas = max_fee_per_blob_gas.unwrap_or_default();
Copy link
Member Author

Choose a reason for hiding this comment

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

using a default value of 0 may be an issue here given 4844 asserts

assert tx.max_fee_per_blob_gas >= get_base_fee_per_blob_gas(block.header)

* downgrade op-revm to 2.0.0 to resolve dep conflict

* op-revm 3.0 uses revm 22

* add `as_mut_dyn` to trait `MaybeFullDatabase` as we now require mut db_ref access (

* Revert "add `as_mut_dyn` to trait `MaybeFullDatabase` as we now require mut db_ref access ("

This reverts commit 84d11f1.

* fix: Inspector should be generic over CTX not DB

* fixes helpers: new_evm_with_inspector_* to use CTX generic

* fix: pass TxEnv to evm.transact

* fix: inspector inference in TransactionExecutor and build_access_list_with_state

* workaround: dup LogCollector to use with AnvilEvmContext

* coz FoundryEvmContext is not generic over DB, instead hardoded to dyn DatabaseExt

* fix tests

* fix traces test

* fix: use default kzg settings in blob validation

* reintroduce OptimismHardfork

* fix: disable nonce check if nonce is None

* fix!: load state tests by addressing breaking changes in state files

* BlockEnv Breaking change:
- most fields now use `u64` instead of `U64` / `U256`
- coinbase renamed to beneficiary
- best_block_number is `u64`, prev `U64`

* fix: access_list test by using evm.inspect_with_tx

* fix: replace evm.transact with evm.inspect_with_tx

* fix: make impl Inspector for AnvilInspector generic over CTX

* fix: clone inspector in TransactionExecutor to enable evm.inspect_commit

* fix: remove cloned inspector from TransactionExecutor

* feat(`anvil`): op support revm 21 (#10407)

* enable OpHardforks in NodeConfig

* feat: add is_optimism flag to foundry_evm::Env

* feat(`anvil`): set is_optimism in Backend

* feat(`anvil`): introducing EvmContext enum holding Eth and Op variants.

* adds OpEnv to foundry_evm_core

* feat: EitherEvm

* impl Evm for EitherEvm

* integrate EitherEvm into RPC and executor

*Map OpHaltReason and OpTransactionError

* rm old evm helpers

* feat(`foundry_evm`): add deposit tx parts field to Env

* fix(`anvil`): set deposit tx parts in tx executor and backend.inspect_tx

* nit

* docs EitherEvm

* nit

* refac: return TxEnv and Deposit parts separately

* nits

* nit

* make anvil result aliases more generic

* nit
zerosnacks and others added 4 commits April 30, 2025 09:58
…ogCollector`, entire codebase builds (#10412)

* temp refactor, still facing issue

* clean up

* clean up

* temp cleanup, can later be refd

* clean up, refactor stack.rs to apply ecx restore from cache to outside lamba

* fix

* clean up

* clean up

* avoid borrowing mutably for clarity

* use EthEvmContext directly

* FoundryEvmContext -> EthEvmContext

* continue

* fix tests

* fix inspectors

* codebase now builds entirely

* fix clippy lints

* remove duplicate LogCollector in Anvil

* fmt

* fix clippy

* fix doctests
Comment on lines +647 to +653
let res = evm.transact(env.tx.clone());

// need to reset the env in case it was modified via cheatcodes during execution
ecx.env = evm.context.evm.inner.env;
ecx.cfg = cached_env.evm_env.cfg_env;
ecx.block = cached_env.evm_env.block_env;
ecx.tx = cached_env.tx;

Copy link
Member Author

@zerosnacks zerosnacks Apr 30, 2025

Choose a reason for hiding this comment

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

In master we are

// need to reset the env in case it was modified via cheatcodes during execution
ecx.env = evm.context.evm.inner.env;

Previously we were applying the cached_env in order to, as the prior comment states, reset the env.

I think we should instead do what we are doing in master, which is applying the changes to the outer EVM.

I think instead we should:

            let res = evm.transact(evm.tx.clone());

            // apply the changes to the env
            *env.cfg = evm.cfg.clone();
            *env.block = evm.block.clone();
            *env.tx = evm.tx.clone();

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this currently results in a underflow of a gas refund, investigating

Comment on lines +458 to +459
ensure!(*newHeight <= U256::from(u64::MAX), "block height must be less than 2^64 - 1");
ccx.ecx.block.number = newHeight.saturating_to();
Copy link
Member Author

@zerosnacks zerosnacks May 1, 2025

Choose a reason for hiding this comment

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

This will automatically address the issue raised in #10367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: dependencies T-chore Type: chore
Projects
Status: In Progress
7 participants