fix(core): queue all future blocks of a batch instead of failing - #2550
fix(core): queue all future blocks of a batch instead of failing#2550gzliudan wants to merge 1 commit 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: Team 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.
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
ed3a128 to
635a9ab
Compare
XDPoS engines check the header timestamp before the parent lookup with zero tolerance, so children of a future block 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 was worse: the tail was silently dropped and the import failed one batch later with a bogus bad-block report against a valid block. Accept ErrFutureBlock in both loops so the whole tail enters the futureBlocks queue; procFutureBlocks imports the queued blocks once their timestamps are reached. Both loops share one queueFutureTail helper. Queuing stays bounded by the maxTimeFutureBlocks window: a block beyond it is logged at Warn and its error is returned to the downloader, as before this change, because a block more than 30s ahead only comes from a skewed local clock or a misbehaving peer, and a node in either state must not report a successful import that moved nothing. A genuine verification error after the processed prefix or the queued tail now surfaces and is reported from the unified exit, so the ChainHeadEvent for any canonical progress is still broadcast before the failure. Only deterministic consensus violations (the exported XDPoS v1 and v2 sentinels) are recorded as bad blocks; verification errors that depend on local database state, such as a QC whose epoch-switch header is not yet available, are logged at Warn and returned without polluting the bad-block table with valid blocks. The exemption set for future, pruned, unknown-ancestor and known blocks is defined once in isBenignImportError and every comparison uses errors.Is, so engines wrapping the sentinels keep the same behaviour. The regression tests live in a self-contained file with their own engine type, so this commit merges cleanly whether it lands before, after, or without the known-block import commit.
635a9ab to
991b51e
Compare
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.