From f771edc9cc4490dfb1dd4bacb11a623467e3a3c6 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 20 Feb 2024 14:36:27 +0400 Subject: [PATCH 01/24] [wip] feat(forge): isolated execution --- crates/anvil/src/eth/backend/mem/inspector.rs | 8 + crates/cheatcodes/src/inspector.rs | 2 - crates/evm/core/src/backend/fuzz.rs | 10 +- crates/evm/evm/src/inspectors/stack.rs | 248 ++++++++++++++++-- testdata/cheats/Broadcast.t.sol | 2 +- 5 files changed, 238 insertions(+), 32 deletions(-) diff --git a/crates/anvil/src/eth/backend/mem/inspector.rs b/crates/anvil/src/eth/backend/mem/inspector.rs index 6072e4f938b7..7bc6e0d0afe7 100644 --- a/crates/anvil/src/eth/backend/mem/inspector.rs +++ b/crates/anvil/src/eth/backend/mem/inspector.rs @@ -51,6 +51,7 @@ impl revm::Inspector for Inspector { fn initialize_interp(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.initialize_interp(interp, data); + None }); } @@ -58,6 +59,7 @@ impl revm::Inspector for Inspector { fn step(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.step(interp, data); + None }); } @@ -71,6 +73,7 @@ impl revm::Inspector for Inspector { ) { call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { inspector.log(evm_data, address, topics, data); + None }); } @@ -78,6 +81,7 @@ impl revm::Inspector for Inspector { fn step_end(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.step_end(interp, data); + None }); } @@ -89,6 +93,7 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Gas, Bytes) { call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { inspector.call(data, call); + None }); (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) @@ -105,6 +110,7 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.call_end(data, inputs, remaining_gas, ret, out.clone()); + None }); (ret, remaining_gas, out) } @@ -117,6 +123,7 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Option
, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.create(data, call); + None }); (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) @@ -134,6 +141,7 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Option
, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.create_end(data, inputs, status, address, gas, retdata.clone()); + None }); (status, address, gas, retdata) } diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index 8290b9b951de..7b5cd07ba3a8 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -849,7 +849,6 @@ impl Inspector for Cheatcodes { debug!(target: "cheatcodes", tx=?self.broadcastable_transactions.back().unwrap(), "broadcastable call"); let prev = account.info.nonce; - account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast.new_origin, nonce=prev+1, prev, "incremented nonce"); } else if broadcast.single_call { let msg = "`staticcall`s are not allowed after `broadcast`; use `startBroadcast` instead"; @@ -1447,7 +1446,6 @@ fn process_create( let prev = account.info.nonce; account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast_sender, nonce=prev+1, prev, "incremented nonce in create2"); - // Proxy deployer requires the data to be `salt ++ init_code` let calldata = [&salt.to_be_bytes::<32>()[..], &bytecode[..]].concat(); Ok((calldata.into(), Some(DEFAULT_CREATE2_DEPLOYER), prev)) diff --git a/crates/evm/core/src/backend/fuzz.rs b/crates/evm/core/src/backend/fuzz.rs index 5a7946d3c998..f7c535201faf 100644 --- a/crates/evm/core/src/backend/fuzz.rs +++ b/crates/evm/core/src/backend/fuzz.rs @@ -11,8 +11,8 @@ use alloy_genesis::GenesisAccount; use alloy_primitives::{Address, B256, U256}; use revm::{ db::DatabaseRef, - primitives::{AccountInfo, Bytecode, Env, ResultAndState}, - Database, Inspector, JournaledState, + primitives::{Account, AccountInfo, Bytecode, Env, HashMap as Map, ResultAndState}, + Database, DatabaseCommit, Inspector, JournaledState, }; use std::{borrow::Cow, collections::HashMap}; @@ -279,3 +279,9 @@ impl<'a> Database for FuzzBackendWrapper<'a> { DatabaseRef::block_hash_ref(self, number) } } + +impl<'a> DatabaseCommit for FuzzBackendWrapper<'a> { + fn commit(&mut self, changes: Map) { + self.backend.to_mut().commit(changes) + } +} diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 6ece0f2da4ff..8f0cff2ecba2 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -4,15 +4,20 @@ use super::{ }; use alloy_primitives::{Address, Bytes, Log, B256, U256}; use alloy_signer::LocalWallet; -use foundry_evm_core::{backend::DatabaseExt, debug::DebugArena}; +use foundry_evm_core::{ + backend::DatabaseExt, + debug::DebugArena, + utils::{eval_to_instruction_result, halt_to_instruction_result}, +}; use foundry_evm_coverage::HitMaps; use foundry_evm_traces::CallTraceArena; use revm::{ + evm_inner, interpreter::{ return_revert, CallInputs, CreateInputs, Gas, InstructionResult, Interpreter, Stack, }, - primitives::{BlockEnv, Env}, - EVMData, Inspector, + primitives::{BlockEnv, Env, ExecutionResult, Output, State, TxEnv}, + DatabaseCommit, EVMData, Inspector, }; use std::{collections::HashMap, sync::Arc}; @@ -176,9 +181,40 @@ impl InspectorStackBuilder { macro_rules! call_inspectors { ([$($inspector:expr),+ $(,)?], |$id:ident $(,)?| $call:expr $(,)?) => {{$( if let Some($id) = $inspector { - $call + if let Some(result) = $call { + return result; + } + } + )+}}; + ([$($inspector:expr),+ $(,)?], |$id:ident $(,)?| $call:expr, $self:ident, $data:ident $(,)?) => { + if $self.in_inner_context { + $data.journaled_state.depth += 1; + } + call_inspectors!([$($inspector),+], |$id| { + $call.map(|result| { + if $self.in_inner_context { + $data.journaled_state.depth -= 1; + } + result + }) + }); + if $self.in_inner_context { + $data.journaled_state.depth -= 1; } - )+}} + } +} + +fn merge_states(main_state: &mut State, new_state: State) { + for (addr, acc) in new_state { + if main_state.contains_key(&addr) { + let acc_mut = main_state.get_mut(&addr).unwrap(); + acc_mut.status |= acc.status; + acc_mut.info = acc.info; + acc_mut.storage.extend(acc.storage); + } else { + main_state.insert(addr, acc); + } + } } /// The collected results of [`InspectorStack`]. @@ -207,6 +243,8 @@ pub struct InspectorStack { pub log_collector: Option, pub printer: Option, pub tracer: Option, + /// Flag marking if we are in the inner EVM context. + pub in_inner_context: bool, } impl InspectorStack { @@ -354,16 +392,19 @@ impl InspectorStack { if new_status != status || (new_status == InstructionResult::Revert && new_retdata != retdata) { - return (new_status, new_gas, new_retdata); + Some((new_status, new_gas, new_retdata)) + } else { + None } - } + }, + self, + data ); - (status, remaining_gas, retdata) } } -impl Inspector for InspectorStack { +impl Inspector for InspectorStack { fn initialize_interp(&mut self, interpreter: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { let res = interpreter.instruction_result; call_inspectors!( @@ -380,10 +421,13 @@ impl Inspector for InspectorStack { // Allow inspectors to exit early if interpreter.instruction_result != res { - #[allow(clippy::needless_return)] - return; + Some(()) + } else { + None } - } + }, + self, + data ); } @@ -404,10 +448,13 @@ impl Inspector for InspectorStack { // Allow inspectors to exit early if interpreter.instruction_result != res { - #[allow(clippy::needless_return)] - return; + Some(()) + } else { + None } - } + }, + self, + data ); } @@ -422,7 +469,10 @@ impl Inspector for InspectorStack { [&mut self.tracer, &mut self.log_collector, &mut self.cheatcodes, &mut self.printer], |inspector| { inspector.log(evm_data, address, topics, data); - } + None + }, + self, + evm_data ); } @@ -442,10 +492,13 @@ impl Inspector for InspectorStack { // Allow inspectors to exit early if interpreter.instruction_result != res { - #[allow(clippy::needless_return)] - return; + Some(()) + } else { + None } - } + }, + self, + data ); } @@ -454,6 +507,11 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { + // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. + // Avoid processing twice. + if self.in_inner_context && data.journaled_state.depth == 0 { + return (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()); + } call_inspectors!( [ &mut self.fuzzer, @@ -468,12 +526,67 @@ impl Inspector for InspectorStack { let (status, gas, retdata) = inspector.call(data, call); // Allow inspectors to exit early - #[allow(clippy::needless_return)] if status != InstructionResult::Continue { - return (status, gas, retdata); + Some((status, gas, retdata)) + } else { + None } - } + }, + self, + data ); + if !call.is_static && !self.in_inner_context && data.journaled_state.depth == 1 { + data.db.commit(data.journaled_state.state.clone()); + let nonce = data + .journaled_state + .load_account(call.context.caller, data.db) + .expect("failed to load caller") + .0 + .info + .nonce; + let mut env = Env { + cfg: data.env.cfg.clone(), + block: BlockEnv { + basefee: U256::ZERO, + gas_limit: U256::MAX, + ..data.env.block.clone() + }, + tx: TxEnv { + caller: call.context.caller, + transact_to: revm::primitives::TransactTo::Call(call.contract), + data: call.input.clone(), + value: call.transfer.value, + gas_limit: call.gas_limit, + nonce: Some(nonce), + gas_price: U256::ZERO, + ..data.env.tx.clone() + }, + }; + self.in_inner_context = true; + let res = evm_inner(&mut env, data.db, Some(self)) + .transact() + .expect("error while transacting"); + self.in_inner_context = false; + let mut gas = Gas::new(call.gas_limit); + + merge_states(&mut data.journaled_state.state, res.state); + + match res.result { + ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { + gas.set_refund(gas_refunded as i64); + gas.record_cost(gas_used); + return (eval_to_instruction_result(reason), gas, output.into_data()); + } + ExecutionResult::Halt { reason, gas_used } => { + gas.record_cost(gas_used); + return (halt_to_instruction_result(reason), gas, Bytes::new()); + } + ExecutionResult::Revert { gas_used, output } => { + gas.record_cost(gas_used); + return (InstructionResult::Revert, gas, output); + } + } + } (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) } @@ -486,8 +599,13 @@ impl Inspector for InspectorStack { status: InstructionResult, retdata: Bytes, ) -> (InstructionResult, Gas, Bytes) { - let res = self.do_call_end(data, call, remaining_gas, status, retdata); + // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. + // Avoid processing twice. + if self.in_inner_context && data.journaled_state.depth == 0 { + return (status, remaining_gas, retdata); + } + let res = self.do_call_end(data, call, remaining_gas, status, retdata); if matches!(res.0, return_revert!()) { // Encountered a revert, since cheatcodes may have altered the evm state in such a way // that violates some constraints, e.g. `deal`, we need to manually roll back on revert @@ -505,6 +623,11 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CreateInputs, ) -> (InstructionResult, Option
, Gas, Bytes) { + // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. + // Avoid processing twice. + if self.in_inner_context && data.journaled_state.depth == 0 { + return (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()); + } call_inspectors!( [ &mut self.debugger, @@ -519,10 +642,71 @@ impl Inspector for InspectorStack { // Allow inspectors to exit early if status != InstructionResult::Continue { - return (status, addr, gas, retdata); + Some((status, addr, gas, retdata)) + } else { + None } - } + }, + self, + data ); + if !self.in_inner_context && data.journaled_state.depth == 1 { + data.db.commit(data.journaled_state.state.clone()); + let nonce = data + .journaled_state + .load_account(call.caller, data.db) + .expect("failed to load caller") + .0 + .info + .nonce; + let mut env = Env { + cfg: data.env.cfg.clone(), + block: BlockEnv { + basefee: U256::ZERO, + gas_limit: U256::MAX, + ..data.env.block.clone() + }, + tx: TxEnv { + caller: call.caller, + transact_to: revm::primitives::TransactTo::Create(call.scheme), + data: call.init_code.clone(), + value: call.value, + gas_limit: call.gas_limit, + nonce: Some(nonce), + gas_price: U256::ZERO, + ..data.env.tx.clone() + }, + }; + + self.in_inner_context = true; + let res = evm_inner(&mut env, data.db, Some(self)) + .transact() + .expect("error while transacting"); + self.in_inner_context = false; + let mut gas = Gas::new(call.gas_limit); + + merge_states(&mut data.journaled_state.state, res.state); + + match res.result { + ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { + gas.set_refund(gas_refunded as i64); + gas.record_cost(gas_used); + let address = match output { + Output::Create(_, address) => address, + _ => unreachable!(), + }; + return (eval_to_instruction_result(reason), address, gas, output.into_data()); + } + ExecutionResult::Halt { reason, gas_used } => { + gas.record_cost(gas_used); + return (halt_to_instruction_result(reason), None, gas, Bytes::new()); + } + ExecutionResult::Revert { gas_used, output } => { + gas.record_cost(gas_used); + return (InstructionResult::Revert, None, gas, output); + } + } + } (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) } @@ -536,6 +720,11 @@ impl Inspector for InspectorStack { remaining_gas: Gas, retdata: Bytes, ) -> (InstructionResult, Option
, Gas, Bytes) { + // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. + // Avoid processing twice. + if self.in_inner_context && data.journaled_state.depth == 0 { + return (status, address, remaining_gas, retdata); + } call_inspectors!( [ &mut self.debugger, @@ -556,9 +745,13 @@ impl Inspector for InspectorStack { ); if new_status != status { - return (new_status, new_address, new_gas, new_retdata); + Some((new_status, new_address, new_gas, new_retdata)) + } else { + None } - } + }, + self, + data ); (status, address, remaining_gas, retdata) @@ -576,6 +769,7 @@ impl Inspector for InspectorStack { ], |inspector| { Inspector::::selfdestruct(inspector, contract, target, value); + None } ); } diff --git a/testdata/cheats/Broadcast.t.sol b/testdata/cheats/Broadcast.t.sol index d542c28c0e6c..7b68c6c99201 100644 --- a/testdata/cheats/Broadcast.t.sol +++ b/testdata/cheats/Broadcast.t.sol @@ -123,7 +123,7 @@ contract BroadcastTest is DSTest { vm.broadcast(ACCOUNT_B); // will trigger a transaction from B - payable(ACCOUNT_A).transfer(2); + payable(ACCOUNT_A).call{value: 2}(""); vm.broadcast(ACCOUNT_B); // will trigger a transaction From 3bf3b3cdfb6a8702757b3dbc8f41b35274080980 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 20 Feb 2024 15:09:45 +0400 Subject: [PATCH 02/24] small fixes --- crates/evm/evm/src/inspectors/stack.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 8f0cff2ecba2..7949b03201cc 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -181,6 +181,7 @@ impl InspectorStackBuilder { macro_rules! call_inspectors { ([$($inspector:expr),+ $(,)?], |$id:ident $(,)?| $call:expr $(,)?) => {{$( if let Some($id) = $inspector { + // Allow inspector to exit early if let Some(result) = $call { return result; } @@ -404,7 +405,7 @@ impl InspectorStack { } } -impl Inspector for InspectorStack { +impl Inspector for InspectorStack { fn initialize_interp(&mut self, interpreter: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { let res = interpreter.instruction_result; call_inspectors!( From 12ae30b5fad5750fa4009195d273ffbd59a48bc1 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 20 Feb 2024 16:36:51 +0400 Subject: [PATCH 03/24] don't panic on transaction error + fixture fix --- crates/evm/evm/src/inspectors/stack.rs | 16 ++++++++++------ .../fixtures/can_use_libs_in_multi_fork.stdout | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 7949b03201cc..68d0a20b07ad 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -564,9 +564,12 @@ impl Inspector for InspectorStack { }, }; self.in_inner_context = true; - let res = evm_inner(&mut env, data.db, Some(self)) - .transact() - .expect("error while transacting"); + + let Ok(res) = evm_inner(&mut env, data.db, Some(self)).transact() else { + self.in_inner_context = false; + return (InstructionResult::Revert, Gas::new(call.gas_limit), Bytes::new()); + }; + self.in_inner_context = false; let mut gas = Gas::new(call.gas_limit); @@ -680,9 +683,10 @@ impl Inspector for InspectorStack { }; self.in_inner_context = true; - let res = evm_inner(&mut env, data.db, Some(self)) - .transact() - .expect("error while transacting"); + let Ok(res) = evm_inner(&mut env, data.db, Some(self)).transact() else { + self.in_inner_context = false; + return (InstructionResult::Revert, None, Gas::new(call.gas_limit), Bytes::new()); + }; self.in_inner_context = false; let mut gas = Gas::new(call.gas_limit); diff --git a/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout b/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout index 5a5b2f621811..28094421442b 100644 --- a/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout +++ b/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout @@ -3,7 +3,7 @@ Solc 0.8.23 finished in 1.95s Compiler run successful! Ran 1 test for test/Contract.t.sol:ContractTest -[PASS] test() (gas: 70360) +[PASS] test() (gas: 127104) Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.21s Ran 1 test suite: 1 tests passed, 0 failed, 0 skipped (1 total tests) From 2f942048e43f2fcac1cdb73a23d1b85b4882d27b Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 20 Feb 2024 16:57:26 +0400 Subject: [PATCH 04/24] stricter call scheme check --- crates/evm/evm/src/inspectors/stack.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 68d0a20b07ad..a068979da502 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -14,7 +14,8 @@ use foundry_evm_traces::CallTraceArena; use revm::{ evm_inner, interpreter::{ - return_revert, CallInputs, CreateInputs, Gas, InstructionResult, Interpreter, Stack, + return_revert, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, + Stack, }, primitives::{BlockEnv, Env, ExecutionResult, Output, State, TxEnv}, DatabaseCommit, EVMData, Inspector, @@ -536,7 +537,10 @@ impl Inspector for InspectorStack { self, data ); - if !call.is_static && !self.in_inner_context && data.journaled_state.depth == 1 { + if call.context.scheme == CallScheme::Call && + !self.in_inner_context && + data.journaled_state.depth == 1 + { data.db.commit(data.journaled_state.state.clone()); let nonce = data .journaled_state From 759a7215a9526707a7c342253ba07870f22cef69 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 20 Feb 2024 19:41:53 +0400 Subject: [PATCH 05/24] refactor and more fixes --- crates/evm/evm/src/inspectors/stack.rs | 198 ++++++++---------- .../tests/fixtures/can_test_repeatedly.stdout | 2 +- 2 files changed, 89 insertions(+), 111 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index a068979da502..d0c495ebf1e4 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -17,7 +17,7 @@ use revm::{ return_revert, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, Stack, }, - primitives::{BlockEnv, Env, ExecutionResult, Output, State, TxEnv}, + primitives::{BlockEnv, Env, ExecutionResult, Output, State, TransactTo}, DatabaseCommit, EVMData, Inspector, }; use std::{collections::HashMap, sync::Arc}; @@ -404,6 +404,76 @@ impl InspectorStack { ); (status, remaining_gas, retdata) } + + fn transact_inner( + &mut self, + data: &mut EVMData<'_, DB>, + transact_to: TransactTo, + caller: Address, + input: Bytes, + gas_limit: u64, + value: U256, + ) -> (InstructionResult, Option
, Gas, Bytes) { + data.db.commit(data.journaled_state.state.clone()); + + let nonce = data + .journaled_state + .load_account(caller, data.db) + .expect("failed to load caller") + .0 + .info + .nonce; + + let cached_env = data.env.clone(); + + data.env.block.basefee = U256::ZERO; + data.env.block.gas_limit = U256::MAX; + data.env.tx.caller = caller; + data.env.tx.transact_to = transact_to; + data.env.tx.data = input; + data.env.tx.value = value; + data.env.tx.nonce = Some(nonce); + // Add 21000 to the gas limit to account for the base cost of transaction. + data.env.tx.gas_limit = gas_limit + 21000; + data.env.tx.gas_price = U256::ZERO; + + self.in_inner_context = true; + let res = evm_inner(data.env, data.db, Some(self)).transact(); + self.in_inner_context = false; + + data.env.tx = cached_env.tx; + data.env.block.basefee = cached_env.block.basefee; + data.env.block.gas_limit = cached_env.block.gas_limit; + + let mut gas = Gas::new(gas_limit); + + let Ok(res) = res else { + // Should we match, encode and propagate error as a revert reason? + return (InstructionResult::Revert, None, gas, Bytes::new()); + }; + + merge_states(&mut data.journaled_state.state, res.state); + + match res.result { + ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { + gas.set_refund(gas_refunded as i64); + gas.record_cost(gas_used); + let address = match output { + Output::Create(_, address) => address, + Output::Call(_) => None, + }; + (eval_to_instruction_result(reason), address, gas, output.into_data()) + } + ExecutionResult::Halt { reason, gas_used } => { + gas.record_cost(gas_used); + (halt_to_instruction_result(reason), None, gas, Bytes::new()) + } + ExecutionResult::Revert { gas_used, output } => { + gas.record_cost(gas_used); + (InstructionResult::Revert, None, gas, output) + } + } + } } impl Inspector for InspectorStack { @@ -541,59 +611,15 @@ impl Inspector for InspectorStack { !self.in_inner_context && data.journaled_state.depth == 1 { - data.db.commit(data.journaled_state.state.clone()); - let nonce = data - .journaled_state - .load_account(call.context.caller, data.db) - .expect("failed to load caller") - .0 - .info - .nonce; - let mut env = Env { - cfg: data.env.cfg.clone(), - block: BlockEnv { - basefee: U256::ZERO, - gas_limit: U256::MAX, - ..data.env.block.clone() - }, - tx: TxEnv { - caller: call.context.caller, - transact_to: revm::primitives::TransactTo::Call(call.contract), - data: call.input.clone(), - value: call.transfer.value, - gas_limit: call.gas_limit, - nonce: Some(nonce), - gas_price: U256::ZERO, - ..data.env.tx.clone() - }, - }; - self.in_inner_context = true; - - let Ok(res) = evm_inner(&mut env, data.db, Some(self)).transact() else { - self.in_inner_context = false; - return (InstructionResult::Revert, Gas::new(call.gas_limit), Bytes::new()); - }; - - self.in_inner_context = false; - let mut gas = Gas::new(call.gas_limit); - - merge_states(&mut data.journaled_state.state, res.state); - - match res.result { - ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { - gas.set_refund(gas_refunded as i64); - gas.record_cost(gas_used); - return (eval_to_instruction_result(reason), gas, output.into_data()); - } - ExecutionResult::Halt { reason, gas_used } => { - gas.record_cost(gas_used); - return (halt_to_instruction_result(reason), gas, Bytes::new()); - } - ExecutionResult::Revert { gas_used, output } => { - gas.record_cost(gas_used); - return (InstructionResult::Revert, gas, output); - } - } + let (res, _, gas, output) = self.transact_inner( + data, + TransactTo::Call(call.contract), + call.context.caller, + call.input.clone(), + call.gas_limit, + call.transfer.value, + ); + return (res, gas, output); } (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) @@ -659,62 +685,14 @@ impl Inspector for InspectorStack { data ); if !self.in_inner_context && data.journaled_state.depth == 1 { - data.db.commit(data.journaled_state.state.clone()); - let nonce = data - .journaled_state - .load_account(call.caller, data.db) - .expect("failed to load caller") - .0 - .info - .nonce; - let mut env = Env { - cfg: data.env.cfg.clone(), - block: BlockEnv { - basefee: U256::ZERO, - gas_limit: U256::MAX, - ..data.env.block.clone() - }, - tx: TxEnv { - caller: call.caller, - transact_to: revm::primitives::TransactTo::Create(call.scheme), - data: call.init_code.clone(), - value: call.value, - gas_limit: call.gas_limit, - nonce: Some(nonce), - gas_price: U256::ZERO, - ..data.env.tx.clone() - }, - }; - - self.in_inner_context = true; - let Ok(res) = evm_inner(&mut env, data.db, Some(self)).transact() else { - self.in_inner_context = false; - return (InstructionResult::Revert, None, Gas::new(call.gas_limit), Bytes::new()); - }; - self.in_inner_context = false; - let mut gas = Gas::new(call.gas_limit); - - merge_states(&mut data.journaled_state.state, res.state); - - match res.result { - ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { - gas.set_refund(gas_refunded as i64); - gas.record_cost(gas_used); - let address = match output { - Output::Create(_, address) => address, - _ => unreachable!(), - }; - return (eval_to_instruction_result(reason), address, gas, output.into_data()); - } - ExecutionResult::Halt { reason, gas_used } => { - gas.record_cost(gas_used); - return (halt_to_instruction_result(reason), None, gas, Bytes::new()); - } - ExecutionResult::Revert { gas_used, output } => { - gas.record_cost(gas_used); - return (InstructionResult::Revert, None, gas, output); - } - } + return self.transact_inner( + data, + TransactTo::Create(call.scheme), + call.caller, + call.init_code.clone(), + call.gas_limit, + call.value, + ); } (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) diff --git a/crates/forge/tests/fixtures/can_test_repeatedly.stdout b/crates/forge/tests/fixtures/can_test_repeatedly.stdout index 3d782aa35872..225191d6660f 100644 --- a/crates/forge/tests/fixtures/can_test_repeatedly.stdout +++ b/crates/forge/tests/fixtures/can_test_repeatedly.stdout @@ -2,7 +2,7 @@ No files changed, compilation skipped Ran 2 tests for test/Counter.t.sol:CounterTest [PASS] testFuzz_SetNumber(uint256) (runs: 256, μ: 26521, ~: 28387) -[PASS] test_Increment() (gas: 28379) +[PASS] test_Increment() (gas: 49443) Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 9.42ms Ran 1 test suite: 2 tests passed, 0 failed, 0 skipped (2 total tests) From 75899e6e693413496c5d689b68d6388f7c6f8378 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 21 Feb 2024 18:32:25 +0400 Subject: [PATCH 06/24] wip --- crates/cheatcodes/src/inspector.rs | 1 + crates/evm/evm/src/inspectors/stack.rs | 101 ++++++++++++++++--------- testdata/cheats/Prank.t.sol | 2 +- testdata/repros/Issue3653.t.sol | 4 +- 4 files changed, 68 insertions(+), 40 deletions(-) diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index 7b5cd07ba3a8..ccb9c550148f 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -849,6 +849,7 @@ impl Inspector for Cheatcodes { debug!(target: "cheatcodes", tx=?self.broadcastable_transactions.back().unwrap(), "broadcastable call"); let prev = account.info.nonce; + account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast.new_origin, nonce=prev+1, prev, "incremented nonce"); } else if broadcast.single_call { let msg = "`staticcall`s are not allowed after `broadcast`; use `startBroadcast` instead"; diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index d0c495ebf1e4..14da92ad650b 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -6,6 +6,7 @@ use alloy_primitives::{Address, Bytes, Log, B256, U256}; use alloy_signer::LocalWallet; use foundry_evm_core::{ backend::DatabaseExt, + constants::CHEATCODE_ADDRESS, debug::DebugArena, utils::{eval_to_instruction_result, halt_to_instruction_result}, }; @@ -427,14 +428,15 @@ impl InspectorStack { let cached_env = data.env.clone(); data.env.block.basefee = U256::ZERO; - data.env.block.gas_limit = U256::MAX; data.env.tx.caller = caller; data.env.tx.transact_to = transact_to; data.env.tx.data = input; data.env.tx.value = value; data.env.tx.nonce = Some(nonce); // Add 21000 to the gas limit to account for the base cost of transaction. - data.env.tx.gas_limit = gas_limit + 21000; + // We might have modified block gas limit earlier and revm will reject tx with gas limit > + // block gas limit, so we adjust. + data.env.tx.gas_limit = std::cmp::min(gas_limit + 21000, data.env.block.gas_limit.to()); data.env.tx.gas_price = U256::ZERO; self.in_inner_context = true; @@ -443,7 +445,6 @@ impl InspectorStack { data.env.tx = cached_env.tx; data.env.block.basefee = cached_env.block.basefee; - data.env.block.gas_limit = cached_env.block.gas_limit; let mut gas = Gas::new(gas_limit); @@ -579,16 +580,45 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { - // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. - // Avoid processing twice. - if self.in_inner_context && data.journaled_state.depth == 0 { - return (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()); + if !(self.in_inner_context && data.journaled_state.depth == 0) { + call_inspectors!( + [&mut self.tracer,], + |inspector| { + let (status, gas, retdata) = inspector.call(data, call); + + // Allow inspectors to exit early + if status != InstructionResult::Continue { + Some((status, gas, retdata)) + } else { + None + } + }, + self, + data + ); + } + + // We don't want to execute calls to cheatcodes as separate transactions because we may + // occur `selectFork` which replaces journaled state. + if call.contract != CHEATCODE_ADDRESS && + call.context.scheme == CallScheme::Call && + !self.in_inner_context && + data.journaled_state.depth == 1 + { + let (res, _, gas, output) = self.transact_inner( + data, + TransactTo::Call(call.contract), + call.context.caller, + call.input.clone(), + call.gas_limit, + call.transfer.value, + ); + return (res, gas, output); } call_inspectors!( [ &mut self.fuzzer, &mut self.debugger, - &mut self.tracer, &mut self.coverage, &mut self.log_collector, &mut self.cheatcodes, @@ -607,20 +637,6 @@ impl Inspector for InspectorStack { self, data ); - if call.context.scheme == CallScheme::Call && - !self.in_inner_context && - data.journaled_state.depth == 1 - { - let (res, _, gas, output) = self.transact_inner( - data, - TransactTo::Call(call.contract), - call.context.caller, - call.input.clone(), - call.gas_limit, - call.transfer.value, - ); - return (res, gas, output); - } (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) } @@ -657,15 +673,36 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CreateInputs, ) -> (InstructionResult, Option
, Gas, Bytes) { - // Inner context calls with depth 0 are being dispatched as top-level calls with depth 1. - // Avoid processing twice. - if self.in_inner_context && data.journaled_state.depth == 0 { - return (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()); + if !(self.in_inner_context && data.journaled_state.depth == 0) { + call_inspectors!( + [&mut self.tracer,], + |inspector| { + let (status, addr, gas, retdata) = inspector.create(data, call); + + // Allow inspectors to exit early + if status != InstructionResult::Continue { + Some((status, addr, gas, retdata)) + } else { + None + } + }, + self, + data + ); + } + if !self.in_inner_context && data.journaled_state.depth == 1 { + return self.transact_inner( + data, + TransactTo::Create(call.scheme), + call.caller, + call.init_code.clone(), + call.gas_limit, + call.value, + ); } call_inspectors!( [ &mut self.debugger, - &mut self.tracer, &mut self.coverage, &mut self.log_collector, &mut self.cheatcodes, @@ -684,16 +721,6 @@ impl Inspector for InspectorStack { self, data ); - if !self.in_inner_context && data.journaled_state.depth == 1 { - return self.transact_inner( - data, - TransactTo::Create(call.scheme), - call.caller, - call.init_code.clone(), - call.gas_limit, - call.value, - ); - } (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) } diff --git a/testdata/cheats/Prank.t.sol b/testdata/cheats/Prank.t.sol index 0e23ed7b805d..0b7e4283ab3b 100644 --- a/testdata/cheats/Prank.t.sol +++ b/testdata/cheats/Prank.t.sol @@ -274,7 +274,7 @@ contract PrankTest is DSTest { function testPrankConstructorSender(address sender) public { vm.prank(sender); ConstructorVictim victim = new ConstructorVictim( - sender, "msg.sender was not set during prank", tx.origin, "tx.origin invariant failed" + sender, "msg.sender was not set during prank", address(this), "tx.origin invariant failed" ); // Ensure we cleaned up correctly diff --git a/testdata/repros/Issue3653.t.sol b/testdata/repros/Issue3653.t.sol index 6e52c49f8aa5..5022af67859f 100644 --- a/testdata/repros/Issue3653.t.sol +++ b/testdata/repros/Issue3653.t.sol @@ -11,13 +11,13 @@ contract Issue3653Test is DSTest { Token token; constructor() { - fork = vm.createSelectFork("rpcAlias", 10); + fork = vm.createSelectFork("rpcAlias", 1000000); token = new Token(); vm.makePersistent(address(token)); } function testDummy() public { - assertEq(block.number, 10); + assertEq(block.number, 1000000); } } From 2633134df793e430a85889044f5a22bab4bcc609 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 21 Feb 2024 18:45:57 +0400 Subject: [PATCH 07/24] fix --- crates/evm/evm/src/inspectors/stack.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 872400f704ad..823e5347577b 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -3,7 +3,6 @@ use super::{ StackSnapshotType, TracePrinter, TracingInspector, TracingInspectorConfig, }; use alloy_primitives::{Address, Bytes, Log, B256, U256}; -use alloy_signer::LocalWallet; use foundry_evm_core::{ backend::DatabaseExt, constants::CHEATCODE_ADDRESS, From 986cd10735a76c085edc427b88a66700f4aee25c Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 21 Feb 2024 23:59:35 +0400 Subject: [PATCH 08/24] wip --- crates/evm/evm/src/inspectors/stack.rs | 47 ++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 823e5347577b..88728b058afe 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -14,8 +14,7 @@ use foundry_evm_traces::CallTraceArena; use revm::{ evm_inner, interpreter::{ - return_revert, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, - Stack, + return_revert, CallContext, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, Stack, Transfer }, primitives::{BlockEnv, Env, ExecutionResult, Output, State, TransactTo}, DatabaseCommit, EVMData, Inspector, @@ -246,6 +245,9 @@ pub struct InspectorStack { pub tracer: Option, /// Flag marking if we are in the inner EVM context. pub in_inner_context: bool, + pub sender: Option
, + pub needed_sender_nonce: Option, + pub original_origin: Option
, } impl InspectorStack { @@ -421,7 +423,7 @@ impl InspectorStack { data.env.block.basefee = U256::ZERO; data.env.tx.caller = caller; - data.env.tx.transact_to = transact_to; + data.env.tx.transact_to = transact_to.clone(); data.env.tx.data = input; data.env.tx.value = value; data.env.tx.nonce = Some(nonce); @@ -431,9 +433,19 @@ impl InspectorStack { data.env.tx.gas_limit = std::cmp::min(gas_limit + 21000, data.env.block.gas_limit.to()); data.env.tx.gas_price = U256::ZERO; + self.sender = Some(caller); + self.original_origin = Some(cached_env.tx.caller); + + if matches!(transact_to, TransactTo::Call(_)) { + self.needed_sender_nonce = Some(nonce); + } + self.in_inner_context = true; let res = evm_inner(data.env, data.db, Some(self)).transact(); self.in_inner_context = false; + self.needed_sender_nonce = None; + self.sender = None; + self.original_origin = None; data.env.tx = cached_env.tx; data.env.block.basefee = cached_env.block.basefee; @@ -588,6 +600,20 @@ impl Inspector for InspectorStack { self, data ); + } else { + if let (Some(sender), Some(needed_nonce)) = (self.sender, self.needed_sender_nonce) { + let account = data + .journaled_state + .state + .get_mut(&sender) + .expect("failed to load sender"); + account.info.nonce = needed_nonce; + self.needed_sender_nonce = None; + } + if let Some(original_origin) = self.original_origin { + data.env.tx.caller = original_origin; + self.original_origin = None; + } } // We don't want to execute calls to cheatcodes as separate transactions because we may @@ -681,7 +707,22 @@ impl Inspector for InspectorStack { self, data ); + } else { + if let (Some(sender), Some(needed_nonce)) = (self.sender, self.needed_sender_nonce) { + let account = data + .journaled_state + .state + .get_mut(&sender) + .expect("failed to load sender"); + account.info.nonce = needed_nonce; + self.needed_sender_nonce = None; + } + if let Some(original_origin) = self.original_origin { + data.env.tx.caller = original_origin; + self.original_origin = None; + } } + if !self.in_inner_context && data.journaled_state.depth == 1 { return self.transact_inner( data, From 9f10bae9a40fa64d14b0bc99e9b1a8555dd13a8d Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 02:26:15 +0400 Subject: [PATCH 09/24] wip --- crates/cheatcodes/src/inspector.rs | 8 +- crates/evm/evm/src/inspectors/stack.rs | 139 ++++++++++--------------- testdata/cheats/Prank.t.sol | 2 +- 3 files changed, 63 insertions(+), 86 deletions(-) diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index af0528a243af..76d2242a91ee 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -849,7 +849,6 @@ impl Inspector for Cheatcodes { debug!(target: "cheatcodes", tx=?self.broadcastable_transactions.back().unwrap(), "broadcastable call"); let prev = account.info.nonce; - account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast.new_origin, nonce=prev+1, prev, "incremented nonce"); } else if broadcast.single_call { let msg = "`staticcall`s are not allowed after `broadcast`; use `startBroadcast` instead"; @@ -939,6 +938,13 @@ impl Inspector for Cheatcodes { if data.journaled_state.depth() == broadcast.depth { data.env.tx.caller = broadcast.original_origin; + if !call.is_static { + let account = + data.journaled_state.state().get_mut(&broadcast.new_origin).unwrap(); + + account.info.nonce += 1; + } + // Clean single-call broadcast once we have returned to the original depth if broadcast.single_call { let _ = self.broadcast.take(); diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 88728b058afe..6eff6482c35c 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -14,7 +14,8 @@ use foundry_evm_traces::CallTraceArena; use revm::{ evm_inner, interpreter::{ - return_revert, CallContext, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, Stack, Transfer + return_revert, CallInputs, CallScheme, CreateInputs, Gas, InstructionResult, Interpreter, + Stack, }, primitives::{BlockEnv, Env, ExecutionResult, Output, State, TransactTo}, DatabaseCommit, EVMData, Inspector, @@ -229,6 +230,14 @@ pub struct InspectorData { pub chisel_state: Option<(Stack, Vec, InstructionResult)>, } +#[derive(Debug, Clone)] +pub struct InnerContextData { + sender: Address, + original_origin: Address, + origin_sender_nonce: u64, + is_create: bool, +} + /// An inspector that calls multiple inspectors in sequence. /// /// If a call to an inspector returns a value other than [InstructionResult::Continue] (or @@ -245,9 +254,7 @@ pub struct InspectorStack { pub tracer: Option, /// Flag marking if we are in the inner EVM context. pub in_inner_context: bool, - pub sender: Option
, - pub needed_sender_nonce: Option, - pub original_origin: Option
, + pub inner_context_data: Option, } impl InspectorStack { @@ -433,19 +440,17 @@ impl InspectorStack { data.env.tx.gas_limit = std::cmp::min(gas_limit + 21000, data.env.block.gas_limit.to()); data.env.tx.gas_price = U256::ZERO; - self.sender = Some(caller); - self.original_origin = Some(cached_env.tx.caller); - - if matches!(transact_to, TransactTo::Call(_)) { - self.needed_sender_nonce = Some(nonce); - } + self.inner_context_data = Some(InnerContextData { + sender: data.env.tx.caller, + original_origin: cached_env.tx.caller, + origin_sender_nonce: nonce, + is_create: matches!(transact_to, TransactTo::Create(_)), + }); self.in_inner_context = true; let res = evm_inner(data.env, data.db, Some(self)).transact(); self.in_inner_context = false; - self.needed_sender_nonce = None; - self.sender = None; - self.original_origin = None; + self.inner_context_data = None; data.env.tx = cached_env.tx; data.env.block.basefee = cached_env.block.basefee; @@ -479,6 +484,24 @@ impl InspectorStack { } } } + + /// Adjusts the EVM data for the inner EVM context. + /// Should be called on the top-level call of inner context (depth == 0 && + /// self.in_inner_context) Decreases sender nonce for CALLs to keep backwards compatibility + /// Updates tx.origin to the value before entering inner context + fn adjust_evm_data_for_inner_context(&mut self, data: &mut EVMData<'_, DB>) { + let inner_context_data = + self.inner_context_data.as_ref().expect("should be called in inner context"); + let sender_acc = data + .journaled_state + .state + .get_mut(&inner_context_data.sender) + .expect("failed to load sender"); + if !inner_context_data.is_create { + sender_acc.info.nonce = inner_context_data.origin_sender_nonce; + } + data.env.tx.caller = inner_context_data.original_origin; + } } impl Inspector for InspectorStack { @@ -586,7 +609,15 @@ impl Inspector for InspectorStack { ) -> (InstructionResult, Gas, Bytes) { if !(self.in_inner_context && data.journaled_state.depth == 0) { call_inspectors!( - [&mut self.tracer,], + [ + &mut self.fuzzer, + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.log_collector, + &mut self.cheatcodes, + &mut self.printer + ], |inspector| { let (status, gas, retdata) = inspector.call(data, call); @@ -601,19 +632,7 @@ impl Inspector for InspectorStack { data ); } else { - if let (Some(sender), Some(needed_nonce)) = (self.sender, self.needed_sender_nonce) { - let account = data - .journaled_state - .state - .get_mut(&sender) - .expect("failed to load sender"); - account.info.nonce = needed_nonce; - self.needed_sender_nonce = None; - } - if let Some(original_origin) = self.original_origin { - data.env.tx.caller = original_origin; - self.original_origin = None; - } + self.adjust_evm_data_for_inner_context(data); } // We don't want to execute calls to cheatcodes as separate transactions because we may @@ -633,28 +652,6 @@ impl Inspector for InspectorStack { ); return (res, gas, output); } - call_inspectors!( - [ - &mut self.fuzzer, - &mut self.debugger, - &mut self.coverage, - &mut self.log_collector, - &mut self.cheatcodes, - &mut self.printer - ], - |inspector| { - let (status, gas, retdata) = inspector.call(data, call); - - // Allow inspectors to exit early - if status != InstructionResult::Continue { - Some((status, gas, retdata)) - } else { - None - } - }, - self, - data - ); (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) } @@ -693,7 +690,14 @@ impl Inspector for InspectorStack { ) -> (InstructionResult, Option
, Gas, Bytes) { if !(self.in_inner_context && data.journaled_state.depth == 0) { call_inspectors!( - [&mut self.tracer,], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.log_collector, + &mut self.cheatcodes, + &mut self.printer + ], |inspector| { let (status, addr, gas, retdata) = inspector.create(data, call); @@ -708,19 +712,7 @@ impl Inspector for InspectorStack { data ); } else { - if let (Some(sender), Some(needed_nonce)) = (self.sender, self.needed_sender_nonce) { - let account = data - .journaled_state - .state - .get_mut(&sender) - .expect("failed to load sender"); - account.info.nonce = needed_nonce; - self.needed_sender_nonce = None; - } - if let Some(original_origin) = self.original_origin { - data.env.tx.caller = original_origin; - self.original_origin = None; - } + self.adjust_evm_data_for_inner_context(data); } if !self.in_inner_context && data.journaled_state.depth == 1 { @@ -733,27 +725,6 @@ impl Inspector for InspectorStack { call.value, ); } - call_inspectors!( - [ - &mut self.debugger, - &mut self.coverage, - &mut self.log_collector, - &mut self.cheatcodes, - &mut self.printer - ], - |inspector| { - let (status, addr, gas, retdata) = inspector.create(data, call); - - // Allow inspectors to exit early - if status != InstructionResult::Continue { - Some((status, addr, gas, retdata)) - } else { - None - } - }, - self, - data - ); (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) } diff --git a/testdata/cheats/Prank.t.sol b/testdata/cheats/Prank.t.sol index 0b7e4283ab3b..0e23ed7b805d 100644 --- a/testdata/cheats/Prank.t.sol +++ b/testdata/cheats/Prank.t.sol @@ -274,7 +274,7 @@ contract PrankTest is DSTest { function testPrankConstructorSender(address sender) public { vm.prank(sender); ConstructorVictim victim = new ConstructorVictim( - sender, "msg.sender was not set during prank", address(this), "tx.origin invariant failed" + sender, "msg.sender was not set during prank", tx.origin, "tx.origin invariant failed" ); // Ensure we cleaned up correctly From 408fb7fa16b45c18a8be6ed06ef5f492482ed934 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 13:41:52 +0400 Subject: [PATCH 10/24] rm cheatcodes check --- crates/evm/evm/src/inspectors/stack.rs | 5 +---- testdata/cheats/Broadcast.t.sol | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 6eff6482c35c..65f89d602cbd 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -635,10 +635,7 @@ impl Inspector for InspectorStack { self.adjust_evm_data_for_inner_context(data); } - // We don't want to execute calls to cheatcodes as separate transactions because we may - // occur `selectFork` which replaces journaled state. - if call.contract != CHEATCODE_ADDRESS && - call.context.scheme == CallScheme::Call && + if call.context.scheme == CallScheme::Call && !self.in_inner_context && data.journaled_state.depth == 1 { diff --git a/testdata/cheats/Broadcast.t.sol b/testdata/cheats/Broadcast.t.sol index 7b68c6c99201..d542c28c0e6c 100644 --- a/testdata/cheats/Broadcast.t.sol +++ b/testdata/cheats/Broadcast.t.sol @@ -123,7 +123,7 @@ contract BroadcastTest is DSTest { vm.broadcast(ACCOUNT_B); // will trigger a transaction from B - payable(ACCOUNT_A).call{value: 2}(""); + payable(ACCOUNT_A).transfer(2); vm.broadcast(ACCOUNT_B); // will trigger a transaction From f8f5107f6d751e0edc06299a5a7ba3c3efcdd40c Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 14:05:11 +0400 Subject: [PATCH 11/24] clippy --- crates/evm/evm/src/inspectors/stack.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 65f89d602cbd..1b3145ba3404 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -5,7 +5,6 @@ use super::{ use alloy_primitives::{Address, Bytes, Log, B256, U256}; use foundry_evm_core::{ backend::DatabaseExt, - constants::CHEATCODE_ADDRESS, debug::DebugArena, utils::{eval_to_instruction_result, halt_to_instruction_result}, }; From 130b29874fc4939bdd385d06e0e0b8a60639e09f Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 18:50:21 +0400 Subject: [PATCH 12/24] update commit logic --- crates/evm/evm/src/inspectors/stack.rs | 45 +++++++++++++++++--------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 1b3145ba3404..7f2f5b0c6789 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -205,15 +205,12 @@ macro_rules! call_inspectors { } } -fn merge_states(main_state: &mut State, new_state: State) { - for (addr, acc) in new_state { - if main_state.contains_key(&addr) { - let acc_mut = main_state.get_mut(&addr).unwrap(); - acc_mut.status |= acc.status; - acc_mut.info = acc.info; - acc_mut.storage.extend(acc.storage); - } else { - main_state.insert(addr, acc); +/// Helper method which updates data in the state with the data from the database. +fn update_state(state: &mut State, db: &mut DB) { + for (addr, acc) in state.iter_mut() { + acc.info = db.basic(*addr).unwrap().unwrap_or_default(); + for (key, val) in acc.storage.iter_mut() { + val.present_value = db.storage(*addr, *key).unwrap(); } } } @@ -233,7 +230,7 @@ pub struct InspectorData { pub struct InnerContextData { sender: Address, original_origin: Address, - origin_sender_nonce: u64, + original_sender_nonce: u64, is_create: bool, } @@ -442,10 +439,9 @@ impl InspectorStack { self.inner_context_data = Some(InnerContextData { sender: data.env.tx.caller, original_origin: cached_env.tx.caller, - origin_sender_nonce: nonce, + original_sender_nonce: nonce, is_create: matches!(transact_to, TransactTo::Create(_)), }); - self.in_inner_context = true; let res = evm_inner(data.env, data.db, Some(self)).transact(); self.in_inner_context = false; @@ -456,12 +452,31 @@ impl InspectorStack { let mut gas = Gas::new(gas_limit); - let Ok(res) = res else { + let Ok(mut res) = res else { // Should we match, encode and propagate error as a revert reason? return (InstructionResult::Revert, None, gas, Bytes::new()); }; - merge_states(&mut data.journaled_state.state, res.state); + // Commit changes after transaction + data.db.commit(res.state.clone()); + + // Update both states with new DB data after commit. + update_state(&mut data.journaled_state.state, data.db); + update_state(&mut res.state, data.db); + + // Merge transaction journal into the active journal. + for (addr, acc) in res.state { + if let Some(acc_mut) = data.journaled_state.state.get_mut(&addr) { + acc_mut.status |= acc.status; + for (key, val) in acc.storage { + if !acc_mut.storage.contains_key(&key) { + acc_mut.storage.insert(key, val); + } + } + } else { + data.journaled_state.state.insert(addr, acc); + } + } match res.result { ExecutionResult::Success { reason, gas_used, gas_refunded, logs: _, output } => { @@ -497,7 +512,7 @@ impl InspectorStack { .get_mut(&inner_context_data.sender) .expect("failed to load sender"); if !inner_context_data.is_create { - sender_acc.info.nonce = inner_context_data.origin_sender_nonce; + sender_acc.info.nonce = inner_context_data.original_sender_nonce; } data.env.tx.caller = inner_context_data.original_origin; } From 934fb038dd3a0ac71354bdacd8ac92070605bf25 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 21:06:57 +0400 Subject: [PATCH 13/24] opt-in --- crates/evm/evm/src/inspectors/stack.rs | 25 +++++++++++++++++-- .../tests/fixtures/can_test_repeatedly.stdout | 2 +- .../can_use_libs_in_multi_fork.stdout | 2 +- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 7f2f5b0c6789..c22860d95093 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -50,6 +50,8 @@ pub struct InspectorStackBuilder { pub print: Option, /// The chisel state inspector. pub chisel_state: Option, + /// Whether to enable call isolation. + pub enable_isolation: bool, } impl InspectorStackBuilder { @@ -129,6 +131,13 @@ impl InspectorStackBuilder { self } + /// Set whether to enable the call isolation. + #[inline] + pub fn enable_isolation(mut self, yes: bool) -> Self { + self.enable_isolation = yes; + self + } + /// Builds the stack of inspectors to use when transacting/committing on the EVM. /// /// See also [`revm::Evm::inspect_ref`] and [`revm::Evm::commit_ref`]. @@ -144,6 +153,7 @@ impl InspectorStackBuilder { coverage, print, chisel_state, + enable_isolation, } = self; let mut stack = InspectorStack::new(); @@ -163,6 +173,8 @@ impl InspectorStackBuilder { stack.print(print.unwrap_or(false)); stack.tracing(trace.unwrap_or(false)); + stack.enable_isolation(enable_isolation); + // environment, must come after all of the inspectors if let Some(block) = block { stack.set_block(&block); @@ -248,6 +260,8 @@ pub struct InspectorStack { pub log_collector: Option, pub printer: Option, pub tracer: Option, + pub enable_isolation: bool, + /// Flag marking if we are in the inner EVM context. pub in_inner_context: bool, pub inner_context_data: Option, @@ -317,6 +331,12 @@ impl InspectorStack { self.debugger = yes.then(Default::default); } + /// Set whether to enable call isolation. + #[inline] + pub fn enable_isolation(&mut self, yes: bool) { + self.enable_isolation = yes; + } + /// Set whether to enable the log collector. #[inline] pub fn collect_logs(&mut self, yes: bool) { @@ -649,7 +669,8 @@ impl Inspector for InspectorStack { self.adjust_evm_data_for_inner_context(data); } - if call.context.scheme == CallScheme::Call && + if self.enable_isolation && + call.context.scheme == CallScheme::Call && !self.in_inner_context && data.journaled_state.depth == 1 { @@ -726,7 +747,7 @@ impl Inspector for InspectorStack { self.adjust_evm_data_for_inner_context(data); } - if !self.in_inner_context && data.journaled_state.depth == 1 { + if self.enable_isolation && !self.in_inner_context && data.journaled_state.depth == 1 { return self.transact_inner( data, TransactTo::Create(call.scheme), diff --git a/crates/forge/tests/fixtures/can_test_repeatedly.stdout b/crates/forge/tests/fixtures/can_test_repeatedly.stdout index 225191d6660f..3d782aa35872 100644 --- a/crates/forge/tests/fixtures/can_test_repeatedly.stdout +++ b/crates/forge/tests/fixtures/can_test_repeatedly.stdout @@ -2,7 +2,7 @@ No files changed, compilation skipped Ran 2 tests for test/Counter.t.sol:CounterTest [PASS] testFuzz_SetNumber(uint256) (runs: 256, μ: 26521, ~: 28387) -[PASS] test_Increment() (gas: 49443) +[PASS] test_Increment() (gas: 28379) Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 9.42ms Ran 1 test suite: 2 tests passed, 0 failed, 0 skipped (2 total tests) diff --git a/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout b/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout index 28094421442b..5a5b2f621811 100644 --- a/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout +++ b/crates/forge/tests/fixtures/can_use_libs_in_multi_fork.stdout @@ -3,7 +3,7 @@ Solc 0.8.23 finished in 1.95s Compiler run successful! Ran 1 test for test/Contract.t.sol:ContractTest -[PASS] test() (gas: 127104) +[PASS] test() (gas: 70360) Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.21s Ran 1 test suite: 1 tests passed, 0 failed, 0 skipped (1 total tests) From af052a57373d573414fae238d6be9f942b98a903 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 22 Feb 2024 21:35:46 +0400 Subject: [PATCH 14/24] enable in gas reports --- crates/forge/bin/cmd/test/mod.rs | 1 + crates/forge/src/gas_report.rs | 11 +++++++++++ crates/forge/src/multi_runner.rs | 11 +++++++++++ 3 files changed, 23 insertions(+) diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index 4f0f9f29940a..456eec19fb0f 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -197,6 +197,7 @@ impl TestArgs { .with_fork(evm_opts.get_fork(&config, env.clone())) .with_cheats_config(CheatsConfig::new(&config, evm_opts.clone(), None)) .with_test_options(test_options.clone()) + .enable_isolation(self.gas_report) .build(project_root, output, env, evm_opts)?; if let Some(debug_test_pattern) = &self.debug { diff --git a/crates/forge/src/gas_report.rs b/crates/forge/src/gas_report.rs index afe41f76dfa5..f6c269d7029e 100644 --- a/crates/forge/src/gas_report.rs +++ b/crates/forge/src/gas_report.rs @@ -7,6 +7,7 @@ use crate::{ }; use comfy_table::{presets::ASCII_MARKDOWN, *}; use foundry_common::{calc, TestFunctionExt}; +use foundry_evm::traces::CallKind; use serde::{Deserialize, Serialize}; use std::{collections::BTreeMap, fmt::Display}; @@ -75,6 +76,16 @@ impl GasReport { return; } + // Only include top-level calls which accout for calldata and base (21.000) cost. + // Only include Calls and Creates as only these calls are isolated in inspector. + if trace.depth != 1 && + (trace.kind == CallKind::Call || + trace.kind == CallKind::Create || + trace.kind == CallKind::Create2) + { + return; + } + let decoded = decoder.decode_function(&node.trace).await; let Some(name) = &decoded.contract else { return }; diff --git a/crates/forge/src/multi_runner.rs b/crates/forge/src/multi_runner.rs index 0651e82d6184..699df0da0566 100644 --- a/crates/forge/src/multi_runner.rs +++ b/crates/forge/src/multi_runner.rs @@ -60,6 +60,8 @@ pub struct MultiContractRunner { pub debug: bool, /// Settings related to fuzz and/or invariant tests pub test_options: TestOptions, + /// Whether to enable call isolation + pub isolation: bool, } impl MultiContractRunner { @@ -179,6 +181,7 @@ impl MultiContractRunner { .trace(self.evm_opts.verbosity >= 3 || self.debug) .debug(self.debug) .coverage(self.coverage) + .enable_isolation(self.isolation) }) .spec(self.evm_spec) .gas_limit(self.evm_opts.gas_limit()) @@ -256,6 +259,8 @@ pub struct MultiContractRunnerBuilder { pub coverage: bool, /// Whether or not to collect debug info pub debug: bool, + /// Whether to enable call isolation + pub isolation: bool, /// Settings related to fuzz and/or invariant tests pub test_options: Option, } @@ -301,6 +306,11 @@ impl MultiContractRunnerBuilder { self } + pub fn enable_isolation(mut self, enable: bool) -> Self { + self.isolation = enable; + self + } + /// Given an EVM, proceeds to return a runner which is able to execute all tests /// against that evm pub fn build( @@ -381,6 +391,7 @@ impl MultiContractRunnerBuilder { coverage: self.coverage, debug: self.debug, test_options: self.test_options.unwrap_or_default(), + isolation: self.isolation, }) } } From fb64f4fdca759c311c221dc54ec83945db2e3664 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Fri, 23 Feb 2024 14:06:20 +0400 Subject: [PATCH 15/24] --isolate --- crates/common/src/evm.rs | 9 +++++++++ crates/config/src/lib.rs | 6 ++++++ crates/evm/core/src/opts.rs | 3 +++ crates/forge/bin/cmd/script/executor.rs | 17 ++++++++++------- crates/forge/bin/cmd/test/mod.rs | 7 ++++++- crates/forge/tests/cli/config.rs | 1 + 6 files changed, 35 insertions(+), 8 deletions(-) diff --git a/crates/common/src/evm.rs b/crates/common/src/evm.rs index 2080951abb4a..3835945bef2e 100644 --- a/crates/common/src/evm.rs +++ b/crates/common/src/evm.rs @@ -139,6 +139,11 @@ pub struct EvmArgs { #[clap(flatten)] #[serde(flatten)] pub env: EnvArgs, + + /// Whether to enable isolation of calls. + #[clap(long)] + #[serde(skip)] + pub isolate: bool, } // Make this set of options a `figment::Provider` so that it can be merged into the `Config` @@ -161,6 +166,10 @@ impl Provider for EvmArgs { dict.insert("ffi".to_string(), self.ffi.into()); } + if self.isolate { + dict.insert("isolate".to_string(), self.isolate.into()); + } + if self.no_storage_caching { dict.insert("no_storage_caching".to_string(), self.no_storage_caching.into()); } diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 879d3ecfb7df..55d53ada9e76 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -372,6 +372,11 @@ pub struct Config { /// Should be removed once EvmVersion Cancun is supported by solc pub cancun: bool, + /// Whether to enable call isolation. + /// + /// Useful for more correct gas accounting and EVM behavior in general. + pub isolate: bool, + /// Address labels pub labels: HashMap, @@ -1808,6 +1813,7 @@ impl Default for Config { profile: Self::DEFAULT_PROFILE, fs_permissions: FsPermissions::new([PathPermission::read("out")]), cancun: false, + isolate: false, __root: Default::default(), src: "src".into(), test: "test".into(), diff --git a/crates/evm/core/src/opts.rs b/crates/evm/core/src/opts.rs index 094606828dfd..52c4f1a7d563 100644 --- a/crates/evm/core/src/opts.rs +++ b/crates/evm/core/src/opts.rs @@ -58,6 +58,9 @@ pub struct EvmOpts { /// The memory limit per EVM execution in bytes. /// If this limit is exceeded, a `MemoryLimitOOG` result is thrown. pub memory_limit: u64, + + /// Whether to enable isolation of calls. + pub isolate: bool, } impl EvmOpts { diff --git a/crates/forge/bin/cmd/script/executor.rs b/crates/forge/bin/cmd/script/executor.rs index e0f8ed8ece1b..071cbcbb219d 100644 --- a/crates/forge/bin/cmd/script/executor.rs +++ b/crates/forge/bin/cmd/script/executor.rs @@ -303,14 +303,17 @@ impl ScriptArgs { if let SimulationStage::Local = stage { builder = builder.inspectors(|stack| { - stack.debug(self.debug).cheatcodes( - CheatsConfig::new( - &script_config.config, - script_config.evm_opts.clone(), - script_wallets, + stack + .debug(self.debug) + .cheatcodes( + CheatsConfig::new( + &script_config.config, + script_config.evm_opts.clone(), + script_wallets, + ) + .into(), ) - .into(), - ) + .enable_isolation(script_config.evm_opts.isolate) }); } diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index 456eec19fb0f..f8e779e5c640 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -143,6 +143,11 @@ impl TestArgs { // Merge all configs let (mut config, mut evm_opts) = self.load_config_and_evm_opts_emit_warnings()?; + // Explicitly enable isolation for gas reports for more correct gas accounting + if self.gas_report { + evm_opts.isolate = true; + } + // Set up the project. let mut project = config.project()?; @@ -197,7 +202,7 @@ impl TestArgs { .with_fork(evm_opts.get_fork(&config, env.clone())) .with_cheats_config(CheatsConfig::new(&config, evm_opts.clone(), None)) .with_test_options(test_options.clone()) - .enable_isolation(self.gas_report) + .enable_isolation(self.evm_opts.isolate) .build(project_root, output, env, evm_opts)?; if let Some(debug_test_pattern) = &self.debug { diff --git a/crates/forge/tests/cli/config.rs b/crates/forge/tests/cli/config.rs index 0c540d65a4fc..da60e981dcb9 100644 --- a/crates/forge/tests/cli/config.rs +++ b/crates/forge/tests/cli/config.rs @@ -119,6 +119,7 @@ forgetest!(can_extract_config_values, |prj, cmd| { fs_permissions: Default::default(), labels: Default::default(), cancun: true, + isolate: true, __non_exhaustive: (), __warnings: vec![], }; From 225840934bf0f75325ab0b27338465b35cb315c2 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Fri, 23 Feb 2024 14:33:00 +0400 Subject: [PATCH 16/24] isolation tests --- crates/forge/tests/cli/test_cmd.rs | 83 ++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/crates/forge/tests/cli/test_cmd.rs b/crates/forge/tests/cli/test_cmd.rs index 25a30a1fd040..49e5e2000477 100644 --- a/crates/forge/tests/cli/test_cmd.rs +++ b/crates/forge/tests/cli/test_cmd.rs @@ -432,3 +432,86 @@ contract CustomTypesTest is Test { .join("tests/fixtures/include_custom_types_in_traces.stdout"), ); }); + +forgetest_init!(can_test_selfdestruct_with_isolation, |prj, cmd| { + prj.wipe_contracts(); + + prj.add_test( + "Contract.t.sol", + r#" +import {Test} from "forge-std/Test.sol"; + +contract Destructing { + function destruct() public { + selfdestruct(payable(address(0))); + } +} + +contract SelfDestructTest is Test { + function test() public { + Destructing d = new Destructing(); + vm.store(address(d), bytes32(0), bytes32(uint256(1))); + d.destruct(); + assertEq(address(d).code.length, 0); + assertEq(vm.load(address(d), bytes32(0)), bytes32(0)); + } +} + "#, + ) + .unwrap(); + + cmd.args(["test", "-vvvv", "--isolate"]).assert_success(); +}); + +forgetest_init!(can_test_transient_storage_with_isolation, |prj, cmd| { + prj.wipe_contracts(); + + prj.add_test( + "Contract.t.sol", + r#"pragma solidity 0.8.24; +import {Test} from "forge-std/Test.sol"; + +contract TransientTester { + function locked() public view returns (bool isLocked) { + assembly { + isLocked := tload(0) + } + } + + modifier lock() { + require(!locked(), "locked"); + assembly { + tstore(0, 1) + } + _; + } + + function maybeReentrant(address target, bytes memory data) public lock { + (bool success, bytes memory ret) = target.call(data); + if (!success) { + // forwards revert reason + assembly { + let ret_size := mload(ret) + revert(add(32, ret), ret_size) + } + } + } +} + +contract TransientTest is Test { + function test() public { + TransientTester t = new TransientTester(); + vm.expectRevert(bytes("locked")); + t.maybeReentrant(address(t), abi.encodeCall(TransientTester.maybeReentrant, (address(0), new bytes(0)))); + + t.maybeReentrant(address(0), new bytes(0)); + assertEq(t.locked(), false); + } +} + + "#, + ) + .unwrap(); + + cmd.args(["test", "-vvvv", "--isolate", "--evm-version", "cancun"]).assert_success(); +}); From 87032a0881c720190f244b7ad27907dddeb98d15 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Fri, 23 Feb 2024 16:55:38 +0400 Subject: [PATCH 17/24] smaller diff --- crates/anvil/src/eth/backend/mem/inspector.rs | 26 +++++------ crates/evm/evm/src/inspectors/stack.rs | 44 ++++++++++--------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/crates/anvil/src/eth/backend/mem/inspector.rs b/crates/anvil/src/eth/backend/mem/inspector.rs index 7bc6e0d0afe7..531cebcd9ab5 100644 --- a/crates/anvil/src/eth/backend/mem/inspector.rs +++ b/crates/anvil/src/eth/backend/mem/inspector.rs @@ -51,7 +51,6 @@ impl revm::Inspector for Inspector { fn initialize_interp(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.initialize_interp(interp, data); - None }); } @@ -59,7 +58,6 @@ impl revm::Inspector for Inspector { fn step(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.step(interp, data); - None }); } @@ -71,17 +69,18 @@ impl revm::Inspector for Inspector { topics: &[B256], data: &Bytes, ) { - call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { - inspector.log(evm_data, address, topics, data); - None - }); + call_inspectors!( + [&mut self.tracer, Some(&mut self.log_collector)], + |inspector| { + inspector.log(evm_data, address, topics, data); + } + ); } #[inline] fn step_end(&mut self, interp: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { call_inspectors!([&mut self.tracer], |inspector| { inspector.step_end(interp, data); - None }); } @@ -91,10 +90,12 @@ impl revm::Inspector for Inspector { data: &mut EVMData<'_, DB>, call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { - call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { - inspector.call(data, call); - None - }); + call_inspectors!( + [&mut self.tracer, Some(&mut self.log_collector)], + |inspector| { + inspector.call(data, call); + } + ); (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) } @@ -110,7 +111,6 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.call_end(data, inputs, remaining_gas, ret, out.clone()); - None }); (ret, remaining_gas, out) } @@ -123,7 +123,6 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Option
, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.create(data, call); - None }); (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()) @@ -141,7 +140,6 @@ impl revm::Inspector for Inspector { ) -> (InstructionResult, Option
, Gas, Bytes) { call_inspectors!([&mut self.tracer], |inspector| { inspector.create_end(data, inputs, status, address, gas, retdata.clone()); - None }); (status, address, gas, retdata) } diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index c22860d95093..286566c2e384 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -193,24 +193,27 @@ impl InspectorStackBuilder { macro_rules! call_inspectors { ([$($inspector:expr),+ $(,)?], |$id:ident $(,)?| $call:expr $(,)?) => {{$( if let Some($id) = $inspector { - // Allow inspector to exit early - if let Some(result) = $call { - return result; - } + $call } - )+}}; + )+}} +} + +/// Same as [call_inspectors] macro, but with depth adjustment for isolated execution. +macro_rules! call_inspectors_adjust_depth { ([$($inspector:expr),+ $(,)?], |$id:ident $(,)?| $call:expr, $self:ident, $data:ident $(,)?) => { if $self.in_inner_context { $data.journaled_state.depth += 1; } - call_inspectors!([$($inspector),+], |$id| { - $call.map(|result| { - if $self.in_inner_context { - $data.journaled_state.depth -= 1; + {$( + if let Some($id) = $inspector { + if let Some(result) = $call { + if $self.in_inner_context { + $data.journaled_state.depth -= 1; + } + return result; } - result - }) - }); + } + )+} if $self.in_inner_context { $data.journaled_state.depth -= 1; } @@ -393,7 +396,7 @@ impl InspectorStack { status: InstructionResult, retdata: Bytes, ) -> (InstructionResult, Gas, Bytes) { - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.fuzzer, &mut self.debugger, @@ -541,7 +544,7 @@ impl InspectorStack { impl Inspector for InspectorStack { fn initialize_interp(&mut self, interpreter: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { let res = interpreter.instruction_result; - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.debugger, &mut self.coverage, @@ -567,7 +570,7 @@ impl Inspector for InspectorStack { fn step(&mut self, interpreter: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { let res = interpreter.instruction_result; - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.fuzzer, &mut self.debugger, @@ -599,7 +602,7 @@ impl Inspector for InspectorStack { topics: &[B256], data: &Bytes, ) { - call_inspectors!( + call_inspectors_adjust_depth!( [&mut self.tracer, &mut self.log_collector, &mut self.cheatcodes, &mut self.printer], |inspector| { inspector.log(evm_data, address, topics, data); @@ -612,7 +615,7 @@ impl Inspector for InspectorStack { fn step_end(&mut self, interpreter: &mut Interpreter<'_>, data: &mut EVMData<'_, DB>) { let res = interpreter.instruction_result; - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.debugger, &mut self.tracer, @@ -642,7 +645,7 @@ impl Inspector for InspectorStack { call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { if !(self.in_inner_context && data.journaled_state.depth == 0) { - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.fuzzer, &mut self.debugger, @@ -721,7 +724,7 @@ impl Inspector for InspectorStack { call: &mut CreateInputs, ) -> (InstructionResult, Option
, Gas, Bytes) { if !(self.in_inner_context && data.journaled_state.depth == 0) { - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.debugger, &mut self.tracer, @@ -775,7 +778,7 @@ impl Inspector for InspectorStack { if self.in_inner_context && data.journaled_state.depth == 0 { return (status, address, remaining_gas, retdata); } - call_inspectors!( + call_inspectors_adjust_depth!( [ &mut self.debugger, &mut self.tracer, @@ -819,7 +822,6 @@ impl Inspector for InspectorStack { ], |inspector| { Inspector::::selfdestruct(inspector, contract, target, value); - None } ); } From 71a4d7456f222978f7f2d721c986cd6b254c8eae Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Fri, 23 Feb 2024 17:00:55 +0400 Subject: [PATCH 18/24] fmt --- crates/anvil/src/eth/backend/mem/inspector.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/crates/anvil/src/eth/backend/mem/inspector.rs b/crates/anvil/src/eth/backend/mem/inspector.rs index 531cebcd9ab5..6072e4f938b7 100644 --- a/crates/anvil/src/eth/backend/mem/inspector.rs +++ b/crates/anvil/src/eth/backend/mem/inspector.rs @@ -69,12 +69,9 @@ impl revm::Inspector for Inspector { topics: &[B256], data: &Bytes, ) { - call_inspectors!( - [&mut self.tracer, Some(&mut self.log_collector)], - |inspector| { - inspector.log(evm_data, address, topics, data); - } - ); + call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { + inspector.log(evm_data, address, topics, data); + }); } #[inline] @@ -90,12 +87,9 @@ impl revm::Inspector for Inspector { data: &mut EVMData<'_, DB>, call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { - call_inspectors!( - [&mut self.tracer, Some(&mut self.log_collector)], - |inspector| { - inspector.call(data, call); - } - ); + call_inspectors!([&mut self.tracer, Some(&mut self.log_collector)], |inspector| { + inspector.call(data, call); + }); (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()) } From 5c4a191727541fc65b23a9b9244fba0ebc58b2ac Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 26 Feb 2024 17:29:30 +0400 Subject: [PATCH 19/24] simplify logic --- crates/evm/evm/src/inspectors/stack.rs | 100 +++++++++++++------------ 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 286566c2e384..e4acb3f51232 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -644,34 +644,35 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CallInputs, ) -> (InstructionResult, Gas, Bytes) { - if !(self.in_inner_context && data.journaled_state.depth == 0) { - call_inspectors_adjust_depth!( - [ - &mut self.fuzzer, - &mut self.debugger, - &mut self.tracer, - &mut self.coverage, - &mut self.log_collector, - &mut self.cheatcodes, - &mut self.printer - ], - |inspector| { - let (status, gas, retdata) = inspector.call(data, call); - - // Allow inspectors to exit early - if status != InstructionResult::Continue { - Some((status, gas, retdata)) - } else { - None - } - }, - self, - data - ); - } else { + if self.in_inner_context && data.journaled_state.depth == 0 { self.adjust_evm_data_for_inner_context(data); + return (InstructionResult::Continue, Gas::new(call.gas_limit), Bytes::new()); } + call_inspectors_adjust_depth!( + [ + &mut self.fuzzer, + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.log_collector, + &mut self.cheatcodes, + &mut self.printer + ], + |inspector| { + let (status, gas, retdata) = inspector.call(data, call); + + // Allow inspectors to exit early + if status != InstructionResult::Continue { + Some((status, gas, retdata)) + } else { + None + } + }, + self, + data + ); + if self.enable_isolation && call.context.scheme == CallScheme::Call && !self.in_inner_context && @@ -723,33 +724,34 @@ impl Inspector for InspectorStack { data: &mut EVMData<'_, DB>, call: &mut CreateInputs, ) -> (InstructionResult, Option
, Gas, Bytes) { - if !(self.in_inner_context && data.journaled_state.depth == 0) { - call_inspectors_adjust_depth!( - [ - &mut self.debugger, - &mut self.tracer, - &mut self.coverage, - &mut self.log_collector, - &mut self.cheatcodes, - &mut self.printer - ], - |inspector| { - let (status, addr, gas, retdata) = inspector.create(data, call); - - // Allow inspectors to exit early - if status != InstructionResult::Continue { - Some((status, addr, gas, retdata)) - } else { - None - } - }, - self, - data - ); - } else { + if self.in_inner_context && data.journaled_state.depth == 0 { self.adjust_evm_data_for_inner_context(data); + return (InstructionResult::Continue, None, Gas::new(call.gas_limit), Bytes::new()); } + call_inspectors_adjust_depth!( + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.log_collector, + &mut self.cheatcodes, + &mut self.printer + ], + |inspector| { + let (status, addr, gas, retdata) = inspector.create(data, call); + + // Allow inspectors to exit early + if status != InstructionResult::Continue { + Some((status, addr, gas, retdata)) + } else { + None + } + }, + self, + data + ); + if self.enable_isolation && !self.in_inner_context && data.journaled_state.depth == 1 { return self.transact_inner( data, From 313a57f3731fdd1ddf2b901244150cf85fb9a8b4 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 26 Feb 2024 19:01:52 +0400 Subject: [PATCH 20/24] docs --- crates/common/src/evm.rs | 2 ++ crates/evm/evm/src/inspectors/stack.rs | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/common/src/evm.rs b/crates/common/src/evm.rs index af07d51f40c3..2230bd760874 100644 --- a/crates/common/src/evm.rs +++ b/crates/common/src/evm.rs @@ -146,6 +146,8 @@ pub struct EvmArgs { pub env: EnvArgs, /// Whether to enable isolation of calls. + /// In isolation mode all top-level calls are executed as a separate transaction in a separate + /// EVM context, enabling more precise gas accounting and transaction state changes. #[clap(long)] #[serde(skip)] pub isolate: bool, diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index e4acb3f51232..7eab3fdd75dd 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -51,6 +51,8 @@ pub struct InspectorStackBuilder { /// The chisel state inspector. pub chisel_state: Option, /// Whether to enable call isolation. + /// In isolation mode all top-level calls are executed as a separate transaction in a separate + /// EVM context, enabling more precise gas accounting and transaction state changes. pub enable_isolation: bool, } @@ -132,6 +134,7 @@ impl InspectorStackBuilder { } /// Set whether to enable the call isolation. + /// For description of call isolation, see [`InspectorStack::enable_isolation`]. #[inline] pub fn enable_isolation(mut self, yes: bool) -> Self { self.enable_isolation = yes; @@ -241,11 +244,21 @@ pub struct InspectorData { pub chisel_state: Option<(Stack, Vec, InstructionResult)>, } +/// Contains data about the state of outer/main EVM which created and invoked the inner EVM context. +/// Used to adjust EVM state while in inner context. +/// +/// We need this to avoid breaking changes due to EVM behavior differences in isolated vs non-isolated mode. +/// For descriptions and workarounds for those changes see: https://github.com/foundry-rs/foundry/pull/7186#issuecomment-1959102195 #[derive(Debug, Clone)] pub struct InnerContextData { + /// The sender of the inner EVM context. + /// It is also an origin of the transaction that created the inner EVM context. sender: Address, - original_origin: Address, + /// Nonce of the sender before invocation of the inner EVM context. original_sender_nonce: u64, + /// Origin of the transaction in the outer EVM context. + original_origin: Address, + /// Whether the inner context was created by a CREATE transaction. is_create: bool, } From 18692090367470b1f4fb05530b69c60736ea1d70 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 26 Feb 2024 19:10:42 +0400 Subject: [PATCH 21/24] fmt --- crates/evm/evm/src/inspectors/stack.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index 7eab3fdd75dd..2a66ac76280d 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -247,8 +247,8 @@ pub struct InspectorData { /// Contains data about the state of outer/main EVM which created and invoked the inner EVM context. /// Used to adjust EVM state while in inner context. /// -/// We need this to avoid breaking changes due to EVM behavior differences in isolated vs non-isolated mode. -/// For descriptions and workarounds for those changes see: https://github.com/foundry-rs/foundry/pull/7186#issuecomment-1959102195 +/// We need this to avoid breaking changes due to EVM behavior differences in isolated vs +/// non-isolated mode. For descriptions and workarounds for those changes see: https://github.com/foundry-rs/foundry/pull/7186#issuecomment-1959102195 #[derive(Debug, Clone)] pub struct InnerContextData { /// The sender of the inner EVM context. From 18acadd34479c68292fdd01da519adc106fed55d Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 28 Feb 2024 03:45:55 +0400 Subject: [PATCH 22/24] enable isolation properly for --gas-report --- crates/forge/bin/cmd/test/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index f8e779e5c640..1b41f41361a2 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -202,7 +202,7 @@ impl TestArgs { .with_fork(evm_opts.get_fork(&config, env.clone())) .with_cheats_config(CheatsConfig::new(&config, evm_opts.clone(), None)) .with_test_options(test_options.clone()) - .enable_isolation(self.evm_opts.isolate) + .enable_isolation(evm_opts.isolate) .build(project_root, output, env, evm_opts)?; if let Some(debug_test_pattern) = &self.debug { From 33814e54f4a6cc1614ed7e06f042124ab51ada0a Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 28 Feb 2024 04:51:08 +0400 Subject: [PATCH 23/24] change nonce incrementing --- crates/cheatcodes/src/inspector.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index ef3995c38e70..d1c5fbb0dbca 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -830,6 +830,8 @@ impl Inspector for Cheatcodes { let account = data.journaled_state.state().get_mut(&broadcast.new_origin).unwrap(); + account.mark_touch(); + self.broadcastable_transactions.push_back(BroadcastableTransaction { rpc: data.db.active_fork_url(), transaction: TransactionRequest { @@ -849,6 +851,7 @@ impl Inspector for Cheatcodes { debug!(target: "cheatcodes", tx=?self.broadcastable_transactions.back().unwrap(), "broadcastable call"); let prev = account.info.nonce; + account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast.new_origin, nonce=prev+1, prev, "incremented nonce"); } else if broadcast.single_call { let msg = "`staticcall`s are not allowed after `broadcast`; use `startBroadcast` instead"; @@ -938,13 +941,6 @@ impl Inspector for Cheatcodes { if data.journaled_state.depth() == broadcast.depth { data.env.tx.caller = broadcast.original_origin; - if !call.is_static { - let account = - data.journaled_state.state().get_mut(&broadcast.new_origin).unwrap(); - - account.info.nonce += 1; - } - // Clean single-call broadcast once we have returned to the original depth if broadcast.single_call { let _ = self.broadcast.take(); @@ -1490,6 +1486,7 @@ fn process_broadcast_create( // We have to increment the nonce of the user address, since this create2 will be done // by the create2_deployer let account = data.journaled_state.state().get_mut(&broadcast_sender).unwrap(); + account.mark_touch(); let prev = account.info.nonce; account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast_sender, nonce=prev+1, prev, "incremented nonce in create2"); From 96b9688b38a5a8d6520fddf2c6a77e85e91f443a Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Wed, 28 Feb 2024 05:17:50 +0400 Subject: [PATCH 24/24] document why we touch --- crates/cheatcodes/src/inspector.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index d1c5fbb0dbca..8334df8b9561 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -830,8 +830,6 @@ impl Inspector for Cheatcodes { let account = data.journaled_state.state().get_mut(&broadcast.new_origin).unwrap(); - account.mark_touch(); - self.broadcastable_transactions.push_back(BroadcastableTransaction { rpc: data.db.active_fork_url(), transaction: TransactionRequest { @@ -851,6 +849,9 @@ impl Inspector for Cheatcodes { debug!(target: "cheatcodes", tx=?self.broadcastable_transactions.back().unwrap(), "broadcastable call"); let prev = account.info.nonce; + + // Touch account to ensure that incremented nonce is committed + account.mark_touch(); account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast.new_origin, nonce=prev+1, prev, "incremented nonce"); } else if broadcast.single_call { @@ -1486,8 +1487,9 @@ fn process_broadcast_create( // We have to increment the nonce of the user address, since this create2 will be done // by the create2_deployer let account = data.journaled_state.state().get_mut(&broadcast_sender).unwrap(); - account.mark_touch(); let prev = account.info.nonce; + // Touch account to ensure that incremented nonce is committed + account.mark_touch(); account.info.nonce += 1; debug!(target: "cheatcodes", address=%broadcast_sender, nonce=prev+1, prev, "incremented nonce in create2"); // Proxy deployer requires the data to be `salt ++ init_code`