From 9249e6089ec4e2eb63f0896961f04d9dbe14651a Mon Sep 17 00:00:00 2001 From: marcofleon Date: Wed, 4 Mar 2026 19:15:34 +0000 Subject: [PATCH 1/3] validation: remove LoadChainTip call from ActivateSnapshot This call is a no-op. PopulateAndValidateSnapshot already sets both the chain tip and the coins cache best block to the snapshot block, so LoadChainTip always hits the early return when it finds that the two match (tip->GetBlockHash() == coins_cache.GetBestBlock()). --- src/validation.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1285edc261b..3ed46c1ac3e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5741,8 +5741,6 @@ util::Result ChainstateManager::ActivateSnapshot( } Chainstate& chainstate{AddChainstate(std::move(snapshot_chainstate))}; - const bool chaintip_loaded{chainstate.LoadChainTip()}; - assert(chaintip_loaded); m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight; LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString()); From 854a6d5a9a0e40329a2852efb2a8dfec4b54886e Mon Sep 17 00:00:00 2001 From: marcofleon Date: Wed, 4 Mar 2026 19:39:20 +0000 Subject: [PATCH 2/3] 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); From 20ae9b98eab20117344cf31f7cde39cadd70ca22 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Fri, 13 Feb 2026 14:08:11 +0000 Subject: [PATCH 3/3] Extend functional test for setBlockIndexCandidates UB Fix the from-disk subtest to use a separate node so it builds on a clean genesis block, rather than the leftover chain from the in-memory subtest. Change from a two-way to a three-way block race. The UB in the old LoadChainTip (mutating nSequenceId, a sort key, while the block is in setBlockIndexCandidates) corrupts the internal tree structure, resulting in a failed erase that leaves stale blocks in the set alongside the tip. With only two competing blocks, this is caught by libstdc++ but not by libc++. A three-way split triggers the bug on both implementations. To trigger CheckBlockIndex (where the crashing assertion is), replace the restart loop with sending a new block after a single restart. --- test/functional/feature_chain_tiebreaks.py | 49 ++++++++++++++-------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py index 707c99473cf..1b4037e180a 100755 --- a/test/functional/feature_chain_tiebreaks.py +++ b/test/functional/feature_chain_tiebreaks.py @@ -15,6 +15,9 @@ class ChainTiebreaksTest(BitcoinTestFramework): self.num_nodes = 2 self.setup_clean_chain = True + def setup_network(self): + self.setup_nodes() + @staticmethod def send_headers(node, blocks): """Submit headers for blocks to node.""" @@ -103,27 +106,29 @@ class ChainTiebreaksTest(BitcoinTestFramework): node.invalidateblock(blocks[0].hash_hex) def test_chain_split_from_disk(self): - node = self.nodes[0] + node = self.nodes[1] peer = node.add_p2p_connection(P2PDataStore()) + self.generate(node, 1, sync_fun=self.no_op) + self.log.info('Precomputing blocks') # - # A1 - # / - # G - # \ - # A2 + # /- A1 + # / + # G -- B1 --- A2 + # \ + # \- A3 # blocks = [] - # Construct two blocks building from genesis. + # Construct three equal-work blocks building from the tip. start_height = node.getblockcount() - genesis_block = node.getblock(node.getblockhash(start_height)) - prev_time = genesis_block["time"] + tip_block = node.getblock(node.getbestblockhash()) + prev_time = tip_block["time"] - for i in range(0, 2): + for i in range(0, 3): blocks.append(create_block( - hashprev=int(genesis_block["hash"], 16), + hashprev=int(tip_block["hash"], 16), tmpl={"height": start_height + 1, # Make sure each block has a different hash. "curtime": prev_time + i + 1, @@ -131,16 +136,24 @@ class ChainTiebreaksTest(BitcoinTestFramework): )) blocks[-1].solve() - # Send blocks and test the last one is not connected - self.log.info('Send A1 and A2. Make sure that only the former connects') + # Send blocks and test that only the first one connects + self.log.info('Send A1, A2, and A3. Make sure that only the former connects') peer.send_blocks_and_test([blocks[0]], node, success=True) peer.send_blocks_and_test([blocks[1]], node, success=False) + peer.send_blocks_and_test([blocks[2]], node, success=False) - self.log.info('Restart the node and check that the best tip before restarting matched the ones afterwards') - # Restart and check enough times for this to eventually fail if the logic is broken - for _ in range(10): - self.restart_node(0) - assert_equal(blocks[0].hash_hex, node.getbestblockhash()) + # Restart and send a new block + self.restart_node(1) + assert_equal(blocks[0].hash_hex, node.getbestblockhash()) + peer = node.add_p2p_connection(P2PDataStore()) + next_block = create_block( + hashprev=blocks[0].hash_int, + tmpl={"height": start_height + 2, + "curtime": prev_time + 10, + } + ) + next_block.solve() + peer.send_blocks_and_test([next_block], node, success=True) def run_test(self): self.test_chain_split_in_memory()