This is joint work with @ mzumsande.
note: this requires a pruned node with deep reorgs to trigger. still it breaks assumptions in the codebase and is good to fix. A similar UB was fixed in #34521.
This PR prevents duplicate insertions into m_blocks_unlinked in FindMostWorkChain. There are 3 ways to insert into m_blocks_unlinked:
LoadBlockIndex- not problematic, as each block index is processed only once.ReceivedBlockTransactions- not problematic, as this is usually only called once per block when it is first accepted inAcceptBlock. in the rare case it’s triggered again after pruning, the block would have been removed fromm_blocks_unlinkedwhen it was initially pruned, so duplicates still can’t arise.FindMostWorkChain- problematic when multiple candidate tips share common chains of ancestors, traversals from each tip to the fork point may insert duplicate (pprev,pindex) entries for blocks whose parents have been pruned.
When the missing parent is later received and ReceivedBlockTransactions processes m_blocks_unlinked, the same entry may be processed multiple times. This can result in the block being re-added to setBlockIndexCandidates with a modified nSequenceId, violating its ordering invariants and leading to undefined behavior. So avoid duplicate insertions into m_blocks_unlinked in FindMostWorkChain.
how to test:
use the updated feature_pruning.py which adds coverage for this scenario.
- on master: the test (with the below diff) fails since
nSequenceIdis being modified for an entry insetBlockIndexCandidates - on this branch: the test (with the below diff) passes
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3814,6 +3814,12 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
}
pindex->m_chain_tx_count = prev_tx_sum(*pindex);
+ for (const auto& c : m_chainstates) {
+ if (c->setBlockIndexCandidates.contains(pindex)) {
+ LogInfo("### pindex UB = %s", pindex);
+ assert(false);
+ }
+ }
pindex->nSequenceId = nBlockSequenceId++;
for (const auto& c : m_chainstates) {
c->TryAddBlockIndexCandidate(pindex);
