Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(tracer): store revert reason data for create #4853

Closed
wants to merge 1 commit into from
Closed

fix(tracer): store revert reason data for create #4853

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion evm/src/executor/inspector/tracer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ where
gas: Gas,
retdata: Bytes,
) -> (InstructionResult, Option<B160>, Gas, Bytes) {
let success = matches!(status, return_ok!());
let code = match address {
Some(address) => data
.journaled_state
Expand All @@ -286,7 +287,7 @@ where
self.fill_trace(
status,
gas_used(data.env.cfg.spec_id, gas.spend(), gas.refunded() as u64),
code,
if success { code } else { retdata.to_vec() },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is independent of the status because in the trace object, result and error are mutually exclusive, failed tx should have error and succeeding txs have result

so we actually need to check the return status here:

foundry/anvil/src/eth/backend/mem/storage.rs

Lines 383 to 391 in 91f69dd

result: Some(result),
trace_address: self.info.trace_address(idx),
subtraces: node.children.len(),
transaction_position: Some(self.info.transaction_index as usize),
transaction_hash: Some(self.info.transaction_hash),
block_number: self.block_number,
block_hash: self.block_hash,
action_type,
error: None,

rn we always set result

this means we need to make a few changes to the CallTrace object I suppose, so it can distinguish between code and output,
perhaps just Option<Bytes> for code? which is then used in parity_result()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for adding a new field to the CallTrace struct.

In this function, retdata contain revert data or return data (mutually exclusive), depending on the status:

foundry/evm/src/executor/inspector/tracer.rs

Lines 228 to 245 in 91f69dd

fn call_end(
&mut self,
data: &mut EVMData<'_, DB>,
_inputs: &CallInputs,
gas: Gas,
status: InstructionResult,
retdata: Bytes,
_is_static: bool,
) -> (InstructionResult, Gas, Bytes) {
self.fill_trace(
status,
gas_used(data.env.cfg.spec_id, gas.spend(), gas.refunded() as u64),
retdata.to_vec(),
None,
);
(status, gas, retdata)
}

Even from the EVM perspective, contract code and revert data are stored at the same data location (return data) - see https://www.evm.codes/#f0?fork=merge and https://www.evm.codes/#f0?fork=merge. Return data contain either the contract code or revert reason data, depending on the status.

I believe the error field in Trace serves a different purpose. It should contain human-readable info about why the trace failed. One of these reasons should be "Reverted". The result field should contain the return value - the data contained in return data data storage (see https://www.evm.codes/about#returndata).
This can be return value or revert data (mutually exclusive) for non-CREATE traces and contract code or revert data (also mutually exclusive) for CREATE traces.

TL;DR
I would leave the CallTrace struct with just one field (named output) because this is how EVM works (return data data storage). The output field would be used accordingly in the parity_result function depending on the trace status. But there is currently no way to set the revert data for CREATE traces and the error field in Trace serves a different purpose.

Perhaps to make it clearer, I can also make a PR to the ethers-rs repository that will be needed prior to this PR anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding pull request in ethers-rs: gakonst/ethers-rs#2392

address.map(b160_to_h160),
);

Expand Down