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 06de793d25f..2847116976f 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -77,8 +77,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)); @@ -493,6 +498,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 67130a31cfd..5c02ec2341c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4595,6 +4595,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; @@ -4603,13 +4609,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(), @@ -4914,6 +4913,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); @@ -4930,19 +4945,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; } @@ -5732,10 +5734,10 @@ 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; + chainstate.PopulateBlockIndexCandidates(); + LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString()); LogInfo("[snapshot] (%.2f MB)", chainstate.CoinsTip().DynamicMemoryUsage() / (1000 * 1000)); @@ -5964,7 +5966,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 482772c0d6f..7aeaafe322a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -812,12 +812,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); 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()