Merge #21582: Fix assumeutxo crash due to missing base_blockhash

fa9b74f Fix assumeutxo crash due to missing base_blockhash (MarcoFalke)
fa8fffe refactor: Prefer clean assert over UB in coinstats (MarcoFalke)

Pull request description:

  This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:

  * Adds an `Assert` to transform the UB into a clean crash, even when sanitizers are disabled
  * Adds an early-fail condition to avoid the crash

ACKs for top commit:
  jamesob:
    ACK fa9b74f ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))
  ryanofsky:
    Code review ACK fa9b74f with no code changes since last review, just splitting up combocommit a little.

Tree-SHA512: dd36808a09f49c647543a9eaa6fdb785b3f1109af48ba4cc983153b22a144da9ca61af22034dcfaa0e192a65b1ee7de744f187555079aff55bec0efa0ce87cd4

Author: MarcoFalke

src/node/coinstats.cpp

@@ -94,7 +94,8 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&
     {
         LOCK(cs_main);
         assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));
-        stats.nHeight = blockman.LookupBlockIndex(stats.hashBlock)->nHeight;
+        const CBlockIndex* block = blockman.LookupBlockIndex(stats.hashBlock);
+        stats.nHeight = Assert(block)->nHeight;
     }
 
     PrepareHash(hash_obj, stats);

src/test/validation_chainstatemanager_tests.cpp

@@ -259,6 +259,11 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Determi
             // Coins count is smaller than coins in file
             metadata.m_coins_count -= 1;
     }));
+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
+        m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
+            // Wrong hash
+            metadata.m_base_blockhash = uint256::ONE;
+    }));
 
     BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root));
 

src/validation.cpp

@@ -5423,6 +5423,15 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
 
     assert(coins_cache.GetBestBlock() == base_blockhash);
 
+    CBlockIndex* snapshot_start_block = WITH_LOCK(::cs_main, return m_blockman.LookupBlockIndex(base_blockhash));
+
+    if (!snapshot_start_block) {
+        // Needed for GetUTXOStats to determine the height
+        LogPrintf("[snapshot] Did not find snapshot start blockheader %s\n",
+                  base_blockhash.ToString());
+        return false;
+    }
+
     CCoinsStats stats;
     auto breakpoint_fnc = [] { /* TODO insert breakpoint here? */ };
 
@@ -5435,31 +5444,6 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
         return false;
     }
 
-    // Ensure that the base blockhash appears in the known chain of valid headers. We're willing to
-    // wait a bit here because the snapshot may have been loaded on startup, before we've
-    // received headers from the network.
-
-    int max_secs_to_wait_for_headers = 60 * 10;
-    CBlockIndex* snapshot_start_block = nullptr;
-
-    while (max_secs_to_wait_for_headers > 0) {
-        snapshot_start_block = WITH_LOCK(::cs_main,
-            return m_blockman.LookupBlockIndex(base_blockhash));
-        --max_secs_to_wait_for_headers;
-
-        if (!snapshot_start_block) {
-            std::this_thread::sleep_for(std::chrono::seconds(1));
-        } else {
-            break;
-        }
-    }
-
-    if (snapshot_start_block == nullptr) {
-        LogPrintf("[snapshot] timed out waiting for snapshot start blockheader %s\n",
-            base_blockhash.ToString());
-        return false;
-    }
-
     // Assert that the deserialized chainstate contents match the expected assumeutxo value.
 
     int base_height = snapshot_start_block->nHeight;