feat: api: improve the correctness of Eth's trace_block

[Stebalien]

Related Issues

fixes #11217
fixes #11355
fixes #11356
fixes #11219
fixes #11218
fixes filecoin-project/ref-fvm#1793

Proposed Changes

This tracing API is supposed to translate FVM traces to ETH traces, translating FVM semantics (calls, actor creation, etc.) into ETH semantics.

This PR improves the current implementation (I hope):

• Improve encoding/decoding of parameters and return values:
  • Encode "native" parameters and return values with Solidity ABI.
  • Correctly decode parameters to "create" calls.
  • Use the correct (ish) output for "create" calls.
  • Handle all forms of "create".
• Make robust with respect to reverts:
  • Use the actor ID/address from the trace instead of looking it up in the state-tree (may not exist in the state-tree due to a revert).
  • Gracefully handle failed actor/contract creation.
• Improve performance:
  • We avoid looking anything up in the state-tree when translating the trace, which should significantly improve performance.
• Improve code readability:
  • Remove all "backtracking" logic.
  • Use an "environment" struct to store temporary state instead of attaching it to the trace.
• Fix random bugs:
  • Fix an allocation bug in the "address" logic (need to set the capacity before modifying the slice).
  • Improved error checking/handling.

There are still some "not great" things here, but I'm reasonably happy with the result.

Limitations

• No "rewards". We could, but I don't yet do that and don't intend to fix that in this patch.
  • Eth API: Trace block rewards in the Ethereum JSON-RPC trace_block output #11633
• No "selfdestruct". Given access to intermediate state along with the state-root of every actor before/actor, we could fish out this information. But it's going to be a non-trivial amount of work.
  • Eth API: Trace selfdestructs in the Ethereum JSON-RPC trace_block output #11634
• All "create" calls return 0xfe as the contract's bytecode instead of the correct. To fix this, we need intermediate state (state discarded during block execution). This could be fixed in the future.
  • Eth API: Trace installed bytecode in the Ethereum JSON-RPC trace_block output #11635

Additional Information

Instead of describing how this logic is supped to work in the PR description, I'm hoping that the comments I've left inline will suffice. If they don't, we need to fix that.

The main entry-point is buildTraces in eth_trace.go.

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[Stebalien]

So, I'm supposed to return the contract's bytecode here. Unfortunately, I can't do that reliably because reverts (the bytecode may not actually exist in the state after the block is executed).

Options include:

1. Adding this information to the trace (that's how we've fixed all of our other "revert" issues). But, I really don't want to do that.
2. Keep this as 0xFE.
3. Omit Output entirely?
4. Return null?

[i-norden]

Is it feasible to return the bytecode in the case where a reversion does not occur, and in cases where it does return 0xFE? Alternatively, if it is easier to return the bytecode whether or not a reversion occurs I think that would be preferable. If neither of those are options then I think returning 0xFE is best as that differentiates it from the nil value which may be returned elsewhere for other reasons.

[Stebalien]

You know, now that i think of it, no. Because:

1. We only flush the state at the end of the block, not at the end of every transaction.
2. A contract can be repeatedly created/destroyed (with the same init code but different bytecode), once per transaction.

That is...

TxA:

1. ContractA: calls Create2 with INITCODE.
2. ContractT (new): INITCODE loads the bytecode from the caller, and returns it.
3. ContractA: does some stuff, then calls back into ContractT.
4. ContractT: Calls SELFDESTRUCT.

TxB:

1. ContractB: calls Create2 with INITCODE (same INITCODE as above).
2. ContractT (re-created): INITCODE loads the bytecode from the caller (ContractB this time) and returns it. This is now different bytecode.
3. ContractB: does some stuff, then calls back into ContractT.
4. ContractT: Calls SELFDESTRUCT.

By the end of the block, even with no reverted transactions, the ContractT could have changed its bytecode any number of times.

[Stebalien]

This is fixable, it just requires even more changes to the FVM:

1. Record an actors state-object when it returns (instead of only when we call it).
2. Add a flag to keep all the intermediate state, and use it when tracing. But that could produce a lot of garbage and I'd only be comfortable doing it if we passed in a special separate datastore per-trace.

The upside is that this would give us nearly complete information on intermediate states in the system. So it's not a bad idea, just something that would likely have to happen in a followup PR.

[Stebalien]

Maybe we should fix this?

[Stebalien]

This is now working (tested locally). However, I still need to figure out how to write real tests. My thinking is:

1. Unit test some of the encoding/decoding functions.
2. Dump a bunch of FVM traces and their corresponding Eth traces to files and then test to make sure we (a) get the same results every time and (b) everything (paths, etc.). Make sense.

Thoughts on this plan?

[Stebalien]

You should be able to test with:

trace_block() {
    local blk="$(printf '0x%x' $1)"
    local req="{\"jsonrpc\": \"2.0\", \"id\": 0, \"method\": \"trace_block\", \"params\": [\"$blk\"]}"
    curl -H "Content-Type: application/json" -X POST --data "$req"http://localhost:1234/rpc/v1
}

Assuming you have a local node with the Eth API enabled.

[Stebalien]

@fridrik01 I believe this field is correct, but correct me if I'm wrong.

[i-norden]

That looks correct to me based on erigon. Erigon trace result also "omitempty" on blockhash, blockNumber, transactionHash, and transactionPosition.

[Stebalien]

Moved to a separate environment type to avoid polluting the trace.

[Stebalien]

I originally said we should do this but... I changed my mind.

[Stebalien]

I believe this change is correct. CC @fridrik01.

[i-norden]

This is correct based on the erigon code.

[fridrik01]

lgtm, some minor nits

[fridrik01]

its not clear where the 33 is coming from?

[fridrik01]

where are these used?

[Stebalien]

lotus/node/impl/full/eth_trace.go

Lines 142 to 159 in cf910a8

	switch code {
	case evm.ErrReverted:
	return "Reverted" // capitalized for compatibility
	case evm.ErrInvalidInstruction:
	return "invalid instruction"
	case evm.ErrUndefinedInstruction:
	return "undefined instruction"
	case evm.ErrStackUnderflow:
	return "stack underflow"
	case evm.ErrStackOverflow:
	return "stack overflow"
	case evm.ErrIllegalMemoryAccess:
	return "illegal memory access"
	case evm.ErrBadJumpdest:
	return "invalid jump destination"
	case evm.ErrSelfdestructFailed:
	return "self destruct failed"
	}

[Stebalien]

They're returned by the EVM on error: https://github.com/filecoin-project/builtin-actors/blob/6e781444cee5965278c46ef4ffe1fb1970f18d7d/actors/evm/src/lib.rs#L35-L42

[Stebalien]

Issue for writing more tests: #11638