Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: don't create a backend unnecessarily in ensure_success #7429

Merged
merged 2 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading