coins: parallel input prevout fetching followups #35738

87cf27f coins: filter coinbase txid from parallel input fetching:

Validated with a new temporary unit test which fails without this line with:

test/coinsviewoverlay_tests.cpp:210: error: in "coinsviewoverlay_tests/fetch_coinbase_spend": check view.AllInputsConsumed() has failed

<details><summary>Details</summary>

BOOST_AUTO_TEST_CASE(fetch_coinbase_spend)
{
    CBlock block{};
    CMutableTransaction coinbase{};
    coinbase.vin.emplace_back();
    coinbase.vout.emplace_back(/*nValue=*/1, CScript{});
    const auto coinbase_tx{MakeTransactionRef(coinbase)};
    const COutPoint coinbase_outpoint{coinbase_tx->GetHash(), /*nIn=*/0};
    block.vtx.push_back(coinbase_tx);

    CMutableTransaction spend{};
    spend.vin.emplace_back(coinbase_outpoint);
    block.vtx.push_back(MakeTransactionRef(spend));

    CCoinsViewCache main_cache{&CoinsViewEmpty::Get()};
    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
    const auto reset_guard{view.StartFetching(block)};
    AddCoins(view, *coinbase_tx, /*nHeight=*/1);
    BOOST_REQUIRE(view.SpendCoin(coinbase_outpoint));
    BOOST_CHECK(view.AllInputsConsumed());
}

</details>

nit:

        if (block.vtx.size()) earlier_txids.emplace(block.vtx[0]->GetHash());

ae0b64a fuzz: use per-level fetch scopes in coinscache_sim:

fetch_scopes has to mirror caches, but two call sites duplicate scope construction and no executable invariant checks that the parallel vectors stay aligned. Could we use one helper and assert their sizes stay equal?

<details><summary>Details</summary>

diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
--- a/src/test/fuzz/coinscache_sim.cpp	(revision ae0b64a5d35b243eccebe80394e201297c21485a)
+++ b/src/test/fuzz/coinscache_sim.cpp	(revision 09ae50595e7347238e9fe928d7337331adf2c4f9)
@@ -265,6 +265,12 @@
         }
     };
 
+    /** Helper creating a fetch scope for the top cache (which must be a CoinsViewOverlay). */
+    const auto make_fetch_scope{[&] {
+        auto& overlay{static_cast<CoinsViewOverlay&>(*caches.back())};
+        return std::make_unique<OverlayFetchScope>(overlay, data.block);
+    }};
+
     // Main simulation loop: read commands from the fuzzer input, and apply them
     // to both the real cache stack and the simulation.
     FuzzedDataProvider provider(buffer.data(), buffer.size());
@@ -278,6 +284,7 @@
             fetch_scopes.emplace_back();
             sim_caches[caches.size()].Wipe();
         }
+        assert(caches.size() == fetch_scopes.size());
 
         // Execute command.
         CallOneOf(
@@ -410,8 +417,7 @@
                         fetch_scopes.emplace_back();
                     } else {
                         caches.emplace_back(new CoinsViewOverlay(&*caches.back(), g_thread_pool, /*deterministic=*/true));
-                        auto& overlay{static_cast<CoinsViewOverlay&>(*caches.back())};
-                        fetch_scopes.emplace_back(std::make_unique<OverlayFetchScope>(overlay, data.block));
+                        fetch_scopes.emplace_back(make_fetch_scope());
                     }
                     // Apply to simulation data.
                     sim_caches[caches.size()].Wipe();
@@ -449,9 +455,8 @@
                 sim_caches[caches.size()].Wipe();
                 // Apply to real caches. Optionally start fetching again.
                 if (fetch_scopes.back() && provider.ConsumeBool()) {
-                    fetch_scopes.back().reset();
-                    auto& overlay{static_cast<CoinsViewOverlay&>(*caches.back())};
-                    fetch_scopes.back() = std::make_unique<OverlayFetchScope>(overlay, data.block);
+                    fetch_scopes.back().reset(); // Stop fetching before starting again.
+                    fetch_scopes.back() = make_fetch_scope();
                 } else {
                     (void)caches.back()->CreateResetGuard();
                 }

</details>

loadtxoutset creates a second chainstate and currently starts another full prevout-fetch pool without a test exposing the extra workers.

Could we let ChainstateManager own one pool and verify snapshot loading does not start another?

<details><summary>Details</summary>

diff --git a/src/validation.cpp b/src/validation.cpp
index 86f70bc51a..68cc4a839b 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1860,16 +1860,11 @@ CoinsViews::CoinsViews(DBParams db_params, CoinsViewOptions options)
     : m_dbview{std::move(db_params), std::move(options)},
       m_catcherview(&m_dbview) {}
 
-void CoinsViews::InitCache(int32_t prevoutfetch_threads)
+void CoinsViews::InitCache(std::shared_ptr<ThreadPool> prevout_fetch_pool)
 {
     AssertLockHeld(::cs_main);
     m_cacheview = std::make_unique<CCoinsViewCache>(&m_catcherview);
-    auto thread_pool{std::make_shared<ThreadPool>("prevout")};
-    if (prevoutfetch_threads > 0) {
-        thread_pool->Start(prevoutfetch_threads);
-        LogInfo("Block input prevout fetching uses %d additional threads", prevoutfetch_threads);
-    }
-    m_connect_block_view = std::make_unique<CoinsViewOverlay>(&*m_cacheview, std::move(thread_pool));
+    m_connect_block_view = std::make_unique<CoinsViewOverlay>(&*m_cacheview, std::move(prevout_fetch_pool));
 }
 
 Chainstate::Chainstate(
@@ -1945,7 +1940,7 @@ void Chainstate::InitCoinsCache(size_t cache_size_bytes)
     AssertLockHeld(::cs_main);
     assert(m_coins_views != nullptr);
     m_coinstip_cache_size_bytes = cache_size_bytes;
-    m_coins_views->InitCache(m_chainman.m_options.prevoutfetch_threads_num);
+    m_coins_views->InitCache(m_chainman.m_prevout_fetch_pool);
 }
 
 // Lock-free: depends on `m_cached_is_ibd`, which is latched by `UpdateIBDStatus()`.
@@ -6158,8 +6153,20 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts)
     return std::move(opts);
 }
 
+//! Create the thread pool for prefetching block input prevouts.
+static std::shared_ptr<ThreadPool> MakePrevoutFetchPool(int32_t num_threads)
+{
+    auto pool{std::make_shared<ThreadPool>("prevout")};
+    if (num_threads > 0) {
+        pool->Start(num_threads);
+        LogInfo("Block input prevout fetching uses %d additional threads", num_threads);
+    }
+    return pool;
+}
+
 ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options)
     : m_script_check_queue{/*batch_size=*/128, std::clamp(options.worker_threads_num, 0, MAX_SCRIPTCHECK_THREADS)},
+      m_prevout_fetch_pool{MakePrevoutFetchPool(options.prevoutfetch_threads_num)},
       m_interrupt{interrupt},
       m_options{Flatten(std::move(options))},
       m_blockman{interrupt, std::move(blockman_options)},
diff --git a/src/validation.h b/src/validation.h
index d89ad3539e..d2ae38ecb1 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -54,6 +54,7 @@
 class Chainstate;
 class CTxMemPool;
 class ChainstateManager;
+class ThreadPool;
 struct ChainTxData;
 class DisconnectedBlockTransactions;
 struct PrecomputedTransactionData;
@@ -506,7 +507,7 @@ public:
     CoinsViews(DBParams db_params, CoinsViewOptions options);
 
     //! Initialize the CCoinsViewCache member.
-    void InitCache(int32_t prevoutfetch_threads) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    void InitCache(std::shared_ptr<ThreadPool> prevout_fetch_pool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 };
 
 enum class CoinsCacheSizeState
@@ -982,6 +983,11 @@ private:
     //! A queue for script verifications that have to be performed by worker threads.
     CCheckQueue<CScriptCheck> m_script_check_queue;
 
+    //! Worker threads for prefetching block input prevouts, shared by every chainstate's
+    //! CoinsViewOverlay since only one chainstate connects blocks at a time (under cs_main).
+    //! Never null; has zero workers when prefetching is disabled.
+    const std::shared_ptr<ThreadPool> m_prevout_fetch_pool;
+
     //! Timers and counters used for benchmarking validation in both background
     //! and active chainstates.
     SteadyClock::duration GUARDED_BY(::cs_main) time_check{};
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 8727a9925e..ac0ffa6e3d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -524,7 +524,9 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.log.info(f"Loading snapshot into second node from {dump_output['path']}")
         # This node's tip is on an ancestor block of the snapshot, which should
         # be the normal case
-        loaded = n1.loadtxoutset(dump_output['path'])
+        prevout_pool_log = "Block input prevout fetching uses 1 additional threads"
+        with n1.assert_debug_log([], unexpected_msgs=[prevout_pool_log]):
+            loaded = n1.loadtxoutset(dump_output['path'])
         assert_equal(loaded['coins_loaded'], SNAPSHOT_BASE_HEIGHT)
         assert_equal(loaded['base_height'], SNAPSHOT_BASE_HEIGHT)

</details>

Is there a reason we don't want both chainstates to have their own pools? That should make connecting blocks both at tip and in background faster?

The point of the threadpool here was to separate other CPU bound work from IO, right? Adding more competing threadpools would defeat the purpose as far as I can tell. Also, pools are not free, so it seems simpler to minimize the need for them, especially since only one of them is expected to work at the same time (most of the time).

ae0b64a fuzz: use per-level fetch scopes in coinscache_sim:

Resetting an active overlay without restarting it leaves a non-null fetch_scopes entry, so later fuzz operations still treat the stopped session as active.

Could we clear the entry unless fetching is restarted?

<details><summary>Details</summary>

diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
--- a/src/test/fuzz/coinscache_sim.cpp	(revision 4167aeba832581186a357ff1d31bcb1e77e964d6)
+++ b/src/test/fuzz/coinscache_sim.cpp	(revision 7cff2cce4b43ab7ef7b6e6b4af6baa6a0284e508)
@@ -454,9 +454,12 @@
             [&]() { // Reset.
                 sim_caches[caches.size()].Wipe();
                 // Apply to real caches. Optionally start fetching again.
-                if (fetch_scopes.back() && provider.ConsumeBool()) {
+                if (fetch_scopes.back()) {
+                    const bool restart_fetching{provider.ConsumeBool()};
                     fetch_scopes.back().reset(); // Stop fetching before starting again.
-                    fetch_scopes.back() = make_fetch_scope();
+                    if (restart_fetching) {
+                        fetch_scopes.back() = make_fetch_scope();
+                    }
                 } else {
                     (void)caches.back()->CreateResetGuard();
                 }

</details>

Doesn't having later fuzz operations optionally treat a stopped session as active increase coverage?

Maybe @dergoegge can help us out here