validation: avoid duplicates in m_blocks_unlinked
What the developer wrote
Authored by stratospher
validation: avoid duplicates in m_blocks_unlinked
In a pruned node undergoing a deep reorg, FindMostWorkChain can
insert duplicate entries into m_blocks_unlinked.
This can happen when:
- Traversing from one candidate tip to the fork point adds blocks
whose parents have been pruned.
- Traversing from another candidate tip over the same fork inserts
the same pairs again, since the blocks are shared across
both branches.
When we finally download the missing parent from our peer and call
ReceivedBlockTransactions to process m_blocks_unlinked, the same
entry may be processed multiple times.
This can lead to re-insertion into setBlockIndexCandidates with
a modified nSequenceId, violating its ordering invariants and
causing undefined behavior. So avoid duplicate insertions into
m_blocks_unlinked here.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
What changed, and why it matters
This Bitcoin Core update fixes a bug where, during a deep chain reorganization on a pruned node, the same block entries could accidentally be added twice to an internal waiting list. If processed twice later, this could corrupt an ordered set used to choose the best chain, potentially causing crashes or undefined behavior. The fix prevents duplicates and adds a safety check.
Apply the patch. It is a targeted correctness fix for pruned nodes. Monitor for related crash reports or consensus anomalies on pruned nodes during large reorgs.
Security signals we found
Data structure invariant violation (setBlockIndexCandidates ordering)
Undefined behavior from duplicate processing
Pruned-node deep reorg trigger condition
Internal consistency assertion added in CheckBlockIndex()
Evidence from the diff
In Chainstate::FindMostWorkChain(), traversing multiple candidate tips over a shared fork can insert duplicate (parent, child) pairs into m_blocks_unlinked when blocks’ parents have been pruned. Later, ReceivedBlockTransactions() may process the same entry more than once, re-inserting the block into setBlockIndexCandidates with a modified nSequenceId and violating the set’s ordering invariants. The patch checks for existing duplicates before insertion and strengthens CheckBlockIndex() to assert no duplicates exist.
Changed components
src/validation.cppChainstate::FindMostWorkChain()ChainstateManager::CheckBlockIndex()m_blocks_unlinkedsetBlockIndexCandidatesInspect captured patch +15 / −10
diff --git a/src/validation.cpp b/src/validation.cpp
index 77e91999..c1fe70da 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3141,12 +3141,18 @@ CBlockIndex* Chainstate::FindMostWorkChain()
}
// Remove the entire chain from the set.
for (CBlockIndex *pindexFailed = pindexNew; pindexFailed != pindexTest; pindexFailed = pindexFailed->pprev) {
+ // If we're missing data and not a descendant of an invalid block,
+ // then add back to m_blocks_unlinked, so that if the block arrives in the future
+ // we can try adding to setBlockIndexCandidates again.
if (fMissingData && !fFailedChain) {
- // If we're missing data and not a descendant of an invalid block,
- // then add back to m_blocks_unlinked, so that if the block arrives in the future
- // we can try adding to setBlockIndexCandidates again.
- m_blockman.m_blocks_unlinked.insert(
- std::make_pair(pindexFailed->pprev, pindexFailed));
+ // Avoid duplicate entries in m_blocks_unlinked. If the same entry is
+ // processed twice in ReceivedBlockTransactions(), it may be re-added to
+ // setBlockIndexCandidates with a modified nSequenceId, breaking ordering
+ // guarantees and leading to undefined behavior.
+ auto range = m_blockman.m_blocks_unlinked.equal_range(pindexFailed->pprev);
+ if (!std::any_of(range.first, range.second, [&](const auto& p) { return p.second == pindexFailed; })) {
+ m_blockman.m_blocks_unlinked.emplace(pindexFailed->pprev, pindexFailed);
+ }
}
setBlockIndexCandidates.erase(pindexFailed);
}
@@ -5340,13 +5346,12 @@ void ChainstateManager::CheckBlockIndex() const
// Check whether this block is in m_blocks_unlinked.
auto rangeUnlinked{m_blockman.m_blocks_unlinked.equal_range(pindex->pprev)};
bool foundInUnlinked = false;
- while (rangeUnlinked.first != rangeUnlinked.second) {
- assert(rangeUnlinked.first->first == pindex->pprev);
- if (rangeUnlinked.first->second == pindex) {
+ for (auto it = rangeUnlinked.first; it != rangeUnlinked.second; ++it) {
+ assert(it->first == pindex->pprev);
+ if (it->second == pindex) {
+ assert(!foundInUnlinked); // No duplicates in m_blocks_unlinked
foundInUnlinked = true;
- break;
}
- rangeUnlinked.first++;
}
if (pindex->pprev && (pindex->nStatus & BLOCK_HAVE_DATA) && pindexFirstNeverProcessed != nullptr && pindexFirstInvalid == nullptr) {
// If this block has block data available, some parent was never received, and has no invalid parents, it must be in m_blocks_unlinked.
Why this scored 54/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.