fix(eth,consensus): gate the proposed-block handler on canonicality and storage

[gzliudan]

fix(eth,consensus): gate the proposed-block handler on canonicality and storage

Summary

importBlockResults handed the tail of every imported batch to the proposed-block handler as long as InsertChain returned nil, but a nil error does not mean the tail is canonical: a fork batch is written as side-chain entries and a parked tail is not written at all. The engine did not compensate for it — processQC updates highestQuorumCert, lockQuorumCert and the commit block before its own existence check, and an existence check cannot distinguish a reorged-away block from a canonical one because the fork stays in the database — so a master node could vote for and commit state against a block it had just reorged away. This PR introduces a single shared judgment, consensus.ShouldHandleProposedBlock, which requires the header to be canonical at its height and its body to be stored, and gates both the downloader and the v2 engine on it. closes #____

Motivation & Context

The downloader treats InsertChain returning nil as "the tail is on the canonical chain", which is only true for a batch that actually extended the canonical head. A fork batch is stored as side entries, and a tail parked by a gap or an interrupt is not stored at all; both still produced a nil error and were handed to the handler. The fetcher and the miner call the same handler and had no gate whatsoever. Canonicality alone is not sufficient either: it is a property of the header, and the fast sync header phase marks a height canonical before its body lands, so a node could processQC against and vote for a block that only ever existed as a header. Voting is not a local-only action — a master node's vote, QC and commit block feed consensus — so the cost of acting on a non-canonical block is a wrong commit, not just a wasted log line.

Changes

• consensus/proposed_block.go (new): CanonicalChain (only GetHeaderByNumber + HasBlock), ShouldHandleProposedBlock, the SkipReason constants (SkipNoCanonicalHeader, SkipNonCanonical, SkipBodyNotStored) and SkipLogLevel. The judgment returns the skip reason and the canonical hash at the height, both for logging. Storage is checked with HasBlock rather than GetBlock: only existence matters, and GetBlock would read and RLP-decode the whole body of every imported block.
• consensus/consensus.go: ChainReader gains HasBlock. core.BlockChain already had it; core.HeaderChain, consensus.XDPoS.verifyChainReader and the core.fakeChainReader / test chain mocks now provide it with the same semantics as their GetBlock.
• eth/downloader/downloader.go: importBlockResults gates the handler on the shared judgment instead of open-coding one out of HasBlock and GetCanonicalHash, and logs the skip with its reason and the observed canonical hash at Info. Its BlockChain interface gains GetHeaderByNumber so it satisfies CanonicalChain.
• consensus/XDPoS/engines/engine_v2/engine.go: ProposedBlockHandler re-checks the same judgment twice — in front of processQC, and again right before sendVote — because x.lock serialises the handler but not InsertChain, so a reorg can land between the two checks. Skip levels follow the reason: SkipNonCanonical (a genuine reorg race) at Warn, the routine sync-phase skips at Info.

Risk & Impact

• Behavioural: fork tails, parked tails and header-only heights no longer reach the handler, so a node stops voting for blocks it has reorged away. On a healthy chain nothing changes; on a reorging or fast-syncing node the extra skips are the intended fix.
• Interface: consensus.ChainReader gains HasBlock, so every implementation must be updated — all in-tree ones are, and out-of-tree implementations of consensus.Engine chains will fail to compile until they add it.
• Performance: one extra GetHeaderByNumber and one HasBlock per proposed block in the downloader, and two extra judgments per handler run; both are existence/header reads, no body decoding.
• Observability: a new Info-level skip log per fork tail during sync, and a Warn on genuine reorg races; log volume during heavy reorging increases.

[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

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 52f7e12a-e89c-4734-b06c-a4f4154f650e

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

---

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

[Copilot]

🔵 Needs a closer look

The proposed checks still permit reorg races around consensus-state mutation and vote broadcasting, and the interface change breaks external implementations.

Review details

Suppressed comments (1)

consensus/XDPoS/engines/engine_v2/engine.go:882

• The re-check is not actually adjacent to the broadcast. sendVote performs epoch lookups and signing, updates highestVotedRound, inserts the vote into the local pool via voteHandler, and only then asynchronously broadcasts it (vote.go:20-59). A reorg after this line can therefore still produce and broadcast a vote for a non-canonical block. Keep canonicality protected through the vote state mutation and enqueue, rather than checking before an unsynchronized sendVote call.

		if ok, reason, canonicalHash := consensus.ShouldHandleProposedBlock(chain, blockHeader); !ok {
			consensus.SkipLogLevel(reason)("[ProposedBlockHandler] skip vote for reorged block", "reason", reason, "hash", blockHeader.Hash(), "number", blockInfo.Number, "canonicalHash", canonicalHash)
			return nil
		}
		return x.sendVote(chain, blockInfo)

• Files reviewed: 17/17 changed files
• Comments generated: 4
• Review effort level: Balanced