REVM tracing and gas reports

[onbjerg]

Ports tracing to REVM (see comments below)

Closes #753
Closes #775

[onbjerg]

Current status: Slowly refactoring towards separating trace collection, decoding and printing.

Plan:

• Traces are collected as raw as possible (done)
• Traces are decoded by user-specified decoders. The forge crate will provide some (in progress)
• Moving towards implementing Display for CallTraceArena etc (in progress)

There will be a few commits that make the code a bit worse but that is because I am working through this in stages (e.g. unifying contract names and labels, returning a string instead of building on one we pass around etc.)

[onbjerg]

Traces are now decoded and displayed, with two bugs that I have found so far:

• The test contract itself is not currently identified because we do not trace the deploy call, so we do not have the code in the trace arena (I know how to fix)
• DSTest logs are not displaying correctly in the traces (investigating)

Required missing things:

• Tracing for fuzz tests
• Porting gas reporting

Things I want to add/achieve:

• Etherscan identifier if we are running in fork mode
• Clean up the decoder a bit
• Clean up ContractRunner - it's a little messy, but I will do this in another PR since it is not required for the port

@gakonst you talked about decoding using 4byte directory. We can do this in the decoder as a fallback if you want, but I'm not sure where we have access to it currently?

[gakonst]

looks great so far- the etherscan decoder is going to be a game changer

[onbjerg]

This should be ready to review, with two caveats:

• There is no Etherscan identifier yet
• Calls to Hardhat's console.log are displayed in traces - how should I proceed here? The current behaviour is that they are not in the traces. Two options: we filter them out, or we try to decode them which would require us to patch the function selectors in the tracer.

[onbjerg]

Spawning the MultiContractRunner on a separate thread instead of the print loop made it a lot easier to integrate with the gas reports.

I should probably have a function in MultiContractRunner that does not collect results itself, since it would save us a lot of clones

[onbjerg]

The CallTraceArenas from ContractRunner are now identified by what part of the flow they are from, since we still need to at least identify contracts in the deployment traces. We only decode the ones we want to display, though.

Tried to explain the conditionals here as best I could

[brockelmore]

nice

[onbjerg]

I simplified most of this and merged it with the final trace/log collection step we used to have in ContractRunner

[onbjerg]

Moved here from ContractRunner

[onbjerg]

The decoder collects information about addresses in the call traces (i.e.. ABIs, labels) using TraceIdentifiers, which allows us to pull info from different sources (locally, vm.label, 4byte directory, etherscan, ...). It also does the final decoding step for a CallTraceArena

[onbjerg]

I put this in a function by itself in case we want to do more cheatcode-specific decoding

[gakonst]

this is great - my main question is around how we'll get the etherscan identifier working

[gakonst]

Is there any easy way to make these functions no-ops if a condition is set? @brockelmore did that in an if/else which duplicates some code here. Would probably need a trait here?

trait Colorizer {
        fn noop(&self) -> bool;

        // do this for all `Cell`s
        fn yellow(cell: Cell) -> Cell {
           if !self.noop() { cell.fg(Color::Yelow) } else { cell }
        }
}

struct Noop;

impl Colorizer for Noop {
        fn noop(&self) -> bool { true }
}

struct Colors;

impl Colorizer for Colors {
      fn noop(&self) -> bool { false }
}

let col: Box<dyn Colorizer> = if self.stdout { Box:new(Noop) } else { Box::new(Colors) }
col.magenta(col.bold(Cell::new("Function Name")))

[onbjerg]

Should be possible without a trait since .color takes self and returns Self, so something like

struct CellColorizer {
  should_color: bool
}

impl CellColorizer {
  fn color(cell: Cell, color: ...) -> Cell {
    if self.should_color {
      cell.color(color)
    } else {
      cell
    }
  }
}

let colorizer = CellColorizer::new(self.stdout);

colorizer.color(Cell::new("Function Name"), Color::Magenta)

Same for attributes like bold etc. (maybe CellFormatter is a better name)

[gakonst]

clean

[gakonst]

How would an EtherscanTraceIdentifier look like? If we have many addresses, we'd like to make all etherscan/4byte dir requests concurrently. Should the trait maybe have a fn identify_addresses function which would fetch Name/Label/ABI for all address/code pairs provided? This would allow us to join all the etherscan futures and block_on all of them at the same time (vs having to block in a loop).

[gakonst]

So we'd add a new identifier here for Etherscan, and then for each trace call identify? I wonder if there's a way to pull this "out" so that we can do 1 identification step for etherscan across all traces

[brockelmore]

We should be able to do something like:

if let Some(fork_url) = fork_url {
    let (mut all_addresses, mut _maybe_codes) = arena.addresses_iter().collect();
    let unidentified_contracts = all_addresses.iter().filter(|address| !identified_contracts.contains(address)).collect();
    let etherscan_identified = unidentified_contracts.par_iter().map(|contract| some_etherscan_contract_getter(contract)).collect();
    etherscan_identified.iter().for_each(|(addr, name, abi)| identified_contracts.insert(addr, (name, abi)));
 }

[brockelmore]

note for debugger it gets a little more challenging because we have to not just get abi but get the contract code and rerun compilation pipeline and write the contracts to files :/

[onbjerg]

Ah, right. We can get the source code, byte code and ABI just fine, but there is no way for us to get the sourcemap from the Etherscan API :(

[gakonst]

If we get the source code, we could always compile it with the right version and get it ourselves?

[brockelmore]

Got most of the way thru this but will look more later

[brockelmore]

We need to change this unfortunately... likely to BTreeMap<[u8; 4], Vec<Function>>, reason being, short signatures don't identify differences in output types. So if a user has myFunc(uint256) external returns (uint256) and just myFunc(uint256) external, the one added first will be overwritten and fail to decode.

we should make it a vector (or make the key different, but having the key be the same as the calldata input is nice), and iteratively attempt decoding. Vast majority of the time there will be only one but still.

Doesnt apply to events so that can stay the same

[onbjerg]

Ah, right. That's.. annoying. I'm adding a test to https://github.com/onbjerg/foundry-test that checks for this behavior:

[onbjerg]

I fixed it using a BTreeMap<Address, BTreeMap<[u8; 4], Function>> since the other solution (using a Vec) would have a lot of oddities, notably if a function returns bytes and another one string they could both decode the output so it would show up wrong (i.e. both might be decoded as bytes or string) :/

[brockelmore]

ehhhhhhhhh - not sure we want to do this. this then requires us to have identified the contract. When in actuality, if a sig matches, we should be able to decode it even if we haven't identified that specific contract (which can happen). I would really prefer to remain agnostic to the address and lean more on as better decoding scheme

[gakonst]

@brockelmore what do you have in mind? The Vec approach assumes there's a unique way to decode things as you iterate through the vector, which isn't right for the reason bjerg mentions above, although we could be opinionated and say "if it can show as ascii then go with string, else with bytes", but I suspect this can happen with any kind of fixed size return type, e.g. u256 and bytes32?

Keying by address I think might prevent us from using 4byte directory later on? How would we decode in that case?

[brockelmore]

it should be relatively rare. i accidentally did it awhile back and ended up just changing my contracts. most of the time i think ppl will do it by accident. gently nudging them to change just for better decoding is probably a good idea. Maybe we leave as is (BTreeMap<[u8;4], Function>) and emit a warning?

[gakonst]

I am OK with leaving as-is and maybe doing a trace::warn (not logged by default), or not doing anything at all and waiting until a user bumps on it as a bug and decide then

[mds1]

Doesnt apply to events so that can stay the same

I'm not sure if this affects implementation here/anywhere, but just noting that events do have a similar problem but with indexed parameters.

If you have topic 0xd8c9334b1a9c2f9da342a0a2b32629c1a229b6445dad78947f674b44444a7550, 4byte.directory tells you the sig is event TextChanged(bytes32,string,string). However, that's not enough to decode the event because two of those paramers are indexed but we don't know which. You'd pretty much have to guess which params are indexed, and depending on the types it's possible multiple sigs may decode and you can't tell which is correct

[brockelmore]

We should be able to do something like:

if let Some(fork_url) = fork_url {
    let (mut all_addresses, mut _maybe_codes) = arena.addresses_iter().collect();
    let unidentified_contracts = all_addresses.iter().filter(|address| !identified_contracts.contains(address)).collect();
    let etherscan_identified = unidentified_contracts.par_iter().map(|contract| some_etherscan_contract_getter(contract)).collect();
    etherscan_identified.iter().for_each(|(addr, name, abi)| identified_contracts.insert(addr, (name, abi)));
 }

[brockelmore]

note for debugger it gets a little more challenging because we have to not just get abi but get the contract code and rerun compilation pipeline and write the contracts to files :/

[brockelmore]

nice

[gakonst]

Looks good! Would recommend a fresh rebase of revm on master because a few bugs were fixed lately