fix(eth): don't sign an unimported or non-canonical block

[gzliudan]

Summary

A nil error from insertBlock is not a promise that the block is on the chain, and the fetcher still read it as one. Three ways a nil error arrives without a stored block:

• the inserter wired up in eth/handler.go returns nil without importing while fast sync is still running;
• the single-block import returns nil without storing a block the downloader is already fetching;
• a batch import reports success while parking its future tail in the future queue instead of writing it.

That is a consensus hazard, not a logging one: processQC writes highestQuorumCert before it checks that the block its QC points at exists, so a master node can end up voting for a block that is not in its chain. The downloader already asks the question - handoffProposedBlock hands over the head a batch left behind, never its tail - so this closes the fetcher as the remaining site that trusted the return value.

 fetcher.insert()
   verifyHeader
   insertBlock
-  signHook                 # signed a block that was never stored
-  handleProposedBlock      # voted for it, after processQC wrote highestQuorumCert
-  broadcastBlock
+  if getBlock(hash) != nil
+    if canSign(block)      # canonical at its height, and within the uncle distance
+      signHook
+    handleProposedBlock    # keeps the existence gate; canonicality stays a later question
+    broadcastBlock
+  else
+    parkForSignature       # one shared park: one goroutine, one ticker, capped
+    broadcastBlock         # relaying is safe either way

scanParkedSignatures later signs a parked block once it is stored and canSign holds, and keeps it parked - rather than dropping it - while it is merely stored but not yet canonical.

Scope

This PR carries only the fetcher half of the branch it replaces. The insertChain half - accepting consensus.ErrFutureBlock in both future-block loops - is already on the base, landed as commit fix(core): park the whole tail when a batch starts in the future, so this revision drops it along with its core and engine-v2 tests. The proposed-block gating revision is out of scope as well; it lives on the fix-downloader-canonical-tail-gate branch (#2554).

A block that is stored but not canonical is covered here too, as a second commit. writeBlockWithState writes the hash -> number mapping before the canonical decision, so a side entry - or a branch this node declined to adopt - is readable by hash and used to pass the existence gate, and the signature transaction was then created for a block that is not on this node's chain. That is a pre-existing defect, not something the existence gate introduced: both the immediate path and the parked path now gate the signature on canSign (canonical at its height, and within the uncle distance).

Evidence

Mutation-verified in both directions: the test hangs on the gates, not on the code around them.

# gate never fires: an unimported block reaches the consensus handler
$ sed -i 's/if f.getBlock(block.Hash()) == nil {/if false {/' eth/fetcher/block_fetcher.go
--- FAIL: TestUnimportedBlockSkipsConsensusHandling (0.10s)
    block_fetcher_test.go:1014: consensus handler ran for unimported block 0x928a1e53f3d5bcd7...

# gate always fires: a stored block loses its consensus handling
$ sed -i 's/if f.getBlock(block.Hash()) == nil {/if true {/' eth/fetcher/block_fetcher.go
--- FAIL: TestUnimportedBlockSkipsConsensusHandling (5.31s)
    block_fetcher_test.go:1045: consensus handler never ran for the imported block

# with the gate as written
$ go test ./eth/fetcher/ -run 'TestUnimportedBlockSkipsConsensusHandling|TestParkedBlockSignsAfterFutureImport|TestParkedSignaturePutsBackWhenNoLongerSignable' -count=1
ok  github.com/XinFinOrg/XDPoSChain/eth/fetcher  0.508s

The never-fires mutation no longer reaches the sign hook: the canonicality gate added by the second commit refuses an unimported block (canSign fails, the tester's chain has no canonical hash at that height), so the mutation is caught by the consensus-handler half instead.

TestUnimportedBlockSkipsConsensusHandling covers both halves - neither hook for a block that was not stored, both hooks for one that reached the chain. TestParkedBlockSignsAfterFutureImport pins the deferred signature: no signing before the block is stored, the relay regardless, and the hook once the block is imported, while the consensus handler stays reserved for procFutureBlocks.

The bound is pinned by TestParkedSignatureSkippedWhileSyncing (nothing is parked while the insert is discarded), TestParkedSignatureWithoutSignHook (a node without a signing hook parks nothing), TestParkedSignatureTimeoutDropsBlock and TestParkedSignatureDroppedWhenStale (the two eviction paths) and TestParkedSignatureLimit (the cap). TestParkedSignatureWaitsForCanonical is two-phase: stored but not canonical -> not signed and still parked; then canonical -> signed and removed. TestImportedNonCanonicalBlockIsNotSigned pins the immediate path: a stored side entry is not signed while its proposed-block handling still runs. TestParkedSignaturePutsBackWhenNoLongerSignable pins the re-check between the scan and the signature: a block that stops being signable in that window goes back to the parked set with its original deadline instead of being dropped, and is signed once its branch is re-adopted.

gofmt/goimports clean; make all, go test ./eth/fetcher/ and make quick-test all pass.

Merge Danger

Door: two-way

Reverting restores the previous unconditional calls. Nothing is written to the chain, to the wire protocol or to the consensus rules.

Blast Radius: eth/fetcher

Only the propagated-block path changes. A node that used to sign and vote for a block it had not stored stops doing so, and a block another importer stores gets its sign transaction from a single shared park (one goroutine, one ticker, capped at maxParkedSignatures) instead of a per-block poll. A node without a signing hook (non-XDPoS) parks nothing, unchanged. During snap sync nothing is parked either: the inserter discards propagated blocks outright, so this path no longer emits a signature - or its timeout warning - for them. Blocks that do reach the chain on this path keep the handling they had.

A stored but non-canonical block is no longer signed: the signature transaction is created only when canSign holds (canonical at its height, and within the uncle distance). A block that loses that race is skipped with a log line and a per-reason counter (the eth/fetcher/block/parked/{refused,stale,deadline,full} meters), and stays parked until it becomes canonical or reaches the stale/deadline bound - it is not dropped for merely being unready.

Suggested label: eth.

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 the first commit (fix(eth): don't sign or vote for an unimported block), and the two-phase test TestParkedSignatureWaitsForCanonical pins it. The re-check variant of the same loss - an entry deleted before the signature is attempted and not put back when the re-check fails - was found in review and fixed in the first commit as well; TestParkedSignaturePutsBackWhenNoLongerSignable 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. The re-check belongs to the shared parker, not to each action callback: it re-runs the injected ready predicate before invoking the action, and on failure skips the action and puts the item back (evicted later by stale/deadline, never for being merely unready), with the action running at most once per item. The put-back half has since landed in scanParkedSignatures (putBackParked, pinned by TestParkedSignaturePutsBackWhenNoLongerSignable); the consolidation must preserve it.

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

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2b43318c-0bf5-46e4-a48b-35dbdc86422c

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

A verification error following queued future blocks is still discarded by the final nil-error return.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Queues entire future-block batches to prevent downloader peer eviction near the chain tip.

Changes:

• Accepts ErrFutureBlock while queuing descendants.
• Adds tests for first/mid-batch queuing and resumed import.

File summaries

File	Description
core/blockchain.go	Expands future-block queue handling.
core/blockchain_futureblocks_test.go	Tests queuing and later processing.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• 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

Two error paths can omit bad-block reporting or silently report an incomplete import as successful.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

core/blockchain.go:1959

• These exemptions currently suppress both the bad-block report and the return. If iteration stops on ErrPrunedAncestor or ErrKnownBlock, block is still unconsumed but execution falls through to the final nil return, recreating a silent partial import. Exempt legitimate states only from reportBlock; every non-nil iterator error must still be returned.

		if err != nil && !errors.Is(err, consensus.ErrFutureBlock) &&
			!errors.Is(err, consensus.ErrPrunedAncestor) && !errors.Is(err, ErrKnownBlock) {
			bc.reportBlock(block, nil, err)
			return it.index, events, coalescedLogs, err

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Import-order races can prematurely discard deferred signatures, and several asynchronous tests do not reliably exercise their intended behavior.

3 open findings

• Premature cleanup loses deferred signature before canonicalization · New
• Recheck canonicality immediately before signing · New
• Test checks parked count before parking completes · New

1 resolved since last review

• When this first-block path advances past the queued future blocks and encounters a genuine…

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The promised per-reason monitoring is absent and two skip conditions currently produce misleading diagnostics.

2 open findings

• Distinguish canonicality and freshness skips with separate metrics · New
• Clarify warning for blocks not signable before deadline · New

3 resolved since last review

• Recheck canonicality immediately before signing
• Premature cleanup loses deferred signature before canonicalization
• Test checks parked count before parking completes

🧠 Review effort: Balanced