feat: use non atomic tracker for snapshot reverts

crates/evm/core/src/backend/fuzz.rs

@@ -39,16 +39,11 @@ pub struct FuzzBackendWrapper<'a> {
     pub backend: Cow<'a, Backend>,
     /// Keeps track of whether the backed is already initialized
     is_initialized: bool,
-    /// Keeps track of whether there was a snapshot failure.
-    ///
-    /// Necessary as the backend is dropped after usage, but we'll need to persist
-    /// the snapshot failure anyhow.
-    has_snapshot_failure: bool,
 }
 
 impl<'a> FuzzBackendWrapper<'a> {
     pub fn new(backend: &'a Backend) -> Self {
-        Self { backend: Cow::Borrowed(backend), is_initialized: false, has_snapshot_failure: false }
+        Self { backend: Cow::Borrowed(backend), is_initialized: false }
     }
 
     /// Executes the configured transaction of the `env` without committing state changes
@@ -73,14 +68,7 @@ impl<'a> FuzzBackendWrapper<'a> {
     ///
     /// This is bubbled up from the underlying Copy-On-Write backend when a revert occurs.
     pub fn has_snapshot_failure(&self) -> bool {
-        self.has_snapshot_failure
-    }
-
-    /// Sets whether there was a snapshot failure in the fuzz backend.
-    ///
-    /// This is bubbled up from the underlying Copy-On-Write backend when a revert occurs.
-    pub fn set_snapshot_failure(&mut self, has_snapshot_failure: bool) {
-        self.has_snapshot_failure = has_snapshot_failure;
+        self.backend.has_snapshot_failure()
     }
 
     /// Returns a mutable instance of the Backend.
@@ -110,11 +98,7 @@ impl<'a> DatabaseExt for FuzzBackendWrapper<'a> {
         current: &mut Env,
     ) -> Option<JournaledState> {
         trace!(?id, "fuzz: revert snapshot");
-        let journaled_state = self.backend_mut(current).revert(id, journaled_state, current);
-        // Persist the snapshot failure in the fuzz backend, as the underlying backend state is lost
-        // after the call.
-        self.set_snapshot_failure(self.has_snapshot_failure || self.backend.has_snapshot_failure());
-        journaled_state
+        self.backend_mut(current).revert(id, journaled_state, current)
     }
 
     fn create_fork(&mut self, fork: CreateFork) -> eyre::Result<LocalForkId> {

crates/evm/core/src/backend/mod.rs

@@ -26,13 +26,7 @@ use revm::{
     },
     Database, DatabaseCommit, Inspector, JournaledState, EVM,
 };
-use std::{
-    collections::{HashMap, HashSet},
-    sync::{
-        atomic::{AtomicBool, Ordering},
-        Arc,
-    },
-};
+use std::collections::{HashMap, HashSet};
 
 mod diagnostic;
 pub use diagnostic::RevertDiagnostic;
@@ -568,11 +562,12 @@ impl Backend {
     ///
     /// This returns whether there was a reverted snapshot that recorded an error
     pub fn has_snapshot_failure(&self) -> bool {
-        self.inner.has_snapshot_failure.load(Ordering::Relaxed)
+        self.inner.has_snapshot_failure
     }
 
-    pub fn set_snapshot_failure(&self, has_snapshot_failure: bool) {
-        self.inner.has_snapshot_failure.store(has_snapshot_failure, Ordering::Relaxed);
+    /// Sets the snapshot failure flag.
+    pub fn set_snapshot_failure(&mut self, has_snapshot_failure: bool) {
+        self.inner.has_snapshot_failure = has_snapshot_failure
     }
 
     /// Checks if the test contract associated with this backend failed, See
@@ -913,7 +908,7 @@ impl DatabaseExt for Backend {
             // need to check whether there's a global failure which means an error occurred either
             // during the snapshot or even before
             if self.is_global_failure(current_state) {
-                self.inner.has_snapshot_failure.store(true, Ordering::Relaxed);
+                self.set_snapshot_failure(true);
             }
 
             // merge additional logs
@@ -1514,7 +1509,7 @@ pub struct BackendInner {
     /// reverted we get the _current_ `revm::JournaledState` which contains the state that we can
     /// check if the `_failed` variable is set,
     /// additionally
-    pub has_snapshot_failure: Arc<AtomicBool>,
+    pub has_snapshot_failure: bool,
     /// Tracks the address of a Test contract
     ///
     /// This address can be used to inspect the state of the contract when a test is being
@@ -1733,7 +1728,7 @@ impl Default for BackendInner {
             created_forks: Default::default(),
             forks: vec![],
             snapshots: Default::default(),
-            has_snapshot_failure: Arc::new(AtomicBool::new(false)),
+            has_snapshot_failure: false,
             test_contract_address: None,
             caller: None,
             next_fork_id: Default::default(),

crates/evm/evm/src/executors/fuzz/mod.rs

@@ -28,9 +28,9 @@ pub use types::{CaseOutcome, CounterExampleOutcome, FuzzOutcome};
 /// After instantiation, calling `fuzz` will proceed to hammer the deployed smart contract with
 /// inputs, until it finds a counterexample. The provided [`TestRunner`] contains all the
 /// configuration which can be overridden via [environment variables](proptest::test_runner::Config)
-pub struct FuzzedExecutor<'a> {
-    /// The VM
-    executor: &'a Executor,
+pub struct FuzzedExecutor {
+    /// The EVM executor
+    executor: Executor,
     /// The fuzzer
     runner: TestRunner,
     /// The account that calls tests
@@ -39,10 +39,10 @@ pub struct FuzzedExecutor<'a> {
     config: FuzzConfig,
 }
 
-impl<'a> FuzzedExecutor<'a> {
+impl FuzzedExecutor {
     /// Instantiates a fuzzed executor given a testrunner
     pub fn new(
-        executor: &'a Executor,
+        executor: Executor,
         runner: TestRunner,
         sender: Address,
         config: FuzzConfig,
@@ -224,7 +224,7 @@ impl<'a> FuzzedExecutor<'a> {
             .map_or_else(Default::default, |cheats| cheats.breakpoints.clone());
 
         let success =
-            self.executor.is_success(address, call.reverted, state_changeset.clone(), should_fail);
+            self.executor.is_raw_call_success(address, state_changeset.clone(), &call, should_fail);
 
         if success {
             Ok(FuzzOutcome::Case(CaseOutcome {

crates/evm/evm/src/executors/invariant/funcs.rs

@@ -40,10 +40,10 @@ pub fn assert_invariants(
 
     // This will panic and get caught by the executor
     let is_err = call_result.reverted ||
-        !executor.is_success(
+        !executor.is_raw_call_success(
             invariant_contract.address,
-            call_result.reverted,
             call_result.state_changeset.take().expect("we should have a state changeset"),
+            &call_result,
             false,
         );
     if is_err {

crates/evm/evm/src/executors/mod.rs

@@ -282,6 +282,10 @@ impl Executor {
     /// Performs a raw call to an account on the current state of the VM.
     ///
     /// Any state modifications made by the call are not committed.
+    ///
+    /// This intended for fuzz calls, which try to minimize [Backend] clones by using a Cow of the
+    /// underlying [Backend] so it only gets cloned when cheatcodes that require mutable access are
+    /// used.
     pub fn call_raw(
         &self,
         from: Address,
@@ -296,9 +300,8 @@ impl Executor {
         let result = db.inspect_ref(&mut env, &mut inspector)?;
 
         // Persist the snapshot failure recorded on the fuzz backend wrapper.
-        self.backend
-            .set_snapshot_failure(self.backend.has_snapshot_failure() || db.has_snapshot_failure());
-        convert_executed_result(env, inspector, result)
+        let has_snapshot_failure = db.has_snapshot_failure();
+        convert_executed_result(env, inspector, result, has_snapshot_failure)
     }
 
     /// Execute the transaction configured in `env.tx` and commit the changes
@@ -313,7 +316,7 @@ impl Executor {
         // execute the call
         let mut inspector = self.inspector.clone();
         let result = self.backend.inspect_ref(&mut env, &mut inspector)?;
-        convert_executed_result(env, inspector, result)
+        convert_executed_result(env, inspector, result, self.backend.has_snapshot_failure())
     }
 
     /// Commit the changeset to the database and adjust `self.inspector_config`
@@ -465,6 +468,34 @@ impl Executor {
         self.ensure_success(address, reverted, state_changeset, should_fail).unwrap_or_default()
     }
 
+    /// This is the same as [Self::is_success] but intended for outcomes of [Self::call_raw] used in
+    /// fuzzing and invariant testing.
+    ///
+    /// ## Background
+    ///
+    /// Executing and failure checking [Executor::ensure_success] are two steps, for ds-test
+    /// legacy reasons failures can be stored in a global variables and needs to be called via a
+    /// solidity call `failed()(bool)`. For fuzz tests we’re using the
+    /// `FuzzBackendWrapper` which is a Cow of the executor’s backend which lazily clones the
+    /// backend when it’s mutated via cheatcodes like `snapshot`. Snapshots make it even
+    /// more complicated because now we also need to keep track of that global variable when we
+    /// revert to a snapshot (because it is stored in state). Now, the problem is that
+    /// the `FuzzBackendWrapper` is dropped after every call, so we need to keep track of the
+    /// snapshot failure in the [RawCallResult] instead.
+    pub fn is_raw_call_success(
+        &self,
+        address: Address,
+        state_changeset: StateChangeset,
+        call_result: &RawCallResult,
+        should_fail: bool,
+    ) -> bool {
+        if call_result.has_snapshot_failure {
+            // a failure occurred in a reverted snapshot, which is considered a failed test
+            return should_fail
+        }
+        self.is_success(address, call_result.reverted, state_changeset, should_fail)
+    }
+
     fn ensure_success(
         &self,
         address: Address,
@@ -646,6 +677,11 @@ pub struct RawCallResult {
     pub exit_reason: InstructionResult,
     /// Whether the call reverted or not
     pub reverted: bool,
+    /// Whether the call includes a snapshot failure
+    ///
+    /// This is tracked separately from revert because a snapshot failure can occur without a
+    /// revert, since assert failures are stored in a global variable (ds-test legacy)
+    pub has_snapshot_failure: bool,
     /// The raw result of the call
     pub result: Bytes,
     /// The gas used for the call
@@ -688,6 +724,7 @@ impl Default for RawCallResult {
         Self {
             exit_reason: InstructionResult::Continue,
             reverted: false,
+            has_snapshot_failure: false,
             result: Bytes::new(),
             gas_used: 0,
             gas_refunded: 0,
@@ -719,6 +756,7 @@ fn convert_executed_result(
     env: Env,
     inspector: InspectorStack,
     result: ResultAndState,
+    has_snapshot_failure: bool,
 ) -> eyre::Result<RawCallResult> {
     let ResultAndState { result: exec_result, state: state_changeset } = result;
     let (exit_reason, gas_refunded, gas_used, out) = match exec_result {
@@ -761,6 +799,7 @@ fn convert_executed_result(
     Ok(RawCallResult {
         exit_reason,
         reverted: !matches!(exit_reason, return_ok!()),
+        has_snapshot_failure,
         result,
         gas_used,
         gas_refunded,

crates/forge/src/runner.rs

@@ -558,7 +558,7 @@ impl<'a> ContractRunner<'a> {
         // Run fuzz test
         let start = Instant::now();
         let fuzzed_executor =
-            FuzzedExecutor::new(&self.executor, runner.clone(), self.sender, fuzz_config);
+            FuzzedExecutor::new(self.executor.clone(), runner.clone(), self.sender, fuzz_config);
         let state = fuzzed_executor.build_fuzz_state();
         let mut result = fuzzed_executor.fuzz(func, address, should_fail, self.errors);
 
@@ -598,7 +598,7 @@ impl<'a> ContractRunner<'a> {
             };
             // rerun the last relevant test with traces
             let debug_result = FuzzedExecutor::new(
-                &debug_executor,
+                debug_executor,
                 runner,
                 self.sender,
                 fuzz_config,

crates/forge/tests/it/repros.rs

@@ -199,5 +199,15 @@ test_repro!(6170, false, None, |res| {
 // https://github.com/foundry-rs/foundry/issues/6180
 test_repro!(6180);
 
+// https://github.com/foundry-rs/foundry/issues/6355
+test_repro!(6355, false, None, |res| {
+    let mut res = res.remove("repros/Issue6355.t.sol:Issue6355Test").unwrap();
+    let test = res.test_results.remove("test_shouldFail()").unwrap();
+    assert_eq!(test.status, TestStatus::Failure);
+
+    let test = res.test_results.remove("test_shouldFaillWithRevertTo()").unwrap();
+    assert_eq!(test.status, TestStatus::Failure);
+});
+
 // https://github.com/foundry-rs/foundry/issues/6437
 test_repro!(6437);

crates/forge/tests/it/test_helpers.rs

@@ -77,7 +77,7 @@ pub static EVM_OPTS: Lazy<EvmOpts> = Lazy::new(|| EvmOpts {
     ..Default::default()
 });
 
-pub fn fuzz_executor<DB: DatabaseRef>(executor: &Executor) -> FuzzedExecutor {
+pub fn fuzz_executor<DB: DatabaseRef>(executor: Executor) -> FuzzedExecutor {
     let cfg = proptest::test_runner::Config { failure_persistence: None, ..Default::default() };
 
     FuzzedExecutor::new(

testdata/repros/Issue6355.t.sol

@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: MIT OR Apache-2.0
+pragma solidity 0.8.18;
+
+import "ds-test/test.sol";
+import "../cheats/Vm.sol";
+
+// https://github.com/foundry-rs/foundry/issues/6355
+contract Issue6355Test is DSTest {
+    Vm constant vm = Vm(HEVM_ADDRESS);
+    uint256 snapshot;
+    Target targ;
+
+    function setUp() public {
+        snapshot = vm.snapshot();
+        targ = new Target();
+    }
+
+    // this non-deterministically fails sometimes and passes sometimes
+    function test_shouldPass() public {
+        assertEq(2, targ.num());
+    }
+
+    // always fails
+    function test_shouldFaillWithRevertTo() public {
+        assertEq(3, targ.num());
+        vm.revertTo(snapshot);
+    }
+
+    // always fails
+    function test_shouldFail() public {
+        assertEq(3, targ.num());
+    }
+}
+
+contract Target {
+    function num() public pure returns (uint256) {
+        return 2;
+    }
+}