fix(revm): preserve syscall inspector stack snapshots for call-like traces#344
fix(revm): preserve syscall inspector stack snapshots for call-like traces#344hedwig0x wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request updates the Changes
Sequence DiagramsequenceDiagram
participant Executor
participant InspectSyscall
participant Frame
participant Inspector
Executor->>InspectSyscall: inspect_syscall(frame, inputs, outputs)
InspectSyscall->>Frame: Save current bytecode & stack
InspectSyscall->>Frame: Set synthetic bytecode for opcode
InspectSyscall->>Frame: Initialize empty stack
InspectSyscall->>Frame: Push input arguments (reverse order)
InspectSyscall->>Inspector: step(synthetic_state)
InspectSyscall->>Frame: Create post-op stack from outputs
InspectSyscall->>Inspector: step_end(synthetic_state)
InspectSyscall->>Frame: Restore original bytecode & stack
InspectSyscall->>Executor: Return (frame unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hedwig0x
force-pushed
the
fix/inspector-syscall-stack-snapshots
branch
from
cd8fcb0 to
b2bd5c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/revm/src/inspector.rs (1)
86-118: Assert bytecode restoration too.This regression only checks that the original stack comes back. The production change also swaps
frame.interpreter.bytecode, so a bytecode-restore regression would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/inspector.rs` around lines 86 - 118, The test inspect_syscall_restores_interpreter_state_and_preserves_io_stacks must also assert that frame.interpreter.bytecode is restored after inspect_syscall: set a distinct bytecode into frame.interpreter.bytecode (or record the existing bytecode clone) before calling inspect_syscall, then after the call add an assertion that the post-call frame.interpreter.bytecode equals the saved/original bytecode; reference the test function name and the inspect_syscall entry point and the frame.interpreter.bytecode field to locate where to add the save-and-assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/inspector.rs`:
- Around line 43-50: The inspector currently fires inspector.step_end(&mut
frame.interpreter, ctx) immediately in inspect_syscall after synthesizing
frame.interpreter.stack from output, but syscall paths in
crates/revm/src/syscall.rs still call this helper with output == [] for
CALL/STATICCALL/CALLCODE/DELEGATECALL/CREATE/CREATE2 so tracers never see the
child-frame return word/address; change the flow to either defer calling
inspector.step_end until the child frame has settled (i.e., move the step_end
invocation to the point in syscall handling where the child frame’s
result/created address is known) or modify the helper signature so the eventual
return word/address is plumbed back into inspect_syscall (and used to populate
output) before calling inspector.step_end; locate and update inspect_syscall and
the syscall handlers in syscall.rs to ensure the synthesized
frame.interpreter.stack contains the real return value when step_end is invoked.
---
Nitpick comments:
In `@crates/revm/src/inspector.rs`:
- Around line 86-118: The test
inspect_syscall_restores_interpreter_state_and_preserves_io_stacks must also
assert that frame.interpreter.bytecode is restored after inspect_syscall: set a
distinct bytecode into frame.interpreter.bytecode (or record the existing
bytecode clone) before calling inspect_syscall, then after the call add an
assertion that the post-call frame.interpreter.bytecode equals the
saved/original bytecode; reference the test function name and the
inspect_syscall entry point and the frame.interpreter.bytecode field to locate
where to add the save-and-assert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d38eac6c-a805-4ead-84a4-11486c2bc6a5
📒 Files selected for processing (3)
crates/revm/src/executor.rscrates/revm/src/inspector.rscrates/revm/src/syscall.rs
| // Provide post-op stack shape for inspectors that persist `step_end` stack snapshots. | ||
| frame.interpreter.stack = Stack::new(); | ||
| for x in output.into_iter().rev() { | ||
| _ = frame.interpreter.stack.push(x); | ||
| } | ||
|
|
||
| // TODO: For CALL*/CREATE* opcodes this should ideally run once the child frame settles. | ||
| inspector.step_end(&mut frame.interpreter, ctx); |
There was a problem hiding this comment.
CALL/CREATE still end with an empty synthetic post-op stack.**
inspect_syscall always fires step_end immediately, but the real CALL/STATICCALL/CALLCODE/DELEGATECALL/CREATE/CREATE2 paths in crates/revm/src/syscall.rs still invoke this helper with [] for output. Tracers that read step_end therefore still never see the success word / created address for the exact call-like opcodes this PR is trying to fix. Please defer step_end until the child frame settles, or plumb the eventual return word/address back into this helper before calling it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/revm/src/inspector.rs` around lines 43 - 50, The inspector currently
fires inspector.step_end(&mut frame.interpreter, ctx) immediately in
inspect_syscall after synthesizing frame.interpreter.stack from output, but
syscall paths in crates/revm/src/syscall.rs still call this helper with output
== [] for CALL/STATICCALL/CALLCODE/DELEGATECALL/CREATE/CREATE2 so tracers never
see the child-frame return word/address; change the flow to either defer calling
inspector.step_end until the child frame has settled (i.e., move the step_end
invocation to the point in syscall handling where the child frame’s
result/created address is known) or modify the helper signature so the eventual
return word/address is plumbed back into inspect_syscall (and used to populate
output) before calling inspector.step_end; locate and update inspect_syscall and
the syscall handlers in syscall.rs to ensure the synthesized
frame.interpreter.stack contains the real return value when step_end is invoked.
hedwig0x
force-pushed
the
fix/inspector-syscall-stack-snapshots
branch
2 times, most recently
from
78572b1 to
a834a3e
Compare
1 participant
Summary
When syscall interruptions are inspected in
crates/revm/src/syscall.rs, we were only forwarding synthetic input stack values toinspect_syscall, and never forwarding synthetic output stack values.For call-like opcodes (
CALL*/CREATE*), this makes inspectorstep/step_endsnapshots inconsistent and can cause missing/incomplete nested call metadata in tracers that derive details from stack snapshots.Root cause
inspect!macro accepted($inputs, $outputs)but only passed$inputs.inspect_syscalldid not preserve/restore the interpreter stack around synthetic inspection.Fix
inspect_syscallto accept bothinputandoutputiterators.inspector.step, and output stack beforeinspector.step_end.syscall.rs,executor.rs).Test
Added a unit test in
crates/revm/src/inspector.rsthat verifies:DELEGATECALLsynthetic opcode is observed by inspector step hookstep_endNotes
I couldn't run Rust tests in this environment because
cargois unavailable in PATH, but the changes are localized and covered by the new unit test.Summary by CodeRabbit
Bug Fixes
Refactor