fix(core): queue all future blocks of a batch instead of failing - #2550
Open
gzliudan wants to merge 2 commits into
Open
fix(core): queue all future blocks of a batch instead of failing#2550gzliudan wants to merge 2 commits into
gzliudan wants to merge 2 commits into
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
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
ErrFutureBlockwhile 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.
gzliudan
force-pushed
the
fix-future-block-batch
branch
from
September 4, 2026 01:42
633d5cc to
8d97961
Compare
There was a problem hiding this comment.
🟡 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
ErrPrunedAncestororErrKnownBlock,blockis still unconsumed but execution falls through to the final nil return, recreating a silent partial import. Exempt legitimate states only fromreportBlock; 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
gzliudan
force-pushed
the
fix-future-block-batch
branch
22 times, most recently
from
September 6, 2026 08:21
15971f6 to
e960998
Compare
gzliudan
force-pushed
the
fix-future-block-batch
branch
12 times, most recently
from
September 6, 2026 21:16
9823b56 to
380e6c9
Compare
gzliudan
force-pushed
the
fix-future-block-batch
branch
7 times, most recently
from
September 7, 2026 03:36
ad6bab5 to
4862bd0
Compare
wanwiset25
approved these changes
Sep 7, 2026
anunay-xin
approved these changes
Sep 8, 2026
benjamin202410
approved these changes
Sep 10, 2026
…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 one registration table per reason: each entry
covers both call sites' graders, and every engine-counted reason carries 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; the counters are pinned by a test enumerating the declared
reasons.
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.
Problem ------- XDPoS engines check the header timestamp before the parent lookup with zero tolerance, so the children of a future block also fail verification with ErrFutureBlock rather than ErrUnknownAncestor. The insertChain future-block loops only accepted ErrUnknownAncestor, so a batch whose first or middle block was in the future stopped at its second block and returned the error, which the downloader wraps into errInvalidChain and answers by dropping the peer on a valid delivery. Before XinFinOrg#2534 the same scenario silently dropped the tail and failed one batch later with a bogus bad-block report. Changes ------- * Accept ErrFutureBlock in both insertChain future-block loops so the whole tail enters the futureBlocks queue and the import reports success; the queued blocks are imported by procFutureBlocks once their timestamps are reached. Queueing is bounded by addFutureBlock's maxTimeFutureBlocks (30s) window: a tail whose timestamps span past it aborts the batch with the enqueue error (non-sentinel, so the downloader still drops the peer) — the same visible outcome as before — while the in-window prefix stays queued. * Gate HandleProposedBlock on canonicality instead of block existence (importBlockResults now requires the tail to be the current head). A nil InsertChain no longer implies the last block reached the chain, and a queued tail can sit at or below the head height (re-delivered history on a side chain); feeding such a block into processQC overwrites highestQuorumCert before its block-existence check, so a master node could end up voting for a block that is not in the chain. * In the fetcher, move the sign hook, the proposed-block handling and the final broadcast behind a "block exists in the chain" gate. * Evict parked blocks that fail to import: procFutureBlocks used to keep every failed block, so garbage delivered inside the future window could fill the queue permanently and be re-verified and re-reported as bad blocks on every futureBlocksLoop tick forever. A block is now evicted unless it is still in the future or its parent is itself parked; blocks are sorted by number, so evicting a failed parent cascades to its queued children within the same pass. * Collapse the duplicated enqueue loops of the two insertChain future paths into the shared queueFutureTail helper, and drop the dead insertStats accounting on the abort paths (stats.report is only called from the main import loop). Tests ----- Future-block regression tests live in core/blockchain_futureblocks_test.go.
gzliudan
force-pushed
the
fix-future-block-batch
branch
from
September 10, 2026 10:52
4862bd0 to
8c37dc4
Compare
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.
Summary
insertChaintreats a mid-batchErrFutureBlockas a fatal verification error: both future-block loops only acceptedErrUnknownAncestor, so a batch whose first or middle block was in the future stopped at its second block and returnedconsensus.ErrFutureBlock. The downloader wraps any non-nilInsertChainerror intoerrInvalidChain(eth/downloader/downloader.go) andSynchroniseanswers that by dropping the delivering peer, aborting the round and retrying with another peer. Near the chain tip this repeats and continuously evicts innocent peers.Root cause
The XDPoS engines check the header timestamp before the parent lookup and with zero tolerance (
header.Time > now), unlike upstream geth's 15-secondallowedFutureBlockTimeSeconds. Consequently the children of a future block fail the future check too — they surface asErrFutureBlock, never asErrUnknownAncestor, so the inner loops (which only acceptErrUnknownAncestor) stop at the second block and the rest of the batch is silently skipped.Before #2534 the same scenario silently dropped the tail and failed one batch later with a bogus bad-block report.
Fix
Accept
consensus.ErrFutureBlockin both future-block loops ofinsertChain(first-block path and tail path), so the whole tail enters thefutureBlocksqueue and the import reports success. No error is swallowed: the loops drain naturally to(nil, nil)and genuine verification errors still propagate. The queued blocks are imported byprocFutureBlocks(100 ms ticker) once their timestamps are reached, matching the upstream geth design intent.Blocks beyond the
now+30sfuture window still return an error (abort + peer drop), which is the documented behaviour for a local clock more than 30 seconds behind — an environment problem, not an invalid chain.A genuine verification error on a block after the queued future tail (e.g. a malformed validator field, which the XDPoS engines check before the timestamp) also surfaces: it is recorded as a bad block and returned, so only a fully drained tail reports success. Future, pruned-ancestor and known blocks remain exempt from the bad-block report — they are legitimate chain states. The same applies to a genuine error after the queued prefix when the batch starts with a future block: it is recorded and returned as well. When the known-block import PR lands, its final-return filter reports the same block again;
WriteBadBlockdeduplicates by (number, hash), so the only effect is one extra BAD BLOCK log line.Tests
New
core/blockchain_futureblocks_test.go(self-contained, ownfailRangeEnginewith anatomic.Uint64fail range because the chain's future-block loop callsVerifyHeadersconcurrently):TestInsertChainQueuesMidBatchFutureBlocks— a batch rejected as future mid-way returnsn == len(blocks), err nil, head stays at blocks[1], the whole tail is infutureBlocks, no bad-block records.TestInsertChainQueuesFutureBatchFromFirstBlock— same for a batch whose first block is already in the future (pre-existing scenario).TestInsertChainProcFutureBlocksResumesImport— once headers verify again, the queued tail imports and the head reaches the batch tip (head polled with a deadline becauseInsertChainusesTryLockand the background loop may be mid-import).All three are mutation-verified: reverting either loop condition alone makes the corresponding test fail.
go test ./core/andgo test ./eth/downloader/pass, also under-race;gofmt/go vetclean.Compatibility
No consensus-rule changes: block/receipt structure, state transition and wire protocol are untouched. Only the error handling of the import path changes. No migration or node-operator action required. Suggested label:
consensus.