Merge bitcoin/bitcoin#30644: fuzz: Faster utxo_snapshot fuzz target

fa899fb fuzz: Speed up utxo_snapshot fuzz target (MarcoFalke)
fa38664 fuzz: Speed up utxo_snapshot by lazy re-init (MarcoFalke)
fa645c7 fuzz: Remove unused DataStream object (MarcoFalke)
fae8c73 test: Disallow fee_estimator construction in ChainTestingSetup (MarcoFalke)

Pull request description:

  Two commits to speed up unit and fuzz tests.

  Can be tested by running the fuzz target and looking at the time it took, or by looking at the flamegraph. For example:

  ```
  FUZZ=utxo_snapshot perf record -g --call-graph dwarf ./src/test/fuzz/fuzz  -runs=100
  hotspot ./perf.data

ACKs for top commit:
  TheCharlatan:
    Re-ACK fa899fb
  marcofleon:
    Re ACK fa899fb
  brunoerg:
    ACK fa899fb

Tree-SHA512: d3a771bb12d7ef491eee61ca47325dd1cea5c20b6ad42554babf13ec98d03bef8e7786159d077e59cc7ab8112495037b0f6e55edae65b871c7cf1708687cf717

Author: achow101

src/test/fuzz/utxo_snapshot.cpp

@@ -1,45 +1,79 @@
-// Copyright (c) 2021-2022 The Bitcoin Core developers
+// Copyright (c) 2021-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <chain.h>
 #include <chainparams.h>
+#include <coins.h>
+#include <consensus/consensus.h>
 #include <consensus/validation.h>
+#include <node/blockstorage.h>
 #include <node/utxo_snapshot.h>
+#include <primitives/block.h>
+#include <primitives/transaction.h>
+#include <serialize.h>
+#include <span.h>
+#include <streams.h>
+#include <sync.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <test/util/mining.h>
 #include <test/util/setup_common.h>
-#include <util/chaintype.h>
+#include <uint256.h>
+#include <util/check.h>
 #include <util/fs.h>
+#include <util/result.h>
 #include <validation.h>
-#include <validationinterface.h>
+
+#include <cstdint>
+#include <functional>
+#include <ios>
+#include <memory>
+#include <optional>
+#include <vector>
 
 using node::SnapshotMetadata;
 
 namespace {
 
 const std::vector<std::shared_ptr<CBlock>>* g_chain;
+TestingSetup* g_setup;
 
+template <bool INVALID>
 void initialize_chain()
 {
     const auto params{CreateChainParams(ArgsManager{}, ChainType::REGTEST)};
     static const auto chain{CreateBlockChain(2 * COINBASE_MATURITY, *params)};
     g_chain = &chain;
+    static const auto setup{
+        MakeNoLogFileContext<TestingSetup>(ChainType::REGTEST,
+                                           TestOpts{
+                                               .setup_net = false,
+                                               .setup_validation_interface = false,
+                                               .min_validation_cache = true,
+                                           }),
+    };
+    if constexpr (INVALID) {
+        auto& chainman{*setup->m_node.chainman};
+        for (const auto& block : chain) {
+            BlockValidationState dummy;
+            bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)};
+            Assert(processed);
+            const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
+            Assert(index);
+        }
+    }
+    g_setup = setup.get();
 }
 
-FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
+template <bool INVALID>
+void utxo_snapshot_fuzz(FuzzBufferType buffer)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
-    std::unique_ptr<const TestingSetup> setup{
-        MakeNoLogFileContext<const TestingSetup>(
-            ChainType::REGTEST,
-            TestOpts{
-                .setup_net = false,
-                .setup_validation_interface = false,
-            })};
-    const auto& node = setup->m_node;
-    auto& chainman{*node.chainman};
+    auto& setup{*g_setup};
+    bool dirty_chainman{false}; // Re-use the global chainman, but reset it when it is dirty
+    auto& chainman{*setup.m_node.chainman};
 
     const auto snapshot_path = gArgs.GetDataDirNet() / "fuzzed_snapshot.dat";
 
@@ -52,7 +86,6 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
             std::vector<uint8_t> metadata{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
             outfile << Span{metadata};
         } else {
-            DataStream data_stream{};
             auto msg_start = chainman.GetParams().MessageStart();
             int base_blockheight{fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 2 * COINBASE_MATURITY)};
             uint256 base_blockhash{g_chain->at(base_blockheight - 1)->GetHash()};
@@ -75,6 +108,16 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
                 height++;
             }
         }
+        if constexpr (INVALID) {
+            // Append an invalid coin to ensure invalidity. This error will be
+            // detected late in PopulateAndValidateSnapshot, and allows the
+            // INVALID fuzz target to reach more potential code coverage.
+            const auto& coinbase{g_chain->back()->vtx.back()};
+            outfile << coinbase->GetHash();
+            WriteCompactSize(outfile, 1);   // number of coins for the hash
+            WriteCompactSize(outfile, 999); // index of coin
+            outfile << Coin{coinbase->vout[0], /*nHeightIn=*/999, /*fCoinBaseIn=*/0};
+        }
     }
 
     const auto ActivateFuzzedSnapshot{[&] {
@@ -90,12 +133,16 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
     }};
 
     if (fuzzed_data_provider.ConsumeBool()) {
-        for (const auto& block : *g_chain) {
-            BlockValidationState dummy;
-            bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)};
-            Assert(processed);
-            const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
-            Assert(index);
+        // Consume the bool, but skip the code for the INVALID fuzz target
+        if constexpr (!INVALID) {
+            for (const auto& block : *g_chain) {
+                BlockValidationState dummy;
+                bool processed{chainman.ProcessNewBlockHeaders({*block}, true, dummy)};
+                Assert(processed);
+                const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
+                Assert(index);
+            }
+            dirty_chainman = true;
         }
     }
 
@@ -119,11 +166,35 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
             }
         }
         Assert(g_chain->size() == coinscache.GetCacheSize());
+        dirty_chainman = true;
     } else {
         Assert(!chainman.SnapshotBlockhash());
         Assert(!chainman.ActiveChainstate().m_from_snapshot_blockhash);
     }
     // Snapshot should refuse to load a second time regardless of validity
     Assert(!ActivateFuzzedSnapshot());
+    if constexpr (INVALID) {
+        // Activating the snapshot, or any other action that makes the chainman
+        // "dirty" can and must not happen for the INVALID fuzz target
+        Assert(!dirty_chainman);
+    }
+    if (dirty_chainman) {
+        setup.m_node.chainman.reset();
+        setup.m_make_chainman();
+        setup.LoadVerifyActivateChainstate();
+    }
 }
+
+// There are two fuzz targets:
+//
+// The target 'utxo_snapshot', which allows valid snapshots, but is slow,
+// because it has to reset the chainstate manager on almost all fuzz inputs.
+// Otherwise, a dirty header tree or dirty chainstate could leak from one fuzz
+// input execution into the next, which makes execution non-deterministic.
+//
+// The target 'utxo_snapshot_invalid', which is fast and does not require any
+// expensive state to be reset.
+FUZZ_TARGET(utxo_snapshot /*valid*/, .init = initialize_chain<false>) { utxo_snapshot_fuzz<false>(buffer); }
+FUZZ_TARGET(utxo_snapshot_invalid, .init = initialize_chain<true>) { utxo_snapshot_fuzz<true>(buffer); }
+
 } // namespace

src/test/policyestimator_tests.cpp

@@ -1,8 +1,9 @@
-// Copyright (c) 2011-2022 The Bitcoin Core developers
+// Copyright (c) 2011-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <policy/fees.h>
+#include <policy/fees_args.h>
 #include <policy/policy.h>
 #include <test/util/txmempool.h>
 #include <txmempool.h>
@@ -18,7 +19,7 @@ BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, ChainTestingSetup)
 
 BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
 {
-    CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator);
+    CBlockPolicyEstimator feeEst{FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES};
     CTxMemPool& mpool = *Assert(m_node.mempool);
     m_node.validation_signals->RegisterValidationInterface(&feeEst);
     TestMemPoolEntryHelper entry;

src/test/util/setup_common.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) 2011-2022 The Bitcoin Core developers
+// Copyright (c) 2011-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -31,7 +31,6 @@
 #include <node/warnings.h>
 #include <noui.h>
 #include <policy/fees.h>
-#include <policy/fees_args.h>
 #include <pow.h>
 #include <random.h>
 #include <rpc/blockchain.h>
@@ -236,7 +235,6 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
         m_node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(*m_node.scheduler));
     }
 
-    m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES);
     bilingual_str error{};
     m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
     Assert(error.empty());
@@ -246,24 +244,34 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
 
     m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings));
 
-    const ChainstateManager::Options chainman_opts{
-        .chainparams = chainparams,
-        .datadir = m_args.GetDataDirNet(),
-        .check_block_index = 1,
-        .notifications = *m_node.notifications,
-        .signals = m_node.validation_signals.get(),
-        .worker_threads_num = 2,
-    };
-    const BlockManager::Options blockman_opts{
-        .chainparams = chainman_opts.chainparams,
-        .blocks_dir = m_args.GetBlocksDirPath(),
-        .notifications = chainman_opts.notifications,
+    m_make_chainman = [this, &chainparams, opts] {
+        Assert(!m_node.chainman);
+        ChainstateManager::Options chainman_opts{
+            .chainparams = chainparams,
+            .datadir = m_args.GetDataDirNet(),
+            .check_block_index = 1,
+            .notifications = *m_node.notifications,
+            .signals = m_node.validation_signals.get(),
+            .worker_threads_num = 2,
+        };
+        if (opts.min_validation_cache) {
+            chainman_opts.script_execution_cache_bytes = 0;
+            chainman_opts.signature_cache_bytes = 0;
+        }
+        const BlockManager::Options blockman_opts{
+            .chainparams = chainman_opts.chainparams,
+            .blocks_dir = m_args.GetBlocksDirPath(),
+            .notifications = chainman_opts.notifications,
+        };
+        m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
+        LOCK(m_node.chainman->GetMutex());
+        m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<BlockTreeDB>(DBParams{
+            .path = m_args.GetDataDirNet() / "blocks" / "index",
+            .cache_bytes = static_cast<size_t>(m_cache_sizes.block_tree_db),
+            .memory_only = true,
+        });
     };
-    m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown), chainman_opts, blockman_opts);
-    m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<BlockTreeDB>(DBParams{
-        .path = m_args.GetDataDirNet() / "blocks" / "index",
-        .cache_bytes = static_cast<size_t>(m_cache_sizes.block_tree_db),
-        .memory_only = true});
+    m_make_chainman();
 }
 
 ChainTestingSetup::~ChainTestingSetup()
@@ -276,7 +284,7 @@ ChainTestingSetup::~ChainTestingSetup()
     m_node.netgroupman.reset();
     m_node.args = nullptr;
     m_node.mempool.reset();
-    m_node.fee_estimator.reset();
+    Assert(!m_node.fee_estimator); // Each test must create a local object, if they wish to use the fee_estimator
     m_node.chainman.reset();
     m_node.validation_signals.reset();
     m_node.scheduler.reset();

src/test/util/setup_common.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2022 The Bitcoin Core developers
+// Copyright (c) 2015-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -55,6 +55,7 @@ struct TestOpts {
     bool block_tree_db_in_memory{true};
     bool setup_net{true};
     bool setup_validation_interface{true};
+    bool min_validation_cache{false}; // Equivalent of -maxsigcachebytes=0
 };
 
 /** Basic testing setup.
@@ -81,6 +82,7 @@ struct ChainTestingSetup : public BasicTestingSetup {
     node::CacheSizes m_cache_sizes{};
     bool m_coins_db_in_memory{true};
     bool m_block_tree_db_in_memory{true};
+    std::function<void()> m_make_chainman{};
 
     explicit ChainTestingSetup(const ChainType chainType = ChainType::MAIN, TestOpts = {});
     ~ChainTestingSetup();