feat(core): extends tracing.Hooks with OnSystemCallStartV2 #30786#2073
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the tracing infrastructure to provide richer context for system call tracing by adding a new OnSystemCallStartV2 hook that receives VMContext as a parameter. Additionally, it refactors several function signatures to eliminate redundant parameters by deriving configuration and state from the EVM instance.
Changes:
- Added
OnSystemCallStartV2hook to the tracing.Hooks interface that provides VMContext for system calls - Refactored
ApplyTransactionandApplyTransactionWithEVMto derive ChainConfig from the EVM instance instead of passing it separately - Refactored
ProcessParentBlockHashto access StateDB through evm.StateDB instead of passing it as a separate parameter
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/tracing/hooks.go | Added new OnSystemCallStartV2 hook type and field to Hooks struct |
| core/state_processor.go | Implemented onSystemCallStart helper with backward compatibility, refactored function signatures to reduce parameter duplication |
| core/state_processor_test.go | Updated test calls to match new function signatures |
| core/chain_makers.go | Updated ApplyTransaction and ProcessParentBlockHash calls |
| miner/worker.go | Updated ProcessParentBlockHash and ApplyTransaction calls |
| eth/tracers/api.go | Updated ProcessParentBlockHash and ApplyTransactionWithEVM calls |
| eth/state_accessor.go | Updated ProcessParentBlockHash call |
| internal/ethapi/simulate.go | Changed sanitizeCall parameter from concrete *state.StateDB to interface vm.StateDB |
| consensus/tests/engine_v1_tests/helper.go | Updated ApplyTransaction call |
| consensus/tests/engine_v2_tests/helper.go | Updated ApplyTransaction call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aba7ef2 to
3b62d27
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tracer := evm.Config.Tracer; tracer != nil { | ||
| if tracer.OnSystemCallStart != nil { | ||
| tracer.OnSystemCallStart() | ||
| } | ||
| onSystemCallStart(tracer, evm.GetVMContext()) | ||
| if tracer.OnSystemCallEnd != nil { |
There was a problem hiding this comment.
OnSystemCallStartV2 receives evm.GetVMContext() before evm.SetTxContext(NewEVMTxContext(msg)) is applied. Since GetVMContext() includes GasPrice from evm.TxContext, the V2 hook can observe the previous tx gas price instead of the system-call context (0), and it will be inconsistent with the subsequent EVM call tracing. Consider setting the tx context first (or constructing a VMContext with the intended gas price) before invoking the start hook.
| func onSystemCallStart(tracer *tracing.Hooks, ctx *tracing.VMContext) { | ||
| if tracer.OnSystemCallStartV2 != nil { | ||
| tracer.OnSystemCallStartV2(ctx) | ||
| } else if tracer.OnSystemCallStart != nil { | ||
| tracer.OnSystemCallStart() | ||
| } |
There was a problem hiding this comment.
New behavior (OnSystemCallStartV2 preference + fallback to OnSystemCallStart) isn’t covered by tests. Since this affects tracer callbacks, please extend the existing state processor tests (there are already tests for ProcessParentBlockHash) to assert: (1) V2 is invoked with a non-nil VMContext when set, and (2) legacy OnSystemCallStart is still invoked when V2 is nil.
Proposed changes
This PR extends the Hooks interface with a new method, OnSystemCallStartV2, which takes VMContext as its parameter.
Motivation
By including VMContext as a parameter, the OnSystemCallStartV2 hook achieves parity with the OnTxStart hook in terms of provided insights. This alignment simplifies the inner tracer logic, enabling consistent handling of state changes and internal calls within the same framework.
Ref: ethereum#30786
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that