[gzliudan]

Summary

The proposed-block handler ran on whatever header its caller handed it, and the engine's own re-check could not make up for it: processQC writes highestQuorumCert, lockQuorumCert and the commit block before it looks the block up, and looking the block up cannot tell a reorged-away fork from the canonical block — the fork stays in the database. A master node could vote for, and commit state against, a block it had just reorged away.

The downloader half of that is already closed on dev-upgrade by 20e3373fb (fix(core,eth): hand the engine the head, not the batch tail): handoffProposedBlock hands the engine the head a batch left behind, and procFutureBlocks tracks the block that actually advanced the head. No batch tail reaches the handler any more, so this PR does not reimplement that — it gates the callers that are still exposed: the v2 engine twice, the fetcher once.

ProposedBlockHandler(header)
+  judge: canonical at its height, and its body stored?
+    no -> log at the reason's grade, count it, return nil
   processQC(quorumCert)        # writes highestQuorumCert / lockQuorumCert / commit block
   verifyVotingRule(...)
+  judge again                  # x.lock serializes the handler, not InsertChain
+    no -> drop the vote; processQC's writes stay (they have no rollback path)
   sendVote(blockInfo)

fetcher imports a propagated block
+  snapSync set? -> skip, counted: that path discards without executing
+  block and its executed state present? -> skip, counted; Error when canonical and near the head
   handleProposedBlock(header)

One judgment, shared so the call sites cannot drift: consensus.ShouldHandleProposedBlock asks a minimal CanonicalChain for the header at the block's height, and a BlockStorer for the body key. Storage goes through HasBlock, not GetBlock — only existence matters, and GetBlock would decode every imported block's body. core.BlockChain asserts both capabilities at compile time; a HeaderChain deliberately lacks BlockStorer, so a header-only chain fails loudly as SkipUnjudgeable instead of silently judging every block unstored. The judgment has no error channel — every outcome is a skip reason — so each caller collapses to one !ok branch.

The fetcher's state half requires HasBlockAndExecutedState, deliberately stopping short of HasBlockAndFullState: a missing XDCX trading or lending state piece is not re-derivable, so demanding it would halt QC processing with no self-healing path. A fast-sync transition grace keeps below-pivot canonical blocks at Warn, so the stall alert does not fire on them.

 consensus/
+├── proposed_block.go                  # the judgment's capability shapes
 ├── XDPoS/XDPoS.go                     # nil-shape guard before the version dispatch
+├── XDPoS/utils/proposed_block.go      # the judgment, its skip reasons, their counters
+└── XDPoS/engines/engine_v2/engine.go  # the two gates
 core/
 ├── blockchain.go                      # HasBlockAndExecutedState, the compile-time assertion
 └── headerchain.go                     # deliberately not a BlockStorer
 eth/
+├── proposed_block.go                  # the fetcher gate, with the fast-sync grace
 ├── fetcher/block_fetcher.go           # a nil callback normalizes to a no-op
 ├── handler.go                         # wires the gated closure to the fetcher
 └── sync.go                            # records the head when fast sync completes

19 files, 2102 insertions and 28 deletions.

Evidence

Before — putting the old behaviour back with go test -overlay (the judgment forced to accept, and the fetcher gate short-circuited to the consensus handler), not by reading the tests:

# the engine's two gates removed
--- FAIL: TestProposedBlockHandlerSkipsNonCanonicalBlock
--- FAIL: TestProposedBlockHandlerSkipsReorgedBlockBeforeProcessQC
--- FAIL: TestProposedBlockHandlerDropsVoteForReorgedBlock
--- FAIL: TestProposedBlockHandlerSkipsBlockWithoutBody
--- FAIL: TestProposedBlockHandlerSkipsNonCanonicalBlockWithMalformedExtra
FAIL  github.com/XinFinOrg/XDPoSChain/consensus/tests/engine_v2_tests  0.796s

# the fetcher gate removed
--- FAIL: TestFetcherProposedBlockHandlerGates
    handler_test.go:1012: fast sync call must not reach the consensus handler
FAIL  github.com/XinFinOrg/XDPoSChain/eth  0.021s

After — unmutated:

ok  github.com/XinFinOrg/XDPoSChain/consensus/tests/engine_v2_tests  12.322s
ok  github.com/XinFinOrg/XDPoSChain/consensus/XDPoS/utils           0.010s
ok  github.com/XinFinOrg/XDPoSChain/eth                             0.021s

make all and make quick-test are green; gofmt and goimports report nothing. Coverage follows the decisions: the judgment's outcomes and its nil shapes, the per-reason log grades and IsCanonicalAt (consensus/XDPoS/utils/proposed_block_test.go); the non-canonical skip, the reorg before processQC, the dropped vote for a reorged block, a canonical header without a body, the miner's self-vote path and the malformed-extra case (consensus/tests/engine_v2_tests/proposed_block_test.go); HasBlockAndExecutedState and the header chain that is not a BlockStorer (core/); the fetcher's gates, its wiring, its counter names and the nil-handler normalization (eth/).

Merge Danger

Door: Two-way. No config, no RPC and no database format changes, so a revert is a plain code revert — the counters and log lines simply stop.

Blast Radius: Consensus-internal — the path a v2 node takes from receiving a proposed block to casting a vote, on both the fetcher and the engine side.

• Every gate skips, none errors. A denied block returns nil, never an error: the fetcher's import loop treats a handler error as an import failure and suppresses the block's broadcast, so erroring would trade a wrong vote for a lost block.
• processQC's writes are deliberately not rolled back. They are monotonic round-wise updates with no rollback path, so only the vote is dropped. The residual reorg window between the second gate and the broadcast is documented and accepted; closing it for real means sinking the canonicality check into processQC before its writes, which is left as a TODO(convergence).
• The other processQC entries stay ungated. initial and SyncInfoHandler feed QCs that already passed verifyQC's quorum threshold, so following the quorum majority is how a forked node converges instead of stalling.
• A wiring bug is loud, not silent. A chain without BlockStorer judges SkipUnjudgeable at Error, and carries no counter on purpose — production wiring cannot reach it, because every entry point takes ProposedBlockChain.
• Alerts. Per-reason counters sit under skipped-proposed-block/ — no-canonical-header and non-canonical (a persistent rate means the node keeps being handed forked blocks), body-not-stored (transient during fast sync), and the fetcher's state and snap-sync. One regex over that prefix aggregates them; the engine gates' proposed-block-skip-total is named outside it on purpose, to be read as a cross-check rather than double-counted. A sustained state rate once the head is past the pivot is a QC/voting stall that needs manual intervention.
• Not carried over from the earlier revision of this branch. Because the downloader no longer runs this judgment, the pre-filter mechanism built for it — PreFilterSkipLogLevel, SkipReason's preFilter grade and the downloader-side tests — is omitted rather than kept as dead code.

Follow-up for the second merge (P3-04 / P3-10)

These two PRs were developed independently, and each adds a "park and retry" mechanism with the same shape but a different purpose:

	#2550 (eth/fetcher)	#2554 (eth)
parked item	parkedSign{block, deadline}	parkedProposed{header, deadline}
waits for	the block to be stored, then creates the signature	the block to be stored and executed, then hands it to the engine
ready predicate	canSign (canonical + freshness)	HasBlockAndExecutedState (canonicality stays with the engine)
cap	maxParkedSignatures 64	parkedProposedMax 64
poll / deadline	100 ms / 90 s	100 ms / 90 s
stale distance	maxUncleDist 7	parkedProposedStaleDistance 7

Neither PR is wrong on its own, and nothing breaks if both land as they are. But after both are merged the same process runs two parkers, and the invariant they now share is written in two files:

A parked item has exactly four exits: acted on and removed / stale / deadline / cap reached. "Not ready at this instant" is not an exit.

That invariant already regressed once during this work — scanParkedSignatures used to delete an entry as soon as the block was readable but not yet canonical, losing the deferred signature permanently (rawdb.WriteBlock flushes the block well before adoptHead writes the canonical head). It was fixed in 96bcd0d73b, and the two-phase test TestParkedSignatureWaitsForCanonical pins it.

Merge order: #2550 first, then #2554, then do the consolidation:

1. Gate ordering: existence (fix(eth): don't sign an unimported or non-canonical block #2550) → snap-sync / executed-state (fix(consensus,core,eth): gate proposed blocks on canonicality and storage #2554) → engine canonicality (fix(consensus,core,eth): gate proposed blocks on canonicality and storage #2554).
2. Unify the two parkers: share the mechanism (map + one waiter + one ticker + cap + deadline) and keep the ready predicate and the action injected per call site. Do not merge the two predicates into one — that would collapse the gate layering in item 1.
3. Merge the two 100 ms waiter goroutines/tickers into one.
4. Constant duplication: parkedProposedStaleDistance (7) duplicates the parking use of maxUncleDist (7). Share it in the direction "the eth side references the fetcher's parking semantics" — do not push parkedProposedStaleDistance back into the fetcher, where maxUncleDist also serves IsPlausibleAnnouncement and the queue-drop predicate.
5. Pin the invariant once: a table-driven set of cases (scenario x expected exit, including the TOCTOU row "ready when collected, no longer ready when the action runs") executed against both injection points, asserting the action happens at most once.

Tracked as P3-04 / P3-10 in .codebuddy/reviews/20261007-230522-8297eadcee.md.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 92c80021-e0c0-474c-aaa5-dc420a932659

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🟡 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 GetCanonicalHash to 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.

[Copilot]

🟡 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 on GetCanonicalHash. 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