Merge bitcoin/bitcoin#31910: qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data

63b534f97e fuzz: sanity check hardcoded snapshot in utxo_snapshot target (Antoine Poinsot)
3b85eba83a test util: split up ConnectBlock from MineBlock (Antoine Poinsot)
d1527f6b88 qa: correct off-by-one in utxo snapshot fuzz target (Antoine Poinsot)

Pull request description:

  The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by introducing a unit test which would break if the snapshot data the fuzz target relies on were to change.

  In implementing this i noticed the height used for coins in the fuzz target is actually off-by-one (as if the first block in the created chain was the genesis but it's block `1`), so fix that too.

ACKs for top commit:
  mzumsande:
    Code Review ACK 63b534f97e
  fjahr:
    tACK 63b534f97e

Tree-SHA512: 2399b6e74db9b78aab8efba67c57a405d2d7d880ae3b7d8518a1c96cc6266f61f5e77722cd999adeac5d3e03e73d84cf9ae7bdbcc0afae198cc87049dde4012f

src/kernel/chainparams.cpp

@@ -589,7 +589,7 @@ public:
             {
                 // For use by fuzz target src/test/fuzz/utxo_snapshot.cpp
                 .height = 200,
-                .hash_serialized = AssumeutxoHash{uint256{"4f34d431c3e482f6b0d67b64609ece3964dc8d7976d02ac68dd7c9c1421738f2"}},
+                .hash_serialized = AssumeutxoHash{uint256{"7e3b7780fbd2fa479a01f66950dc8f728dc1b11f03d06d5bf223168520df3a48"}},
                 .m_chain_tx_count = 201,
                 .blockhash = consteval_ctor(uint256{"5e93653318f294fb5aa339d00bbf8cf1c3515488ad99412c37608b139ea63b27"}),
             },

src/test/fuzz/utxo_snapshot.cpp

@@ -7,6 +7,7 @@
 #include <coins.h>
 #include <consensus/consensus.h>
 #include <consensus/validation.h>
+#include <kernel/coinstats.h>
 #include <node/blockstorage.h>
 #include <node/utxo_snapshot.h>
 #include <primitives/block.h>
@@ -39,7 +40,31 @@ using node::SnapshotMetadata;
 namespace {
 
 const std::vector<std::shared_ptr<CBlock>>* g_chain;
-TestingSetup* g_setup;
+TestingSetup* g_setup{nullptr};
+
+/** Sanity check the assumeutxo values hardcoded in chainparams for the fuzz target. */
+void sanity_check_snapshot()
+{
+    Assert(g_chain && g_setup == nullptr);
+
+    // Create a temporary chainstate manager to connect the chain to.
+    const auto tmp_setup{MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST, TestOpts{.setup_net = false})};
+    const auto& node{tmp_setup->m_node};
+    for (auto& block: *g_chain) {
+        ProcessBlock(node, block);
+    }
+
+    // Connect the chain to the tmp chainman and sanity check the chainparams snapshot values.
+    LOCK(cs_main);
+    auto& cs{node.chainman->ActiveChainstate()};
+    cs.ForceFlushStateToDisk();
+    const auto stats{*Assert(kernel::ComputeUTXOStats(kernel::CoinStatsHashType::HASH_SERIALIZED, &cs.CoinsDB(), node.chainman->m_blockman))};
+    const auto cp_au_data{*Assert(node.chainman->GetParams().AssumeutxoForHeight(2 * COINBASE_MATURITY))};
+    Assert(stats.nHeight == cp_au_data.height);
+    Assert(stats.nTransactions + 1 == cp_au_data.m_chain_tx_count); // +1 for the genesis tx.
+    Assert(stats.hashBlock == cp_au_data.blockhash);
+    Assert(AssumeutxoHash{stats.hashSerialized} == cp_au_data.hash_serialized);
+}
 
 template <bool INVALID>
 void initialize_chain()
@@ -47,6 +72,10 @@ void initialize_chain()
     const auto params{CreateChainParams(ArgsManager{}, ChainType::REGTEST)};
     static const auto chain{CreateBlockChain(2 * COINBASE_MATURITY, *params)};
     g_chain = &chain;
+
+    // Make sure we can generate a valid snapshot.
+    sanity_check_snapshot();
+
     static const auto setup{
         MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST,
                                            TestOpts{
@@ -101,7 +130,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
             std::vector<uint8_t> file_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
             outfile << std::span{file_data};
         } else {
-            int height{0};
+            int height{1};
             for (const auto& block : *g_chain) {
                 auto coinbase{block->vtx.at(0)};
                 outfile << coinbase->GetHash();

src/test/util/mining.cpp

@@ -92,6 +92,11 @@ COutPoint MineBlock(const NodeContext& node, std::shared_ptr<CBlock>& block)
         assert(block->nNonce);
     }
 
+    return ProcessBlock(node, block);
+}
+
+COutPoint ProcessBlock(const NodeContext& node, const std::shared_ptr<CBlock>& block)
+{
     auto& chainman{*Assert(node.chainman)};
     const auto old_height = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveHeight());
     bool new_block;

src/test/util/mining.h

@@ -32,6 +32,11 @@ COutPoint MineBlock(const node::NodeContext&,
  **/
 COutPoint MineBlock(const node::NodeContext&, std::shared_ptr<CBlock>& block);
 
+/**
+ * Returns the generated coin (or Null if the block was invalid).
+ */
+COutPoint ProcessBlock(const node::NodeContext&, const std::shared_ptr<CBlock>& block);
+
 /** Prepare a block to be mined */
 std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&);
 std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext& node,