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

fix(forge): selectively enable Etherscan trace resolving when a test in ran in a forked environment and return block number in trace on a failed test #7606

Closed
wants to merge 52 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
68bfdb8
add has_forks to backend
zerosnacks Apr 8, 2024
7251ef4
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 8, 2024
5eca07c
pass down environment, add dynamic enabled / disabled flag on a per t…
zerosnacks Apr 9, 2024
004dc11
add clarifying comment
zerosnacks Apr 9, 2024
46ae039
check has_forks at test runtime
zerosnacks Apr 9, 2024
acba4c4
pass down block number
zerosnacks Apr 9, 2024
f3fc621
fix build issues, cast block number as u64
zerosnacks Apr 9, 2024
d63e4fc
flip order
zerosnacks Apr 9, 2024
0ff4a3f
clarify
zerosnacks Apr 9, 2024
c42c3a6
remove unused code
zerosnacks Apr 9, 2024
f9ad7c0
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 10, 2024
c5a4690
simplify by making enable_etherscan flip based on parameter `yes`
zerosnacks Apr 10, 2024
15a9bfd
add environment report formatter, add block on failure
zerosnacks Apr 10, 2024
7e4bb99
add block format to test, looks incorrect, especially in the USDTCall…
zerosnacks Apr 10, 2024
c473f01
fix, has_forks was inverted
zerosnacks Apr 10, 2024
7f1f3c2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 15, 2024
efc05d3
to be able to capture forking state inside of calls (as in vm.createS…
zerosnacks Apr 15, 2024
d2ba46b
filter out block number at genesis, assume non-forked environment in …
zerosnacks Apr 15, 2024
5ae23e5
add basic tests cases
zerosnacks Apr 15, 2024
4edb749
simplify has_forks check, we can simply check `issued_local_forks_ids…
zerosnacks Apr 15, 2024
8fb7445
add specific test for verbose tracing and non verbose tracing for bot…
zerosnacks Apr 15, 2024
676b249
revert simplification
zerosnacks Apr 15, 2024
d48c3b3
add note
zerosnacks Apr 15, 2024
b48a587
pull in latest master
zerosnacks Apr 16, 2024
5404c5c
revert repro_6531, clean up comments
zerosnacks Apr 16, 2024
059decc
make sure to output block number in both test results as well as the …
zerosnacks Apr 16, 2024
5799cc2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 22, 2024
07dde75
merge in master
zerosnacks Apr 23, 2024
07fceb5
remove env passing, to implement an alternative, move etherscan toggl…
zerosnacks Apr 23, 2024
bfca63c
add context capturing, collecting all block changes during execution
zerosnacks Apr 23, 2024
85bb139
fix overflow
zerosnacks Apr 23, 2024
39807c9
fix clippy
zerosnacks Apr 23, 2024
56e1006
add test trace for multiblock fork
zerosnacks Apr 23, 2024
52e2ca5
fix title
zerosnacks Apr 23, 2024
28986ac
add context tracking to setup
zerosnacks Apr 23, 2024
ad74f98
add context tracking support for fuzz and invariant counter example c…
zerosnacks Apr 23, 2024
4fcbdce
enhance with context, move definition to core
zerosnacks Apr 23, 2024
a2ba880
re-add environment with context
zerosnacks Apr 23, 2024
dd2592b
fix clippy
zerosnacks Apr 23, 2024
715d8b2
Merge branch 'master' into zerosnacks/fix-tracing-regression
zerosnacks Apr 25, 2024
f6310ff
appears formatting issue could relate to diff error throw
zerosnacks Apr 25, 2024
00897fa
update trace
zerosnacks Apr 25, 2024
163e337
add fuzz test case, also logs block correctly - avoid pushing genesis…
zerosnacks Apr 25, 2024
e5037d1
remove debug trace
zerosnacks Apr 25, 2024
2b6865f
improve documentation
zerosnacks Apr 25, 2024
b311550
pull in master
zerosnacks Apr 30, 2024
1536799
only rely on context tracing to determine fork status
zerosnacks Apr 30, 2024
af34ff8
remove context as inspector, simply inline
zerosnacks Apr 30, 2024
d42eb2e
remove inspector, only rely on executor env block number at the end o…
zerosnacks Apr 30, 2024
2e5918b
re-add inspector - cleaner than inlining and allows capture of transf…
zerosnacks Apr 30, 2024
c0775aa
add comment on block number modification due to cheat codes
zerosnacks Apr 30, 2024
747e17c
remove wrongly merged funcs file, avoid formatting diff
zerosnacks Apr 30, 2024
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
2 changes: 1 addition & 1 deletion crates/evm/core/src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,7 @@ pub struct BackendInner {
/// issued _local_ numeric identifier, that remains constant, even if the underlying fork
/// backend changes.
pub issued_local_fork_ids: HashMap<LocalForkId, ForkId>,
/// tracks all the created forks
/// Tracks all the created forks
/// Contains the index of the corresponding `ForkDB` in the `forks` vec
pub created_forks: HashMap<ForkId, ForkLookupIndex>,
/// Holds all created fork databases
Expand Down
9 changes: 9 additions & 0 deletions crates/evm/core/src/fork/context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use alloy_primitives::U256;
use serde::{Deserialize, Serialize};

/// An execution context
#[derive(Clone, Default, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
pub struct Context {
/// The block number of the context.
pub block_number: U256,
}
3 changes: 3 additions & 0 deletions crates/evm/core/src/fork/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ pub use init::environment;
mod cache;
pub use cache::{BlockchainDb, BlockchainDbMeta, JsonBlockCacheDB, MemDb};

mod context;
pub use context::Context;

pub mod database;

mod multi;
Expand Down
7 changes: 6 additions & 1 deletion crates/evm/evm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ foundry-evm-traces.workspace = true

alloy-dyn-abi = { workspace = true, features = ["arbitrary", "eip712"] }
alloy-json-abi.workspace = true
alloy-primitives = { workspace = true, features = ["serde", "getrandom", "arbitrary", "rlp"] }
alloy-primitives = { workspace = true, features = [
"serde",
"getrandom",
"arbitrary",
"rlp",
] }
alloy-sol-types.workspace = true
revm = { workspace = true, default-features = false, features = [
"std",
Expand Down
1 change: 1 addition & 0 deletions crates/evm/evm/src/executors/fuzz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ impl FuzzedExecutor {
logs: call.logs,
labeled_addresses: call.labels,
traces: last_run_traces,
contexts: call.contexts,
gas_report_traces: traces,
coverage: coverage.into_inner(),
};
Expand Down
7 changes: 6 additions & 1 deletion crates/evm/evm/src/executors/invariant/replay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use alloy_dyn_abi::JsonAbiExt;
use alloy_primitives::Log;
use eyre::Result;
use foundry_common::{ContractsByAddress, ContractsByArtifact};
use foundry_evm_core::constants::CALLER;
use foundry_evm_core::{constants::CALLER, fork::Context};
use foundry_evm_coverage::HitMaps;
use foundry_evm_fuzz::{
invariant::{BasicTxDetails, InvariantContract},
Expand All @@ -26,6 +26,7 @@ pub fn replay_run(
mut ided_contracts: ContractsByAddress,
logs: &mut Vec<Log>,
traces: &mut Traces,
contexts: &mut Vec<Context>,
coverage: &mut Option<HitMaps>,
inputs: Vec<BasicTxDetails>,
) -> Result<Option<CounterExample>> {
Expand All @@ -40,6 +41,7 @@ pub fn replay_run(
executor.call_raw_committing(*sender, *addr, bytes.clone(), U256::ZERO)?;
logs.extend(call_result.logs);
traces.push((TraceKind::Execution, call_result.traces.clone().unwrap()));
contexts.extend(call_result.contexts);

if let Some(new_coverage) = call_result.coverage {
if let Some(old_coverage) = coverage {
Expand Down Expand Up @@ -77,6 +79,7 @@ pub fn replay_run(
)?;
traces.push((TraceKind::Execution, error_call_result.traces.clone().unwrap()));
logs.extend(error_call_result.logs);
contexts.extend(error_call_result.contexts);
}

Ok((!counterexample_sequence.is_empty())
Expand All @@ -93,6 +96,7 @@ pub fn replay_error(
ided_contracts: ContractsByAddress,
logs: &mut Vec<Log>,
traces: &mut Traces,
contexts: &mut Vec<Context>,
coverage: &mut Option<HitMaps>,
) -> Result<Option<CounterExample>> {
match failed_case.test_error {
Expand All @@ -116,6 +120,7 @@ pub fn replay_error(
ided_contracts,
logs,
traces,
contexts,
coverage,
calls,
)
Expand Down
7 changes: 6 additions & 1 deletion crates/evm/evm/src/executors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use foundry_evm_core::{
},
debug::DebugArena,
decode::RevertDecoder,
fork::Context,
utils::StateChangeset,
};
use foundry_evm_coverage::HitMaps;
Expand Down Expand Up @@ -662,6 +663,8 @@ pub struct RawCallResult {
pub labels: HashMap<Address, String>,
/// The traces of the call
pub traces: Option<CallTraceArena>,
/// The contexts created during the call
pub contexts: Vec<Context>,
/// The coverage info collected during the call
pub coverage: Option<HitMaps>,
/// The debug nodes of the call
Expand Down Expand Up @@ -696,6 +699,7 @@ impl Default for RawCallResult {
logs: Vec::new(),
labels: HashMap::new(),
traces: None,
contexts: Vec::new(),
coverage: None,
debug: None,
transactions: None,
Expand Down Expand Up @@ -810,7 +814,7 @@ fn convert_executed_result(
_ => Bytes::new(),
};

let InspectorData { logs, labels, traces, coverage, debug, cheatcodes, chisel_state } =
let InspectorData { logs, labels, traces, coverage, contexts, debug, cheatcodes, chisel_state } =
inspector.collect();

let transactions = match cheatcodes.as_ref() {
Expand All @@ -831,6 +835,7 @@ fn convert_executed_result(
logs,
labels,
traces,
contexts,
coverage,
debug,
transactions,
Expand Down
38 changes: 38 additions & 0 deletions crates/evm/evm/src/inspectors/context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
use alloy_primitives::U256;
use foundry_evm_core::fork::Context;
use revm::{
interpreter::{CallInputs, CallOutcome},
Database, EvmContext, Inspector,
};

/// An inspector that collects EVM context during execution.
#[derive(Clone, Debug, Default)]
pub struct ContextCollector {
/// The collected execution contexts.
pub contexts: Vec<Context>,
}
Comment on lines +10 to +13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need an inspector for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added as an inspector so that it would run on every call occurring inside the function body, that way we were able to track the modifications made to the environment (such as the updating of the block number).

It isn't strictly necessary as we only need the most recent change to the block env, excluding a reset to genesis


impl<DB: Database> Inspector<DB> for ContextCollector {
fn call(&mut self, ecx: &mut EvmContext<DB>, _call: &mut CallInputs) -> Option<CallOutcome> {
// Note: in case there are any cheatcodes executed that modify the environment, this will
// update the `env` of the `EvmContext` and the block number will be reflect that change.
let block_number = ecx.inner.env.block.number;

// Skip if the block number is at genesis
if block_number == U256::from(1) {
return None;
}

// Skip if the previous context is the same
if let Some(Context { block_number: prev_block_number }) = self.contexts.last() {
if *prev_block_number == block_number {
return None;
}
}

// Push the new context
self.contexts.push(Context { block_number });
Comment on lines +19 to +34
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, not sure this will work, because there are also cheatcodes that modify the block number.

the fork context is essentially already tracked in the Backend internally.
can we instead extract this after executing a test in the testresult?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cheatcodes overwrite the env of the backend referenced here

/// Note: in case there are any cheatcodes executed that modify the environment, this will
/// update the given `env` with the new values.

By extracting it after the test we can only capture the last context as the backend overwrites the env, sufficient for the current usecase


None
}
}
3 changes: 3 additions & 0 deletions crates/evm/evm/src/inspectors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ pub use revm_inspectors::access_list::AccessListInspector;
mod chisel_state;
pub use chisel_state::ChiselState;

mod context;
pub use context::ContextCollector;

mod debugger;
pub use debugger::Debugger;

Expand Down
23 changes: 19 additions & 4 deletions crates/evm/evm/src/inspectors/stack.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use super::{
Cheatcodes, CheatsConfig, ChiselState, CoverageCollector, Debugger, Fuzzer, LogCollector,
StackSnapshotType, TracingInspector, TracingInspectorConfig,
Cheatcodes, CheatsConfig, ChiselState, ContextCollector, CoverageCollector, Debugger, Fuzzer,
LogCollector, StackSnapshotType, TracingInspector, TracingInspectorConfig,
};
use alloy_primitives::{Address, Bytes, Log, U256};
use foundry_evm_core::{
backend::{update_state, DatabaseExt},
debug::DebugArena,
fork::Context,
InspectorExt,
};
use foundry_evm_coverage::HitMaps;
Expand Down Expand Up @@ -36,12 +37,14 @@ pub struct InspectorStackBuilder {
pub gas_price: Option<U256>,
/// The cheatcodes config.
pub cheatcodes: Option<Arc<CheatsConfig>>,
/// Whether contexts should be collected.
pub contexts: Option<bool>,
/// Whether to enable the debugger.
pub debug: Option<bool>,
/// The fuzzer inspector and its state, if it exists.
pub fuzzer: Option<Fuzzer>,
/// Whether to enable tracing.
pub trace: Option<bool>,
/// Whether to enable the debugger.
pub debug: Option<bool>,
/// Whether logs should be collected.
pub logs: Option<bool>,
/// Whether coverage info should be collected.
Expand Down Expand Up @@ -152,6 +155,7 @@ impl InspectorStackBuilder {
fuzzer,
trace,
debug,
contexts,
logs,
coverage,
print,
Expand All @@ -172,6 +176,7 @@ impl InspectorStackBuilder {
}
stack.collect_coverage(coverage.unwrap_or(false));
stack.collect_logs(logs.unwrap_or(true));
stack.collect_contexts(contexts.unwrap_or(true));
stack.enable_debugger(debug.unwrap_or(false));
stack.print(print.unwrap_or(false));
stack.tracing(trace.unwrap_or(false));
Expand Down Expand Up @@ -249,6 +254,7 @@ pub struct InspectorData {
pub logs: Vec<Log>,
pub labels: HashMap<Address, String>,
pub traces: Option<CallTraceArena>,
pub contexts: Vec<Context>,
pub debug: Option<DebugArena>,
pub coverage: Option<HitMaps>,
pub cheatcodes: Option<Cheatcodes>,
Expand Down Expand Up @@ -285,6 +291,7 @@ pub struct InspectorStack {
pub debugger: Option<Debugger>,
pub fuzzer: Option<Fuzzer>,
pub log_collector: Option<LogCollector>,
pub context_collector: Option<ContextCollector>,
pub printer: Option<CustomPrintTracer>,
pub tracer: Option<TracingInspector>,
pub enable_isolation: bool,
Expand Down Expand Up @@ -370,6 +377,12 @@ impl InspectorStack {
self.log_collector = yes.then(Default::default);
}

/// Set whether to enable the context collector.
#[inline]
pub fn collect_contexts(&mut self, yes: bool) {
self.context_collector = yes.then(Default::default);
}

/// Set whether to enable the trace printer.
#[inline]
pub fn print(&mut self, yes: bool) {
Expand Down Expand Up @@ -404,6 +417,7 @@ impl InspectorStack {
})
.unwrap_or_default(),
traces: self.tracer.map(|tracer| tracer.get_traces().clone()),
contexts: self.context_collector.map(|context| context.contexts).unwrap_or_default(),
debug: self.debugger.map(|debugger| debugger.arena),
coverage: self.coverage.map(|coverage| coverage.maps),
cheatcodes: self.cheatcodes,
Expand Down Expand Up @@ -654,6 +668,7 @@ impl<DB: DatabaseExt + DatabaseCommit> Inspector<&mut DB> for InspectorStack {
&mut self.debugger,
&mut self.tracer,
&mut self.log_collector,
&mut self.context_collector,
&mut self.cheatcodes,
],
|inspector| {
Expand Down
4 changes: 4 additions & 0 deletions crates/evm/fuzz/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ extern crate tracing;
use alloy_dyn_abi::{DynSolValue, JsonAbiExt};
use alloy_primitives::{Address, Bytes, Log};
use foundry_common::{calc, contracts::ContractsByAddress};
use foundry_evm_core::fork::Context;
use foundry_evm_coverage::HitMaps;
use foundry_evm_traces::CallTraceArena;
use itertools::Itertools;
Expand Down Expand Up @@ -150,6 +151,9 @@ pub struct FuzzTestResult {
/// `num(fuzz_cases)` traces, one for each run, which is neither helpful nor performant.
pub traces: Option<CallTraceArena>,

/// Execution contexts
pub contexts: Vec<Context>,

/// Additional traces used for gas report construction.
/// Those traces should not be displayed.
pub gas_report_traces: Vec<CallTraceArena>,
Expand Down
12 changes: 12 additions & 0 deletions crates/evm/traces/src/identifier/etherscan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ pub struct EtherscanIdentifier {
invalid_api_key: Arc<AtomicBool>,
pub contracts: BTreeMap<Address, Metadata>,
pub sources: BTreeMap<u32, String>,
// Tracks whether the Etherscan identifier is enabled
// Enabled for forking tests
// Disabled for local tests
pub enabled: bool,
}

impl EtherscanIdentifier {
Expand All @@ -53,9 +57,17 @@ impl EtherscanIdentifier {
invalid_api_key: Arc::new(AtomicBool::new(false)),
contracts: BTreeMap::new(),
sources: BTreeMap::new(),
// By default, the Etherscan identifier is enabled to cover edge cases.
// It is disabled for local tests and enabled for forked tests on a per test basis.
enabled: true,
}))
}

/// Enables or disables the Etherscan identifier.
pub fn enable(&mut self, yes: bool) {
self.enabled = yes;
}

/// Goes over the list of contracts we have pulled from the traces, clones their source from
/// Etherscan and compiles them locally, for usage in the debugger.
pub async fn get_compiled_contracts(&self) -> eyre::Result<ContractSources> {
Expand Down
11 changes: 10 additions & 1 deletion crates/evm/traces/src/identifier/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ impl TraceIdentifier for TraceIdentifiers<'_> {
identities.extend(local.identify_addresses(addresses.clone()));
}
if let Some(etherscan) = &mut self.etherscan {
identities.extend(etherscan.identify_addresses(addresses));
if etherscan.enabled {
identities.extend(etherscan.identify_addresses(addresses));
}
}
identities
}
Expand All @@ -86,6 +88,13 @@ impl<'a> TraceIdentifiers<'a> {
Ok(self)
}

/// Enables or disables the Etherscan identifier.
pub fn enable_etherscan(&mut self, yes: bool) {
if let Some(etherscan) = &mut self.etherscan {
etherscan.enable(yes);
}
}

/// Returns `true` if there are no set identifiers.
pub fn is_empty(&self) -> bool {
self.local.is_none() && self.etherscan.is_none()
Expand Down
7 changes: 7 additions & 0 deletions crates/forge/bin/cmd/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,13 @@ impl TestArgs {
.labels
.extend(result.labeled_addresses.iter().map(|(k, v)| (*k, v.clone())));

// Enable Etherscan decoding for forking tests.
// Disable Etherscan decoding for local tests to avoid unnecessary API calls
// that can slow down execution significantly if rate-limited.
if identify_addresses {
identifier.enable_etherscan(result.is_fork());
}

// Identify addresses and decode traces.
let mut decoded_traces = Vec::with_capacity(result.traces.len());
for (kind, arena) in &result.traces {
Expand Down
Loading
Loading