Skip to content

Commit

Permalink
chore: don't create a backend unnecessarily in ensure_success (#7429)
Browse files Browse the repository at this point in the history
* chore: don't create a backend unnecessarily in ensure_success

* chore: cow it up
  • Loading branch information
DaniPopes authored Mar 18, 2024
1 parent 3865e57 commit 71ad565
Show file tree
Hide file tree
Showing 7 changed files with 48 additions and 49 deletions.
19 changes: 10 additions & 9 deletions crates/evm/evm/src/executors/fuzz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -208,19 +208,16 @@ impl FuzzedExecutor {
should_fail: bool,
calldata: alloy_primitives::Bytes,
) -> Result<FuzzOutcome, TestCaseError> {
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,
);
Expand All @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions crates/evm/evm/src/executors/invariant/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
);
Expand Down
15 changes: 7 additions & 8 deletions crates/evm/evm/src/executors/invariant/funcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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() {
Expand Down
13 changes: 6 additions & 7 deletions crates/evm/evm/src/executors/invariant/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
41 changes: 21 additions & 20 deletions crates/evm/evm/src/executors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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 {
Expand All @@ -511,31 +511,32 @@ impl Executor {
&self,
address: Address,
reverted: bool,
state_changeset: StateChangeset,
state_changeset: Cow<'_, StateChangeset>,
should_fail: bool,
) -> Result<bool, DatabaseError> {
if self.backend.has_snapshot_failure() {
// a failure occurred in a reverted snapshot, which is considered a failed test
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 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.
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.into_owned());

// Check if a DSTest assertion failed
let executor =
Executor::new(backend, self.env.clone(), self.inspector.clone(), self.gas_limit);
Expand Down
2 changes: 0 additions & 2 deletions crates/evm/fuzz/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)")]
Expand Down
3 changes: 2 additions & 1 deletion crates/forge/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use foundry_evm::{
use proptest::test_runner::TestRunner;
use rayon::prelude::*;
use std::{
borrow::Cow,
collections::{BTreeMap, HashMap},
time::Instant,
};
Expand Down Expand Up @@ -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,
);

Expand Down

0 comments on commit 71ad565

Please sign in to comment.