Merge bitcoin/bitcoin#34521: validation: fix UB in LoadChainTip

20ae9b98eab20117344cf31f7cde39cadd70ca22 Extend functional test for setBlockIndexCandidates UB (marcofleon)
854a6d5a9a0e40329a2852efb2a8dfec4b54886e validation: fix UB in LoadChainTip (marcofleon)
9249e6089ec4e2eb63f0896961f04d9dbe14651a validation: remove LoadChainTip call from ActivateSnapshot (marcofleon)

Pull request description:

  Addresses https://github.com/bitcoin/bitcoin/issues/34503. See this issue for more details as well.

  Fixes a bug where, under certain conditions, `setBlockIndexCandidates` had blocks in it that were worse than the tip. The block index candidate set uses `nSequenceId` as a sort key, so modifying this field while blocks are in the set results in undefined behavior. This PR populates `setBlockIndexCandidates` after the `nSequenceId` modifications, avoiding the UB.

ACKs for top commit:
  achow101:
    ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
  sedited:
    Re-ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
  sipa:
    Code review ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22

Tree-SHA512: 121c170bb70fb6365089d578db63c811e7926e129d7206e569947f7a1f6c5ddc8d5f4937b80f1ba1b7d7daa42789b143ca5b56f154b7ab968a1cd55f925f378d

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(); })) {

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

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

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<CBlockIndex*> 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<void> 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));

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

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