validation: check invariants when inserting into m_blocks_unlinked
What the developer wrote
Authored by stratospher
validation: check invariants when inserting into m_blocks_unlinked
For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.
Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
this entry gets popped twice in ReceivedBlockTransactions and
happens to be in setBlockIndexCandidates)
2 bugs (#35070 and #35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
What changed, and why it matters
This commit adds safety checks around a Bitcoin Core internal data structure called m_blocks_unlinked, which tracks blocks whose parent chain is missing. The change prevents two problems: inserting blocks that have no actual stored data, and inserting the same block twice. The commit message says both duplicate entries and missing-data entries could lead to undefined behavior inside the block validation engine, including corrupting the candidate-chain ordering. The patch is defensive hardening rather than a complete fix for the two referenced bugs.
Treat this as a defensive hardening commit that mitigates symptoms of two related bugs. Review the root-cause fixes for issues #35070 and #35168 separately, and consider whether additional runtime checks or fuzzing around m_blocks_unlinked state transitions are warranted. Node operators should upgrade to a release containing this and the related fixes once available.
Security signals we found
Insertion into m_blocks_unlinked now enforces BLOCK_HAVE_DATA invariant
Duplicate (parent, child) entries are now rejected at all insertion sites
Commit message states duplicate entries can cause undefined behavior when popped twice in ReceivedBlockTransactions
Commit message references two recently discovered bugs (#35070 and #35168) stemming from these invariants being violated
Use of Assume() macros indicates these are treated as internal-consistency assumptions
Evidence from the diff
A new BlockManager::AddUnlinkedBlock helper is introduced and used at all three insertion sites for m_blocks_unlinked. The helper asserts that the block pointer is non-null, that BLOCK_HAVE_DATA is set, and that no duplicate (pprev, block) pair already exists before inserting. Previously, only one site (FindMostWorkChain) checked for duplicates, and none enforced BLOCK_HAVE_DATA. The commit message explicitly links the invariants to recently discovered bugs #35070 and #35168 and warns that duplicate entries can be popped twice in ReceivedBlockTransactions, re-adding an entry to setBlockIndexCandidates with a changed nSequenceId and causing undefined behavior.
Changed components
src/node/blockstorage.cppsrc/node/blockstorage.hsrc/validation.cppBlockManager::m_blocks_unlinkedChainstate::FindMostWorkChainChainstateManager::ReceivedBlockTransactionsBlockManager::LoadBlockIndexInspect captured patch +16 / −6
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index b060108c..8726c741 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -255,6 +255,18 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde
return pindexNew;
}
+void BlockManager::AddUnlinkedBlock(CBlockIndex* block)
+{
+ AssertLockHeld(cs_main);
+ Assume(block != nullptr);
+ Assume(block->nStatus & BLOCK_HAVE_DATA);
+ auto range = m_blocks_unlinked.equal_range(block->pprev);
+ for (auto it = range.first; it != range.second; ++it) {
+ if (it->second == block) return; // don't insert duplicates
+ }
+ m_blocks_unlinked.emplace(block->pprev, block);
+}
+
void BlockManager::PruneOneBlockFile(const int fileNumber)
{
AssertLockHeld(cs_main);
@@ -487,7 +499,7 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
} else {
pindex->m_chain_tx_count = 0;
if (pindex->nStatus & BLOCK_HAVE_DATA) {
- m_blocks_unlinked.insert(std::make_pair(pindex->pprev, pindex));
+ AddUnlinkedBlock(pindex);
}
}
} else {
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index f9c5753b..0ab595ac 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -354,6 +354,7 @@ public:
* All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
*/
std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;
+ void AddUnlinkedBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
std::unique_ptr<BlockTreeDB> m_block_tree_db GUARDED_BY(::cs_main);
diff --git a/src/validation.cpp b/src/validation.cpp
index c1fe70da..30074bc8 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3149,10 +3149,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
// 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);
- }
+ m_blockman.AddUnlinkedBlock(pindexFailed);
}
setBlockIndexCandidates.erase(pindexFailed);
}
@@ -3816,7 +3813,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
}
} else {
if (pindexNew->pprev && pindexNew->pprev->IsValid(BLOCK_VALID_TREE)) {
- m_blockman.m_blocks_unlinked.insert(std::make_pair(pindexNew->pprev, pindexNew));
+ m_blockman.AddUnlinkedBlock(pindexNew);
}
}
}
Why this scored 59/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.