fix(tracer): store revert reason data for create

[michprev]

Motivation

This PR fixes an issue when there are missing revert reason data for CREATE transactions in debug_traceTransaction.

Before this PR, a debug_traceTransaction call for a tx creating this contract:

contract Foo {
    constructor() {
        revert("aaa");
    }
}

would return 0x as returnValue.

After this PR, returnValue is set to 0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000036161610000000000000000000000000000000000000000000000000000000000 (revert reason data). This is the same behavior as for non-CREATE transactions.

Solution

The transaction tracer has been modified so that it returns the contract code if the contract creation was successful and revert reason data if the contract creation failed.

[michprev]

@mattsse I made this a draft because the PR makes trace_transaction responses misleading.

Before this PR, trace_transaction for the same contract creation would return

[
   {
      "action":{
         "from":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266",
         "value":"0x0",
         "gas":"0x99",
         "init":"0x6080604052348015600f57600080fd5b5060405162461bcd60e51b815260206004820152600360248201526261616160e81b604482015260640160405180910390fdfe"
      },
      "result":{
         "gasUsed":"0x99",
         "code":"0x",
         "address":"0x5fbdb2315678afecb367f032d93f642f64180aa3"
      },
      "traceAddress":[
         
      ],
      "subtraces":0,
      "transactionPosition":0,
      "transactionHash":"0xce61ff4ad07c5a5b460b1f18413182e1e055b830cb56af4095b73eafc8f92ab8",
      "blockNumber":1,
      "blockHash":"0x1a9eddec415173ef63af4b07831310153e4a4f432f927dd09357f29dce318195",
      "type":"create"
   }
]    

but after this PR, it returns

[
   {
      "action":{
         "from":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266",
         "value":"0x0",
         "gas":"0x99",
         "init":"0x6080604052348015600f57600080fd5b5060405162461bcd60e51b815260206004820152600360248201526261616160e81b604482015260640160405180910390fdfe"
      },
      "result":{
         "gasUsed":"0x99",
         "code":"0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000036161610000000000000000000000000000000000000000000000000000000000",
         "address":"0x5fbdb2315678afecb367f032d93f642f64180aa3"
      },
      "traceAddress":[
         
      ],
      "subtraces":0,
      "transactionPosition":0,
      "transactionHash":"0xce61ff4ad07c5a5b460b1f18413182e1e055b830cb56af4095b73eafc8f92ab8",
      "blockNumber":1,
      "blockHash":"0x1a9eddec415173ef63af4b07831310153e4a4f432f927dd09357f29dce318195",
      "type":"create"
   }
]

Note that result.code was empty but now it contains the revert reason data.

I would suggest adding one more field (named output, revertData or something else) to the CreateResult struct - https://github.com/gakonst/ethers-rs/blob/68bc044a88a04ef771e395cadcbc929fd33cfdbd/ethers-core/src/types/trace/filter.rs#L165-L175 to distinguish between the contract code and revert reason data for create traces.

[mattsse]

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()?

[michprev]

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.

[michprev]

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

[Evalir]

This PR is stale and anvil is about to be migrated to use a different provider and RPC types, eventually enabling us to inherit reth's tracers. I will close this PR as it'd be duplicate work to get this in and then move the tracers, and we'll open an issue instead. Tracking on #6197