feat(cheatcode): startDebugTraceRecording and stopDebugTraceRecording for ERC4337 testing

[boolafish]

Motivation

ERC4337 (account abstraction) has several rules/restrictions on what the UserOperation can do (see: ERC7562). Specifically, it has rules on what opcodes it can access and limitations on certain storage access as well.

These limitations are quite implicit and might not be easily identified during contract development. We want to provide an easier way for developers and security researchers to check/test if the rules are being followed. We aim to enable these checks by running forge test after writing specific tests for that purpose.

Solution

To support this, we need new cheatcodes that can record the debug traces during EVM execution so we can know the opcodes and storages being accessed. With this cheatcode, we will be able to have a helper contract that checks whether the test executions comply with ERC4337 restrictions.

In this solution, we added two cheatcodes:

1. startDebugTraceRecording: This starts the recording of the debug trace data.
2. stopAndReturnDebugTraceRecording: This stops and returns the recording of the debug trace data.

Test

• An ERC4337 checker tool and its tests are in the repo: https://github.com/boolafish/erc4337-checker/blob/main/test/ERC4337Checker.t.sol

[DaniPopes]

This is not the right path, we already have a tracer and we should reuse that through CheatcodeExecutor

this can likely just be:

• start: enables tracer at opcode tracing level if not already enabled, saves the current trace index
• stop: takes the range of traces from saved start to current and abi encodes it

cc @klkvr

[klkvr]

yep, I think this could be possible by extending CheatcodesExecutor with something like this:

fn start_steps_recording(&mut self, cheats: &mut Cheatcodes);
fn get_recorded_step(&mut self, cheats: &mut Cheatcodes) -> Vec<CallTraceStep>;

where CallTraceStep is coming from revm-inspectors and recorded by altering config of InspectorStack.inner.tracer

[boolafish]

oooh, so if I get this comment right, the idea here is to use self.tracer instead where it already have all the tracing needed. And it seems like by self.tracer.traces().into_nodes() I can get the Vec<CallTraceNode>, where it has the CallTrace that I need.

Am i getting this right?

also, I can assume the nodes are append only, so I can just rely on the idx field of the CallTraceNode to determine the range according to your description above, right?

[klkvr]

yep, once you alter its config it would start recording steps, and you can just collect them by going through nodes

we'd also need some flattening logic here similar to (but likely simpler as we don't need to collect DebugNodes, just a list of steps)

foundry/crates/debugger/src/node.rs

Line 37 in 26a7559

pub fn flatten_call_trace(arena: CallTraceArena, out: &mut Vec<DebugNode>) {

as I believe we want to allow recording of trace steps for subcalls

[boolafish]

would like to consult a bit, got one problem related to this approach after trying it:

For some background on my local env, I have a repository with forge tests that uses this new cheatcodes and I build the forge locally to use on the test repository. When I try the tracer approach, It panics with the error message: more traces were filled than started. I can, however, temporary fix this by turn on the tracing from start in the following function in crates/evm/evm/src/inspectors/stack.rs to fix this:

pub fn tracing(&mut self, mode: TraceMode) {
        if let Some(config) = mode.into_config() {
            *self.tracer.get_or_insert_with(Default::default).config_mut() = config;
        } else {
            // self.tracer = None;  <-- comment out this line
           // Chage: ensures that the tracing will occur on start.
            self.tracer.get_or_insert_with(Default::default);
        }
    }

A sample implementation that updates the config is in the same file:

impl CheatcodesExecutor for InspectorStackInner {
   fn get_inspector<'a, DB: DatabaseExt>(
       &'a mut self,
       cheats: &'a mut Cheatcodes,
   ) -> impl InspectorExt<DB> + 'a {
       InspectorStackRefMut { cheatcodes: Some(cheats), inner: self }
   }

  // Newly added function here!
   fn start_steps_recording(&mut self, cheats: &mut Cheatcodes) {
       // Ensure the tracer exists and configure it
       let tracer = self.tracer.get_or_insert_with(Default::default);

       tracer.update_config(|_config| TracingInspectorConfig::all());
   }

   .....skip
   
}

Wonder if you know any idea/directions to solve the "more traces were filled than started" issue? The current one(turning on the tracer) does not really seems like a great idea....

[boolafish]

Still having the same issue. Currently, I set the trace to use "none()" configuration so make it a dummy one on start instead of having none in the tracer field on start to solve the issue.

[zerosnacks]

This sounds related to #6704, tagging it here

Would like to make sure this PR covers the design goals of #6704 as the proposed cheatcode name slightly differs

[boolafish]

@DaniPopes @klkvr just tagging as I am not sure if re-open a PR from WIP will trigger notifications or not. This should be ready and has accommodated the previous review comments. Sorry for taken so long due to OOO and fixing some OOM bugs on my side.

[boolafish]

Sorry for the failed CI, have fixed those and tested in my own repo workflow: https://github.com/boolafish/foundry/actions/runs/10535493491?pr=1

[klkvr]

I think we should move logic for start_steps_recording and stop_and_get_recorded_step from CheatcodesExecutor to cheatcode implementations

We've just merged #8696 which added getter for tracing_inspector to CheatcodesExecutor and example of how we can track step ranges.

regarding more traces were filled than started I think this occurs because in cases when tracing is disabled and vm.startDebugTraceRecording enables tracer, tracer will receive a call_end invocation for that cheatcode call which will not have a node to fill. we can try working around this by creating a fake trace node in this situation. Another approach could be to always enable tracer by default (as it's done now), though this might be expensive in some cases. wdyt @DaniPopes ?

[boolafish]

@klkvr thanks for the review, should have fixed all comments aside from the one re: more traces were filled than started.

we can try working around this by creating a fake trace node in this situation

Might need a bit more elaboration on this. Not exactly sure how to do this on my end. But will also wait to see if that is the preferred approach I guess.

[boolafish]

thanks @klkvr for a fast turn-around!!

Have fixed the comment regarding node-step issue. Please take a look when available again 🙏
Can @DaniPopes also help take a look as well for the requested change part 🙏

[klkvr]

lgtm

if summing up this adds steps tracing through cheatodes, only supported when tracing is enabled via increased verbosity or other flags

Possible follow-ups:

• look into ways to enable tracer on demand, even if inspector was not configured with it initially. this will likely require some helpers on revm-inspectors side
• consider extending returned DebugStep struct with more context (returdata, read storage, etc) in a way that does not increase memory usage too much

ptal @DaniPopes @zerosnacks @grandizzy

[zerosnacks]

Thanks for your PR, lgtm - pending other reviewers

[boolafish]

slight nudge for the review 🙏 (cc @DaniPopes @grandizzy which were tagged previously by @/klkvr )

[grandizzy]

lgtm, have only couple of nits, optionally to be applied. thank you

[zerosnacks]

Thanks @boolafish! Merging

[boolafish]

Thanks all!!