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: inject call to CREATE2 factory through custom revm handler #7653

Merged
merged 10 commits into from
Apr 17, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Apr 13, 2024

Motivation

ref #7632 (comment)

This changes flow for CREATE2 deployments routed through CREATE2 factory. Earlier we would just patch the sender of CREATE2 frame which resulted in data appearing in traces and state diff being different from the actual transaction data.

The way this PR addresses this is by implementing a revm handler register which replaces CREATE2 frame with CALL frame to factory when needed.

Thus, trace of a simple counter CREATE2 deployment changes from:

[47791] DeployCounter::run()
  ├─ [0] VM::startBroadcast()
  │   └─ ← [Return] 
  ├─ [12666] → new Counter@0xbe77DC606202Cf861Ba32c026c42926c34b62C40
  │   └─ ← [Return] 63 bytes of code
  └─ ← [Stop] 

to:

[79908] DeployCounter::run()
  ├─ [0] VM::startBroadcast()
  │   └─ ← [Return] 
  ├─ [44783] Create2Deployer::create2()
  │   ├─ [12666] → new Counter@0xbe77DC606202Cf861Ba32c026c42926c34b62C40
  │   │   └─ ← [Return] 63 bytes of code
  │   └─ ← [Return] 0xbe77dc606202cf861ba32c026c42926c34b62c40
  └─ ← [Stop] 

Solution

Implement InspectorExt trait with should_use_create2_factory method returning false by default which allows our inspectors to decide if the CREATE frame should be routed through create2 factory.

Implement handler register overriding create and insert_call_outcome hooks:

  • Overriden create hook returns CALL frame instead of CREATE frame when should_use_create2_factory fn returns true for the current frame.
  • Overriden insert_call_outcome hook decodes result of the create2 factory invocation and inserts it directly into interpreter.

Solution is not really clean because I had to duplicate parts of call and insert_call_outcome hooks from revm inspector hander register instead of just invoking them in register. The motivation behind this is following:

This impl lets us keep cheatcodes inspector and overall flow simpler however it introduces additional complexity in EVM construction step and might result in issues if revm implementation of inspector handler register changes, so not sure if we should include that

crates/anvil/src/eth/backend/mem/inspector.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/evm/core/src/lib.rs Show resolved Hide resolved
crates/evm/core/src/utils.rs Outdated Show resolved Hide resolved
crates/evm/core/src/utils.rs Outdated Show resolved Hide resolved
Copy link
Member

@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 is great,
only have pedantic doc nits for the register function

crates/evm/core/src/lib.rs Show resolved Hide resolved
crates/evm/core/src/utils.rs Show resolved Hide resolved
@klkvr klkvr requested a review from mattsse April 15, 2024 11:53
Copy link
Member

@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.

lgtm, pending @DaniPopes

@@ -31,6 +31,7 @@ foundry-cli.workspace = true
foundry-common.workspace = true
foundry-config.workspace = true
foundry-evm.workspace = true
foundry-evm-core.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

unused

let is_fixed_gas_limit = check_if_fixed_gas_limit(ecx, call.gas_limit);

let account = ecx.journaled_state.state().get(&broadcast.new_origin).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let account = ecx.journaled_state.state().get(&broadcast.new_origin).unwrap();
let account = ecx.journaled_state.state()[&broadcast.new_origin];

@@ -1,17 +1,26 @@
use std::{cell::RefCell, rc::Rc, sync::Arc};

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

};

pub use foundry_compilers::utils::RuntimeOrHandle;
pub use revm::primitives::State as StateChangeset;

pub use crate::ic::*;
use crate::{constants::DEFAULT_CREATE2_DEPLOYER, InspectorExt};
Copy link
Member

Choose a reason for hiding this comment

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

up

@@ -99,6 +108,129 @@ pub fn gas_used(spec: SpecId, spent: u64, refunded: u64) -> u64 {
spent - (refunded).min(spent / refund_quotient)
}

pub fn get_create2_factory_call_inputs(salt: U256, inputs: CreateInputs) -> CallInputs {
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be public?

@klkvr klkvr force-pushed the klkvr/fix-create2 branch from cabfe24 to 2713557 Compare April 17, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants