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 |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
and
a balanced review from Copilot
September 19, 2026 10:22
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
eth_simulateV1 still omits the TIPSigning activation transition and can simulate against a noncanonical pre-state.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Centralizes the TIPSigning activation state transition so block generation and debug tracing reproduce canonical execution.
Changes:
- Adds
ApplyTIPSigningHardForkand integrates it into processing, mining, generation, and tracing paths. - Adds an activation-block intermediate-root regression test.
- Updates tracing test backends to apply the transition.
| File | Description |
|---|---|
core/state_processor.go |
Centralizes the TIPSigning state transition. |
core/chain_makers.go |
Applies the transition during chain generation. |
miner/worker.go |
Uses the shared helper while building blocks. |
eth/state_accessor.go |
Corrects transaction replay pre-state. |
eth/tracers/api.go |
Corrects block and intermediate-root replay state. |
eth/tracers/api_test.go |
Updates tracing test backends. |
eth/tracers/api_tipsigning_activation_test.go |
Tests activation-block roots and import consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gzliudan
changed the title
gzliudan
changed the title
gzliudan
force-pushed
the
fix-trace-replay-tipsigning
branch
from
September 19, 2026 10:58
09b1eb4 to
8cd497f
Compare
Block processing removes the legacy block signers account before the first transaction of the block that activates TIPSigning, and Process, ProcessBlockNoValidator and the miner each carried their own copy of that condition and of the deletion. Pull them into core.ApplyTIPSigningHardFork, so that a path which has to rebuild a block can run the same removal, and replace the three copies with a call to it. Two of the copies guarded on blockNumber.Sign() > 0 and the miner copy did not; the shared condition keeps that guard, which holds for every block the miner builds, so block processing and the miner produce the same state as before. No behaviour change.
…Signing GenerateChain mirrors the block level mutations of the DAO fork and of Prague, so a generated chain is expected to carry blocks whose root matches what block processing computes. It never ran the TIPSigning one, so a chain that activates the fork inside it is generated with a root block processing does not reproduce, and such a block cannot be imported at all (invalid merkle root). Run the shared removal there too. This is a pre-existing gap: the base commit carries the same GenerateChain without it. geth has no TIPSigning fork, so there is no upstream fix to port.
…ivation block debug_traceTransaction and debug_traceCall (stateAtTransaction), debug_intermediateRoots (IntermediateRoots), the blocks debug_traceChain walks and the parallel feeder of traceBlock all rebuild the pre-state of a block from its parent state, and none of them ran the block level removal the activation block performs before its first transaction. The transaction traces and every intermediate root of that block therefore describe a state the chain never had, one that still holds an account canonical execution had already removed. Call core.ApplyTIPSigningHardFork in all four paths. 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. The test 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. 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.
…_simulateV1 eth_simulateV1 rebuilds a block from a selectable base state: processBlock applies the state overrides and the Prague parent block hash, then executes the calls. It ran none of the other block level state changes block processing performs before the first transaction, so simulating the block that activates TIPSigning over its parent kept the legacy block signers account canonical execution had already removed, and returned calls and a state root for a pre-state the chain never had. Call core.ApplyTIPSigningHardFork before the first simulated call, the same helper the replay paths use. Add TestSimulateV1AppliesTIPSigningActivation, which simulates that block twice, over a base state that holds the legacy account and over one that does not, and fails when the two do not land on the same state root; without the call the first one keeps the account and lands elsewhere. The account has to come from the genesis allocation, because DeleteAddress removes it from the trie only and a state override would be written back by the block finalisation. This is a pre-existing gap: the base commit carries the same processBlock and the same missing removal. geth has no TIPSigning fork, so there is no upstream fix to port.
gzliudan
force-pushed
the
fix-trace-replay-tipsigning
branch
from
September 19, 2026 11:10
8cd497f to
ad4f2f0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.eth_simulateV1was a fourth path:processBlockrebuilds a block from a selectable base state, applies the state overrides and the Prague parent block hash, and then executes the calls, but it ran none of the other block level state changes block processing performs before the first transaction. Simulating the activation block over its parent therefore kept the legacy account there too, and returned calls and a state root for a pre-state the chain never had.Symptoms
debug_intermediateRootsroot differs.GenerateChainproduces a state root block processing does not reproduce (invalid merkle root).eth_simulateV1returns calls and a state root for the activation block computed on a pre-state that still holds the legacy account, when the caller selects that block's parent as the base state.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,GenerateChainandeth_simulateV1'sprocessBlockcall as well. The branch is four commits: one behaviour-preserving refactor that shares the removal, plus one fix for each path group (the chain makers, the tracing replays, andeth_simulateV1). 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.internal/ethapi/simulate_tipsigning_test.go—TestSimulateV1AppliesTIPSigningActivation: simulates the activation block twice, over a base state that holds the legacy account and over one that does not, and requires both to land on the same state root; without the call the first one keeps the account and lands elsewhere. The account has to come from the genesis allocation, becauseDeleteAddressremoves it from the trie only and a state override would be written back by the block finalisation.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
eth_simulateV1, plus the TIPSigning removal they now share with the miner and the chain makers)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.
#2589 carries the shared block processing routing and the replay entry point, and rewrites the
stateAtTransactionreplay — thenonce too lowofdebug_traceTransaction. #2590 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.Its scope is the missing block level TIPSigning removal on every path that rebuilds a block's pre-state: the three tracing replay paths, the chain makers and
eth_simulateV1, which is why the title covers more than tracing.This PR carries its own mirror of the block processing replay in its test, because the shared helper comes with #2590. 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 #2589 rewrites.