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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
🟢 Approval recommended
The implementation and regression coverage address both routing cases; only minor comment inaccuracies remain.
Pull request overview
Fixes block tracing so XDCX system-address transactions follow block-processing routing and remain present in trace results.
Changes:
- Removes unconditional skip-nonce filtering.
- Replays parallel feeder transactions through
ApplyTransactionWithEVM. - Adds fork-active and fork-inactive regression tests.
File summaries
| File | Description |
|---|---|
eth/tracers/api.go |
Aligns serial and parallel tracing with transaction execution. |
eth/tracers/api_test.go |
Covers nonce handling and complete trace results. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
82705cf to
66c3633
Compare
66c3633 to
809c798
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The call tracer can return a synthetic trace for transactions that execute through the EVM outside the fork window.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
debug_traceBlockByNumber, debug_traceBlockByHash and debug_traceBlock dropped every transaction sent to the XDCX system addresses 0x91/0x92/0x93/0x94, with an unconditional tx.IsSkipNonceTransaction() check that does not consult the fork at all. Outside the receiver fork window block processing does execute those transactions and does bump the sender nonce, so dropping them lost exactly the nonce increment the following transaction relies on and the whole call failed with "nonce too high" (mainnet block 37849457). While the fork is active the skip kept the state right, but left a null hole in the result array where the trace of a transaction should be. Two loops skipped them: the transaction loop of traceBlock, and the state feeder of traceBlockParallel, which only tracers that evaluate JS code reach. The feeder had a second divergence: it advanced the state the workers trace against with core.ApplyMessage, which knows nothing about the routing block processing applies, so while the fork is active it bumped a nonce the chain leaves alone. Trace them instead. Both loops now replay through core.ApplyTransactionWithEVM, which routes those addresses the way block processing does, so every transaction of the block gets one entry and the pre-state handed to the next trace stays in step with execution. Add TestTraceBlockSkipNonceTransactions and TestTraceBlockParallelSkipNonceTransactions, which drive TraceBlockByNumber over both receiver fork settings on the serial path and on the parallel one, with the default struct logger and with a named tracer, together with the two fixtures the skip-nonce tests share, skipNonceForkCases and newSkipNonceBackend. The state tracer is registered in init() so that parallel tests cannot race on the tracer directory, and the probe tracer that reports isJS reaches the parallel path with the JS evaluator not linked into the test binary. No corresponding fix exists upstream: geth has no non-EVM transaction concept. Refs: gzliudan/XDPoSChain#371, gzliudan/XDPoSChain#260
809c798 to
83414f1
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The implementation aligns both tracing paths with block execution and the tests cover both nonce-routing branches and result completeness.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Proposed changes
debug_traceBlockByNumber,debug_traceBlockByHashanddebug_traceBlockdropped every transaction sent to the XDCX system addresses0x91/0x92/0x93/0x94, with an unconditionaltx.IsSkipNonceTransaction()check that does not consult the fork at all.Outside the XDCX receiver fork window (mainnet
TIPXDCXBlock38,383,838…TIPXDCXReceiverDisableBlock80,370,900) block processing does execute those transactions and does bump the sender nonce, so dropping them lost exactly the nonce increment the following transaction of the same sender relies on, and the whole call failed withnonce too high. Inside the window the skip kept the state right, but left anullhole in the result array where the trace of a transaction should be.Two loops skipped them: the transaction loop of
traceBlock, and the state feeder oftraceBlockParallel, which only tracers that evaluate JS code reach. The feeder had a second divergence: it advanced the state the workers trace against withcore.ApplyMessage, which knows nothing about the routing block processing applies, so while the fork was active it bumped a nonce the chain leaves alone.Both loops now replay through
core.ApplyTransactionWithEVM, the entry point the serial tracer already uses, which routes those addresses the way block processing does, so every transaction of the block gets one entry and the pre-state handed to the next trace stays in step with execution.Symptoms
nonce too high—debug_traceBlockByNumber/ByHash/traceBlockon a block outside the receiver fork window that contains a transaction to a system address followed by another transaction of the same sender. The skipped transaction is the one that bumped the nonce, so the follower is replayed one nonce short. Mainnet block 37,849,457:tracing failed: nonce too high: address xdcE3bC38418f89C386d1093aee89d606564012e329, tx: 1547093 state: 1547092; the whole call fails, no trace at all.nonce too highon the same block, becauseDefaultDirectory.IsJSanswers true for a name that is not registered, so such a tracer sendstraceBlockthroughtraceBlockParalleland through its state feeder.nullentry instead of a trace for those transactions indebug_traceBlockByNumber/ByHash/traceBlock, whenever the fork is active (mainnet block 39,083,312).Out of scope
debug_intermediateRoots(a root missing for those transactions, and a last root that diverges from the block state root when the block charges a fee),flatCallTracer(invalid number of callson any block containing a sign transaction) anddebug_traceTransaction(nonce too lowon a transaction that follows a system-address transaction while the fork is active) are separate defects with the same root cause. They are not touched here; this PR is limited to the two loops above.Upstream
No upstream fix to port: geth has no non-EVM transaction concept, so neither the unconditional skip nor the trailing
ApplyMessagehave a geth counterpart.Tests
eth/tracers/api_test.go—TestTraceBlockSkipNonceTransactions: drivesTraceBlockByNumberover both receiver fork settings, with the default struct logger and with a named tracer, and asserts the block gets one result per transaction with nonullentry.eth/tracers/api_test.go—TestTraceBlockParallelSkipNonceTransactions: the same overtraceBlockParallel, reached with a registered tracer that reportsisJS = true. Its two sub-cases pin the two directions of the bug, and neither half of the fix passes both: a feeder that keeps the skip fails the outside-the-window case withnonce too high, one that drops the skip but keepsApplyMessagefails the inside-the-window case withnonce too low.init()so that parallel tests cannot race on the tracer directory, and the probe tracer that reportsisJSreaches the parallel path with the JS evaluator not linked into the test binary.End-to-end verification
The binary built from this commit was run against a mainnet archive node, on the same data directory and the same node as the baseline binary (
dev-upgrade@cdce8fc5c), and the tracing RPCs were compared on the same blocks.Mainnet block 37,849,457 (
0x2418971, 204 transactions, outside the receiver fork window; tx 0 goes to0x92, tx 95 to0x90from the same sender with nonce +1):debug_traceBlockByNumber(callTracer)tracing failed: nonce too high: address xdcE3bC38…, tx: 1547093 state: 1547092nulldebug_traceBlockByNumber(an inline JS tracer, the parallel path)nonce too high: address xdcE3bC38…, tx: 1547093 state: 1547092nullMainnet block 39,083,312 (
0x2545d30, 14 transactions, inside the window; tx 0 goes to0x92, tx 5 to0x90from the same sender with the same nonce):debug_traceBlockByNumber(callTracer)nullnullManual test plan: start a node on an archive data directory with
--rpcapi debugand calldebug_traceBlockByNumberon0x2418971and0x2545d30, withcallTracerand with an inline JS tracer; the baseline binary fails both blocks and this branch passes them.Regression: the node imported mainnet segments normally, with no bad block, no panic and no error attributable to the change.
Types of changes
Impacted Components
Checklist
Relation to other work
#2579 contains this commit as its first one (same sha), followed by the fixes for the defects listed under "Out of scope" and a shared
core.ApplyTransactionForReplayentry point. Merging this PR first keeps #2579 a trivial rebase: its first commit is already applied and is dropped by patch id. #2578 toucheseth/state_accessor.goonly and shares no file with this PR.