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.
🟡 Changes recommended
The canonical hash can reference a fast-synced header whose full block was never stored, so the new gate needs an additional block-presence check.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a canonicality gate before downloaded blocks reach XDPoS consensus handling.
Changes:
- Adds
GetCanonicalHashto the downloader blockchain interface. - Adds canonicality scenarios and downloader test hooks.
File summaries
| File | Description |
|---|---|
eth/downloader/downloader.go |
Gates proposed-block handling on canonical hash. |
eth/downloader/downloader_test.go |
Adds canonicality and concurrent-head test scenarios. |
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.
gzliudan
force-pushed
the
fix-downloader-canonical-tail-gate
branch
4 times, most recently
from
September 7, 2026 03:12
678dd3d to
3978be7
Compare
gzliudan
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The test fork-choice model diverges from production for equal-TD and shorter heavier reorgs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
eth/downloader/downloader_test.go:390
- This fork-choice model does not fully match
BlockChain.writeBlockWithState: production also promotes an equal-TD block when it has a higher number (core/blockchain.go:1670-1675), and a reorg to a shorter but heavier fork deletes canonical markers above the new head (core/blockchain.go:2614-2627). As written, the tester can report a different canonical hash than the real chain for both cases, weakening tests built onGetCanonicalHash. Mirror the tie-break and clear entries above every newly selected head.
_, headHash := dl.canonicalHead(true)
if td := dl.ownChainTd[headHash]; td == nil || dl.ownChainTd[block.Hash()].Cmp(td) > 0 {
dl.canonicalize(block.Hash(), block.NumberU64())
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
gzliudan
force-pushed
the
fix-downloader-canonical-tail-gate
branch
2 times, most recently
from
September 7, 2026 04:18
b46701f to
f7e17f2
Compare
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
September 7, 2026 04:19
wanwiset25
approved these changes
Sep 7, 2026
gzliudan
changed the title
gzliudan
force-pushed
the
fix-downloader-canonical-tail-gate
branch
3 times, most recently
from
September 7, 2026 05:59
834c8f7 to
f5e3893
Compare
gzliudan
changed the title
gzliudan
force-pushed
the
fix-downloader-canonical-tail-gate
branch
5 times, most recently
from
September 7, 2026 10:26
f2e2f12 to
661dabe
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The body-only gate can still permit QC processing and voting for fast-sync blocks whose execution state has not been validated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Balanced
gzliudan
force-pushed
the
fix-downloader-canonical-tail-gate
branch
26 times, most recently
from
September 9, 2026 12:10
91778ec to
a9d1790
Compare
…ity and storage
The proposed-block handler ran on whatever header its caller handed it, so
consensus state advanced on blocks that were never canonical. The downloader
passed the tail of every imported batch, which a fork batch stores as side
entries and a parked tail not at all; the fetcher passed blocks fast sync had
propagated but discarded before executing. processQC then wrote
highestQuorumCert, lockQuorumCert and the commit block for a reorged-away or
body-less block, and sendVote voted for one. A nil header or nil number
panicked in the dispatch that dereferences it.
Add consensus.ShouldHandleProposedBlock, shared by every call site so the
judgment cannot drift: a header is handled only if it is the canonical block at
its height — an existence check cannot tell a reorged-away fork from a
canonical one, both stay in the database — and its body is stored, since fast
sync marks a height canonical before its body lands. It reports a SkipReason
and the canonical hash. A chain without BlockStorer fails loudly as
SkipUnjudgeable instead of silently judging every block unstored. That wiring bug
skips every block and halts QC and voting for good; every gate logs it at Error,
and production wiring cannot reach it — every entry point takes
ProposedBlockChain — so it carries no counter and the Error logs are the alarm.
Gate on it in three places. The v2 engine checks before processQC and again
before sendVote — x.lock serializes the handler, not InsertChain, so a reorg
can still land in between — and the second gate keeps the vote's unguarded
window down to the broadcast; processQC's writes are deliberately not rolled
back, they are monotonic round-wise updates with no rollback path, so only the
vote goes. The downloader pre-filters the batch tail, because a nil error from
InsertChain does not make the tail canonical, but keeps the ungated callback:
its fast sync calls run after the pivot commit, so gating it there would skip
every proposed block and stall voting. The fetcher gate checks the snapSync
flag and executed state, leaving canonicality to the engine. Every gate returns
nil, never an error: the fetcher's import loop treats a handler error as an
import failure and suppresses the block's broadcast. The XDPoS wrapper and the
engine guard the nil shape before any dereference.
Skips are graded and counted by the reason itself: each reason declares its log
grade at both call sites and, when it can reach a counting gate, its own
counter under skipped-proposed-block/, so a transient fast-sync
body-not-stored burst separates from a persistent non-canonical stall. The shared total is the engine gates' aggregate view; SkipNilHeader logs
at Error but counts nothing. The fetcher gate's state guard escalates to Error
when the skipped block is already canonical — nothing will re-execute it and the
fetcher judges each propagation only once, so the stall does not self-heal (the
block's QC still converges through the deliberately ungated SyncInfoHandler and
vote path); there is no HasBlock fallback, which would undo the gate, so the
near-head Error plus sustained counter growth is the manual-intervention signal,
unlike a transient fast-sync race. Its two counters are
named skipped-proposed-block/{state,snap-sync}; every skip counter — engine
reasons, downloader pre-filter, fetcher gate — shares that single root, so one
alert regex aggregates them all.
core gains HasBlockAndExecutedState — HasBlock plus an opening state trie, no
body decoded, and no XDCX trading/lending completeness, which is not
re-derivable and must not become a permanent voting halt on a correctly
executed block — sharing its trie criterion with HasFullState, plus a
compile-time check that *BlockChain answers both halves of the judgment. The
shared criterion is documented as degenerating to HasBlock for a zero root and
types.EmptyRootHash, which trie.New opens without resolving, so a block with
an empty state root reports as executed once it is known.
The miner's self-vote path (worker.wait calling HandleProposedBlock on its own
block right after WriteBlockWithState) is pinned by an engine test covering
both write outcomes: a canonical self-mined block passes the gate, runs
processQC and broadcasts its vote without moving any skip counter, and a
same-height side-chain block — the core.SideStatTy outcome when the peer's
block landed first — is skipped with the non-canonical reason counted and no
vote broadcast.
Wiring is pinned through new ProposedBlockHandler accessors on the downloader
and the fetcher — exported for cross-package wiring tests only, not part of the
supported API; the per-reason grades and counters are part of each reason's
declaration, so there is no registration table to keep in sync with the reason
list.
Document why the other two processQC entries (SyncInfoHandler and the
vote path) deliberately skip the canonicality gate: their QCs already
pass the quorum signature threshold, so a QC for a locally side-chain
block means the network fork choice has diverged from ours and
accepting it is the convergence behavior; the handler comment no longer
implies the gate covers every processQC entry, and the TODO(convergence)
note now asks the future reorg invalidation to also cover the state
those two entries write.
The fetcher gate's stall alert is completed on three fronts: an ops
runbook (docs/proposed-block-stall-runbook.md) documents the alert
signals, the expected no-state zones, the triage steps (pruning, SetHead
rollback, trie corruption) and the recovery actions, and the residual-
risk comment points at it; a fast-sync transition grace — the head height
recorded when fast sync completes — keeps canonical below-pivot blocks at
Warn until the head moves proposedBlockStallWindow past it, so the
post-commit window no longer mislabels expected no-state announces as a
voting stall; the recorded head is read through the same nil guard as the
other CurrentBlock() readers; and the stale distance check now also guards
against a nil header number.
The downloader tester's fork-choice mirror is cross-validated against a
real core.BlockChain: the same GenerateChain blocks go through both
InsertChain paths through a side-entry stage and a heavier-takeover reorg,
and the canonical table, canonical headers and head getters must agree at
every height.
Docs/comments fix-ups: list all skipped-proposed-block counters
(no-canonical-header, body-not-stored) in the stall runbook's counter
table so one-regex alerting covers every counted skip reason, and
correct the processQC entry-point enumeration in engine.go — the
ungated entries are SyncInfoHandler, the vote path and Initial; note
at the Initial call site that its QC comes from the local canonical
head and needs no canonicality gate.
Tighten the skip-counting and stall-recovery docs so they match the
wiring: the IncSkipReasonCounter comment now states that an unjudgeable
skip moves only the engine-gate total — it has no per-reason counter of
its own and its Error grade is the alarm — instead of implying an
unjudgeable counter exists; the stall runbook's counter table describes
skipped-proposed-block as skips at both engine gates (before processQC
and before the vote) and drops the batch-tail claim from the
body-not-stored row; the runbook's Triage section and the fetcher
gate's residual-risk comment state that retransmission does not
self-heal — a re-announced block hits ErrKnownBlock in getResultBlock
and returns before the gate is reached — and the downloadTester comment
ties its fork-choice mirror to the downloader pre-filter's judgment and
consensus.PreFilterSkipLogLevel so future criteria changes update both
together.
Address the review follow-ups so the alerting story is consistent and the
package layering is clean: the engine gates' total counter is renamed
proposed-block-skip-total, deliberately outside the skipped-proposed-block/
prefix, so one alert regex on that prefix sums the per-reason and per-site
counters exactly once and cannot double-count the total — the runbook
documents the aggregate and marks the total as a read-only cross-check. The
fast-sync transition grace is recorded before the snapSync flag is cleared,
closing the window in which a below-pivot canonical block could escalate to
a voting-stall Error — exactly the false positive the grace suppresses. The
proposed-block judgment, its skip reasons, grades and per-reason counters
move from the generic consensus package into the leaf
consensus/XDPoS/utils so the engine and eth can share them without making
consensus carry single-engine semantics; the capability interfaces
(CanonicalChain, BlockStorer, ProposedBlockChain) stay in consensus. The miner self-vote test's
comment now pins the gate semantics its call site depends on and states the
coverage boundary explicitly instead of implying WriteBlockWithState
coverage.
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
importBlockResultshanded the tail of every batch to the proposed-block handler wheneverInsertChainreturned nil, and a nil error does not make the tail canonical: a fork batch is stored as side-chain entries, a parked tail is not stored at all, and the fast sync header phase marks a height canonical before its body lands. The engine did not make up for it —processQCupdateshighestQuorumCert,lockQuorumCertand the commit block before its own existence check, and an existence check cannot tell a reorged-away block from a canonical one because the fork stays in the database. A master node could therefore vote for, and commit state against, a block it had just reorged away.Judge the block once, in one place:
consensus.ShouldHandleProposedBlockreports whether a header is the canonical block at its height and whether its body is stored, together with a skip reason and the canonical hash for the skip log. It takes a minimalCanonicalChain(GetHeaderByNumberalone) that bothconsensus.ChainReaderand the downloader'sBlockChainsatisfy, so the callers cannot drift into two diverging judgments. The storage half goes through the optional capability interfaceconsensus.BlockStorer, whose single method isHasBlockrather thanGetBlock— only existence matters, andGetBlockwould RLP-decode the body of every imported block.core.BlockChainimplements it behind a compile-time assertion, aHeaderChaindeliberately does not, so a header-only chain fails loudly asSkipUnjudgeableinstead of silently skipping every block as not stored. The judgment has no error channel: every outcome is a skip reason, so each caller collapses to a single!okbranch. The miner, the fetcher,procFutureBlocksand the downloader all share it.The fetcher additionally drops proposals while
snapSyncis set: that path discards propagated blocks without executing them, so a body written by the fast sync receipt phase would pass both halves of the judgment and driveprocessQCand the vote on an unvalidated state transition. Its gate otherwise requiresHasBlockAndExecutedState(a hash-keyedHasBlockplus an open state trie), deliberately stopping short ofHasBlockAndFullState: a missing XDCX trading or lending state piece is not re-derivable, so demanding it would halt QC processing with no self-healing path.Skips are graded by reason (
consensus.SkipLogLevelandPreFilterSkipLogLevel, both driven by one registration table) and counted per gate:consensus/skipped-proposed-blockfor the engine gates,consensus/unjudgeable-proposed-blockfor the wiring bug, andeth/skipped-proposed-block-statefor the fetcher's state half — each owns its own alert, so a persistent QC/voting stall is observable beyond the logs. The residual reorg window between the second gate check and the vote broadcast is documented and accepted.Tests cover the judgment's outcomes and its nil-header/nil-number caller-bug skips, both engine re-checks (no
processQCstate change, no vote broadcast), the per-reason log levels, the fetcher'ssnapSyncand state gates, and six downloader batch shapes (parked tail, stored fork batch, head advanced past the canonical tail, heavier fork, fast sync height with no body), withdownloadTestergaining a canonical number-to-hash table resolved by total difficulty.Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) thatThe change is consensus-internal gating plus metrics and logging: no new config, no new RPC or database format, so no documentation update is required. Private-network, devnet and mixed-version runs are still pending on my side; the manual plan is to run a multi-node XDPoS v2 devnet, force a fork tail during sync and during a reorg, and confirm that the node neither votes for nor commits the reorged block while
consensus/skipped-proposed-blockandconsensus/unjudgeable-proposed-blockstay flat (any sustained growth indicates a QC/voting stall).