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 3 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❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (70.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #2293 +/- ##
===========================================
+ Coverage 53.69% 53.80% +0.10%
===========================================
Files 897 898 +1
Lines 160388 160836 +448
===========================================
+ Hits 86122 86534 +412
+ Misses 68927 68923 -4
- Partials 5339 5379 +40
... and 43 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
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.