core: exempt empty no-op blocks from sidechain ghost-state rejection - #2293
core: exempt empty no-op blocks from sidechain ghost-state rejection#2293bit2swaz wants to merge 8 commits into
Conversation
|
@claude review once |
There was a problem hiding this comment.
Pull request overview
Fixes a false-positive “sidechain ghost-state attack” rejection during sidechain import when encountering empty (no-op) blocks whose state root legitimately matches canonical state at the same height, which could otherwise wedge sync after restarts.
Changes:
- Narrow
insertSideChain’s ghost-state rejection by exempting blocks that appear to have made no state transition (root matches parent). - Add regression/security tests covering the empty-block false positive and ensuring the original rejection still triggers for a true shadow-state scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
core/blockchain.go |
Adjusts the ghost-state attack heuristic in insertSideChain to exempt apparent no-op blocks. |
core/blockchain_test.go |
Adds tests reproducing the empty-block sync wedge and asserting the attack path remains rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This narrows a consensus-critical guard in insertSideChain, so I'd like a human to sign off on the ghost-state security reasoning even though the change itself looks well-scoped and correct.
Extended reasoning...
Overview
The PR modifies core/blockchain.go:insertSideChain to exempt empty (no-op) blocks from the ghost-state attack rejection. The fix adds a guard: only reject when the block's root differs from its parent's root — i.e., when the block actually asserts a state transition. When the block's root equals its parent's, it is a genuine empty block that legitimately shares a state root with the canonical block at the same height. If the parent header is missing, the code falls back to the original conservative reject.
Security risks
This is consensus-relevant code — insertSideChain is on the sidechain re-import path triggered by ErrPrunedAncestor. The fix only narrows the rejection, never broadens acceptance for blocks with a real state transition, so it cannot cause the original attack (a forged block claiming canonical state to skip re-execution) to succeed. For the exemption to fire, block.Root() == parent.Root() == canonical.Root(), meaning the block introduces no new state and there is nothing forged to skip-verify. The check remains applied recursively per block in the loop, so a chain of forged non-empty blocks still fails at the first block whose root differs from its parent's. The nil-parent fallback preserves the conservative behavior.
Level of scrutiny
High — this touches security-sensitive chain insertion logic (falls under blockchain-security.md scope) that guards against shadow-state attacks on the pruning mechanism. Even a narrow relaxation of a consensus guard warrants human sign-off. The reasoning is sound and the tests directly assert both the false-positive fix and that the real attack path is still rejected across both hash and path schemes, but a human familiar with the ghost-state threat model and the bor sidechain re-import path should confirm the argument holds.
Other factors
Two new tests exercise the fix and the security counterpart, both across HashScheme and PathScheme. TestSideImportPrunedBlocks (existing) is preserved. The description explicitly references bor's live path (validator-based difficulty, TD, setHead reorgs), suggesting the author validated the code path. SonarQube quality gate passed. The go-ethereum upstream likely has the same bug — worth noting for maintenance.
|
@pratikspatil024 sorry to disturb, but do you reckon i could get a review on this? |
|
@bit2swaz could you address the comments by copilot? |
Address PR 0xPolygon#2293 review feedback: - Copilot: gate the empty-block exemption on the *canonical* chain also being a no-op at that height, measured against the trusted canonical parent header — not only the side block's own parent, which may be unverified/attacker-controlled in insertSideChain. This closes the forge-both-roots bypass where an attacker crafts matching side parent+child roots to skip state verification. - diffguard (Quality metrics): extract the decision into isSidechainGhostState, dropping insertSideChain's complexity and size deltas back to zero. Add TestSidechainGhostStateCanonicalGate covering the hardening: a no-op side block at a height where the canonical chain changed state is still rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 5ac7160 for both. On Copilot's ghost-state point: it's right. the exemption now checks whether the canonical chain was a no-op at that height too using the canonical parent header from On diffguard: pulling the check out into its own function also brought |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2293 +/- ##
===========================================
+ Coverage 55.16% 55.22% +0.05%
===========================================
Files 912 912
Lines 165796 165879 +83
===========================================
+ Hits 91468 91608 +140
+ Misses 68867 68814 -53
+ Partials 5461 5457 -4
... and 24 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
|
@pratikspatil024 could you take a look again whenever youve got the chance? |
|
@bit2swaz - sorry for the delay here. We will pick this up after |
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
hey @pratikspatil024. i saw v2.10.1 is out, so just wanted to give you a nudge on this. lmk if anything is needed from my side |
|
codegenie review |
|
@claude review |
🧞 Codegenie Review✅ No credible findings. CoverageReviewed 3/3 hunks. 🙋 Needs Human Attention
Stats
✅ No FindingsNo credible findings were found. Everything looks good. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed all 3 changed hunks (2 deep, 1 normal); no hunks skipped and verification completed for every finding.
The empty-block liveness fix in (*BlockChain).insertSideChain works for the legitimate case, but the new isSidechainGhostState helper decides "no-op" purely by comparing declared state roots, and one of those roots (sideParent.Root) comes from a sidechain header that insertSideChain itself persisted via writeBlockWithoutState without any execution. At any height where the canonical block is itself a no-op — the common case this PR targets — both predicates are attacker-satisfiable, so the ghost-state guard is skipped and the subsequent ancestor-recovery walk terminates immediately, leaving the side blocks' state transitions unverified. This is reported as one merged medium-severity security finding; both independent reviewers converged on the same root cause (root-equality inference over untrusted headers plus no body-level emptiness check), and it is flagged as an intentional contract change needing author/spec confirmation rather than an accidental regression. Test coverage for the hardening only exercises the state-changing canonical height and the nil-parent fallback, so the residual path is untested.
Reviewed 3/3 hunks.
Coverage levels: deep 2, normal 1, light 0, skip 0.
— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job
There was a problem hiding this comment.
Beyond the confirmed inline finding, I also checked whether Bor's sprint-start state mutations (checkAndCommitSpan / CommitStates in consensus/bor/bor.go) could make isSidechainGhostState misclassify a legitimate no-op sprint boundary — a sprint-start block that actually commits span/state-sync data correctly fails the sideParent.Root == block.Root() no-op check (roots differ) and is therefore not wrongly exempted, so this doesn't introduce an extra false-positive/false-negative path beyond the one already flagged inline.
Extended reasoning...
This run re-verified the previously identified isSidechainGhostState design gap at core/blockchain.go:3815-3826 (the confirmed inline finding: sideParent/block.Root() come from unverified, attacker-controlled headers written via writeBlockWithoutState earlier in the same loop, so the "canonical-gated" no-op check can still be defeated by forging both side headers' roots equal whenever the real canonical chain happens to be a no-op at that height). Beyond that, this run specifically traced Bor's sprint-start Finalize path (IsSprintStart → checkAndCommitSpan/CommitStates in consensus/bor/bor.go) to check for a second, Bor-specific false-negative/false-positive interaction with the new no-op gate, and confirmed that a sprint-start block with a real state transition correctly produces a root different from its parent, so it is not spuriously exempted by the new check — ruling out that candidate concern rather than leaving it unexamined.
|
both findings claim the guard can be bypassed because
the remaining concern is DoS. a rogue authorized validator at a no-op canonical height can force re-execution instead of cheap early rejection. this is bounded by the PoA validator set the test gap was valid so i added |
|
diffguard in the CI is failing. can you check that and see if you can fix it? |
|
the diffguard everything else passes: complexity, code sizes, dead code, mutation testing 100%. anything else? |
…mpty-block' into fix/2224-sidechain-ghost-state-empty-block
|
codegenie review |
|
@claude review |
pratikspatil024
left a comment
pratikspatil024
left a comment
There was a problem hiding this comment.
Thanks — the predicate rewrite is right, and the per-conjunct tests are better than what I asked for. Three things left.
1. Three of the five emptiness conjuncts can't be reached on bor, and the comments claim otherwise. Bor.verifyHeader rejects UncleHash != EmptyUncleHash (consensus/bor/bor.go:536), non-nil WithdrawalsHash (:554) and non-nil RequestsHash (:558) — the last is a != nil test, so EmptyRequestsHash is rejected too. Those errors land in it.errors, so it.next() returns them instead of ErrPrunedAncestor and the insertSideChain loop never reaches the guard with such a block. (The Finalize-side rejections at :1237/:1411/:1471 don't matter here — nothing executes on this path.) Only the transaction count and GasUsed do real work on bor.
Keep all five — this is go-ethereum-derived code you propose upstreaming, and the predicate should mean "provably carries nothing that can move state" on its own rather than depending on three engine-level rejections holding across future merges. But please fix the prose:
- the
// EIP-7685 requests: nil before the fork, a pointer to the empty-set hash after itcomment describes a fork bor doesn't have; say instead that uncles, withdrawals and requests cannot reach this path on bor, are retained as defense-in-depth and for upstream parity, and that the live predicates are the transaction count and gas; - the
// The empty-set requests hash is what a post-fork empty block actually carriescase asserts an exemption on a shape no bor block can have. Same for thereqOnly/wdOnly/uncleOnlyrejection cases — fine as unit tests (they only construct because the tests runnoRewardEngine{ethash.NewFaker()}, not bor's rules), but the comments shouldn't imply these blocks occur in production.
2. The PR description still hasn't been updated. It reads "the block is exempt only when its root equals its parent's root", which is the predicate this PR removed, and the rollout notes still say "it never accepts a block the old code would have accepted" and "it still rejects every block whose root differs from its parent's" — both now false. Executed tests omit the two new helper tests. For a consensus-path change this is the first thing an operator or a future upstream-merge reviewer reads.
3. The strict sibling requirement fixes one divergent block, not the class. With the node's head at N, divergence at D < N, and both sides empty (the quiet-network condition #2224 describes): the peer's block at D is a sibling of canonical D and is exempted, but the peer's block at D+1 has the peer's D as parent while canonical D+1 has the node's D — not siblings, so it's rejected and the node stays wedged. #2224 lands at k=1 only because v2 was stopped exactly at the divergence; a producer partitioned for a few seconds comes back with k = blocks it produced.
The safe extension is small: track the hashes exempted inside the loop and accept an exempted parent as an anchor —
sibling := block.ParentHash() == canonical.ParentHash() || exempted[block.ParentHash()]Every input stays trusted: an exempted parent was itself proven a no-op whose root chains back to the canonical fork header, and in an all-empty run the declared root is the pruned fork-parent root throughout, so it still can't terminate the recovery walk or satisfy a child's ValidateBody. Please either add that, or say explicitly in the description that the fix covers single-block divergence and file the rest like you did with #2386.
Separately, on codegenie's open question about the pruning-boundary constants: setupPrunedGhostStateChain asserts HasState(forkParent.Root()) is false with a t.Fatalf, so drift there fails loudly rather than passing vacuously — same idiom as the existing testSideImportPrunedBlocks.
CI note: the red codecov check is fork-PR infra (No token specified, then 429 Rate limit reached), not yours. Everything else is green including diffguard.
Summary
Fixes #2224.
After a validator restart, a node could refuse to sync past an empty (zero-transaction) block, drop every peer with
sidechain ghost-state attack detected, and need manual intervention to recover. In the logssiderootandcanonrootare the same value.The cause is in
insertSideChain()incore/blockchain.go. When a sidechain block hits a pruned ancestor, the code rejects any block whose state root equals the canonical block's state root at the same height, treating the match as a shadow-state attack (a block trying to reuse existing canonical state so it can skip re-execution of its ancestors). That assumption only holds for a block that carried a state transition. An empty block makes no state transition, so its state root equals its parent's. Two legitimately distinct empty blocks at the same height (different seal, timestamp, or coinbase, so different hashes) therefore share a state root, and the check fires a false positive.The fix narrows the rejection with one guard: the block is exempt only when its root equals its parent's root, which means it asserted no new state and there is nothing forged to skip-verifying. A block that claims the canonical root but differs from its parent's root (the real shadow-state attack, including a forged empty-looking block) is still rejected. If the parent header is unavailable, the original conservative behavior (reject) is kept.
This is go-ethereum derived code, so the same bug likely exists upstream.
Executed tests
TestSideImportEmptyBlockGhostState(new): builds a freezer-pruned canonical chain, generates a distinct empty sibling at a pruned height whose root equals both its parent's and the canonical block's, re-imports it, and asserts no ghost-state error. Confirmed it fails against the pre-fix code with the exactsidechain ghost-state attackerror, then passes after the fix.TestSideImportGhostStateStillRejected(new): asserts the attack path is still rejected. A side block that performs a real state transition (root differs from its parent) but claims a root equal to the canonical block must still return the ghost-state error.TestSideImportPrunedBlocks(existing): still passes; the non-empty sidechain re-import path is unchanged.go test ./core/passes with no regressions.gofmt -l core/blockchain.go core/blockchain_test.gois clean.go build ./...compiles.Both new tests run against the hash and path trie schemes.
Rollout notes
This change is consensus-relevant but backwards-compatible. It only narrows an existing rejection: it never accepts a block the old code would have accepted, and it still rejects every block whose root differs from its parent's. Empty blocks that previously wedged a node now import as normal sidechain blocks and fall through to the existing TD and reorg logic. No new dependencies, no config or storage format changes.
The guarded path is live in bor:
insertSideChainruns onErrPrunedAncestorwithsetHead, and bor uses validator-based difficulty and TD, which is the path the issue's production trace shows.