-
Notifications
You must be signed in to change notification settings - Fork 487
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
[NIT-2640] Add EVM tracing for Stylus programs #2530
Conversation
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.
generally, this is a very good direction. Great work.
Two comments/questions:
- Will return data be captured if the contract exists normally and not via exit early? (and if so - is there a test that covers that?)
- I would be happy if we could test some sort of equivalence between stylus and non-stylus. At least for a few instructions, at least at first.
One way would be to deploy rustFile("multicall") and mocksgen.DeployMultiCallTest. These two contracts have the same interface to wrap some of opcodes - at least SSTORE/SLOAD/CALL/LOG. Calling both contracts with the same data should not create identical trace.. but both should have the basic opcode tested.
@@ -534,7 +534,7 @@ pub trait UserHost<DR: DataReader>: GasMeteredMachine { | |||
fn return_data_size(&mut self) -> Result<u32, Self::Err> { | |||
self.buy_ink(HOSTIO_INK)?; | |||
let len = *self.evm_return_data_len(); | |||
trace!("return_data_size", self, be!(len), &[], len) | |||
trace!("return_data_size", self, &[], be!(len), len) |
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.
Why don't I see a signature change for the trace! macro in this PR?
Was it modified in an earlier PR?
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.
The trace
macro was already working correctly on the Stylus side. This PR adds the trace handling on the host/nitro side.
Regarding this change, I swapped the arguments and outputs to better reflect the semantics of the fields. The return value was being sent as an argument instead of an output. It wasn't a problem until now because this was not being used before.
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 you might need a corresponding change to cargo-stylus here, but I'm not sure https://github.com/OffchainLabs/cargo-stylus/blob/794221e183cdfd791a2b5be04ec2ab762b381269/replay/src/trace.rs#L367
(also applies to the other tracing order changes)
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 opened a PR on cargo stylus updating the API: OffchainLabs/cargo-stylus#73
The right way to log return might be from CallProgram in program.go (or from the native callProgram / etc). In this case - don't log exit_early and let the same place log normal and early exit |
Makes sense, I will fix it.
Makes sense. I will add this test. |
Capture the call opcodes before performing the call, like it is done for solidity. Also, always capture the exit/return/revert opcodes at the end of the stylus call.
…cing' into gligneul/stylus-tracing
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.
LGTM
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.
left a minor request, but I'm very happy with it. Great work.
Not design-approving yet because I want Lee's take as well.
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.
The approach generally looks good to me, though I have a couple comments on details. I haven't reviewed the full details of each opcode tracing yet
@@ -534,7 +534,7 @@ pub trait UserHost<DR: DataReader>: GasMeteredMachine { | |||
fn return_data_size(&mut self) -> Result<u32, Self::Err> { | |||
self.buy_ink(HOSTIO_INK)?; | |||
let len = *self.evm_return_data_len(); | |||
trace!("return_data_size", self, be!(len), &[], len) | |||
trace!("return_data_size", self, &[], be!(len), len) |
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 you might need a corresponding change to cargo-stylus here, but I'm not sure https://github.com/OffchainLabs/cargo-stylus/blob/794221e183cdfd791a2b5be04ec2ab762b381269/replay/src/trace.rs#L367
(also applies to the other tracing order changes)
This commit adds a mirror to the internal stylus cache that keeps track of which load and store operations should be reported.
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.
LGTM
I tested this PR using the Nitro test node and the main branch of cargo-stylus, including the HostIO fixes in Stylus replay. I tested a few HostIOs. Here is an example of an opcode I tested that is broken in the current nitro version and is fixed on this PR. I created a simple stylus contract based on the stylus-hello-world example repository. Then, I added a function to the contract that calls the gas_left HostIO and stores the value in storage. Here is the snippet of the function.
This HostIO is currently broken on replay because nitro passes the return value on the arguments field instead of the outputs field. (This PR fixes this issue.) If you were to make a transaction on Sepolia and try to trace the transaction, you would get the following error.
In the test node with this patch, I can fetch the trace using the cargo stylus trace and replay the execution with cargo stylus replay. The following snippet shows that I could set a breakpoint on lldb and verify the return value of the gas left opcode, as expected. (Running replay on Apple silicon requires the following patch OffchainLabs/cargo-stylus@main...stylus-debugging).
|
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.
LGTM
Stylus programs perform HostIO calls when they need to interact with the EVM state, for instance, when loading a value from storage. When performing these calls, we want to capture EVM traces as if they were being executed on the EVM. So, when a stylus program loads a value from storage, we want to emit a trace for the SLOAD opcode.
We should set up a fake EVM stack for each opcode as if it were being executed in the EVM. We should also emit a POP opcode after emitting a trace for an opcode that leaves a value on the stack.
When a Stylus program performs a HostIO (mostly in
arbitrator/wasm-libraries/user-host-trait/src/lib.rs
), the host-calling library also calls a specialCaptureHostIO
HostIO, passing the call's name, arguments, and outputs.On the host side, Nitro now handles this call on
arbos/programs/api.go
. The API now calls theTracingInfo.CaptureEVMTraceForHostio
method to generate the EVM traces for all HostIOs based on their names.This PR centralizes all the EVM traces in the
CaptureEVMTraceForHostio
method and removes redundant tracing code on the programs' API. It also adds tests specifically for the EVM traces on Stylus.Example
Here is an example trace for a transaction that performs a write and then a read to the storage contract using the multi-call contract.