#33854 fix assumevalid is ignored during reindex
https://github.com/bitcoin/bitcoin/pull/33854 · · +285/-96 in 11 files, 8 commits · labels: Validation, Needs rebase
Goal
- Prevent fallback to slow full script verification when reindexing after an interrupted initial sync
- Ensure assumevalid applies even if the previous sync stopped before reaching minimum chainwork
This PR modifies reindex startup so that if the chainwork of the best header on disk is below minimumchainwork, the node defers ActivateBestChain(). Instead, it allows headers sync over P2P during reindexing until the header chain reaches minimumchainwork, ensuring that assumevalid is applied rather than falling back to full script verification.
Problem: When reindexing after an interrupted IBD that stopped before accumulating minimumchainwork, the node fails to locate the assumevalid block in its headers, forcing full script verification for all blocks on disk and significantly slowing reindex times.
Category: P2P (#60 of 65)
P4 · speedup
- P4 because headers sync during block loading is only an auxiliary hook for validation reindex logic
- Does not resolve an independent P2P networking issue or performance bottleneck
The change to net_processing allows headers synchronization while loading blocks, but it does so purely as an auxiliary mechanism for validation's reindex logic without solving an independent P2P issue.
Membership: Modifies net_processing.cpp to permit HEADERS sync while LoadingBlocks() is true.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 0
Category: Validation (#35 of 48)
P3 · speedup
- P3 because missing assumevalid only causes redundant script checks, keeping validation strictly safe
- Benefits only the rare scenario where an initial sync was interrupted before minimum chainwork
Fixes issue #31494 where assumevalid is bypassed on reindex if previous IBD was interrupted before minimum chainwork. The bug only results in redundant script verification (which is strictly safer), so there is no consensus or security risk, making it a performance optimization for a rare scenario.
Membership: Changes reindex behavior, ImportBlocks, ActivateBestChain, and block validation ordering in src/validation.cpp and src/node/blockstorage.cpp.
Factors: security/stability 0, bug 1, performance 2, user value 1, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with the main branch
The PR has merge conflicts with the main branch and is flagged dirty.
Author status: silent since 2026-07-21, though sedited's critique was posted recently on 2026-09-17
Open concerns:
- sedited questioned the approach, noting high complexity, the inability to reindex offline without minchainwork, and whether the edge case justifies the added code
- mzumsande warned about potential msghand thread stalling if large chains are connected during P2P processing
Resolved concerns:
- Eunovo moved deferred block connection to the background init thread using a condition variable to avoid blocking msghand
Agreement: Disputed
- Concept approval verified with a reproducer (l0rinc)
- Thinks added complexity is not worth the benefit for a rare salvage scenario (sedited)
- Concerned about breaking offline reindexing and potential peer disconnects (sedited, mzumsande)
Concept ACK from l0rinc, but sedited questions whether the complexity is worth the squeeze and objects to breaking offline reindex
While l0rinc supported the concept and verified the reproducer, sedited argued that the change adds too much complexity for an infrequent edge case and impairs offline reindexing.
- l0rinc: 'Concept ACK' and confirmed the fix with a reproducer
- mzumsande: raised concerns regarding peer disconnects from blocking msghand and offline reindexing failure
- gmaxwell: noted security assumptions around not recommending users lower -minimumchainwork
- sedited: 'This is a quite complicated change for what should be such a simple concept! ... Maybe the juice is also just not worth the squeeze for a salvage scenario that does not happen often.'
Review verdicts (DrahtBot): 0
- Concept ACK: l0rinc
Files
124 lines under test/bench/ci.
- src/init.cpp +49/-20
- src/node/blockstorage.cpp +39/-24
- src/validation.cpp +31/-25
- test/functional/feature_reindex.py +55/-1
- test/functional/p2p_unrequested_blocks.py +27/-14
- src/node/blockstorage.h +32/-1
- test/functional/feature_assumevalid.py +26/-1
- src/net_processing.cpp +5/-9
- src/validation.h +13/-0
- src/node/kernel_notifications.h +4/-1
- src/node/kernel_notifications.cpp +4/-0
Card
This PR modifies reindex behavior so that if the best header on disk has less than minimumchainwork, block activation is deferred while the node syncs headers from peers over P2P. This prevents bitcoind from bypassing assumevalid and executing full script verification when reindexing after an early-interrupted IBD. While l0rinc gave Concept ACK, reviewers including mzumsande and sedited highlighted that this adds significant complexity, complicates or disables offline reindexing, and might not be worth the cost for a rare recovery scenario. The PR currently has merge conflicts and needs a rebase.