Skip to content

chore(forge): revert diagnostic inspector #10446

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2301,7 +2301,7 @@ impl EthApi {
.coerce_status()
{
if let Some(reason) =
RevertDecoder::new().maybe_decode(&output, None)
RevertDecoder::new().maybe_decode(&output, None, None)
{
tx.other.insert(
"revertReason".to_string(),
Expand Down
1 change: 1 addition & 0 deletions crates/anvil/src/eth/backend/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,6 +1304,7 @@ impl Backend {
let r = RevertDecoder::new().decode(
info.out.as_ref().map(|b| &b[..]).unwrap_or_default(),
Some(info.exit),
None,
);
node_info!(" Error: reverted with: {r}");
}
Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ impl<T: Serialize> ToRpcResponseResult for Result<T> {
let mut msg = "execution reverted".to_string();
if let Some(reason) = data
.as_ref()
.and_then(|data| RevertDecoder::new().maybe_decode(data, None))
.and_then(|data| RevertDecoder::new().maybe_decode(data, None, None))
{
msg = format!("{msg}: {reason}");
}
Expand Down
4 changes: 2 additions & 2 deletions crates/cheatcodes/src/test/revert_handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ fn handle_revert(
let (actual, expected) = if let Some(contracts) = known_contracts {
let decoder = RevertDecoder::new().with_abis(contracts.values().map(|c| &c.abi));
(
&decoder.decode(actual_revert.as_slice(), Some(status)),
&decoder.decode(expected_reason, Some(status)),
&decoder.decode(actual_revert.as_slice(), Some(status), None),
&decoder.decode(expected_reason, Some(status), None),
)
} else {
(&stringify(&actual_revert), &stringify(expected_reason))
Expand Down
46 changes: 40 additions & 6 deletions crates/evm/core/src/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::abi::{console, Vm};
use alloy_dyn_abi::JsonAbiExt;
use alloy_json_abi::{Error, JsonAbi};
use alloy_primitives::{hex, map::HashMap, Log, Selector};
use alloy_primitives::{hex, map::HashMap, Address, Log, Selector};
use alloy_sol_types::{
ContractError::Revert, RevertReason, RevertReason::ContractError, SolEventInterface,
SolInterface, SolValue,
Expand All @@ -13,6 +13,26 @@ use itertools::Itertools;
use revm::interpreter::InstructionResult;
use std::{fmt, sync::OnceLock};

#[derive(Debug, Clone, Copy)]
pub enum DetailedRevertReason {
CallToNonContract(Address),
DelegateCallToNonContract(Address),
}

impl fmt::Display for DetailedRevertReason {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::CallToNonContract(addr) => {
write!(f, "call to non-contract address `{addr}`")
}
Self::DelegateCallToNonContract(addr) => write!(
f,
"delegatecall to non-contract address `{addr}` (usually an unliked library)"
),
}
}
}

/// A skip reason.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct SkipReason(pub Option<String>);
Expand Down Expand Up @@ -127,8 +147,13 @@ impl RevertDecoder {
///
/// Note that this is just a best-effort guess, and should not be relied upon for anything other
/// than user output.
pub fn decode(&self, err: &[u8], status: Option<InstructionResult>) -> String {
self.maybe_decode(err, status).unwrap_or_else(|| {
pub fn decode(
&self,
err: &[u8],
status: Option<InstructionResult>,
revert_reason: Option<DetailedRevertReason>,
) -> String {
self.maybe_decode(err, status, revert_reason).unwrap_or_else(|| {
if err.is_empty() {
"<empty revert data>".to_string()
} else {
Expand All @@ -140,7 +165,12 @@ impl RevertDecoder {
/// Tries to decode an error message from the given revert bytes.
///
/// See [`decode`](Self::decode) for more information.
pub fn maybe_decode(&self, err: &[u8], status: Option<InstructionResult>) -> Option<String> {
pub fn maybe_decode(
&self,
err: &[u8],
status: Option<InstructionResult>,
revert_reason: Option<DetailedRevertReason>,
) -> Option<String> {
if let Some(reason) = SkipReason::decode(err) {
return Some(reason.to_string());
}
Expand Down Expand Up @@ -196,6 +226,10 @@ impl RevertDecoder {
}

if let Some(status) = status {
if let Some(reason) = revert_reason {
return Some(format!("EvmError: {reason}"));
}

if !status.is_ok() {
return Some(format!("EvmError: {status:?}"));
}
Expand Down Expand Up @@ -272,7 +306,7 @@ mod tests {
"0xe17594de"
"756688fe00000000000000000000000000000000000000000000000000000000"
);
assert_eq!(decoder.decode(data, None), "ValidationFailed(0x)");
assert_eq!(decoder.decode(data, None, None), "ValidationFailed(0x)");

/*
abi.encodeWithSelector(ValidationFailed.selector, abi.encodeWithSelector(InvalidNonce.selector))
Expand All @@ -283,6 +317,6 @@ mod tests {
"0000000000000000000000000000000000000000000000000000000000000004"
"756688fe00000000000000000000000000000000000000000000000000000000"
);
assert_eq!(decoder.decode(data, None), "ValidationFailed(0x756688fe)");
assert_eq!(decoder.decode(data, None, None), "ValidationFailed(0x756688fe)");
}
}
2 changes: 1 addition & 1 deletion crates/evm/evm/src/executors/fuzz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ impl FuzzedExecutor {
// since that input represents the last run case, which may not correspond with
// our failure - when a fuzz case fails, proptest will try to run at least one
// more case to find a minimal failure case.
let reason = rd.maybe_decode(&outcome.1.result, Some(status));
let reason = rd.maybe_decode(&outcome.1.result, Some(status), None);
execution_data.borrow_mut().logs.extend(outcome.1.logs.clone());
execution_data.borrow_mut().counterexample = outcome;
// HACK: we have to use an empty string here to denote `None`.
Expand Down
2 changes: 1 addition & 1 deletion crates/evm/evm/src/executors/invariant/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl FailedInvariantCaseData {
let revert_reason = RevertDecoder::new()
.with_abis(targeted_contracts.targets.lock().values().map(|c| &c.abi))
.with_abi(invariant_contract.abi)
.decode(call_result.result.as_ref(), Some(call_result.exit_reason));
.decode(call_result.result.as_ref(), Some(call_result.exit_reason), None);

let func = invariant_contract.invariant_function;
debug_assert!(func.inputs.is_empty());
Expand Down
21 changes: 17 additions & 4 deletions crates/evm/evm/src/executors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use foundry_evm_core::{
CALLER, CHEATCODE_ADDRESS, CHEATCODE_CONTRACT_HASH, DEFAULT_CREATE2_DEPLOYER,
DEFAULT_CREATE2_DEPLOYER_CODE, DEFAULT_CREATE2_DEPLOYER_DEPLOYER,
},
decode::{RevertDecoder, SkipReason},
decode::{DetailedRevertReason, RevertDecoder, SkipReason},
utils::StateChangeset,
InspectorExt,
};
Expand Down Expand Up @@ -769,6 +769,9 @@ impl From<DeployResult> for RawCallResult {
pub struct RawCallResult {
/// The status of the call
pub exit_reason: InstructionResult,
/// Fallback reason for reverts.
/// Should only be used if no custom errors, or string errors can be decoded.
pub maybe_reason: Option<DetailedRevertReason>,
/// Whether the call reverted or not
pub reverted: bool,
/// Whether the call includes a snapshot failure
Expand Down Expand Up @@ -810,6 +813,7 @@ impl Default for RawCallResult {
fn default() -> Self {
Self {
exit_reason: InstructionResult::Continue,
maybe_reason: None,
reverted: false,
has_state_snapshot_failure: false,
result: Bytes::new(),
Expand Down Expand Up @@ -853,7 +857,8 @@ impl RawCallResult {
if let Some(reason) = SkipReason::decode(&self.result) {
return EvmError::Skip(reason);
}
let reason = rd.unwrap_or_default().decode(&self.result, Some(self.exit_reason));
let reason =
rd.unwrap_or_default().decode(&self.result, Some(self.exit_reason), self.maybe_reason);
EvmError::Execution(Box::new(self.into_execution_error(reason)))
}

Expand Down Expand Up @@ -950,8 +955,15 @@ fn convert_executed_result(
_ => Bytes::new(),
};

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

if logs.is_empty() {
logs = exec_logs;
Expand All @@ -964,6 +976,7 @@ fn convert_executed_result(

Ok(RawCallResult {
exit_reason,
maybe_reason,
reverted: !matches!(exit_reason, return_ok!()),
has_state_snapshot_failure,
result,
Expand Down
2 changes: 2 additions & 0 deletions crates/evm/evm/src/inspectors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ pub use script::ScriptExecutionInspector;

mod stack;
pub use stack::{InspectorData, InspectorStack, InspectorStackBuilder};

mod revert_diagnostic;
132 changes: 132 additions & 0 deletions crates/evm/evm/src/inspectors/revert_diagnostic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
use alloy_primitives::Address;
use foundry_evm_core::{
backend::DatabaseExt,
constants::{CHEATCODE_ADDRESS, HARDHAT_CONSOLE_ADDRESS},
decode::DetailedRevertReason,
};
use revm::{
interpreter::{
opcode::EXTCODESIZE, CallInputs, CallOutcome, CallScheme, InstructionResult, Interpreter,
},
precompile::{PrecompileSpecId, Precompiles},
primitives::SpecId,
Database, EvmContext, Inspector,
};

const IGNORE: [Address; 2] = [HARDHAT_CONSOLE_ADDRESS, CHEATCODE_ADDRESS];

/// An inspector that tracks call context to enhances revert diagnostics.
/// Useful for understanding reverts that are not linked to custom errors or revert strings.
#[derive(Clone, Debug, Default)]
pub struct RevertDiagnostic {
/// Tracks calls with calldata that target an address without executable code
pub non_contract_call: Option<(Address, CallScheme, u64)>,
/// Tracks EXTCODESIZE checks that target an address without executable code
pub non_contract_size_check: Option<(Address, u64)>,
/// Tracks whether a failed call has been spotted or not.
pub reverted: bool,
}

impl RevertDiagnostic {
fn is_delegatecall(&self, scheme: CallScheme) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these can be all helper functions, not RevertDiagnostic specific

Copy link
Author

Choose a reason for hiding this comment

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

would u keep them in the inspector file? or should i place them somewhere else (like utils or similar)?

matches!(
scheme,
CallScheme::DelegateCall | CallScheme::ExtDelegateCall | CallScheme::CallCode
)
}

fn is_precompile(&self, spec_id: SpecId, target: Address) -> bool {
let precompiles = Precompiles::new(PrecompileSpecId::from_spec_id(spec_id));
precompiles.contains(&target)
}

pub fn no_code_target_address(&self, inputs: &mut CallInputs) -> Address {
if self.is_delegatecall(inputs.scheme) {
inputs.bytecode_address
} else {
inputs.target_address
}
}

pub fn reason(&self) -> Option<DetailedRevertReason> {
if self.reverted {
if let Some((addr, scheme, _)) = self.non_contract_call {
let reason = if self.is_delegatecall(scheme) {
DetailedRevertReason::DelegateCallToNonContract(addr)
} else {
DetailedRevertReason::CallToNonContract(addr)
};

return Some(reason);
}
}

if let Some((addr, _)) = self.non_contract_size_check {
// call never took place, so schema is unknown --> output most generic msg
return Some(DetailedRevertReason::CallToNonContract(addr));
}

None
}
}

impl<DB: Database + DatabaseExt> Inspector<DB> for RevertDiagnostic {
/// Tracks the first call, with non-zero calldata, that targeted a non-contract address.
/// Excludes precompiles and test addresses.
fn call(&mut self, ctx: &mut EvmContext<DB>, inputs: &mut CallInputs) -> Option<CallOutcome> {
let target = self.no_code_target_address(inputs);

if IGNORE.contains(&target) || self.is_precompile(ctx.spec_id(), target) {
return None;
}

if let Ok(state) = ctx.code(target) {
if state.is_empty() && !inputs.input.is_empty() && !self.reverted {
self.non_contract_call = Some((target, inputs.scheme, ctx.journaled_state.depth()));
}
}
None
}

/// Tracks `EXTCODESIZE` targeted to addresses without code. Clears the cache when the call
/// stack depth changes.
fn step(&mut self, interpreter: &mut Interpreter, ctx: &mut EvmContext<DB>) {
if let Some((_, depth)) = self.non_contract_size_check {
if depth != ctx.journaled_state.depth() {
self.non_contract_size_check = None;
}

return;
}

if EXTCODESIZE == interpreter.current_opcode() {
if let Ok(word) = interpreter.stack().peek(0) {
let addr = Address::from_word(word.into());
if let Ok(state) = ctx.code(addr) {
Copy link
Collaborator

@grandizzy grandizzy May 16, 2025

Choose a reason for hiding this comment

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

this warms the account (load_account) hence the gas changes in tests, we should probably just record these addresses and their depth and check for the code in call_end, if call reverted

if state.is_empty() {
self.non_contract_size_check = Some((addr, ctx.journaled_state.depth()));
}
}
}
}
}

/// Records whether the call reverted or not. Only if the revert call depth matches the
/// inspector cache.
fn call_end(
&mut self,
ctx: &mut EvmContext<DB>,
_inputs: &CallInputs,
outcome: CallOutcome,
) -> CallOutcome {
if let Some((_, _, depth)) = self.non_contract_call {
if outcome.result.result == InstructionResult::Revert &&
ctx.journaled_state.depth() == depth - 1
{
self.reverted = true
};
}

outcome
}
}
Loading
Loading