Skip to content

Commit

Permalink
chore: clean up ds-test failure related code (#8244)
Browse files Browse the repository at this point in the history 
* feat: add feature to enable tracy

* perf: add more early returns in is_success logic

* try

* readd snapshot check

* update

* com

* docs

* clean

* chore: remove extra checks

* fix
  • Loading branch information
@DaniPopes
DaniPopes committed Jun 24, 2024
1 parent c9046f1 commit 32f01e3
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 121 deletions.
108 changes: 18 additions & 90 deletions crates/evm/core/src/backend/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
InspectorExt,
};
use alloy_genesis::GenesisAccount;
use alloy_primitives::{b256, keccak256, Address, B256, U256};
use alloy_primitives::{keccak256, uint, Address, B256, U256};
use alloy_rpc_types::{Block, BlockNumberOrTag, BlockTransactions, Transaction};
use alloy_serde::WithOtherFields;
use eyre::Context;
Expand Down Expand Up @@ -59,10 +59,12 @@ type ForkLookupIndex = usize;
const DEFAULT_PERSISTENT_ACCOUNTS: [Address; 3] =
[CHEATCODE_ADDRESS, DEFAULT_CREATE2_DEPLOYER, CALLER];

/// Slot corresponding to "failed" in bytes on the cheatcodes (HEVM) address.
/// Not prefixed with 0x.
const GLOBAL_FAILURE_SLOT: B256 =
b256!("6661696c65640000000000000000000000000000000000000000000000000000");
/// `bytes32("failed")`, as a storage slot key into [`CHEATCODE_ADDRESS`].
///
/// Used by all `forge-std` test contracts and newer `DSTest` test contracts as a global marker for
/// a failed test.
pub const GLOBAL_FAIL_SLOT: U256 =
uint!(0x6661696c65640000000000000000000000000000000000000000000000000000_U256);

/// An extension trait that allows us to easily extend the `revm::Inspector` capabilities
#[auto_impl::auto_impl(&mut)]
Expand Down Expand Up @@ -537,10 +539,8 @@ impl Backend {
/// This will also grant cheatcode access to the test account
pub fn set_test_contract(&mut self, acc: Address) -> &mut Self {
trace!(?acc, "setting test account");

self.add_persistent_account(acc);
self.allow_cheatcode_access(acc);
self.inner.test_contract_address = Some(acc);
self
}

Expand All @@ -559,11 +559,6 @@ impl Backend {
self
}

/// Returns the address of the set `DSTest` contract
pub fn test_contract_address(&self) -> Option<Address> {
self.inner.test_contract_address
}

/// Returns the set caller address
pub fn caller_address(&self) -> Option<Address> {
self.inner.caller
Expand All @@ -583,80 +578,12 @@ impl Backend {
self.inner.has_snapshot_failure = has_snapshot_failure
}

/// Checks if the test contract associated with this backend failed, See
/// [Self::is_failed_test_contract]
pub fn is_failed(&self) -> bool {
self.has_snapshot_failure() ||
self.test_contract_address()
.map(|addr| self.is_failed_test_contract(addr))
.unwrap_or_default()
}

/// Checks if the given test function failed
///
/// DSTest will not revert inside its `assertEq`-like functions which allows
/// to test multiple assertions in 1 test function while also preserving logs.
/// Instead, it stores whether an `assert` failed in a boolean variable that we can read
pub fn is_failed_test_contract(&self, address: Address) -> bool {
/*
contract DSTest {
bool public IS_TEST = true;
// slot 0 offset 1 => second byte of slot0
bool private _failed;
}
*/
let value = self.storage_ref(address, U256::ZERO).unwrap_or_default();
value.as_le_bytes()[1] != 0
}

/// Checks if the given test function failed by looking at the present value of the test
/// contract's `JournaledState`
///
/// See [`Self::is_failed_test_contract()]`
///
/// Note: we assume the test contract is either `forge-std/Test` or `DSTest`
pub fn is_failed_test_contract_state(
&self,
address: Address,
current_state: &JournaledState,
) -> bool {
if let Some(account) = current_state.state.get(&address) {
let value = account
.storage
.get(&revm::primitives::U256::ZERO)
.cloned()
.unwrap_or_default()
.present_value();
return value.as_le_bytes()[1] != 0;
}

false
}

/// In addition to the `_failed` variable, `DSTest::fail()` stores a failure
/// in "failed"
/// See <https://github.com/dapphub/ds-test/blob/9310e879db8ba3ea6d5c6489a579118fd264a3f5/src/test.sol#L66-L72>
pub fn is_global_failure(&self, current_state: &JournaledState) -> bool {
if let Some(account) = current_state.state.get(&CHEATCODE_ADDRESS) {
let slot: U256 = GLOBAL_FAILURE_SLOT.into();
let value = account.storage.get(&slot).cloned().unwrap_or_default().present_value();
return value == revm::primitives::U256::from(1);
}

false
}

/// When creating or switching forks, we update the AccountInfo of the contract
pub(crate) fn update_fork_db(
&self,
active_journaled_state: &mut JournaledState,
target_fork: &mut Fork,
) {
debug_assert!(
self.inner.test_contract_address.is_some(),
"Test contract address must be set"
);

self.update_fork_db_contracts(
self.inner.persistent_accounts.iter().copied(),
active_journaled_state,
Expand Down Expand Up @@ -973,10 +900,17 @@ impl DatabaseExt for Backend {
if action.is_keep() {
self.inner.snapshots.insert_at(snapshot.clone(), id);
}
// 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.set_snapshot_failure(true);

// https://github.com/foundry-rs/foundry/issues/3055
// Check if an error occurred either during or before the snapshot.
// DSTest contracts don't have snapshot functionality, so this slot is enough to check
// for failure here.
if let Some(account) = current_state.state.get(&CHEATCODE_ADDRESS) {
if let Some(slot) = account.storage.get(&GLOBAL_FAIL_SLOT) {
if !slot.present_value.is_zero() {
self.set_snapshot_failure(true);
}
}
}

// merge additional logs
Expand Down Expand Up @@ -1585,11 +1519,6 @@ pub struct BackendInner {
/// check if the `_failed` variable is set,
/// additionally
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
/// executed. E.g. the `_failed` variable of `DSTest`
pub test_contract_address: Option<Address>,
/// Tracks the caller of the test function
pub caller: Option<Address>,
/// Tracks numeric identifiers for forks
Expand Down Expand Up @@ -1778,7 +1707,6 @@ impl Default for BackendInner {
forks: vec![],
snapshots: Default::default(),
has_snapshot_failure: false,
test_contract_address: None,
caller: None,
next_fork_id: Default::default(),
persistent_accounts: Default::default(),
Expand Down
78 changes: 47 additions & 31 deletions crates/evm/evm/src/executors/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use alloy_json_abi::Function;
use alloy_primitives::{Address, Bytes, Log, U256};
use alloy_sol_types::{sol, SolCall};
use foundry_evm_core::{
backend::{Backend, CowBackend, DatabaseError, DatabaseExt, DatabaseResult},
backend::{Backend, CowBackend, DatabaseError, DatabaseExt, DatabaseResult, GLOBAL_FAIL_SLOT},
constants::{
CALLER, CHEATCODE_ADDRESS, CHEATCODE_CONTRACT_HASH, DEFAULT_CREATE2_DEPLOYER,
DEFAULT_CREATE2_DEPLOYER_CODE,
Expand Down Expand Up @@ -434,7 +434,7 @@ impl Executor {
self.inspector_mut().set_env(&result.env);
}

/// Checks if a call to a test contract was successful.
/// Returns `true` if a test can be considered successful.
///
/// This is the same as [`Self::is_success`], but will consume the `state_changeset` map to use
/// internally when calling `failed()`.
Expand All @@ -452,20 +452,9 @@ impl Executor {
)
}

/// Checks if a call to a test contract was successful.
/// Returns `true` if a test can be considered successful.
///
/// This is the same as [`Self::is_success`] but intended for outcomes of [`Self::call_raw`].
///
/// ## Background
///
/// Executing and failure checking `Executor::is_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)`.
///
/// Snapshots make this task 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 `CowBackend` is dropped after every call, so we need to keep track
/// of the snapshot failure in the [`RawCallResult`] instead.
/// This is the same as [`Self::is_success`], but intended for outcomes of [`Self::call_raw`].
pub fn is_raw_call_success(
&self,
address: Address,
Expand All @@ -480,21 +469,27 @@ impl Executor {
self.is_success(address, call_result.reverted, state_changeset, should_fail)
}

/// Check if a call to a test contract was successful.
/// Returns `true` if a test can be considered successful.
///
/// This function checks both the VM status of the call, DSTest's `failed` status and the
/// `globalFailed` flag which is stored in `failed` inside the `CHEATCODE_ADDRESS` contract.
/// If the call succeeded, we also have to check the global and local failure flags.
///
/// DSTest will not revert inside its `assertEq`-like functions which allows
/// to test multiple assertions in 1 test function while also preserving logs.
/// These are set by the test contract itself when an assertion fails, using the internal `fail`
/// function. The global flag is located in [`CHEATCODE_ADDRESS`] at slot [`GLOBAL_FAIL_SLOT`],
/// and the local flag is located in the test contract at an unspecified slot.
///
/// If an `assert` is violated, the contract's `failed` variable is set to true, and the
/// `globalFailure` flag inside the `CHEATCODE_ADDRESS` is also set to true, this way, failing
/// asserts from any contract are tracked as well.
/// This behavior is inherited from Dapptools, where initially only a public
/// `failed` variable was used to track test failures, and later, a global failure flag was
/// introduced to track failures across multiple contracts in
/// [ds-test#30](https://github.com/dapphub/ds-test/pull/30).
///
/// In order to check whether a test failed, we therefore need to evaluate the contract's
/// `failed` variable and the `globalFailure` flag, which happens by calling
/// `contract.failed()`.
/// The assumption is that the test runner calls `failed` on the test contract to determine if
/// it failed. However, we want to avoid this as much as possible, as it is relatively
/// expensive to set up an EVM call just for checking a single boolean flag.
///
/// See:
/// - Newer DSTest: <https://github.com/dapphub/ds-test/blob/e282159d5170298eb2455a6c05280ab5a73a4ef0/src/test.sol#L47-L63>
/// - Older DSTest: <https://github.com/dapphub/ds-test/blob/9ca4ecd48862b40d7b0197b600713f64d337af12/src/test.sol#L38-L49>
/// - forge-std: <https://github.com/foundry-rs/forge-std/blob/19891e6a0b5474b9ea6827ddb90bb9388f7acfc0/src/StdAssertions.sol#L38-L44>
pub fn is_success(
&self,
address: Address,
Expand All @@ -513,13 +508,34 @@ impl Executor {
reverted: bool,
state_changeset: Cow<'_, StateChangeset>,
) -> bool {
// The call reverted.
if reverted {
return false;
}

// A failure occurred in a reverted snapshot, which is considered a failed test.
if self.backend().has_snapshot_failure() {
// a failure occurred in a reverted snapshot, which is considered a failed test
return false;
}

let mut success = !reverted;
if success {
// Check the global failure slot.
// TODO: Wire this up
let legacy = true;
if !legacy {
if let Some(acc) = state_changeset.get(&CHEATCODE_ADDRESS) {
if let Some(failed_slot) = acc.storage.get(&GLOBAL_FAIL_SLOT) {
return failed_slot.present_value().is_zero();
}
}
let Ok(failed_slot) = self.backend().storage_ref(CHEATCODE_ADDRESS, GLOBAL_FAIL_SLOT)
else {
return false;
};
return failed_slot.is_zero();
}

// Finally, resort to calling `DSTest::failed`.
{
// Construct a new bare-bones backend to evaluate success.
let mut backend = self.backend().clone_empty();

Expand All @@ -542,14 +558,14 @@ impl Executor {
match call {
Ok(CallResult { raw: _, decoded_result: ITest::failedReturn { failed } }) => {
trace!(failed, "DSTest::failed()");
success = !failed;
!failed
}
Err(err) => {
trace!(%err, "failed to call DSTest::failed()");
true
}
}
}
success
}

/// Creates the environment to use when executing a transaction in a test context
Expand Down

0 comments on commit 32f01e3

Please sign in to comment.