From 854a6d5a9a0e40329a2852efb2a8dfec4b54886e Mon Sep 17 00:00:00 2001 From: marcofleon Date: Wed, 4 Mar 2026 19:39:20 +0000 Subject: [PATCH] validation: fix UB in LoadChainTip The removal of the chain tip from setBlockIndexCandidates was happening after nSequenceId was modified. Since the set uses nSequenceId as a sort key, modifying it while the element is in the set is undefined behavior, which can cause the erase to fail. With assumeutxo, a second form of UB exists: two chainstates each have their own candidate set, but share the same CBlockIndex objects. Calling LoadChainTip on one chainstate mutates nSequenceIds that are also in the other chainstate's set. Fix by populating setBlockIndexCandidates after all changes to nSequenceId. --- src/node/chainstate.cpp | 7 +++ src/test/util/chainstate.h | 2 +- .../validation_chainstatemanager_tests.cpp | 10 ++++- src/validation.cpp | 45 ++++++++++--------- src/validation.h | 4 ++ 5 files changed, 45 insertions(+), 23 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index b3e9753e5a7..ff3de298227 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -126,6 +126,13 @@ static ChainstateLoadResult CompleteChainstateInitialization( } } + // Populate setBlockIndexCandidates in a separate loop, after all LoadChainTip() + // calls have finished modifying nSequenceId. Because nSequenceId is used in the + // set's comparator, changing it while blocks are in the set would be UB. + for (const auto& chainstate : chainman.m_chainstates) { + chainstate->PopulateBlockIndexCandidates(); + } + const auto& chainstates{chainman.m_chainstates}; if (std::any_of(chainstates.begin(), chainstates.end(), [](const auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) { diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index 1e0d41cdf86..3ceed5697e3 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -84,7 +84,6 @@ CreateAndActivateUTXOSnapshot( chain.InitCoinsDB(1 << 20, /*in_memory=*/true, /*should_wipe=*/false); chain.InitCoinsCache(1 << 20); chain.CoinsTip().SetBestBlock(gen_hash); - chain.setBlockIndexCandidates.insert(node.chainman->m_blockman.LookupBlockIndex(gen_hash)); chain.LoadChainTip(); node.chainman->MaybeRebalanceCaches(); @@ -106,6 +105,7 @@ CreateAndActivateUTXOSnapshot( pindex->nSequenceId = 0; pindex = pindex->pprev; } + chain.PopulateBlockIndexCandidates(); } BlockValidationState state; if (!node.chainman->ActiveChainstate().ActivateBestChain(state)) { diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 40f99690ce0..31740f8d98e 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -76,8 +76,13 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) LOCK(::cs_main); c2.InitCoinsCache(1 << 23); c2.CoinsTip().SetBestBlock(active_tip->GetBlockHash()); - c2.setBlockIndexCandidates.insert(manager.m_blockman.LookupBlockIndex(active_tip->GetBlockHash())); + for (const auto& cs : manager.m_chainstates) { + cs->ClearBlockIndexCandidates(); + } c2.LoadChainTip(); + for (const auto& cs : manager.m_chainstates) { + cs->PopulateBlockIndexCandidates(); + } } BlockValidationState _; BOOST_CHECK(c2.ActivateBestChain(_, nullptr)); @@ -492,6 +497,9 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) BOOST_CHECK(cs->setBlockIndexCandidates.empty()); } chainman.LoadBlockIndex(); + for (const auto& cs : chainman.m_chainstates) { + cs->PopulateBlockIndexCandidates(); + } }; // Ensure that without any assumed-valid BlockIndex entries, only the current tip is diff --git a/src/validation.cpp b/src/validation.cpp index 3ed46c1ac3e..a2571787432 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4604,6 +4604,12 @@ bool Chainstate::LoadChainTip() m_chainman.UpdateIBDStatus(); tip = m_chain.Tip(); + // nSequenceId is one of the keys used to sort setBlockIndexCandidates. Ensure all + // candidate sets are empty to avoid UB, as nSequenceId is about to be modified. + for (const auto& cs : m_chainman.m_chainstates) { + assert(cs->setBlockIndexCandidates.empty()); + } + // Make sure our chain tip before shutting down scores better than any other candidate // to maintain a consistent best tip over reboots in case of a tie. auto target = tip; @@ -4612,13 +4618,6 @@ bool Chainstate::LoadChainTip() target = target->pprev; } - // Block index candidates are loaded before the chain tip, so we need to replace this entry - // Otherwise the scoring will be based on the memory address location instead of the nSequenceId - setBlockIndexCandidates.erase(tip); - TryAddBlockIndexCandidate(tip); - PruneBlockIndexCandidates(); - - tip = m_chain.Tip(); LogInfo("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f", tip->GetBlockHash().ToString(), m_chain.Height(), @@ -4923,6 +4922,22 @@ void Chainstate::ClearBlockIndexCandidates() setBlockIndexCandidates.clear(); } +void Chainstate::PopulateBlockIndexCandidates() +{ + AssertLockHeld(::cs_main); + + for (CBlockIndex* pindex : m_blockman.GetAllBlockIndices()) { + // With assumeutxo, the snapshot block is a candidate for the tip, but it + // may not have BLOCK_VALID_TRANSACTIONS (e.g. if we haven't yet downloaded + // the block), so we special-case it here. + if (pindex == SnapshotBase() || pindex == TargetBlock() || + (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && + (pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) { + TryAddBlockIndexCandidate(pindex); + } + } +} + bool ChainstateManager::LoadBlockIndex() { AssertLockHeld(cs_main); @@ -4939,19 +4954,6 @@ bool ChainstateManager::LoadBlockIndex() for (CBlockIndex* pindex : vSortedByHeight) { if (m_interrupt) return false; - // If we have an assumeutxo-based chainstate, then the snapshot - // block will be a candidate for the tip, but it may not be - // VALID_TRANSACTIONS (eg if we haven't yet downloaded the block), - // so we special-case the snapshot block as a potential candidate - // here. - if (pindex == CurrentChainstate().SnapshotBase() || - (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && - (pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) { - - for (const auto& chainstate : m_chainstates) { - chainstate->TryAddBlockIndexCandidate(pindex); - } - } if (pindex->nStatus & BLOCK_FAILED_VALID && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) { m_best_invalid = pindex; } @@ -5743,6 +5745,8 @@ util::Result ChainstateManager::ActivateSnapshot( Chainstate& chainstate{AddChainstate(std::move(snapshot_chainstate))}; m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight; + chainstate.PopulateBlockIndexCandidates(); + LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString()); LogInfo("[snapshot] (%.2f MB)", chainstate.CoinsTip().DynamicMemoryUsage() / (1000 * 1000)); @@ -5971,7 +5975,6 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( assert(index); assert(index == snapshot_start_block); index->m_chain_tx_count = au_data.m_chain_tx_count; - snapshot_chainstate.setBlockIndexCandidates.insert(snapshot_start_block); LogInfo("[snapshot] validated snapshot (%.2f MB)", coins_cache.DynamicMemoryUsage() / (1000 * 1000)); diff --git a/src/validation.h b/src/validation.h index 675d7061ab1..83f6f04c5b0 100644 --- a/src/validation.h +++ b/src/validation.h @@ -811,12 +811,16 @@ public: /** Ensures we have a genesis block in the block tree, possibly writing one to disk. */ bool LoadGenesisBlock(); + /** Add a block to the candidate set if it has as much work as the current tip. */ void TryAddBlockIndexCandidate(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void PruneBlockIndexCandidates(); void ClearBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Populate the candidate set by calling TryAddBlockIndexCandidate on all valid block indices. */ + void PopulateBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Find the last common block of this chain and a locator. */ const CBlockIndex* FindForkInGlobalIndex(const CBlockLocator& locator) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);