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);