go-ethereum
go-ethereum copied to clipboard
eth/tracers: add native flatCallTracer (proxy mode)
This PR adds support for a flat call tracer, Similar to the parity tracing output, although with a few differences that are highlighted below.
Implementation: To minimize code duplication the flatCallTracer
uses callTracer
as a proxy to collect callstack info and at the end formats the output. Initial work on this PR had started here: https://github.com/etclabscore/ethereum.go-ethereum/pull/14
Incompatibilities with Parity
- Parity excludes calls to precompiles (when done via CALL and STATICCALL). We have maintained this behaviour, but added an option
includePrecompiles: true
. - The error messages returned from parity and geth are different. If for compatibility reasons you want to receive parity-style errors pass the option
convertParityErrors: true
. - In Parity
gasUsed
field of the main message does not account for intrinsic gas and refunds. Geth does. - When tracing a whole block Parity will return one result for whole block including one error. Geth will nest the tx results in a
result
field of the response object. And each tx will potentially have its own error. - Parity returns block and uncle rewards. Geth doesn't. Issuance is now handled by consensus clients, not EVM.
-
blockNumber
andtransactionPosition
fields in Parity are decimal numbers. Geth returns them as hex for consistency.
All tests passing in this branch: https://github.com/s1na/go-ethereum/commits/feat/flat-call-tracer. Some semantic differences between parity and our style we need to consider:
- Parity excludes calls to precompiles whereas we include. But weirdly they only exclude for CALL and STATICCALL. Chris suggested adding an option to let user decide. But we should stay consistent and either exclude or include for all CALL* variants.
- Delegatecall value is taken from its parent. in our call tracer we simply report zero for the value. We should consider adopting this change to our call tracer.
- The first frame of a parity trace reports gasUsed as we did before (not accounting for intrinsic gas and refunds at end of tx).
I refactored the result processing logic, i.e. taking the nested call frame, converting it to the flat format and encoding that:
- The encoding is now fully handled by gencodec. I.e. the flat structs store normal types and are overriden to hexutil types by gencodec. This allows us to add RLP encoding more easily in future.
- Many of the fields were pointers. I changed them back to value fields. I tried to only change in places where empty values don't need to be omitted from the JSON. But this is something we have to confirm. Which fields exactly need to be omitted when empty.
@s1na sorry for missing out that the commits were on your fork.
- I really love you approach on using gencodec for handling the encoding.
- With regards struct fields being pointers, this was mostly because of reusing the same struct between tests and application code. In advance for some context fields, we had to use pointers as the context was available only in
debug_traceBlock
family calls and not indebug_traceTransaction
family calls. To verify that everything is being set correctly and we remain compatible with the old Parity tracers, I will spin up an old OE instance and compare the full chain side-by-side with this PR.
While running the comparison tests with core-geth tracers, I found the following incompatibilities so far (Goerli block reached = 5644).
core-geth has been used for running those tests for the following reasons:
- core-geth has been run the same test with OE on ETC Mordor testnet and it is compatible with OE
- both go-ethereum and core-geth are able to sync with a common chain (Goerli is used), and in that sense we can use core-geth to run any tests for any on-chain transaction or even for new ones. OE isn't capable of syncing Goerli after merge, and even before that.
-
debug_traceBlockByNumber
returnstxTraceResult
which adds a nestedresult
field. Oncore-geth
thetrace_block
handles this for compatibility with OE. By keeping it as it is in go-ethereum the user is able to still get trace results with errors for each transaction in the block, while OE returns an error for the whole block. - Dealing with empty fields:
- omit
-
action.author
-
action.address
-
action.refundAddress
-
result.address
-
- keep
-
action.gas
-
action.input
-
result.gasUsed
-
result.output
-
- omit
Those have been touched at https://github.com/ethereum/go-ethereum/pull/26377/commits/a402ba8b90259fb88d145e3595045fb1962e1dcd and I said I will run tests and report any diffs.
-
blockNumber
is a number and not hex in OE. We can keep it as hex for consistency with other methods. -
transactionPosition
is a number and not hex in OE. - core-geth and OE, return entries for block and uncle rewards.
@s1na I am going to handle 2 at the moment in order I can continue testing. For the rest, I will just tell the tests to ignore the changes, until we discuss.
Thanks for the fixes!
- Yes I'd rather we keep the
traceBlock
API as is and not change it for a specific tracer. - I hate that now basically every field has to be a pointer. It can lead to weird behavior. For example I remember I had to copy the
from
andto
addresses, but don't remember the exact bug I was trying to address (see https://github.com/ethereum/go-ethereum/pull/26377/commits/b2eeb0dc51d7d01032ddaa40a17715688d4868b6). I wish there was another way. Can you please copy the addrs as the mentioned commit? - Keep hex for consistency
- Keep hex for consistency
- Rewards have changed since the merge. There are no more uncles and block reward is handled by the beacon chain. Tho we could add a field for all gas fees going to the miner?
- I know what you mean, as I found this in the past too. I handle another case as well by this https://github.com/ethereum/go-ethereum/pull/26377/commits/af653978b4dafd123af14c7df2a426eb1e59f98b which covers what you mentioned. I still have to verify at Mordor, where we have this bug. Are you ok, handling it this way.
@ziogaschr I will think about an alternative approach. But I won't hold this PR because of it.
I updated the description of the PR to reflect all points on which we differ from Parity. Could you please add if I missed something? IMO 2. (delegated call value) and 5. (gas fees going to coinbase) can be considered. On other points I would stick with current behavior.
Sounds great. I will verify things in description and I will also verify that the code reflects them after I finish with the compatibility testing. In the same time I will note anything else found in code.
- (delegated call value)
Can you please explain more on this one?
- (gas fees going to coinbase)
I can see that this might be useful to have. Will you add this as a new result in the array?
Can you please explain more on this one?
You cannot send value via DELEGATECALL. Similar to how the callee will inherit the storage of caller, it will also inherit the value that was sent to caller. Parity explicitly returns this value for delegatecall frames. But we historically returned 0 well because this call doesnt have a value of its own. But I can see the point in how parity does it.
Will you add this as a new result in the array?
Maybe in the same field as block reward only with a slightly different meaning post-merge.
@s1na I totally get it now for "2. (delegated call value)". I couldn't follow the context because of the "2." prefix, which felt like you were answering to point 2 from the above list
Team discussion: we should add delegate call value to the existing callTracer in a separate PR (as in our normal calltracer should also like parity return the parent's value for delegatecall frame).
However gas fees going to coinbase is unnecessary to add. It can be computed for each tx by gasPrice * gasUsed easily.
Otherwise PR is very good!
@s1na PR #26632 handles the delegatecall frame, though I removed this logic from this PR of flat call tracer. Based on this change a test fails, until #26632 is merged. After the merge, we can merge this PR to bring it in and have the tests pass. What else we can do, is to already merge #26632 in this one.
I pushed some changes discussed with @s1na, that we decided to keep for better compatibility with Parity and in order the end users, who already use this tracer on other clients can continue using it on geth by passing the needed options to the tracer.