feat(forge): revert diagnostic inspector

[0xrusowsky]

closes

• Better error reporting when interacting with undeployed contracts #4141

design

Created a new RevertDiagnostic inspector that provides detailed information of otherwise undefined evm reverts. For the time being, the inspector suports:

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

Additionally, enhanced the traces decoder to determine when a function call to an unkown selector takes places (only for local contracts with artifacts).

output

these would be the new error messages that users would see:

Traces:
  [6350] NonContractCallRevertTest::test_non_contract_call_failure()
    ├─ [0] console::log("test non contract call failure") [staticcall]
    │   └─ ← [Stop]
    ├─ [0] 0xdEADBEeF00000000000000000000000000000000::number()
    │   └─ ← [Stop]
    └─ ← [Revert] call to non-contract address 0xdEADBEeF00000000000000000000000000000000
    // rest of the logs

Traces:
  [255303] NonContractDelegateCallRevertTest::test_unlinked_library_call_failure()
    ├─ [0] console::log("Test: Simulating call to unlinked library") [staticcall]
    │   └─ ← [Stop]
    ├─ [214746] → new LibraryCaller@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 960 bytes of code
    ├─ [3896] LibraryCaller::foobar(10)
    │   ├─ [0] 0xdEADBEeF00000000000000000000000000000000::foo(10) [delegatecall]
    │   │   └─ ← [Stop]
    │   └─ ← [Revert] delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library)
    // rest of the logs

Traces:
  [8620] NonContractCallRevertTest::test_non_supported_selector_call_failure()
    ├─ [0] console::log("test non supported fn selector call failure") [staticcall]
    │   └─ ← [Stop]
    ├─ [145] Counter::random()
    │   └─ ← [Revert] unrecognized function selector 0x5ec01e4d for contract 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, which has no fallback function.
    // rest of the logs

benchmarks forge test -vvvv

Project	Before [v1.2.1]	After	Overhead
morpho-blue	1.93s	2.14s	10.8%
spark-psm	39.56s	44.88s	13.4%
uniswap/v4-core	6.93s	7.72s	11.4%
vectorized/solady	2.44s	2.67s	9.4%

notes:

• benchmarks only measure running of tests and are pre-build
• benchmarks were run on an m2pro with 16gb of RAM
• each test run 5 times and the reported time is the avg

conclusion: the new inspector makes tests (on average) ~11% slower... the bright side is that it would only impact those test runs where traces are active (which i would assume to only be when actively debugging).

do we feel like this is acceptable?

[grandizzy]

IMO on the right track, @klkvr pls share your thoughts re the approach.
left some minor comments / nits.
To note that such default option could add perf penalty for invariant tests running with fail_on_revert = false but probably bearable.

[grandizzy]

maybe here we could directly set the reason? we can also do more checks if target address is actually a contract, (like checking if selector available, etc. some other similar hh checks https://github.com/NomicFoundation/hardhat/blob/67f1e95e1f3904f7b2e8a5560115c1551e899f64/packages/hardhat-core/src/internal/hardhat-network/stack-traces/solidity-errors.ts#L218-L341)

[0xrusowsky]

i thought that storing a bool could be more efficient + fn reason() would allow us to isolate the logic, but totally fine for me to do it directly

[klkvr]

I think we need some smarter strategy here to ensure that call we've caught in call hook is indeed the one that caused revert (e.g compare depth, ensure there were no calls after it)

[0xrusowsky]

addresses the PR feedback by checking the call stack depth + tracking EXTCODESIZE calls to empty addresses (confirmed that the initial impl wouldn't diagnose those).

the only downside is extra performance overhead, but i tried to make the fn setp as efficient as possible (improvement suggestions are welcome)

[0xrusowsky]

i also enhanced the trace decoder to identify function calls for a selector that is not supported by the called contract (if the tracer has access to its abi)

[grandizzy]

lgtm! one comment re enabling diagnostics and tracing, pls check

[0xrusowsky]

@klkvr i've addressed all the feedback that u left:

• improved docs
• made things more restrictive by enforcing "empty revert data" as a condition
• injected the revert reason into the inspector --> to do this, i had to move the fn call_end() logic into fn step()

lmk if i should fix something else 🙂

PS: as discussed, we'd leave the trace decoder improvement (where we would analyze that there are no matches in the fn dispatcher) for another PR

[klkvr]

lgtm, only have small nits

[klkvr]

note: pattern CALL -> REVERT actually means that Solidity was not able to decode outputs which might happen even when contract has code but reported with invalid abi encoding like in e.g common case of ERC20 transfer not returning bool for some tokens. Ideally we should also be able to catch this case but we can track this separately

[0xrusowsky]

i'll open a new issue for that one and the unsupported selector.

just to be certain that i don't miss anything:

1. missmatch between bytecode and assumed abi: we can catch it cause there is a successful call, followed by a revert at the same depth where the call took place --> to avoid false positives, we should know check the returndata validation logic, but that requires careful inspection of the opcodes following the call. any hints on what u'd do exactly?
2. call to non supported fn selector: we can catch it (at least assuming that the target is a solidity/vyper contract) if, when a call takes places, there is a revert without any previous positive JUMPI, as that would mean that the PC walks through the whole fn dispatcher without finding a match --> using the traces decoder is limited to local contracts, but will never throw false positives. with this logic we could do everything with the inspector alone, which i like. however, do you think the approach is sound enough?

[klkvr]

to avoid false positives, we should know check the returndata validation logic, but that requires careful inspection of the opcodes following the call. any hints on what u'd do exactly?

I think it's likely that Solidity generates some common chunk of code that's always reached in such cases. you could try writing a contract that has many such calls that are failing to decode the calldata in different methods. I'd expect all of those calls to end up in a similar REVERT opcode which we can then somehow identify and use to catch such cases

we can catch it (at least assuming that the target is a solidity/vyper contract) if, when a call takes places, there is a revert without any previous positive JUMPI, as that would mean that the PC walks through the whole fn dispatcher without finding a match

not sure if the JUMPI approach would work because iirc there are some checks around call value also, we could similarly try to see how the fallbackless contract revert looks like, I'd expect Solidity to generate similar bytecode for such

[0xrusowsky]

@grandizzy finally ready, with @klkvr last round of feedback incorporated!

[grandizzy]

thank you, good to send it from my pov! We should make some comparison with the project we use to have as a baseline (see #10183 (comment)) to be aware of penalty it introduce when forge test with traces.

@klkvr @zerosnacks @DaniPopes last pass through before merge please, thank you!

[zerosnacks]

lgtm!

[DaniPopes]

Please remember that step and step_end are INCREDIBLY hot functions, so any minimal change has a huge effect on execution performance

Please look at cheatcodes inspector on how to outline the if conditions inside of step and step_end; in this case you should do something like this:

match opcode {
    op::REVERT => self.handle_revert(...),
    op::EXTCODESIZE => self.handle_extcodesize(...),
    _ => {}
}

#[cold]
fn handle_* ...

[DaniPopes]

these return are incorrect if more checks are added in this function, outside of the if; however if you extract to separate functions as mentioned in the other comment these are fine

[DaniPopes]

same here as in step