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 |
d462c3e to
5a7f0c7
Compare
5a7f0c7 to
cb9c2b9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Replay still diverges from canonical processing for synthetic log indexes and sequential TRC21 fee-capacity accounting.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes debug tracing replay so transaction routing and trace frames match canonical block processing.
Changes:
- Adds a shared routing-aware replay path.
- Corrects synthetic versus real call frames.
- Adds regression coverage for nonce, root, and tracer behavior.
File summaries
| File | Description |
|---|---|
| eth/tracers/native/call.go | Selects real or synthetic call frames correctly. |
| eth/tracers/native/call_test.go | Tests real-frame preservation. |
| eth/tracers/native/call_flat.go | Supports synthetic non-EVM frames. |
| eth/tracers/native/call_flat_test.go | Tests flat tracer frame handling. |
| eth/tracers/internal/tracetest/non_evm_trace_test.go | Tests routing through production processing. |
| eth/tracers/api.go | Replays block state through shared routing. |
| eth/tracers/api_test.go | Adds block, transaction, and root regressions. |
| eth/state_accessor.go | Uses routing-aware transaction replay. |
| eth/state_accessor_test.go | Tests nonce preservation during replay. |
| core/state_processor.go | Centralizes routing and adds replay processing. |
Review details
Suppressed comments (2)
eth/tracers/internal/tracetest/non_evm_trace_test.go:74
- The test only sends to
TradingStateAddrBinaryand toggles the receiver fork, so it never exercisesrouteSign/ApplySignTransaction. Add a case usingBlockSignersBinarywithTIPSigningBlockactive; this is especially important because the PR's reportedflatCallTracerfailure involved a sign transaction and the new replay branch has distinct nonce and log behavior.
key, _ := crypto.GenerateKey()
from := crypto.PubkeyToAddress(key.PublicKey)
to := common.TradingStateAddrBinary
blockNumber := common.Big1
eth/state_accessor.go:266
- These replay-error returns discard the
releasecallback obtained fromStateAtBlock(..., readOnly=true), so repeated failed traces can retain live trie references. This leak predates this replay change and is already addressed by the linked #2578; ensure that dependency lands before this branch is merged, or include the same deferred release guard here.
if err := core.ApplyTransactionForReplay(msg, new(core.GasPool).AddGas(tx.Gas()), block.Number(), tx, evm, balance); err != nil {
// An EVM this replay cannot use is a problem of this caller, not of the
// transaction: report it as it is instead of blaming the transaction.
if errors.Is(err, core.ErrReplayTracingEVM) || errors.Is(err, core.ErrReplayStateType) {
return nil, vm.BlockContext{}, nil, nil, err
}
return nil, vm.BlockContext{}, nil, nil, fmt.Errorf("transaction %#x failed: %v", tx.Hash(), err)
- Files reviewed: 10/10 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.
eae1995 to
96fd012
Compare
96fd012 to
411dc8f
Compare
411dc8f to
bb21871
Compare
bb21871 to
09cb6e8
Compare
09cb6e8 to
a5b9cbb
Compare
a5b9cbb to
0201b80
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Pre-Byzantium replay redundantly hashes trie state for every transaction, adding avoidable cost to historical tracing.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Shared state-transition routing and historical fork behavior require final human validation.
Review details
Suppressed comments (1)
eth/tracers/api_test.go:1467
- This helper does not pin its returned last root to
block.Root():rootsis populated before consensus finalization, andethash.Finalizethen credits the block reward beforefinal.Root()is checked. The comment currently describes a guarantee the helper deliberately does not provide; clarify that only the separately finalized state is checked against the canonical root.
// replayIntermediateRoots rebuilds the per-transaction state roots of a block with the
// block processing entry point, giving tests an independent expectation for the tracer
// result. It also closes the block the way chain generation does, so the last root is
// pinned to the block root on chain rather than to another replay path.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
a844fec to
76e7c3c
Compare
76e7c3c to
28e2f19
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Core replay-routing changes affect consensus-adjacent state transitions, and trie-reference cleanup remains unresolved.
Review details
Suppressed comments (1)
eth/tracers/api_test.go:1544
- The only
IntermediateRootsregression fixture forces receiver routing active. It covers the missing-root case, but not the removed skip's outside-window failure, where the system-address transaction must execute through the EVM and increment the nonce before a nonce+1 follower. Please parameterize this test with an inactive/disabled-fork case and assert both roots there as well.
config := *params.TestChainConfig // TIPXDCXBlock is 0: the receiver fork is active
config.TIPXDCXReceiverDisableBlock = nil
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
28e2f19 to
81daad8
Compare
34ba45d to
5c7a5cb
Compare
…ing activation block Block processing removes the legacy block signers account before the first transaction of the block that activates TIPSigning, but none of the replays did it: stateAtTransaction, debug_intermediateRoots and the blocks debug_traceChain walks rebuild the pre-state of that block from the parent state, so the transaction traces and every intermediate root of the activation block describe a state the chain never had, one that still holds an account canonical execution had already removed. Share the removal as core.ApplyTIPSigningHardFork, call it from the three replay paths, and replace the three copies in Process, ProcessBlockNoValidator and the miner with it. The condition fires on the activation block only and all three copies already agreed on it, so block processing and the miner build the same block as before. The chain makers had the same gap: they mirror the block level mutations of the DAO fork and of Prague, so a generated chain that activates TIPSigning inside it produces blocks whose root does not match what block processing computes, and such a block cannot be imported at all. They now run the shared helper too. Add TestIntermediateRootsMatchesBlockProcessingAtTIPSigningActivation, which traces a block that activates TIPSigning over a parent state holding the legacy account, pins that premise from both sides (the account is in the parent state and gone from the state the imported block carries) and compares the intermediate roots against a block processing replay pinned to block.Root(); without the call the very first root already differs. This is a pre-existing gap: the base commit replays those blocks through ApplyMessage and without the removal as well. geth has no TIPSigning fork, so there is no upstream fix to port. TestIntermediateRootsMatchesBlockProcessingAtTIPSigningActivation carries its own mirror of the block processing replay instead of building on the helper of the intermediate-roots fix, so this commit can be reviewed and imported on its own.
5c7a5cb to
facfa93
Compare
|
Branch renamed to |



Proposed changes
Block processing removes the legacy block signers account (
common.BlockSignersBinary) before the first transaction of the block that activates TIPSigning (mainnetTIPSigningBlock3,000,000), the same way it applies the DAO hard fork at the DAO fork block. None of the replays ran that removal:stateAtTransaction(the replay behinddebug_traceTransactionanddebug_traceCall),IntermediateRoots(behinddebug_intermediateRoots) and the per-block state buildertraceChainuses all rebuild the block from its parent state, so they keep an account canonical execution had already deleted, and the traces and every intermediate root of the activation block describe a state the chain never had.The chain makers had the same gap, which is what makes it visible in tests rather than only on the live networks:
GenerateChainmirrors the block level mutations of the DAO fork and of Prague, so a generated chain that activates TIPSigning produces a block whose root does not match what block processing computes, and such a block cannot be imported at all.Symptoms
debug_intermediateRootsroot differs.GenerateChainproduces a state root block processing does not reproduce (invalid merkle root).Fix
The removal moves into
core.ApplyTIPSigningHardFork, whichProcess,ProcessBlockNoValidatorand the miner call instead of each keeping its own copy of the condition, and which the three replay paths above andGenerateChaincall as well. The condition is unchanged — it fires on the activation block only — so block processing and the miner build the same blocks as before.Upstream
No upstream fix to port: geth has no TIPSigning fork, so the block-level removal the replays missed has no counterpart. It has no non-EVM transaction concept either, which is why the
ApplyMessagereplays the other two PRs of this series replace have none.Tests
eth/tracers/api_tipsigning_activation_test.go—TestIntermediateRootsMatchesBlockProcessingAtTIPSigningActivation: traces a block that activates TIPSigning over a parent state that holds the legacy account, pins that premise from both sides (the account is in the parent state, and it is gone from the state the imported block carries) and compares the intermediate roots against a block processing replay pinned toblock.Root(); without the call the very first root already differs. The file carries its own mirror of the block processing replay, so it does not build on the helper the intermediate-roots PR adds.End-to-end verification
The branch binary was run against archive nodes of both live networks, on the same data directory and the same node as the baseline binary (
dev-upgrade@cdce8fc5c): on both networks this branch imported testnet and mainnet segments normally, with no bad block, no panic and no error attributable to the change.The activation block itself was not probed on the live networks, because it is far outside the segments the archive nodes re-import. Its coverage is the unit test above, which pins its own premise from both sides and checks the comparison path against
block.Root().Manual test plan: start a node on mainnet archive data with
--rpcapi debugand calldebug_intermediateRootsanddebug_traceTransactionon mainnet block 3,000,000; the baseline binary replays that block without the removal, so its first intermediate root already differs from the one block processing produces.Types of changes
Impacted Components
Checklist
Relation to other work
This is the third of three PRs that split a single debug tracing series, so every defect is reviewable on its own.
#2587 carries the shared block processing routing and the replay entry point, and rewrites the
stateAtTransactionreplay — thenonce too lowofdebug_traceTransaction. #2588 fixes the missing intermediate roots of a block. Neither is a prerequisite for this one: the removal introduced here is independent of the routing they share, and the call sites it adds are all in code the base already has.This PR carries its own mirror of the block processing replay in its test, because the shared helper comes with #2588. When the other two land, that copy is dropped in favour of the shared helper, which is the only change this branch needs on rebase.
#2584 carries the two
eth/tracersfixes this series used to contain —callTracershadowing the real top-level frame of a system-address transaction, andflatCallTracerfailing the whole call withinvalid number of callson a block that carries a non-EVM transaction.#2581 fixes the unconditional skip in
traceBlockand in the state feeder of the JS tracer path — thenonce too highand thenullhole indebug_traceBlock*.#2578 fixes the give-up paths of the same
stateAtTransactionfunction #2587 rewrites.