From d7af2a8ca4b63aba60b996951cb550ef1141f993 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Mon, 18 Mar 2024 17:41:07 +0100 Subject: [PATCH 1/2] chore: don't create a backend unnecessarily in ensure_success --- crates/evm/evm/src/executors/mod.rs | 31 +++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/crates/evm/evm/src/executors/mod.rs b/crates/evm/evm/src/executors/mod.rs index 469d6b6e4a01..63d976b6f393 100644 --- a/crates/evm/evm/src/executors/mod.rs +++ b/crates/evm/evm/src/executors/mod.rs @@ -519,23 +519,24 @@ impl Executor { return Ok(should_fail) } - // Construct a new VM with the state changeset - let mut backend = self.backend.clone_empty(); - - // we only clone the test contract and cheatcode accounts, that's all we need to evaluate - // success - for addr in [address, CHEATCODE_ADDRESS] { - let acc = self.backend.basic_ref(addr)?.unwrap_or_default(); - backend.insert_account_info(addr, acc); - } - - // If this test failed any asserts, then this changeset will contain changes `false -> true` - // for the contract's `failed` variable and the `globalFailure` flag in the state of the - // cheatcode address which are both read when we call `"failed()(bool)"` in the next step - backend.commit(state_changeset); - let mut success = !reverted; if success { + // Construct a new VM with the state changeset + let mut backend = self.backend.clone_empty(); + + // we only clone the test contract and cheatcode accounts, that's all we need to + // evaluate success + for addr in [address, CHEATCODE_ADDRESS] { + let acc = self.backend.basic_ref(addr)?.unwrap_or_default(); + backend.insert_account_info(addr, acc); + } + + // If this test failed any asserts, then this changeset will contain changes `false -> + // true` for the contract's `failed` variable and the `globalFailure` flag + // in the state of the cheatcode address which are both read when we call + // `"failed()(bool)"` in the next step + backend.commit(state_changeset); + // Check if a DSTest assertion failed let executor = Executor::new(backend, self.env.clone(), self.inspector.clone(), self.gas_limit); From 708eb93e42dece0bf5b0cbb3fd5e7dca1a02696d Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Mon, 18 Mar 2024 18:04:57 +0100 Subject: [PATCH 2/2] chore: cow it up --- crates/evm/evm/src/executors/fuzz/mod.rs | 19 +++++++------- .../evm/evm/src/executors/invariant/error.rs | 4 +-- .../evm/evm/src/executors/invariant/funcs.rs | 15 +++++------ crates/evm/evm/src/executors/invariant/mod.rs | 13 +++++----- crates/evm/evm/src/executors/mod.rs | 26 +++++++++---------- crates/evm/fuzz/src/error.rs | 2 -- crates/forge/src/runner.rs | 3 ++- 7 files changed, 40 insertions(+), 42 deletions(-) diff --git a/crates/evm/evm/src/executors/fuzz/mod.rs b/crates/evm/evm/src/executors/fuzz/mod.rs index ec980c01ae3c..dfb7b6c6050d 100644 --- a/crates/evm/evm/src/executors/fuzz/mod.rs +++ b/crates/evm/evm/src/executors/fuzz/mod.rs @@ -18,7 +18,7 @@ use foundry_evm_fuzz::{ }; use foundry_evm_traces::CallTraceArena; use proptest::test_runner::{TestCaseError, TestError, TestRunner}; -use std::cell::RefCell; +use std::{borrow::Cow, cell::RefCell}; mod types; pub use types::{CaseOutcome, CounterExampleOutcome, FuzzOutcome}; @@ -208,19 +208,16 @@ impl FuzzedExecutor { should_fail: bool, calldata: alloy_primitives::Bytes, ) -> Result { - let call = self + let mut call = self .executor .call_raw(self.sender, address, calldata.clone(), U256::ZERO) .map_err(|_| TestCaseError::fail(FuzzError::FailedContractCall))?; - let state_changeset = call - .state_changeset - .as_ref() - .ok_or_else(|| TestCaseError::fail(FuzzError::EmptyChangeset))?; + let state_changeset = call.state_changeset.take().unwrap(); // Build fuzzer state collect_state_from_call( &call.logs, - state_changeset, + &state_changeset, state.clone(), &self.config.dictionary, ); @@ -235,8 +232,12 @@ impl FuzzedExecutor { .as_ref() .map_or_else(Default::default, |cheats| cheats.breakpoints.clone()); - let success = - self.executor.is_raw_call_success(address, state_changeset.clone(), &call, should_fail); + let success = self.executor.is_raw_call_success( + address, + Cow::Owned(state_changeset), + &call, + should_fail, + ); if success { Ok(FuzzOutcome::Case(CaseOutcome { diff --git a/crates/evm/evm/src/executors/invariant/error.rs b/crates/evm/evm/src/executors/invariant/error.rs index da29e657e0ef..47ebc877687f 100644 --- a/crates/evm/evm/src/executors/invariant/error.rs +++ b/crates/evm/evm/src/executors/invariant/error.rs @@ -13,7 +13,7 @@ use proptest::test_runner::TestError; use rand::{seq, thread_rng, Rng}; use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; use revm::primitives::U256; -use std::sync::Arc; +use std::{borrow::Cow, sync::Arc}; /// Stores information about failures and reverts of the invariant tests. #[derive(Clone, Default)] @@ -244,7 +244,7 @@ impl FailedInvariantCaseData { .expect("bad call to evm"); let is_success = executor.is_raw_call_success( self.addr, - call_result.state_changeset.take().unwrap(), + Cow::Owned(call_result.state_changeset.take().unwrap()), &call_result, false, ); diff --git a/crates/evm/evm/src/executors/invariant/funcs.rs b/crates/evm/evm/src/executors/invariant/funcs.rs index 237b4dad84ec..7f4694cdb0cb 100644 --- a/crates/evm/evm/src/executors/invariant/funcs.rs +++ b/crates/evm/evm/src/executors/invariant/funcs.rs @@ -9,6 +9,7 @@ use foundry_evm_coverage::HitMaps; use foundry_evm_fuzz::invariant::{BasicTxDetails, InvariantContract}; use foundry_evm_traces::{load_contracts, TraceKind, Traces}; use revm::primitives::U256; +use std::borrow::Cow; /// Given the executor state, asserts that no invariant has been broken. Otherwise, it fills the /// external `invariant_failures.failed_invariant` map and returns a generic error. @@ -39,14 +40,12 @@ pub fn assert_invariants( ) .expect("EVM error"); - // This will panic and get caught by the executor - let is_err = call_result.reverted || - !executor.is_raw_call_success( - invariant_contract.address, - call_result.state_changeset.take().expect("we should have a state changeset"), - &call_result, - false, - ); + let is_err = !executor.is_raw_call_success( + invariant_contract.address, + Cow::Owned(call_result.state_changeset.take().unwrap()), + &call_result, + false, + ); if is_err { // We only care about invariants which we haven't broken yet. if invariant_failures.error.is_none() { diff --git a/crates/evm/evm/src/executors/invariant/mod.rs b/crates/evm/evm/src/executors/invariant/mod.rs index 591fa7cf444d..00877a00dc54 100644 --- a/crates/evm/evm/src/executors/invariant/mod.rs +++ b/crates/evm/evm/src/executors/invariant/mod.rs @@ -30,7 +30,7 @@ use proptest::{ test_runner::{TestCaseError, TestRunner}, }; use revm::{primitives::HashMap, DatabaseCommit}; -use std::{cell::RefCell, collections::BTreeMap, sync::Arc}; +use std::{borrow::Cow, cell::RefCell, collections::BTreeMap, sync::Arc}; mod error; pub use error::{InvariantFailures, InvariantFuzzError, InvariantFuzzTestResult}; @@ -236,7 +236,7 @@ impl<'a> InvariantExecutor<'a> { &inputs, &mut failures.borrow_mut(), &targeted_contracts, - state_changeset, + &state_changeset, self.config.fail_on_revert, self.config.shrink_sequence, self.config.shrink_run_limit, @@ -772,7 +772,7 @@ fn can_continue( calldata: &[BasicTxDetails], failures: &mut InvariantFailures, targeted_contracts: &FuzzRunIdentifiedContracts, - state_changeset: StateChangeset, + state_changeset: &StateChangeset, fail_on_revert: bool, shrink_sequence: bool, shrink_run_limit: usize, @@ -781,10 +781,9 @@ fn can_continue( let mut call_results = None; // Detect handler assertion failures first. - let handlers_failed = targeted_contracts - .lock() - .iter() - .any(|contract| !executor.is_success(*contract.0, false, state_changeset.clone(), false)); + let handlers_failed = targeted_contracts.lock().iter().any(|contract| { + !executor.is_success(*contract.0, false, Cow::Borrowed(state_changeset), false) + }); // Assert invariants IFF the call did not revert and the handlers did not fail. if !call_result.reverted && !handlers_failed { diff --git a/crates/evm/evm/src/executors/mod.rs b/crates/evm/evm/src/executors/mod.rs index 63d976b6f393..51048002d49a 100644 --- a/crates/evm/evm/src/executors/mod.rs +++ b/crates/evm/evm/src/executors/mod.rs @@ -32,7 +32,7 @@ use revm::{ SpecId, TransactTo, TxEnv, }, }; -use std::collections::HashMap; +use std::{borrow::Cow, collections::HashMap}; mod builder; pub use builder::ExecutorBuilder; @@ -197,7 +197,7 @@ impl Executor { match res.state_changeset.as_ref() { Some(changeset) => { let success = self - .ensure_success(to, res.reverted, changeset.clone(), false) + .ensure_success(to, res.reverted, Cow::Borrowed(changeset), false) .map_err(|err| EvmError::Eyre(eyre::eyre!(err.to_string())))?; if success { Ok(res) @@ -472,7 +472,7 @@ impl Executor { &self, address: Address, reverted: bool, - state_changeset: StateChangeset, + state_changeset: Cow<'_, StateChangeset>, should_fail: bool, ) -> bool { self.ensure_success(address, reverted, state_changeset, should_fail).unwrap_or_default() @@ -496,7 +496,7 @@ impl Executor { pub fn is_raw_call_success( &self, address: Address, - state_changeset: StateChangeset, + state_changeset: Cow<'_, StateChangeset>, call_result: &RawCallResult, should_fail: bool, ) -> bool { @@ -511,7 +511,7 @@ impl Executor { &self, address: Address, reverted: bool, - state_changeset: StateChangeset, + state_changeset: Cow<'_, StateChangeset>, should_fail: bool, ) -> Result { if self.backend.has_snapshot_failure() { @@ -521,21 +521,21 @@ impl Executor { let mut success = !reverted; if success { - // Construct a new VM with the state changeset + // Construct a new bare-bones backend to evaluate success. let mut backend = self.backend.clone_empty(); - // we only clone the test contract and cheatcode accounts, that's all we need to - // evaluate success + // We only clone the test contract and cheatcode accounts, + // that's all we need to evaluate success. for addr in [address, CHEATCODE_ADDRESS] { let acc = self.backend.basic_ref(addr)?.unwrap_or_default(); backend.insert_account_info(addr, acc); } - // If this test failed any asserts, then this changeset will contain changes `false -> - // true` for the contract's `failed` variable and the `globalFailure` flag - // in the state of the cheatcode address which are both read when we call - // `"failed()(bool)"` in the next step - backend.commit(state_changeset); + // If this test failed any asserts, then this changeset will contain changes + // `false -> true` for the contract's `failed` variable and the `globalFailure` flag + // in the state of the cheatcode address, + // which are both read when we call `"failed()(bool)"` in the next step. + backend.commit(state_changeset.into_owned()); // Check if a DSTest assertion failed let executor = diff --git a/crates/evm/fuzz/src/error.rs b/crates/evm/fuzz/src/error.rs index 145afa5e7274..1371f49692ad 100644 --- a/crates/evm/fuzz/src/error.rs +++ b/crates/evm/fuzz/src/error.rs @@ -8,8 +8,6 @@ pub enum FuzzError { UnknownContract, #[error("Failed contract call")] FailedContractCall, - #[error("Empty state changeset")] - EmptyChangeset, #[error("`vm.assume` reject")] AssumeReject, #[error("The `vm.assume` cheatcode rejected too many inputs ({0} allowed)")] diff --git a/crates/forge/src/runner.rs b/crates/forge/src/runner.rs index e8dba45a234c..9ee3cf6897a9 100644 --- a/crates/forge/src/runner.rs +++ b/crates/forge/src/runner.rs @@ -28,6 +28,7 @@ use foundry_evm::{ use proptest::test_runner::TestRunner; use rayon::prelude::*; use std::{ + borrow::Cow, collections::{BTreeMap, HashMap}, time::Instant, }; @@ -415,7 +416,7 @@ impl<'a> ContractRunner<'a> { let success = executor.is_success( setup.address, reverted, - state_changeset.expect("we should have a state changeset"), + Cow::Owned(state_changeset.unwrap()), should_fail, );