-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(cheatcode): startDebugTraceRecording
, stopDebugTraceRecording
, and getDebugTraceByIndex
for ERC4337 testing
#8571
base: master
Are you sure you want to change the base?
Conversation
50d4c0f
to
d81495d
Compare
crates/cheatcodes/src/inspector.rs
Outdated
current_opcode, interpreter.stack(), | ||
) | ||
); | ||
let mem_inputs = get_memory_input_for_opcode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 DebugNode
s, 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>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
startDebugTraceRecording
and stopAndReturnDebugTraceRecording
startDebugTraceRecording
and stopAndReturnDebugTraceRecording
for ERC4337 testing
ed3ad33
to
72d6ca9
Compare
startDebugTraceRecording
and stopAndReturnDebugTraceRecording
for ERC4337 testingstartDebugTraceRecording
, stopDebugTraceRecording
, and getDebugTraceByIndex
for ERC4337 testing
@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. |
9ff4d0e
to
03bacff
Compare
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 |
03bacff
to
0575405
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
cfac4e3
to
bde1de2
Compare
@klkvr thanks for the review, should have fixed all comments aside from the one re:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, sorry for the delay here
left nit on trace mode and question on UX of fetching steps, I'd prefer us to return an array of them if possible
cc @DaniPopes do you have an idea on how we could support this without requiring users to increase verbosity for all tests?
b0f4fd0
to
141aea7
Compare
thanks for the review! Have fixed for the comments. Personally I think it will be great to have a way to specify like a |
cacd9eb
to
1dad1a1
Compare
bounce for review again 🙏 |
ad998df
to
c1a6c5b
Compare
feat: capture stack inputs as part of the opcode feat: record opcode -> record debug trace fix: memory OOG, need to only use needed stack, mem input fix: missing op code, instruction results fix: accessing out-of-bound idx memory When running on some project, we noticed that it sometimes try to access memory with out of bound index and panics. This commit fix it by: 1. Enfore reset to Nonce after stopDebugTraceRecording(), this ensures the `some(..) = ...` part will not be triggered 2. Change how opcode_utils.rs accesses memory. Return empty vector if trying access out-of-bound memory.
This commit also cleans up the previous implementaiton on inspector. And then change the cheatcode interface to be of three steps: 1. start recording debug trace 2. stop recording 3. get the debug trace by index The reason is to avoid out-of-memory issue by returning the whole traces at once.
Since enabling dummy tracer still come with performance impact, remove the auto dummy tracer initiation. The cheatcode will return explicit error and require the test to be run in -vvv mode to have the tracer enabled by default.
There was OOM concern but using the get-by-index style, despite improved, does not solve the root cause. The main issue is that the tracer config did not turn off after the stop recording cheatcode being called. It seems too much burden for the tracer to record the returned traces inside forge tests as the tests will also pass around the debug traces, causing memory boost. This commit also only turns on necessary tracer config instead of using all().
c1a6c5b
to
8c3ca8c
Compare
bounce for a review 🙏 @DaniPopes @klkvr |
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:
startDebugTraceRecording
: This starts the recording of the debug trace data.stopAndReturnDebugTraceRecording
: This stops and returns the recording of the debug trace data.Test