eth/tracers: add pc field to callTracer #33107
Conversation
|
There are some failing tests ATM, not sure what's the cleanesst way to update the tests -- I've read the guide for creating tracer test files for a given TX, but I couldn't find the transaction hashes to regenerate the files. |
eth/tracers/native/call.go
Outdated
| // Position of the log relative to subcalls within the same trace | ||
| // See https://github.com/ethereum/go-ethereum/pull/28389 for details | ||
| Position hexutil.Uint `json:"position"` | ||
| Pc uint64 `json:"pc"` |
There was a problem hiding this comment.
Any particular reason to include the PC in the emitted logs?
There was a problem hiding this comment.
Same idea as the calls, map the logs to the appropriate LOG instruction in the emitting contract.
There was a problem hiding this comment.
We need a bit more info on the purpose. It doesn't make sense to me on the first look.
There was a problem hiding this comment.
The PC in the call frames helps map back to the CALL instruction of the caller. Similarly, the PC in the log objects helps map back to the LOGX instruction that emitted the log. I included the PC in both because both are "entities" in the call tracers return payload.
This kind of information is really useful when debugging transactions/working with call traces.
There was a problem hiding this comment.
It sounds like a very advanced usage, or probably just over-complicated.
Do you really need to map the logs to the execution steps?
There was a problem hiding this comment.
I think it's weird to have the program counter for calls but not for the logs -- both are important entities in the trace that benefit from this information.
It's pretty standard in transaction debuggers, see this tx trace from Tenderly: https://dashboard.tenderly.co/tx/0x5a73efb84b8ba5ca688f4f6140dac5ba7222a1f980239a6942137e6a3dabe2e7?trace=0.1.8
With the program counter for the last emitted LOG, we can map it to the correct emit line in source, which is very useful when debugging transactions.
There was a problem hiding this comment.
Any update on this? I don't find having a PC for the logs an advanced use case -- every tool/use case that can map calls to callsites using the PC in the call frames, they can do the same for emitted logs.
|
Please add comments to the struct fields. It is unintuitive and needs to be explained. |
Added some comments to the new struct fields |
|
This is causing a bunch of tests to fail, because the expected result doesn't have the new field. They need to be updated. |
I'm aware, but I'm not sure what transactions were used to generate the current tests. Should I just get whatever is the new output in the tests and set that as the expected result? |
Yes I think that's ok. |
|
I've updated the test files. I've also included a small change to assist with future changes to the trace format. More specifically, I've added an optional environment variable that if set will update the test files while the tests are running. The invocation for this looks like: I should probably document this somewhere but I'm not sure where it would be best. |
|
Thanks for the update! I can certainly get behind the UPDATE_TESTS flag, but what I don't like is that it changes the order of the keys in the whole test artifact. Notably it differs from the ordering of the JS |
|
Yeah understandable, the change in the JSON key order was also something that concerned me. I'll try to restore the original key ordering |
3 participants
This PR adds a
pcfield to the call tracer's callframes + logs. This field is very useful for debugging transactions and mapping them to the corresponding call sites in the code.While one could implement this as a custom JS tracer, most RPC providers do not support them (at least for lower tiers) and I feel like it's a small but useful change.