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

importBlockResults handed the tail of every batch to the proposed-block
handler as long as InsertChain returned nil. 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 make up for it:
processQC updates highestQuorumCert, lockQuorumCert and 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. The fetcher and the miner call the same handler and had no gate
at all. The miner is not gated separately: it hands the *core.BlockChain
it already holds to the shared handler, so the engine's canonicality and
storage re-checks cover that path too — a block WriteBlockWithState files
as a side-chain entry is skipped, and no processQC or vote runs on it.
Canonicality is not enough on its own 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 and vote for a block
that only ever existed as a header. A master node could therefore end up
voting for, and committing state against, a block it had just reorged
away.

Judge the block once, in one place. consensus.ShouldHandleProposedBlock
reports whether a header is the canonical block at its height and whether
its body is stored, together with the reason it must be skipped and the
canonical hash at that height, both for the skip log. It takes a minimal
CanonicalChain (GetHeaderByNumber alone) that both consensus.ChainReader
and the downloader's BlockChain satisfy, so the callers cannot drift into
two diverging judgments; on the downloader side that costs one addition of
GetHeaderByNumber to its BlockChain interface. The storage half goes
through the optional capability interface consensus.BlockStorer, whose
single method is HasBlock rather than GetBlock: only existence matters,
and GetBlock would read and RLP-decode the whole body of every imported
block. core.BlockChain implements BlockStorer — a compile-time assertion
keeps it from dropping off the type — while a HeaderChain deliberately
does not, so handing a header-only chain to the judgment fails it loudly
as the SkipUnjudgeable skip reason instead of every block silently
failing as not stored. The judgment has no error channel: every outcome
is a skip reason, so each caller collapses to a single !ok branch. The
BlockStorer assertion runs before any chain read, so the unjudgeable
skip is reported at every height: a header-only chain must not get a
silent "not stored" from the canonicality half for the heights it
happens to answer.

The downloader gates importBlockResults on that judgment instead of
open-coding the same rule out of HasBlock and GetCanonicalHash, and logs
the skip with its reason and the canonical hash it observed. The v2 engine
re-checks the same judgment twice inside ProposedBlockHandler: in front of
processQC, and again right before sendVote, because x.lock serialises the
handler but not InsertChain, so a reorg can still land between the two
checks. A judgment that cannot run — a chain type that lost the
BlockStorer half to wrapping or replacement — surfaces as the
SkipUnjudgeable skip, which the handler logs and returns as nil: the
fetcher's import loop treats any handler error as an import failure and
would suppress the broadcast of an already imported block, so a skip
must never surface as an error. That skip means the node stops
processing QCs and voting outright — in production the chain always
implements BlockStorer, so it is a wiring bug. The judgment itself counts
every such skip in a metrics counter (consensus/unjudgeable-proposed-block)
at the single return all callers share, so the liveness halt is observable
beyond the logs on every call site: the downloader's pre-filter hits the
same return before its handler would ever run, where an engine-side-only
counter would stay silent exactly where the wiring bug is easiest to trip.

consensus.SkipLogLevel grades the skips by reason:
SkipUnjudgeable is the one Error — not an observation about the block but
a wiring bug that will skip every block — SkipNonCanonical is a genuine
reorg race and stays at Warn, while the sync-phase skips are routine and
stay at Info so they do not drown the level in noise. The empty reason
grades Warn too: it means a caller logged a skip for an accepted block, a
pure misuse that should surface, not hide at Info. The downloader's own
skip log stays at Info on purpose: it is the pre-filter for the routine
cases and correctness rests on the handler's re-checks, so grading it too
would fire a Warn for every fork tail of a sync. That grading now lives in
consensus.PreFilterSkipLogLevel, right next to SkipLogLevel and the
SkipReason constants, instead of an open-coded switch inside the
downloader: both graders sit in one file, and each grades an unregistered
reason as Warn so a forgotten registration surfaces. The downloader's
pre-filter has no dedicated unjudgeable branch — that path is statically
unreachable there, and if an interface change ever makes it reachable,
the table grades SkipUnjudgeable Error at that call site too.

The engine's two re-checks now share one counter of their own
(consensus/skipped-proposed-block), incremented at the gate that declined
the block — at most once per handler invocation, since a first-gate skip
returns before the second gate; the same block judged once through the
fetcher path and once through the downloader path is counted once per
path. It deliberately counts only the engine gates and
not the downloader's pre-filter: fork tails arriving there are routine
sync noise logged at Info, while a block that got past the pre-filter and
was still declined is exactly the anomaly logs alone could bury. The
reason stays in the skip log; one counter for all reasons keeps the
metric surface flat. And because the same proposal can reach the shared
judgment through both the fetcher and the downloader paths, the
unjudgeable counter's increments are judgment counts, not deduplicated
blocks — its comment says so now. The window comment also names what the
second check leaves open: sendVote itself reads the chain again via
getEpochSwitchInfo before signing and broadcasting, so the residual
window spans that read plus the signature and the broadcast. The fetcher
gate's comment now cross-references the engine side — canonicality is
covered there, so the two gates interlock rather than duplicate — and
points at the post-pivot full import in processFastSyncContent, the
downloader call site the ungated closure's justification rests on.

The fetcher is the one caller whose fast sync calls must not reach the
handler at all. While snapSync runs, it discards propagated blocks before
executing them, so a body already written by the fast sync receipt phase
would pass both halves of the judgment — canonical and stored — and drive
processQC and the vote path on a block whose state transition was never
validated. Its callback therefore carries the same snapSync guard the
inserter and prepare closures already have, instead of asking the
judgment a question the judgment cannot answer. The guard sits in a
fetcher-specific wrapper and not in the shared closure: the same closure
also feeds the downloader, whose fast sync handler calls run after the
pivot commit, on blocks InsertChain has fully executed, and snapSync
stays set until the whole Synchronise returns. Behind that flag the
callback also requires the block's executed state (HasBlockAndExecutedState:
GetBlock plus an open state trie) rather than the state root alone: its
GetBlock half is keyed by the block hash, so a discarded import — whose
body was never stored — fails there, before the root-keyed OpenTrie half
could leak a parent's executed state to an empty block whose root repeats
it. The two halves are documented as such: the hash-keyed gate half and
the root-keyed state half each do their part. Unlike the judgment's storage half — which reads
HasBlock to avoid decoding whole bodies — this check pays a GetBlock body
read plus OpenTrie: proposed blocks arrive at consensus cadence, orders
of magnitude rarer than sync-path blocks, so the fuller check is
affordable here and closes the unexecuted-state hole the cheap HasBlock
check would leave open. It deliberately stops short of
HasBlockAndFullState: that method also demands the XDCX trading and
lending state pieces at TIPXDCX heights, and a correctly executed block
missing one of them — a state the node cannot re-derive by reprocessing —
would fail it forever, permanently halting QC processing and voting with
no self-healing path. Completeness of the auxiliary state is an
observation for the trading/lending services, not a voting precondition.
A nil header or nil number fails the same SkipNilHeader guard the shared judgment applies — this closure dereferences the header before any chain read, so it keeps that caller-bug contract instead of panicking — logging Error through consensus.SkipLogLevel, consumed rather than registered so the grading table stays fetcher-free, and without a counter of its own. Its skips are graded by what they mean: the
fast-sync flag skip is routine and stays at Debug, but a state-half skip
still halts QC processing and voting for that block, so it logs at Warn
and increments its own counter (eth/skipped-proposed-block-state)
— the same logs-alone-could-bury-it discipline as the unjudgeable
counter, so a persistent post-sync liveness stall is observable beyond
the logs. Its comment now also names the counter as the sentinel to
alert on: sustained growth means a QC/voting stall, not a transient
race, while trading/lending state completeness is no longer a voting
precondition at all.

The split of storage-check strengths across the three gates — the shared
judgment and engine gates on HasBlock, the fetcher gate on
HasBlockAndExecutedState — is now documented as such in the judgment's own
doc comment: each path is self-consistent (miner/fetcher run on a tip
with full state, the downloader reaches the handler only after the
post-pivot full import), and a future caller with neither guarantee must
add a stronger gate of its own rather than weaken these. The second
engine gate's comment and the vote-drop test's state anchor both state
why the processQC state write is retained, not rolled back, when a block
is reorged away between the gates: the block's embedded QC certifies its
parent, and that certification stays valid after the reorg — only the
vote for the reorged block must go. The fetcher's gated callback is a free function the tests can construct
instead of a test-only accessor on the fetcher.

Tests:

- TestShouldHandleProposedBlock covers the outcomes of the shared
  judgment plus the nil-header and nil-number caller-bug skips and the
  header-only chain at an absent height, whose
  unjudgeable skip is the point of the entry-side assertion; the two
  storerless cases also pin that the judgment increments
  unjudgeableProposedBlock exactly once per skip, and
  TestHeaderChainShouldHandleProposedBlock pins that the header chain's
  interface stubs never pass it at any height.
- TestProposedBlockHandlerSkipsNonCanonicalBlock,
  TestProposedBlockHandlerSkipsReorgedBlockBeforeProcessQC,
  TestProposedBlockHandlerDropsVoteForReorgedBlock and
  TestProposedBlockHandlerSkipsBlockWithoutBody cover the two engine
  re-checks: highestQuorumCert, lockQC, the timeout certificate, the
  voted round and the commit block must stay untouched and no vote may be
  broadcast. The two reorg tests inject the reorg through a ChainReader
  wrapper that serves a fork header once N judgment gates have completed —
  anchored on the gate boundary, where each gate that passes the
  canonicality check ends in exactly one HasBlock of the watched height —
  so reads added in front of or between the handler's gates stay truthful
  and cannot move where the injection lands, instead of coupling the test
  to the handler's read ordinals.
- TestProposedBlockHandlerGradesSkipLogLevelByReason pins the log level
  of each skip reason.
- TestProposedBlockHandlerSkipsUnjudgeableChain feeds the handler a
  ChainReader wrapper without the BlockStorer half — the shape a wrapped
  or replaced chain type presents — and pins that the chain skips as
  SkipUnjudgeable, logged at Error and returned as nil, not surfaced to a
  caller that would treat it as an import failure. No vote may be
  broadcast either: that assertion waits out a two-second timeout window
  rather than checking BroadcastCh non-blockingly, because a vote travels
  through broadcastToBftChannel's own goroutine and an immediate
  default-branch check would pass vacuously even if a vote were sent.
- TestFetcherWiresNoopHandlerWithoutConsensus pins the non-XDPoS
  wiring: without a consensus engine the fetcher handler is an explicit
  no-op that logs nothing in either sync mode, and it is still wired
  non-nil as the literal argument handed to fetcher.NewBlockFetcher.
- TestFetcherProposedBlockHandlerGates exercises the gated closure
  directly: with snapSync set it is inert and logs the skip before
  reaching the handler, the same call in full sync reaches the handler,
  and the state-half skip is pinned with a block forged for real: its
  header and body are written straight into the database via rawdb so
  the hash-keyed GetBlock half finds it, while its root exists nowhere
  in the state database so the state half — the OpenTrie of
  HasBlockAndExecutedState — is the part that fails — mutating the current header's root alone would change its hash
  and fail at the GetBlock half instead, never exercising the state
  check — and the counter's one increment per skip is asserted.
  A nil header and a nil number fail the SkipNilHeader guard: no panic, no handler call, an Error log and no counter movement.
- TestImportBlockResultsProposedBlockHandler (with its helpers it lives
  in eth/downloader/downloader_proposed_block_test.go) covers six
  downloader shapes: a parked tail, a fully imported batch, a stored fork batch
  re-delivered after the local chain grew past it, a head advanced past
  the canonical tail by a concurrent import, a heavier fork that stays
  canonical, and a fast sync height that is canonical without a body.
- downloadTester gains a canonical number-to-hash table, picked by total
  difficulty the way the real chain resolves a reorg, plus hooks to park
  a batch tail and to extend it after the insert.
  TestStoreBlockCleansStaleCanonicalMarkers,
  TestRollbackClearsCanonicalMarkers and
  TestInsertChainErrorReportsPosition pin the parts of that table the
  downloader tests rely on.
- TestShouldNotSendVoteMsgIfBlockNotExtendedFromAncestor no longer
  proposes a forked block, which the new entry re-check now
  short-circuits; it proposes a canonical block below the locked ancestor
  instead. The fork case moved to
  TestShouldNotSendVoteMsgIfCanonicalBlockNotExtendedFromForkedAncestor,
  where the parent walk of isExtendingFromAncestor actually runs.

A review pass over the change tightened it further. The skip-log grading
for the downloader's pre-filter moved out of the downloader into
consensus.PreFilterSkipLogLevel, next to SkipLogLevel and the SkipReason
constants, so both graders live in one file instead of relying on a
cross-package comment to stay in sync; the downloader logs its routine
skips through it and its tests pin the Info level end to end.
SkipLogLevel now grades the misused empty reason Warn instead of Info.
The second pre-vote gate's comment covers processQC's remaining state
writes — commitBlocks and setNewRound (the currentRound bump, the
timeoutPool clear and the newRoundCh signal) — and
TestProposedBlockHandlerSkipsNonCanonicalBlock pins that no newRoundCh
signal is emitted, with the state assertions covering the rest.
TestProposedBlockHandlerSkipsNonCanonicalBlock and
TestProposedBlockHandlerDropsVoteForReorgedBlock assert the engine
gates' skipped-proposed-block counter through the metrics registry, and
TestPreFilterSkipLogLevelGradesByReason pins the pre-filter grading at
its definition site. The fetcher wiring checks avoid reflect
function-pointer comparisons — the spec does not guarantee unique
pointers for function values — in favor of behavioral ones: the wired
callback is called and must produce the log its closure emits. Comments across the touched files were
condensed to the reasoning each spot needs; the call-site grading, the
three-gate strength split and the counter semantics keep their one-line
statements.

A second review pass closed the remaining loose ends. The two log graders
now share one mechanism instead of two parallel switches: the
reason-to-level mappings live in maps next to an allSkipReasons list,
and TestSkipReasonRegisteredInBothGraders fails when a new SkipReason is
added without being registered in both of them; the empty and
unregistered reasons fall to a single Warn fallback in each grader,
ending the asymmetry between the two switches' handling of the misused
empty reason. The unjudgeable counter's comment spells out the alerting
overlap: a skip is counted both there and at the engine gates'
skipped-proposed-block counter, so alerts belong on the unjudgeable
counter alone — one halted node would otherwise raise both alerts at
twice the rate. The fetcher's gated callback moved out of
NewProtocolManager into the free function newFetcherProposedBlockHandler
and the test-only HandleProposedBlockForTest accessor on the fetcher is
gone, so the fetcher package carries no test-only exported surface while
the eth package tests keep their behavioral wiring verification against
the field that is literally handed to fetcher.NewBlockFetcher. The
engine tests' counter helper now fails its test instead of panicking
when the metrics registration is missing, and both test files state
that their cases must not run in parallel: they compare global counter
values before and after a call, which parallel cases would race on.

A third review pass tightened the remaining edges. The fast-sync wiring
test's first handler call now asserts a nil error like its three
siblings instead of swallowing it in an empty branch. The two counter comments now state the full alerting matrix
instead of contradicting each other: unjudgeableProposedBlock alone
owns the wiring-bug alert, skipped-proposed-block alone owns the
gate-stall alert, and neither substitutes for the other (a wiring bug
raises both counters, a gate stall raises only skipped-proposed-block).
The residual reorg window between the second gate check and the vote
broadcast is documented intentional design, recorded here as wontfix.

A fourth review pass collapsed the SkipReason registration from three
hand-maintained places into one. The two per-callsite level maps and
the test-only allSkipReasons list are replaced by a single skipGrades
table mapping each reason to a skipGrade struct with named handler and
preFilter fields, so a newly added reason is registered for both
graders by construction and can no longer be missed for one of them;
the graders keep their signatures and the Warn fallback for
unregistered or empty reasons, so grading behavior is unchanged. The
registration test that policed the old two-map layout lost its purpose
and is deleted along with allSkipReasons, and the reason constants'
comment now points at the one table. newFetcherProposedBlockHandler no
longer takes a blockchain parameter duplicating pm.blockchain: the
closure reads pm.blockchain directly, so its full-state gate can never
silently diverge from the chain the manager holds.

A fifth review pass unified the downloader's pre-filter with the
registration table and stopped charging non-XDPoS chains for gates they
never use. The pre-filter's dedicated SkipUnjudgeable branch — statically
unreachable there, since downloader.BlockChain itself requires HasBlock
so every dynamic type satisfies consensus.BlockStorer — is removed:
every skip now flows through PreFilterSkipLogLevel, so the table's
preFilter entry for SkipUnjudgeable has its call site and the
single-registration claim holds at both graders; grading stays Error,
the reason stays in the log fields, and the grader's comment now says
the downloader has no dedicated branch. Without a consensus engine
(config.XDPoS == nil) the manager wires an explicit no-op into the
fetcher instead of the gated closure and hands nil to the downloader,
which guards on nil, so such chains no longer pay HasBlockAndExecutedState's
GetBlock plus OpenTrie — or the shared judgment — per imported block.
MockChainReader gains the minimal shape handler-level tests need to run
consensus.ShouldHandleProposedBlock without the integration package:
AddHeader also records the canonical header per height for
GetHeaderByNumber, and HasBlock reports a registered header as stored
(the mock stores headers only). The fetcher wiring test is split to
follow the new wiring: TestFetcherWiresNoopHandlerWithoutConsensus pins
the no-op an ethash manager gets, and
TestFetcherProposedBlockHandlerGates exercises the gated closure
directly, keeping the fast-sync skip, the full-sync pass-through and
the forged unexecuted-state skip with its counter assertion. A
comment-only pass condensed the unjudgeable counter's alerting note and
the fetcher gate's rationale without touching the alert matrix or the
wontfix reorg-window record.

A sixth review pass removed the XDCX/lending state halves from the
fetcher gate and hardened the judgment against a nil header. The
fetcher's executed-state check now runs HasBlockAndExecutedState — a new
BlockChain method that checks GetBlock plus an open state trie and
deliberately leaves out HasFullState's trading and lending halves —
because a correctly executed block missing one auxiliary state piece
would otherwise fail the old HasBlockAndFullState gate forever,
permanently halting QC processing and voting with no self-healing path;
HasFullState keeps its full semantics for its other callers (block
validation, dry-run import). The strength split — judgment and engine
gates on HasBlock, the fetcher gate on executed state, the full-state
callers outside the gates — is documented at both the gate and the
judgment. ShouldHandleProposedBlock now fails a nil header or nil number
as a new SkipNilHeader skip before any chain read instead of panicking
on the number dereference; the reason is registered in the skipGrades
table at Error for both graders, TestShouldHandleProposedBlock pins both
shapes, and both grader tests pin the Error level. proposed_block.go
carries a file header naming it XDPoS v2 specific, and the second
pre-vote gate's comment now points at the chain-side invariant that
actually protects highestCommitBlock — BlockChain.reorg refuses any
reorg whose common ancestor sits strictly below
GetLatestCommittedBlockInfo and whose dropped branch contains the
committed block, while a fork point exactly at that height is allowed
(the committed block is then the fork point itself, never dropped) —
instead of resting the non-reversion on the QC argument alone. The two-grader/registry/counter structure serving
the 20-line judgment is reviewed and kept: the single skipGrades table
is what keeps a new reason from being registered at one call site and
missed at the other, and renaming or moving the counters is churn
without behavioral gain. The proposed-block downloader tests moved to
their own file, eth/downloader/downloader_proposed_block_test.go, so
downloader_test.go stays near 3200 lines: a pure move of
TestImportBlockResultsProposedBlockHandler and its helpers, verified
referenced nowhere outside the moved block.

Seventh review round, all comment-accuracy fixes with zero behavior
change. The second gate's reorg-invariant comment in engine.go was
rewritten to match what core/blockchain.go's guard actually enforces —
rejection requires a common ancestor strictly below the committed height
and a dropped branch containing the committed block at that hash — since
the earlier "at or below" phrasing contradicted the referenced code at
the cmp == 0 boundary, where a reorg whose fork point is the committed
block itself is allowed. The skipGrades table comment now explicitly
declares the fetcher's fast-sync/state gate (eth/handler.go
newFetcherProposedBlockHandler, counter skippedProposedBlockState)
outside the single-registration table: it produces no SkipReason — it
gates on the snapSync flag and HasBlockAndExecutedState before the
consensus handler runs — and grades Warn with its own counter, so the
table's "covers every reason" claim no longer overreaches. The fetcher
gate comment's "hash-keyed state check" shorthand was corrected: only
the GetBlock half is hash-keyed, the OpenTrie half stays root-keyed, and
the leak it guards against is unreachable because a discarded block has
no stored body; HasBlockAndExecutedState deliberately keeps GetBlock —
retaining the approved stronger guarantee — so its one whole-body decode
per propagated block is the price of the interlocking design, not an
accident.

Eighth review round. The fetcher's gated closure gains the nil guard the shared judgment already models: a nil header or nil number now fails as SkipNilHeader — logged Error through consensus.SkipLogLevel, consumed rather than registered so the grading table stays fetcher-free as the previous round recorded — returning nil with no counter of its own, instead of panicking on the dereference if a future call site ever passes an unchecked header; the wiring is unreachable with nil today, so this is a contract fix, and the gates test pins both shapes (no panic, no handler call, an Error log, no counter movement). The non-XDPoS wiring comment was tightened to say what the no-op actually skips per block — no snapSync load, state reads, or counter — since the closure call itself still happens. The proposal to replace the skipGrades table with per-call-site switches or a level-returning grader API is declined again, per the standing rulings: the single table is what keeps the two graders from diverging, an unregistered reason already falls back to a loud Warn at both sites by design, and the 'judge once' phrase refers to log grading, not to a single gate — the two storage strengths are documented intentional design. The remaining points are reviewed and kept: the fetcher field is not a pure test seam (it is the production argument handed to fetcher.NewBlockFetcher, and its comment states both roles), the global logger swap cannot race the package's parallel tests (the two swapping tests are sequential, and Go runs a package's parallel tests only after its sequential ones finish), and the text-parsed level assertions verify emitted levels behaviorally, which is what catches a wrong logger stored in the grading table.

Ninth review round. The engine's ProposedBlockHandler now judges the nil shapes itself before getExtraFields — that call dereferences header.Number, so a nil header or nil number used to panic before the shared judgment's own guard could run, leaving SkipNilHeader unreachable on the engine path and the documented "before any chain read" contract broken there. The guard logs Error through consensus.SkipLogLevel (consumed, not registered — the grading table stays engine-free exactly as it stays fetcher-free), returns nil, and touches no counter: a caller bug is not a block observation, matching the ruling that fixed the fetcher closure. A test pins both shapes on the engine path — no panic, nil returned, an Error log, the counter unmoved — and the existing graded-level assertions are unchanged. The newFetcherProposedBlockHandler doc comment lost a duplicated "Kept a free function" sentence left by two successive edits, reordered to purpose, nil guard, chain source, testability. HasFullState and HasBlockAndExecutedState now share a hasExecutedState helper, so the executed-state criterion — a state trie that opens — is defined once and cannot drift between the two gates; pure extraction, no behavior change. The proposal to centralize the three skip counters into the consensus package with exported accessors is declined: each counter increments at exactly one site so there is no divergence to prevent, the fetcher's state counter was ruled outside the shared table in an earlier round, and moving the counters would grow the consensus package's public API and blur counter ownership without any behavioral gain.

Author: gzliudan

consensus/XDPoS/engines/engine_v2/engine.go

@@ -27,11 +27,23 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/core/vm"
 	"github.com/XinFinOrg/XDPoSChain/ethdb"
 	"github.com/XinFinOrg/XDPoSChain/log"
+	"github.com/XinFinOrg/XDPoSChain/metrics"
 	"github.com/XinFinOrg/XDPoSChain/params"
 	"github.com/XinFinOrg/XDPoSChain/trie"
 	"golang.org/x/sync/errgroup"
 )
 
+// skippedProposedBlock counts skips at this engine's two gates (before
+// processQC and before sendVote): a block that got past the downloader's
+// pre-filter was still declined, so sustained growth is a liveness stall.
+// This counter is the alert target for gate-type stalls (reorg races);
+// unjudgeable skips reaching the gates raise it too, so it is NOT an
+// independent second alert for the wiring bug — that one hangs on
+// unjudgeableProposedBlock alone. (Routine fork-tail skips
+// at the pre-filter are expected and not counted.) At most once per handler
+// invocation; the reason stays in the skip log, one counter for all reasons.
+var skippedProposedBlock = metrics.NewRegisteredCounter("consensus/skipped-proposed-block", nil)
+
 type XDPoS_v2 struct {
 	chainConfig *params.ChainConfig // Chain & network configuration
 
@@ -834,6 +846,25 @@ func (x *XDPoS_v2) ProposedBlockHandler(chain consensus.ChainReader, blockHeader
 		return err
 	}
 
+	// Re-check canonicality and storage at two points. x.lock only serializes
+	// this handler, not InsertChain, so a reorg can land anywhere between the
+	// callers' gates and these checks. The first check protects processQC,
+	// which writes highestQuorumCert, lockQuorumCert and the commit block
+	// before its own existence check; the second keeps the vote's unguarded
+	// window down to the broadcast. An existence check cannot distinguish a
+	// reorged-away block (forks stay in the database as side entries).
+	ok, reason, canonicalHash := consensus.ShouldHandleProposedBlock(chain, blockHeader)
+	if !ok {
+		skippedProposedBlock.Inc(1)
+		consensus.SkipLogLevel(reason)("[ProposedBlockHandler] skip block before processQC", "reason", reason, "hash", blockHeader.Hash(), "number", blockHeader.Number, "canonicalHash", canonicalHash)
+		// An unjudgeable skip is the node stopping voting: the BlockStorer half
+		// is off the chain type (a wiring bug), so processQC and sendVote never
+		// run again; the counter makes the halt observable. Return nil, never
+		// an error: the fetcher's import loop treats a handler error as an
+		// import failure and suppresses the broadcast of an imported block.
+		return nil
+	}
+
 	// Generate blockInfo
 	blockInfo := &types.BlockInfo{
 		Hash:   blockHeader.Hash(),
@@ -856,6 +887,28 @@ func (x *XDPoS_v2) ProposedBlockHandler(chain consensus.ChainReader, blockHeader
 		return err
 	}
 	if verified {
+		// x.lock does not block InsertChain, so a reorg can still land between
+		// the processQC re-check and the broadcast; drop the vote if the block
+		// has been reorged away meanwhile. processQC's state writes are
+		// deliberately not rolled back: the block's embedded QC certifies its
+		// parent, so advancing highestQuorumCert, lockQuorumCert and the commit
+		// block (commitBlocks) on it stays correct. That the committed block itself
+		// is never reorged away is a chain-side invariant, not this QC argument:
+		// BlockChain.reorg refuses any reorg whose common ancestor sits strictly
+		// below GetLatestCommittedBlockInfo and whose dropped branch contains the
+		// committed block (core/blockchain.go, "Ensure XDPoS engine committed
+		// block will be not reverted"); a fork point exactly at that height is
+		// allowed, and then the committed block is the fork point itself, never
+		// dropped. And setNewRound's side
+		// effects — the currentRound bump, the timeoutPool clear and the
+		// newRoundCh signal — are round progression driven by that same valid
+		// QC. Only the vote for the reorged block must go.
+		ok, reason, canonicalHash = consensus.ShouldHandleProposedBlock(chain, blockHeader)
+		if !ok {
+			skippedProposedBlock.Inc(1)
+			consensus.SkipLogLevel(reason)("[ProposedBlockHandler] skip vote for reorged block", "reason", reason, "hash", blockHeader.Hash(), "number", blockHeader.Number, "canonicalHash", canonicalHash)
+			return nil
+		}
 		return x.sendVote(chain, blockInfo)
 	}
 

consensus/XDPoS/engines/engine_v2/vote_test.go

@@ -48,21 +48,30 @@ func (h *memoryHandler) Records() []slog.Record {
 	return out
 }
 
-// MockChainReader is a mock implementation of consensus.ChainReader
+// MockChainReader is a mock implementation of consensus.ChainReader that
+// also satisfies consensus.BlockStorer, so handler-level tests can run
+// consensus.ShouldHandleProposedBlock against it without the integration
+// package. Headers registered via AddHeader serve as the canonical chain
+// (GetHeaderByNumber) and count as stored blocks (HasBlock): the mock
+// stores headers only, so header presence is its storage notion.
 type MockChainReader struct {
 	headers map[common.Hash]*types.Header
+	numbers map[uint64]*types.Header
 }
 
 // NewMockChainReader creates a new mock chain reader
 func NewMockChainReader() *MockChainReader {
 	return &MockChainReader{
 		headers: make(map[common.Hash]*types.Header),
+		numbers: make(map[uint64]*types.Header),
 	}
 }
 
-// AddHeader adds a header to the mock chain
+// AddHeader adds a header to the mock chain, making it the canonical
+// header at its height (last write wins) and marking its block stored.
 func (m *MockChainReader) AddHeader(header *types.Header) {
 	m.headers[header.Hash()] = header
+	m.numbers[header.Number.Uint64()] = header
 }
 
 // Config implements consensus.ChainReader
@@ -82,7 +91,13 @@ func (m *MockChainReader) GetHeader(hash common.Hash, number uint64) *types.Head
 
 // GetHeaderByNumber implements consensus.ChainReader
 func (m *MockChainReader) GetHeaderByNumber(number uint64) *types.Header {
-	return nil
+	return m.numbers[number]
+}
+
+// HasBlock implements consensus.BlockStorer: a registered header counts as
+// a stored block (the mock stores headers only).
+func (m *MockChainReader) HasBlock(hash common.Hash, number uint64) bool {
+	return m.headers[hash] != nil
 }
 
 // GetHeaderByHash implements consensus.ChainReader
@@ -122,3 +137,63 @@ func TestVerifyVoteMessage_VoteRoundTooOld(t *testing.T) {
 	assert.False(t, verified, "Should return false for vote with round < currentRound")
 	assert.NoError(t, err, "Should not return an error for old round votes")
 }
+
+// blockInfoOf turns a header into the BlockInfo shape the voting rule and
+// forensics pass around. The round is irrelevant to isExtendingFromAncestor,
+// which only walks hashes and numbers.
+func blockInfoOf(h *types.Header) *types.BlockInfo {
+	return &types.BlockInfo{Hash: h.Hash(), Number: h.Number}
+}
+
+// TestIsExtendingFromAncestor covers the parent walk of the HotStuff voting
+// rule at the rule layer. The handler-level tests cannot reach the positive
+// branch anymore: since ProposedBlockHandler gates on canonicality, a
+// proposed block on the locked ancestor's own chain that passes the gate
+// always outranks the lockQC round and returns before the walk (see
+// TestShouldNotSendVoteMsgIfCanonicalBlockNotExtendedFromForkedAncestor),
+// and the forensics caller's positive path is only exercised by a skipped
+// test. Both branches of the walk are safety-critical — a false positive
+// lets a node vote off the locked chain, a false negative stalls it — so
+// the walk itself gets direct coverage here.
+func TestIsExtendingFromAncestor(t *testing.T) {
+	// 1 <- 2 <- 3, with 2' a same-height fork of 2.
+	mockChain := NewMockChainReader()
+	h1 := &types.Header{Number: big.NewInt(1)}
+	h2 := &types.Header{Number: big.NewInt(2), ParentHash: h1.Hash()}
+	h3 := &types.Header{Number: big.NewInt(3), ParentHash: h2.Hash()}
+	forkH2 := &types.Header{Number: big.NewInt(2), ParentHash: h1.Hash(), Coinbase: common.BytesToAddress([]byte{0x02})}
+	for _, h := range []*types.Header{h1, h2, h3} {
+		mockChain.AddHeader(h)
+	}
+	engine := &XDPoS_v2{}
+
+	// Positive branch: the walk runs two hops down the parent chain and
+	// lands exactly on the locked ancestor.
+	extended, err := engine.isExtendingFromAncestor(mockChain, blockInfoOf(h3), blockInfoOf(h1))
+	assert.NoError(t, err)
+	assert.True(t, extended, "h3 extends the locked ancestor h1")
+
+	// Negative branch with the walk executed: h3's parent chain bottoms out
+	// at h1, not at the forked ancestor 2', so the final hash comparison
+	// rejects the block. This is the geometry
+	// TestShouldNotSendVoteMsgIfCanonicalBlockNotExtendedFromForkedAncestor
+	// exercises through verifyVotingRule.
+	extended, err = engine.isExtendingFromAncestor(mockChain, blockInfoOf(h3), blockInfoOf(forkH2))
+	assert.NoError(t, err)
+	assert.False(t, extended, "h3 does not extend the forked ancestor 2'")
+
+	// Zero-iteration mismatch: the proposed block sits below the locked
+	// ancestor, so the walk never runs and only the direct hash comparison
+	// can reject it. This is the geometry of
+	// TestShouldNotSendVoteMsgIfBlockNotExtendedFromAncestor.
+	extended, err = engine.isExtendingFromAncestor(mockChain, blockInfoOf(h1), blockInfoOf(h2))
+	assert.NoError(t, err)
+	assert.False(t, extended, "h1 is below the locked ancestor h2")
+
+	// A missing parent aborts the walk with an error instead of silently
+	// reporting false: the proposed block's own header is not in the chain.
+	missing := &types.BlockInfo{Hash: common.StringToHash("missing"), Number: big.NewInt(3)}
+	extended, err = engine.isExtendingFromAncestor(mockChain, missing, blockInfoOf(h1))
+	assert.Error(t, err)
+	assert.False(t, extended)
+}

consensus/proposed_block.go

@@ -0,0 +1,183 @@
+// Copyright 2026 The go-ethereum Authors
+// This file is part of the go-ethereum library.
+//
+// The go-ethereum library is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Lesser General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// The go-ethereum library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package consensus
+
+// This file is XDPoS v2 specific: the proposed-block gating judgment
+// (ShouldHandleProposedBlock), its SkipReason/skipGrades plumbing and the
+// CanonicalChain/BlockStorer capability interfaces serve only the v2 engine's
+// proposed-block handler and its eth/downloader, eth/fetcher gates. The
+// root consensus package hosts them so both the engine and eth can share
+// the judgment without an import cycle.
+
+import (
+	"github.com/XinFinOrg/XDPoSChain/common"
+	"github.com/XinFinOrg/XDPoSChain/core/types"
+	"github.com/XinFinOrg/XDPoSChain/log"
+	"github.com/XinFinOrg/XDPoSChain/metrics"
+)
+
+// unjudgeableProposedBlock counts SkipUnjudgeable skips at the single
+// judgment site every caller shares. Increments are judgment counts, not
+// deduplicated blocks: a proposal can arrive via both fetcher and
+// downloader paths. Alerting: a skip is counted here *and* by
+// consensus/skipped-proposed-block at the engine gates — alert on this
+// counter alone for the wiring bug (gate stalls raise only
+// skipped-proposed-block); each counter owns one alert class and the two
+// are not substitutes.
+var unjudgeableProposedBlock = metrics.NewRegisteredCounter("consensus/unjudgeable-proposed-block", nil)
+
+// CanonicalChain is the subset of ChainReader needed for the canonicality half
+// of the judgment whether a proposed block should be handled by the consensus
+// engine.
+type CanonicalChain interface {
+	// GetHeaderByNumber retrieves a block header from the database by number.
+	GetHeaderByNumber(number uint64) *types.Header
+}
+
+// BlockStorer is the optional capability the judgment needs for its storage
+// half. Chains that only carry headers (e.g. *core.HeaderChain) deliberately
+// do not implement it: handing one to ShouldHandleProposedBlock surfaces as a
+// loud SkipUnjudgeable, not as every block silently failing "not stored".
+type BlockStorer interface {
+	// HasBlock reports whether a block with the given hash and number is
+	// stored in the database.
+	HasBlock(hash common.Hash, number uint64) bool
+}
+
+// SkipReason describes why a proposed block header was rejected by
+// ShouldHandleProposedBlock. The empty value means the header was accepted.
+type SkipReason string
+
+// The reasons ShouldHandleProposedBlock can report for rejecting a header.
+// Log grading is split by call site: SkipLogLevel at the handler/engine gates,
+// PreFilterSkipLogLevel at the downloader's pre-filter (routine sync noise).
+// When adding a reason, register it in the skipGrades table below, which
+// both graders read.
+const (
+	// SkipNoCanonicalHeader: no canonical header exists at the height.
+	SkipNoCanonicalHeader SkipReason = "no canonical header at height"
+
+	// SkipNonCanonical: the header is not the canonical block at its height.
+	SkipNonCanonical SkipReason = "non-canonical"
+
+	// SkipBodyNotStored: the canonical block's body has not landed in the
+	// database yet, e.g. during the fast sync header phase.
+	SkipBodyNotStored SkipReason = "block body not stored"
+
+	// SkipUnjudgeable: the chain does not implement BlockStorer, so the
+	// judgment cannot run at all. It is a wiring bug (a wrapped or replaced
+	// chain type), not an observation about the block: every block gets
+	// skipped, halting processQC and voting — hence graded Error.
+	SkipUnjudgeable SkipReason = "unjudgeable"
+
+	// SkipNilHeader: the caller passed a nil header or a header without a
+	// number. This is a caller bug, not an observation about a block — the
+	// judgment cannot even address the proposal — hence graded Error like a
+	// wiring bug.
+	SkipNilHeader SkipReason = "nil header"
+)
+
+// ShouldHandleProposedBlock reports whether a proposed block header should
+// reach the consensus handler, plus the SkipReason it must be skipped for
+// and the canonical hash at its height, both for skip logging. The header
+// must be the canonical block at its height (an existence check cannot
+// distinguish a reorged-away fork from the canonical entry) and its body
+// must be stored (fast sync marks a height canonical before its body
+// lands); HasBlock, not GetBlock, avoids decoding whole bodies. The chain
+// reads are not atomic — a reorg can land between or after them; the v2
+// engine re-checks before processQC and before sendVote, and the residual
+// window there spans sendVote's own chain read plus signature and
+// broadcast (documented intentional design). A chain without BlockStorer
+// fails loudly as SkipUnjudgeable before any chain read, incrementing
+// unjudgeableProposedBlock. A nil header or nil number fails as
+// SkipNilHeader for the same reason class (caller bug), also before any
+// chain read. The storage half is graded at two strengths by design: this
+// judgment and the engine gates check HasBlock only, the fetcher gate
+// (eth/handler.go) checks HasBlockAndExecutedState; a future caller with
+// neither guarantee must add its own stronger gate, not weaken these.
+func ShouldHandleProposedBlock(chain CanonicalChain, header *types.Header) (bool, SkipReason, common.Hash) {
+	if header == nil || header.Number == nil {
+		return false, SkipNilHeader, common.Hash{}
+	}
+	storer, ok := chain.(BlockStorer)
+	if !ok {
+		// Single inc point: every caller funnels through this return.
+		unjudgeableProposedBlock.Inc(1)
+		return false, SkipUnjudgeable, common.Hash{}
+	}
+	canonical := chain.GetHeaderByNumber(header.Number.Uint64())
+	if canonical == nil {
+		return false, SkipNoCanonicalHeader, common.Hash{}
+	}
+	if canonical.Hash() != header.Hash() {
+		return false, SkipNonCanonical, canonical.Hash()
+	}
+	if !storer.HasBlock(header.Hash(), header.Number.Uint64()) {
+		return false, SkipBodyNotStored, canonical.Hash()
+	}
+	return true, "", canonical.Hash()
+}
+
+// skipGrade holds the two call-site log grades for one SkipReason: handler
+// serves SkipLogLevel (the handler/engine gates), preFilter serves
+// PreFilterSkipLogLevel (the downloader's pre-filter, routine sync noise).
+type skipGrade struct {
+	handler   func(msg string, ctx ...interface{})
+	preFilter func(msg string, ctx ...interface{})
+}
+
+// skipGrades is the single registration table for every SkipReason: one
+// entry grades a reason at both call sites, so a newly added reason cannot
+// be registered for one grader and missed for the other. An unregistered
+// reason — including the misused empty reason — falls back to Warn in both.
+// The fetcher's fast-sync/state gate (eth/handler.go
+// newFetcherProposedBlockHandler, counter skippedProposedBlockState) is
+// deliberately outside this table: it produces no SkipReason — it gates on
+// the snapSync flag and HasBlockAndExecutedState before the consensus
+// handler runs — and grades Warn with its own counter.
+var skipGrades = map[SkipReason]skipGrade{
+	SkipUnjudgeable:       {log.Error, log.Error},
+	SkipNilHeader:         {log.Error, log.Error},
+	SkipNonCanonical:      {log.Warn, log.Info},
+	SkipNoCanonicalHeader: {log.Warn, log.Info},
+	SkipBodyNotStored:     {log.Info, log.Info},
+}
+
+// SkipLogLevel grades a skip once the handler is reached: SkipUnjudgeable
+// and SkipNilHeader are the Errors (wiring/caller bugs that halt QC
+// processing and voting), SkipNonCanonical/SkipNoCanonicalHeader are reorg
+// races (Warn), SkipBodyNotStored is the expected fast-sync skip (Info).
+// Unregistered reasons, including the misused empty reason, grade Warn so
+// they surface.
+func SkipLogLevel(reason SkipReason) func(msg string, ctx ...interface{}) {
+	if g, ok := skipGrades[reason]; ok {
+		return g.handler
+	}
+	return log.Warn
+}
+
+// PreFilterSkipLogLevel grades a skip at the downloader's pre-filter, where
+// fork tails are routine sync noise: the three routine reasons grade Info.
+// SkipUnjudgeable and SkipNilHeader grade Error here too — the downloader
+// has no dedicated branches for them, so this grader is their only
+// call-site grading path — and an unregistered reason grades Warn.
+func PreFilterSkipLogLevel(reason SkipReason) func(msg string, ctx ...interface{}) {
+	if g, ok := skipGrades[reason]; ok {
+		return g.preFilter
+	}
+	return log.Warn
+}