validation: fetch block input prevouts in parallel during ConnectBlock #35295

22c4c1d validation: add -inputfetchthreads configuration option:

https://corecheck.dev/bitcoin/bitcoin/pulls/35295 indicates we're not testing this part of the code - could we add a unit test for this?

diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
--- a/src/test/validation_chainstatemanager_tests.cpp	(revision d6946a12ab306c5054b535b0b7fac0101fb306ae)
+++ b/src/test/validation_chainstatemanager_tests.cpp	(revision 82be6bdbe682cf9b4d1fb46820302cbbfb5c34b6)
@@ -991,6 +991,13 @@
 
     BOOST_CHECK(!get_opts({"-minimumchainwork=xyz"}));                                                               // invalid hex characters
     BOOST_CHECK(!get_opts({"-minimumchainwork=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
+
+    // test -inputfetchthreads
+    BOOST_CHECK_EQUAL(get_valid_opts({}).inputfetch_threads_num, DEFAULT_INPUTFETCH_THREADS);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-inputfetchthreads=0"}).inputfetch_threads_num, 0);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-inputfetchthreads=3"}).inputfetch_threads_num, 3);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-inputfetchthreads=100"}).inputfetch_threads_num, MAX_INPUTFETCH_THREADS);
+    BOOST_CHECK(!get_opts({"-inputfetchthreads=-1"}));
 }
 
 BOOST_AUTO_TEST_SUITE_END()

a3981aa coins: introduce thread pool in CoinsViewOverlay:

nit: do we want to be able to init the cache with > MAX_INPUTFETCH_THREADS threads? That's not really possible currently as far as I can tell, but we may want to clamp here again to make it future-proof.

70ee8ba coins: add ready flag to InputToFetch:

nit: https://corecheck.dev/bitcoin/bitcoin/pulls/35295 coverage indicates this is unused - maybe a simple unit test could cover it. Not sure it matters...

1f0c895 coins: fetch inputs in parallel:

https://corecheck.dev/bitcoin/bitcoin/pulls/35295 indicates this path is never taken

In commit "coins: fetch inputs in parallel" (1f0c8957e1e483608148dc4bf7f396b82d2c6e44)

Unclear why clearing the inputs is the right thing to do when the thread pool submit fails.

Since thread pool is passed externally and not only controlled by this code, it would seem good to handle failures by doing something like raising an exception or logging an error. Of if clearing m_inputs is a good way to handle this, it would be worth saying what this does in a comment.

I added a comment explaining why it's the correct action to take, and I also added a warning log here.

fec8a2b coins: stop fetching before mutating base:

Same, https://corecheck.dev/bitcoin/bitcoin/pulls/35295 indicates this is never actually called. This is probably a consequence of the Coins interface bloat; maybe we could throw, log, or Assume instead?

It is called in the fuzz harness.

If it's test-only code maybe we could throw, log, or Assume instead?

fec8a2b coins: stop fetching before mutating base:

I'm a bit surprised this is never covered in https://corecheck.dev/bitcoin/bitcoin/pulls/35295 - can you please investigate?

What is the expected behavior when we have a second fetch session attached to the same underlying base, e.g.

BOOST_AUTO_TEST_CASE(fetch_state_is_cleared_by_mutating_operations)
{
    const auto block{CreateBlock()};
    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
    CCoinsViewCache main_cache{&db};
    CCoinsViewCache alternate_cache{&db};
    PopulateView(block, main_cache);
    PopulateView(block, alternate_cache);

    for (const bool flush : {false, true}) {
        CoinsViewOverlay view{&main_cache, StartedThreadPool()};
        const auto first_guard{view.StartFetching(block)};
        view.SetBestBlock(uint256::ONE);
        if (flush) {
            view.Flush();
        } else {
            view.Sync();
        }

        const auto second_guard{view.StartFetching(block)};
        CheckCache(block, view);
    }

    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
    const auto first_guard{view.StartFetching(block)};
    view.SetBackend(alternate_cache);

    const auto second_guard{view.StartFetching(block)};
    CheckCache(block, view);
}

a154704 validation: collect block inputs in CoinsViewOverlay before ConnectBlock:

It should be safe to start twice, we could cover that with a test case, maybe something like:

BOOST_AUTO_TEST_CASE(fetch_state_is_reusable_after_reset)
{
    const auto block{CreateBlock()};
    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
    CCoinsViewCache main_cache{&db};
    PopulateView(block, main_cache);
    CoinsViewOverlay view{&main_cache, StartedThreadPool()};

    {
        const auto reset_guard{view.StartFetching(block)};
        CheckCache(block, view);
        BOOST_CHECK_GT(view.GetCacheSize(), 0U);
    }

    BOOST_CHECK_EQUAL(view.GetCacheSize(), 0U);

    const auto reset_guard{view.StartFetching(block)};
    CheckCache(block, view);
}

a3981aa coins: introduce thread pool in CoinsViewOverlay:

Since the workers call PeekCoin() concurrently on the shared base cache, maybe we could force routing the lookup through std::as_const to make the concurrent-read contract explicit at the access site:

std::optional<Coin> CCoinsViewCache::PeekCoin(const COutPoint& outpoint) const
{
    const auto& coins{std::as_const(cacheCoins)};
    if (auto it{coins.find(outpoint)}; it != coins.end()) {
        return it->second.coin.IsSpent() ? std::nullopt : std::optional{it->second.coin};
    }
    return base->PeekCoin(outpoint);
}

I did not take this. This seems out of scope for this change.

d6946a1 fuzz: add coins_view_stacked fuzz harness to test concurrent leveldb reads:

The generated block inputs are mostly arbitrary misses this way - could we help the fuzzer by allowing a fuzz-selected subset of the staged external prevouts into the DB before starting the overlay, so the same concurrent read path covers real backing-store hits as well as misses:

void PopulateFetchInputs(CCoinsView& view, const CBlock& block, FuzzedDataProvider& fuzzed_data_provider)
{
    CCoinsViewCache cache{&view, /*deterministic=*/true};
    cache.SetBestBlock(uint256::ONE);
    for (const auto& tx : block.vtx | std::views::drop(1)) {
        for (const auto& input : tx->vin) {
            if (!fuzzed_data_provider.ConsumeBool()) continue;
            auto coin{ConsumeDeserializable<Coin>(fuzzed_data_provider)};
            if (!coin || coin->IsSpent()) continue;
            cache.AddCoin(input.prevout, std::move(*coin), /*possible_overwrite=*/true);
        }
    }
    cache.Flush();
}
...
CCoinsViewDB backend_base_coins_view{std::move(db_params), CoinsViewOptions{}};
CBlock block{BuildRandomBlock(fuzzed_data_provider)};
PopulateFetchInputs(backend_base_coins_view, block, fuzzed_data_provider);
CCoinsViewCache backend_cache{&backend_base_coins_view, /*deterministic=*/true};

I did something similar. I add the utxos when I create the block.

650416e test: add unit tests for CoinsViewOverlay::StartFetching:

Would it make sense to cover the thread pool interruption explicitly?

BOOST_AUTO_TEST_CASE(fetch_interrupted_thread_pool_uses_normal_lookup)
{
    const auto block{CreateBlock()};
    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
    CCoinsViewCache main_cache{&db};
    PopulateView(block, main_cache);

    auto thread_pool{std::make_shared<ThreadPool>("fetch_interrupted")};
    thread_pool->Start(DEFAULT_INPUTFETCH_THREADS);
    thread_pool->Interrupt();

    {
        CoinsViewOverlay view{&main_cache, thread_pool};
        const auto reset_guard{view.StartFetching(block)};
        CheckCache(block, view);
    }

    thread_pool->Stop();
}

and maybe the theoretic-but-documented out-of-order flow:

BOOST_AUTO_TEST_CASE(fetch_out_of_order_input_uses_normal_lookup)
{
    const auto block{CreateBlock()};
    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
    CCoinsViewCache main_cache{&db};
    PopulateView(block, main_cache);

    std::vector<COutPoint> fetched_inputs;
    std::unordered_set<Txid, SaltedTxidHasher> txids;
    txids.reserve(block.vtx.size() - 1);
    for (const auto& tx : block.vtx | std::views::drop(1)) {
        for (const auto& input : tx->vin) {
            if (!txids.contains(input.prevout.hash)) fetched_inputs.push_back(input.prevout);
        }
        txids.emplace(tx->GetHash());
    }
    BOOST_REQUIRE_GE(fetched_inputs.size(), 2U);

    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
    const auto reset_guard{view.StartFetching(block)};

    const auto& out_of_order_input{fetched_inputs[1]};
    BOOST_CHECK(!view.AccessCoin(out_of_order_input).IsSpent());
    BOOST_CHECK(view.HaveCoinInCache(out_of_order_input));

    CheckCache(block, view);
}

a154704 validation: collect block inputs in CoinsViewOverlay before ConnectBlock:

nit: maybe we could harden destruction of the overlay (though callers must still keep the reset guard inside the overlay lifetime)

    ~CoinsViewOverlay() noexcept override { StopFetching(); }

Since the prod usually uses AddCoin for the unspent setup paths maybe we could exercise that as well:

            if (spent) {
                cache.EmplaceCoinInternalDANGER(COutPoint{in.prevout}, std::move(coin));
            } else {
                coin.out.nValue = 1;
                cache.AddCoin(in.prevout, std::move(coin), /*possible_overwrite=*/false);
            }

In commit "coins: add ready flag to InputToFetch" (70ee8ba66055aed16cfa9e0a8ebeea6db0aad1b9)

Is there a reason to call ProcessInput() here instead of input.ready.wait()?

The comment above says this lets the main thread make progress instead of waiting for a specific worker, but was this benchmarked, or is there a reason to expect it to help? Since the main thread is the only thread that can continue validation, it seems possible that it should prioritize waiting for the needed input instead of switching to fetching later inputs, which may involve blocking I/O and may not make the current input ready any sooner.

(Sorry if this was already asked somewhere. Either way it would be good to update the "Otherwise the main thread calls ProcessInput()..." comment above with a more specific rationale.)

I think this was done with an older design where we might not have any threads in the thread pool, and therefore wouldn't make any progress in that case. With the current design where we clear m_inputs if we didn't successfully submit tasks to at least 1 worker thread, it makes sense to just wait here. It simplifies this code and continues validation immediately instead of having the main thread potentially wait for expensive IO. I also bumped the default worker threads from 4 to 8. I believe we saw a speedup here due to this, but I think that was because we would now be fetching on a 5th thread by default. So bumping the default and just waiting on the main thread should be even faster.

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (a15470468ba89f2f86184191423c2a4d5c2ffcc1)

This is not a performance hotspot, and if these preconditions are wrong it would seem to indicate a serious problem with validation logic. Would suggest either replacing Assume with Assert or at least taking some action such logging a warning if the Assume returns false, not just ignoring the errors

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (a15470468ba89f2f86184191423c2a4d5c2ffcc1)

Since this is a virtual method it means StopFetching could be called (through CreateResetGuard) even if StartFetching is never called, which is surprising, even if safe. Seems worth mentioning StopFetching could be called without StartFetching if this is intentional.

More ideally it would seem good to just drop the CCoinsViewCache::CreateResetGuard method now that the only non-test code calling it is StartFetching. None of the test code actually uses the reset guard, except as a kludge to immediately reset the cache, so it seems like the tests would be better off either recreating the cache or calling a test-only reset method.

I added a comment to note that StopFetching is idempotent, and can be safely called when not fetching.

I'm not sure it's worth refactoring the change to drop CreateResetGuard in this PR, as there is already a lot happening. I opted to not do that (can be refactored in a follow-up).

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (a15470468ba89f2f86184191423c2a4d5c2ffcc1)

i is replaced with m_inputs_head immediately in the next commit, so it seems like it would reduce churn in the PR and make it clearer by introducing both m_input_head and m_input_tail this commit, instead of adding m_input_head and i now and replacing i later.

Bundled m_input_head into the first commit.

In commit "coins: filter same-block spends in StartFetching" (ec77ed61467a80112e172e27ebafa6e8011b7a5c)

It's not obvious that these inputs "won't be requested by FetchCoinFromBase" as stated in the commit message. That fact seems worth mentioning in this comment, otherwise it looks like this will result in extra PeekCoin calls.

In commit "coins: add ready flag to InputToFetch" (70ee8ba66055aed16cfa9e0a8ebeea6db0aad1b9)

This seems like a fragile way to ensure vector elements will not be moved. It would seem safer to replace the vector with a std::array (and an m_num_inputs field) to guarantee they will never move.

Isn't the stack limit 1MB on Windows? std::array would be inline in CoinsViewOverlay, so local instances would put 80 * 24390 = 1,951,200 bytes on the stack just for m_inputs. A heap-backed std::unique_ptr<std::array<...>> could work, but a direct std::array member seems risky. It would also need another state field to track the number of valid inputs. We might be able to get around that with a noop sentinel, but that seems more risky than keeping the current explicit size.

I was experimenting with this, and I don't think an array is ideal here. I think instead of asserting that we will never move, we can properly implement the move constructor here but assert in it that ready will be false and coin will be nullopt. We can get rid of the MAX_INPUTS_PER_BLOCK commit and just let it resize inside of StartFetching.

What changed since https://mirror.b10c.me/bitcoin-bitcoin/31132#discussion_r3166350935?

What changed since https://mirror.b10c.me/bitcoin-bitcoin/31132#discussion_r3166350935?

The comment above stating that it seems fragile. The previous version in the referenced thread of #31132 was just partially moving the outpoint and ignoring the other fields. Instead, we can move all fields and assert that ready is false and coin is nullopt.

Updated to drop the MAX_INPUTS_PER_BLOCK and implement the move constructor but assert that ready is false and coin is nullopt.

In commit "coins: stop fetching before mutating base" (fec8a2b732bda4c2287bbeab8cfb29af43e9e254)

Commit description seems makes it clear why SetBackend / Flush / Sync would not be safe to call while inputs are being fetched, but it's unclear (1) what would be calling these methods while inputs are being fetched and (2) why stopping fetching is the right behavior in these cases. It would be good to say more about this in the commit message or code comments.

Also, it seems fragile for CoinsViewOverlay to have to override these 3 methods individually, because if new methods or overloads are added to the parent class, CoinsViewOverlay could be silently broken. If CoinsViewOverlay needs to stop fetching when the base view is mutated it would seem better to have a single UpdatingBaseView method it could override, or something like that, instead of needing to override 3 methods.

I updated the commit message to explain why these changes were needed. In production code we don't need any of them, since Flush is the only method to be called on CoinsViewOverlay, and for a valid block all inputs would have already been fetched to get there. Nevertheless these methods are called by our fuzz harnesses, which exercise the full API of CCoinsViewCache. So, it allows us to substitute CoinsViewOverlay with CCoinsViewCache there. I also added unit tests to cover all these.

The UpdatingBaseView idea would be an interesting refactor but seems out of scope here, so I opted not to take it.

In commit "validation: add -inputfetchthreads configuration option" (22c4c1d737dc64d60dd22f9a635866b8ff5236a3)

"input fetch threads" sounds pretty generic, and it would seem good to be more specific about what these threads are. LLM suggests the following name & description which I think would be better:

argsman.AddArg("-prevoutfetchthreads=<n>", strprintf("Set the number of threads used to prefetch block input prevouts from the chainstate database (0 disables, up to %d, default: %d). Negative values are rejected.", MAX_INPUTFETCH_THREADS, DEFAULT_INPUTFETCH_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

In commit "validation: add -inputfetchthreads configuration option" (22c4c1d737dc64d60dd22f9a635866b8ff5236a3)

Should probably say "parallel input fetching" instead of "input fetching"

In commit "coins: introduce thread pool in CoinsViewOverlay" (a3981aa8a01195e3d4442e5d258f8f3198b49400)

Unless there's a specific reason to share state across tests, it would seem better to make them self contained and not leak resources after they finish executing. Would suggest using a fixture instead of global like:

<details><summary>diff</summary> <p>

--- a/src/test/coinsviewoverlay_tests.cpp
+++ b/src/test/coinsviewoverlay_tests.cpp
@@ -20,7 +20,15 @@
 #include <memory>
 #include <ranges>
 
-BOOST_AUTO_TEST_SUITE(coinsviewoverlay_tests)
+struct CoinsViewOverlaySetup {
+    std::shared_ptr<ThreadPool> m_thread_pool{[] {
+        auto pool{std::make_shared<ThreadPool>("fetch_test")};
+        pool->Start(DEFAULT_INPUTFETCH_THREADS);
+        return pool;
+    }()};
+};
+
+BOOST_FIXTURE_TEST_SUITE(coinsviewoverlay_tests, CoinsViewOverlaySetup)
 
 namespace {
 
@@ -58,17 +66,6 @@ void PopulateView(const CBlock& block, CCoinsView& view, bool spent = false)
     cache.Flush();
 }
 
-//! Returns a started thread pool shared across tests, mirroring how production reuses pools.
-std::shared_ptr<ThreadPool> StartedThreadPool()
-{
-    static const auto thread_pool{[] {
-        auto pool{std::make_shared<ThreadPool>("fetch_test")};
-        pool->Start(DEFAULT_INPUTFETCH_THREADS);
-        return pool;
-    }()};
-    return thread_pool;
-}
-
 void CheckCache(const CBlock& block, const CCoinsViewCache& cache)
 {
     uint32_t counter{0};
@@ -98,7 +95,7 @@ BOOST_AUTO_TEST_CASE(fetch_inputs_from_db)
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     PopulateView(block, db);
     CCoinsViewCache main_cache{&db};
-    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
+    CoinsViewOverlay view{&main_cache, m_thread_pool};
     const auto& outpoint{block.vtx[1]->vin[0].prevout};
 
     BOOST_CHECK(view.HaveCoin(outpoint));
@@ -125,7 +122,7 @@ BOOST_AUTO_TEST_CASE(fetch_inputs_from_cache)
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     CCoinsViewCache main_cache{&db};
     PopulateView(block, main_cache);
-    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
+    CoinsViewOverlay view{&main_cache, m_thread_pool};
     CheckCache(block, view);
 
     const auto& outpoint{block.vtx[1]->vin[0].prevout};
@@ -145,7 +142,7 @@ BOOST_AUTO_TEST_CASE(fetch_no_double_spend)
     CCoinsViewCache main_cache{&db};
     // Add all inputs as spent already in cache
     PopulateView(block, main_cache, /*spent=*/true);
-    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
+    CoinsViewOverlay view{&main_cache, m_thread_pool};
     for (const auto& tx : block.vtx) {
         for (const auto& in : tx->vin) {
             const auto& c{view.AccessCoin(in.prevout)};
@@ -163,7 +160,7 @@ BOOST_AUTO_TEST_CASE(fetch_no_inputs)
     const auto block{CreateBlock()};
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     CCoinsViewCache main_cache{&db};
-    CoinsViewOverlay view{&main_cache, StartedThreadPool()};
+    CoinsViewOverlay view{&main_cache, m_thread_pool};
     for (const auto& tx : block.vtx) {
         for (const auto& in : tx->vin) {
             const auto& c{view.AccessCoin(in.prevout)};

</p> </details>

Yeah, I suggested deduplicating, this is likely the cause of @theStack's failure as well, your solution is more elegant

In commit "coins: fetch inputs in parallel" (1f0c8957e1e483608148dc4bf7f396b82d2c6e44)

Similar to other comments made previously it would seem good to replace these Assume checks with asserts or do something besides ignoring failures they return

d0aa46a coins: add ready flag to InputToFetch:

Since each input slot should only be claimed once via m_input_head, could this assume the flag was not already set? It shouldn't fail if multiple threads are fetching the same (or the same, multiple times), but it would signal some kind of bug:

Assume(!input.ready.test_and_set(std::memory_order_release));

If #34844 gets merged first, this is a good candidate

cfdc1b9 coins: introduce thread pool in CoinsViewOverlay:

How come these are still static? It's moved in a later commit, can you please reduce churn and correct it here, too?

78ea779 validation: add -prevoutfetchthreads configuration option:

Is it critical that we read this as a 64 bit value and cast later? It seems to me this would also work:

diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
--- a/src/node/chainstatemanager_args.cpp	(revision 371c525623e42424ead869114baff07ab60505b8)
+++ b/src/node/chainstatemanager_args.cpp	(date 1780400814309)
@@ -18,6 +18,8 @@
 #include <util/translation.h>
 #include <validation.h>
 
 #include <algorithm>
 #include <chrono>
 #include <cstdint>
 #include <string>
@@ -60,11 +62,11 @@
     // Subtract 1 because the main thread counts towards the par threads.
     opts.worker_threads_num = script_threads - 1;
 
-    if (auto value{args.GetIntArg("-prevoutfetchthreads")}) {
+    if (auto value{args.GetArg<int32_t>("-prevoutfetchthreads")}) {
         if (*value < 0) {
             return util::Error{Untranslated(strprintf("-prevoutfetchthreads must be non-negative (got %d). Use 0 to disable parallel input fetching.", *value))};
         }
-        opts.prevoutfetch_threads_num = static_cast<int32_t>(std::min<int64_t>(*value, MAX_PREVOUTFETCH_THREADS));
+        opts.prevoutfetch_threads_num = std::min(*value, MAX_PREVOUTFETCH_THREADS);
     }
 
     if (auto max_size = args.GetIntArg("-maxsigcachesize")) {

nit, we can either have an ThreadPool::IsStarted() method or use WorkersCount as a boolean:

    if (!g_thread_pool->WorkersCount()) g_thread_pool->Start(DEFAULT_PREVOUTFETCH_THREADS);

cc: @theStack

Applies to src/test/fuzz/coinscache_sim.cpp as well

nit:

                if (auto futures{m_thread_pool->Submit(std::move(tasks))}) {

0f70a37 coins: fetch inputs in parallel:

Up until the previous commit, the cache was also warmed on a single thread; here we're reverting to the original behavior when there are no threads (because the main thread isn't fetching anymore). It's not incorrect, but it changes behavior back and forth. It's also not obvious what the speed difference should be between 1 thread and 0 threads. My understanding is that with 0 threads there is no pre-warming (i.e., done during validation), while with 1 thread it is prefetched so the validation code always finds the value already in cache. And the fact that StartFetching doesn't always "start" anything complicates things further (and prefixing with Maybe* wouldn't help). I can accept that keeping the old behavior as a fallback is necessary, but the 0 vs 1 ambiguity adds complexity that could maybe be avoided (similarly to how -par=1 means 0 additional workers for scripts, which is also quite confusing). cc: @theStack

The parallel fetching is disabled with 0 threads. Current behavior is retained. StartFetching is just essentially CreateResetGuard. With 1 or more threads, the prevouts are fetched in the background, and the main thread will not fetch them from main cache or disk. With only 1 worker thread we can expect only slightly better performance, since the main thread can continue any validation while the background worker fetches from disk. This would be especially true with -par=1, since the main thread can do CPU intensive validation while the background thread waits on disk IO.

So what's the advantage of keeping the old behavior?

I don't think there's an advantage. I would not configure it to 0 myself. It was requested as an option in #31132 (comment). BIP30 checks will still read concurrently with background reads with 1 thread.

It was requested as an option in #31132 (comment).

I don't see it, can you please quote that part? Especially since the fetches are still single threaded with -prevoutfetchthreads=1.

I don't think there's an advantage. I would not configure it to 0 myself.

I've compared -prevoutfetchthreads=0 with -prevoutfetchthreads=1, there's basically no difference. My preference would be to disallow -prevoutfetchthreads=0 and always pre-warm the cache from now on - it's just confusing to have this optionality. cc: @willcl-ark, @ryanofsky, @theStack

0637a1cc6f fuzz: add coins_view_stacked fuzz harness to test concurrent leveldb reads

2026-06-04 | reindex-chainstate | 950059 blocks | dbcache 1000 | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | HDD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=950059 -dbcache=1000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 -prevoutfetchthreads=0 (COMMIT = 0637a1cc6f2b2f28d4ab5d97aaf8a91ec436fc8f)
  Time (abs ≡):        30800.474 s               [User: 34040.905 s, System: 1333.384 s]
 
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=950059 -dbcache=1000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 -prevoutfetchthreads=1 (COMMIT = 0637a1cc6f2b2f28d4ab5d97aaf8a91ec43
  Time (abs ≡):        30736.822 s               [User: 36206.873 s, System: 2918.100 s]

quote that part?

It is this:

this could also be an argument for adding a config option to disable parallel fetch, if we cannot assure ourselves enough?

the fetches are still single threaded with -prevoutfetchthreads=1

Sorry if I was unclear from my initial reply. For BIP30 checks they will fall back to reading from the chainstate db concurrently with the single background thread.

One other advantage is less memory overhead from one less thread, if the user is very concerned about memory.

Another advantage of keeping prevoutfetchthreads=0 that came to mind is that it very nicely avoids all new code paths and reverts back to the previous fetching logic exactly. If we discover a bug with parallel fetching post-release, being able to recommend a configuration option workaround will let us resolve this without requiring users to upgrade.

cfdc1b9 coins: introduce thread pool in CoinsViewOverlay:

This is actually only guarded in the next commit

0f70a37 coins: fetch inputs in parallel:

nit: Personally I find the !empty pattern harder to read than if (m_inputs.size()) { or if (m_inputs.size() > 0) {.

nit: we don't usually have 50% internap spends, I remember only having around 5% on average

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

nit: the EmplaceCoinInternalDANGER vs AddCoin difference could likely be added in earlier commit.

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

What we're checking here is that cache presence and an internal spend are mutually exclusive. It might be simpler to use BOOST_CHECK_NE for that:

                const auto have{cache.HaveCoinInCache(outpoint)};
                BOOST_CHECK_NE(txids.contains(outpoint.hash), have);
                counter += have;

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

We could move this closer to the first usage to make it obvious that this was unknown during construction:

const COutPoint missing_outpoint{Txid::FromUint256(uint256::ONE), 0};
BOOST_CHECK(view.AccessCoin(missing_outpoint).IsSpent());
BOOST_CHECK(!view.HaveCoinInCache(missing_outpoint));

nit: could as well start out with an empty block here, to ensure that it's actually a non-input hit below?

Ah this actually uncovered that the test was passing for the wrong reason!

Below we define the missing_outpoint as having txid 1. But, the block does have txid 1! It was still missing from the db, but it wasn't taking the fallback path.

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

nit: can likely be inlined for simplicity (like we do in fetch_out_of_order_input_uses_normal_lookup or use HaveCoin directly):

    BOOST_CHECK(!view.AccessCoin(outpoint).IsSpent());

cc: @theStack

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

It's a bit confusing that we have a started shared thread pool in the fixture that we're not actually using in a few of the tests. This one doesn't start the thread pool, hence the fallback, but the fixture has a started one. Maybe we could add these to a different fixture to make this distinction obvious. cc: @theStack

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

Coverage proves this does indeed exercise the fallback 👍

<img width="855" height="307" alt="Image" src="https://github.com/user-attachments/assets/77b92691-9dcd-4bd9-b986-3b4e704d88cc" />

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

We might want to add a comment to the test header about FetchCoinFromBase refusing to advance for out-of-order requests (causing the fallback) and maybe prove that AccessCoin makes HaveCoinInCache pass:

const auto& out_of_order_input{fetched_inputs[1]};
BOOST_CHECK(!view.HaveCoinInCache(out_of_order_input));
BOOST_CHECK(!view.AccessCoin(out_of_order_input).IsSpent());
BOOST_CHECK(view.HaveCoinInCache(out_of_order_input));

cc: @theStack

3e995af test: add unit tests for CoinsViewOverlay::StartFetching:

Commenting out StopFetching for SetBackend/Flush/Sync separately all makes this test fail 👍

78ea779 validation: add -prevoutfetchthreads configuration option:

/** Maximum number of dedicated threads allowed for prefetching block input prevouts */

d175618 coins: filter same-block spends in StartFetching:

Any reason for doing this differently in the previous commit?

We had to scan in the previous commit, because we were also fetching prevouts for inputs created in that block. The next commit we filter those out, so here we won't have to scan.

in a92444eedc27537953bf797364bbd3196a8f7adc: was this meant to be

            Assert(!other.coin);

? That would make more sense to me, as the current assert is just "stating the obvious", i.e. that a freshly-default constructed std::optional member is std::nullopt.

Nice catch! Done.

d3dac3c coins: filter same-block spends in StartFetching:

In previous versions this collected all txids, but now it's the earlier txids, i.e. no more forward references possible: maybe we could rename to reflect that

        std::unordered_set<Txid, SaltedTxidHasher> earlier_txids;

Note: technically it's "earlier, except coinbase", which means that an invalid block trying to spend its own coinbase would still go to disk (likely not something we should care about)

d7bb17e doc: update CoinsViewOverlay docstring to describe parallel fetching:

nit: size is an invalid index:

 * index is greater than or equal to the size of m_inputs, no more inputs can be fetched and false is returned.

d7bb17e doc: update CoinsViewOverlay docstring to describe parallel fetching:

nit: not "all" inputs can be prefeched

 * CCoinsViewCache subclass that asynchronously fetches most block input prevouts in parallel during ConnectBlock without

2c9efd4 validation: collect block inputs in CoinsViewOverlay before ConnectBlock:

nit: wouldn't this return true for the last element?

     * [@return](/github-metadata-backup-bitcoin-bitcoin/contributor/return/) true if an input prevout was fetched
     * [@return](/github-metadata-backup-bitcoin-bitcoin/contributor/return/) false if there are no more input prevouts in the queue to fetch

d7bb17e doc: update CoinsViewOverlay docstring to describe parallel fetching:

nit: StopFetching does not stop or destroy the ThreadPool worker threads anymore:

 * stopping fetch tasks before calling superclass.

2c9efd46f98e9c0b2e9698d7550e96a0082e14ea

Fetch coins from the m_inputs vector in FetchCoinFromBase by scanning all inputs starting at m_input_tail until we discover the input with the correct outpoint.

Is this still accurate?

In commit "coins: filter same-block spends in StartFetching" (2b0d87e8e0322119eb5f428d90045b71c3e19bf3)

Can we add an assert below this comment to verify the loop above is working and the comment is actually true?

Assert(std::none_of(m_inputs.begin(), m_inputs.end(), [&](const InputToFetch& i) { return i.outpoint == outpoint; }));

I don't think adding this should have any performance impact because this code should never be hit for normal fetches, only for BIP30 checks in old blocks which are small. Also it should be ok later if -prevoutfetchthreads is 0 because m_inputs will be empty.

We could do this and it will be true for production code, but it will break in our unit and fuzz tests. The unit tests we could remove, but the fuzz tests would have to be modified. The fuzz tests are not like ConnectBlock, they request input prevouts in any order.

We could make the assertion check that none of the inputs exist after m_input_tail. That would be a little tighter, but it would miss inputs requested out of order (which is not possible with current fuzz harnesses). So with that we would keep the loop instead of changing it to the if statement here.

What about something like this:

diff --git a/src/coins.h b/src/coins.h
index 27c1a42cec..8b846d786f 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -769,6 +769,8 @@ public:
         return CreateResetGuard();
     }
 
+    bool AllInputsConsumed() const noexcept { return m_input_tail == m_inputs.size(); }
+
     void SetBackend(CCoinsView& view) override
     {
         StopFetching();
diff --git a/src/validation.cpp b/src/validation.cpp
index c4e99c399f..2970fafe1f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3046,6 +3046,7 @@ bool Chainstate::ConnectTip(
             LogError("%s: ConnectBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
             return false;
         }
+        Assume(view.AllInputsConsumed());
         time_3 = SteadyClock::now();
         m_chainman.time_connect_total += time_3 - time_2;
         assert(m_chainman.num_blocks_total > 0);

re: #35295 (review)

We could do this and it will be true for production code, but it will break in our unit and fuzz tests.

Seem to indicate a problem with the tests if they are violating assumptions code needs work properly. Maybe a clean solution would be to leave m_inputs empty in these tests? Like add prevoutfetch_threads_num as the first commit, and only populate m_inputs if it is > 0. This would be nice anyway, because it would simplify the the later commit 93d101c1505bd8cafb8226ede860642ac7d15064 and further reduce code churn in the PR. For example, the for (const auto& tx : block.vtx | std::views::drop(1)) loop would no longer have to become conditional in that commit, it would be conditional from the beginning.

I don't think there would be a downside to not populating m_inputs in tests that are not using it anyway.

The other assertion you suggest seems better than nothing, but weaker than assumption this code is actually making. I also think it would be ok not to have an assert, since if the assumption is wrong the code will be slow and broken but still return valid results. I just think it would make things better to verify the code is working as expected.

re: #35295 (review)

AllInputsConsumed

I don't have as much context as you, but when I look at this code the thing that seems fragile about is the assumptions it makes about what order the calls to fetch happen. Because if the order changes even slightly, the entire optimization this PR is implementing could be negated without any sign other than slower performance. Checking whether all inputs are consumed could be good, too, but I don't see that as being related.

leave m_inputs empty in these tests

Hmm I think we want to have the unit and especially fuzz tests be able to exercise the CoinsViewOverlay public API with m_inputs populated. Maybe I'm misunderstanding what you're suggesting here.

add prevoutfetch_threads_num as the first commit, and only populate m_inputs if it is > 0

I will experiment with this, to see if we can reduce code churn. But, I don't think we want to run unit tests without exercising m_inputs. Will think more on this.

if the order changes even slightly, the entire optimization this PR is implementing could be negated without any sign other than slower performance

It seems to me the invariant we want to check is that we indeed used the optimized path during ConnectBlock. So if the order of lookups in ConnectBlock is changed, we want to ensure we do not break the optimization. So, in the second commit 4216895f798ff8ae809634451ad4958f2efa19a4 I add an Assume(view.AllInputsConsumed()); check after ConnectBlock succeeds. This will test exactly what you are suggesting. If ConnectBlock is modified in such a way that we do not read through m_inputs in the optimized path, it will trip the Assume and break CI. An invalid block or fuzz tests will not necessarily pass this invariant, so we only do it on a successful ConnectBlock and not internal to CoinsViewOverlay, which has no holistic concept of the block being valid.

I reordered the commits to add the prevoutfetch_threads_num as the first commit, followed by introducing the threadpool into CoinsViewOverlay. The rest of the commits follow the same flow, but there is less code churn inside StartFetching because of this.

nit: https://github.com/bitcoin/bitcoin/blob/93c6ea147422199777ddbc5f6d04134b82420b76/src/coins.h#L450-L452 throws unconditionally, doesn't make a lot of sense to me to fuzz it (I don't mind pushing this in a separate PR myself)

re: #35295 (review)

Thanks for the clarification on AllInputsConsumed I was still thinking in terms of the original commit 41e90e14ddfc1e6fe4cf3accc47714f185fc8a60 where tail could be advanced without inputs actually being used. But with later code, AllInputsConsumed is a nice way of checking the optimization is applied and resolves my concern about assumptions the optimization relies on.

More minor feedback:

• I think the check should be done using assert not assume. Or else it should be using assume and checking the return value and logging a warning, or returning an error, or throwing an exception when it is false. I don't think unchecked assume expressions are appropriate outside of hot code paths where checking them could be bad for performance.

• I still don't understand the point of populating m_inputs in tests if the inputs will not be used. It seems like it would be better to move the AllInputsConsumed check out of validation code into coins code to check that whenever m_inputs is populated it will always be used, and no caller is creating a bunch of threads doing work that is useless.

it would be better to move the AllInputsConsumed check out of validation code into coins code to check that whenever m_inputs is populated it will always be used, and no caller is creating a bunch of threads doing work that is useless.

The issue here is that m_inputs is populated by a block that can be invalid. We don't know yet if it is valid or not because that will happen in ConnectBlock.

Consider an invalid block which contains an input that does not exist in the utxo set. We will populate the m_inputs with this invalid input, ConnectBlock will access it in FetchCoinFromBase and receive a nullopt, and ConnectBlock will fail validation and return early. Now we will go the unhappy path and call StopFetching when the ResetGuard is destroyed, but all inputs after the missing one will never have been accessed. So there's no guarantee that can be had in coins code that all m_inputs will be used. If we asserted that all m_inputs must be accessed then we would crash on an invalid block.

Removed in #35562, can be resolved

I updated the AllInputsConsumed to be Assert.

I think the check should be done using assert not assume. Or else it should be using assume and checking the return value and logging a warning, or returning an error, or throwing an exception when it is false. I don't think unchecked assume expressions are appropriate outside of hot code paths where checking them could be bad for performance.

I think I will revert this and use Assume after all and log a warning. This is checking that an optimization was successful. When testing it would be good to break if it fails. In production, if a block is otherwise valid but fails to achieve a speed optimization it will crash the node. I don't think that's a good tradeoff if for some reason a change to our validation code is made that breaks the assumption in a subtle way and goes unnoticed in testing.

803a8bcb3e84b50920da58f3fb158b14acd0433e

(aside from BIP30 checks, an invalid block, or when the thread pool is not yet.

re: #35295 (review)

If we asserted that all m_inputs must be accessed then we would crash on an invalid block.

It would seem good to have a separate CoinsViewOverlay::CancelFetching method callers could use in this case, that would stop fetching new inputs and allow already fetched ones to be unused.

An analogy would be with the std::thread destructor. To destroy the thread object you either have to join it or detach it or move from it, and if you don't do any of these things it's a bug and an error.

e92862f coins: fetch inputs in parallel:

nit: this also seems to be a pre-threadpool framing:

            // Only submit tasks if we have something to fetch.

d3dac3c coins: filter same-block spends in StartFetching:

nit: We're spending outputs

        // Loop through the block inputs and set their prevouts in the queue.
        // Filter inputs that spend outputs created earlier in the same block. These outputs will be created

4548f64 fuzz: update harnesses to cover CoinsViewOverlay::StartFetching:

If I understand correctly, this stops fetching because the final backend consistency check calls HaveCoin/GetCoin which can mutate CCoinsViewCache.

Maybe we can avoid that by using PeekCoin when mutation is prohibited, something like:

std::optional<Coin> coin_in_backend;
bool exists_using_have_coin_in_backend;
if (dynamic_cast<CoinsViewOverlay*>(&coins_view_cache)) {
    // PeekCoin does not mutate cacheCoins, so async workers can keep running.
    coin_in_backend = backend_coins_view->PeekCoin(random_out_point);
    exists_using_have_coin_in_backend = coin_in_backend.has_value();
} else {
    exists_using_have_coin_in_backend = backend_coins_view->HaveCoin(random_out_point);
    coin_in_backend = backend_coins_view->GetCoin(random_out_point);
}

which allows us to avoid those calls later:

// If the backend has the coin, it must also be on the cache if the coin wasn't spent.
if (!is_spent_using_access_coin && exists_using_have_coin_in_backend) {
    assert(exists_using_have_coin);
}
if (coin_in_backend) {
    assert(exists_using_have_coin_in_backend);
    // Note we can't assert that the backend coin matches the cache coin because the coin in
    // the cache may have been modified but not yet flushed.
} else {
    assert(!exists_using_have_coin_in_backend);
}

e0a95b5 coins: stop fetching before mutating base:

Should BatchWrite also stop fetching for API consistency? The fuzz harness can call the inherited BatchWrite while fetching is active (even though this cannot happen in production, as far as I can tell).

void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override
{
    StopFetching();
    CCoinsViewCache::BatchWrite(cursor, block_hash);
}

BatchWrite() only modifies this cacheCoins, it does not modify base in any way! So, it's safe to call. Flush() and Sync() call BatchWrite() on base.

In commit "coins: filter same-block spends in StartFetching" (38b18bc5b33a626c108fa4ecc6beed24a97f0a34)

Not sure Assume really makes sense here.

If there is a performance reason for ignoring whether the flag was previously set in release builds. It would seem to make more sense to a plain store like input.ready.store(true, std::memory_order_release) instead of test_and_set and atomic<bool> over atomic_flag.

Alternately if performance is not a factor here it would seem safest to replace Assume with Assert

Changed to Assert. The check shouldn't make a difference to performance, which is dominated by the prevout lookup.

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (aeb4bf72d9dc99f476446c68de7764d7f3d250d0)

The comment added in the next commit seems like it would be helpful here.

        // This assumes ConnectBlock accesses all inputs in the same order as
        // they are added to m_inputs in StartFetching.

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (aeb4bf72d9dc99f476446c68de7764d7f3d250d0)

Would suggest a comment for this continue that would explain why this is needed (and also why it is being removed next commit) like "// Keep advancing because m_inputs can contain same-block inputs that Fetch will never be called for"

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (41e90e14ddfc1e6fe4cf3accc47714f185fc8a60)

It looks like BIP30 checks are actually done before inputs are fetched so I don't think this comment is accurate in this commit (but would be ok in the next commit 2b0d87e8e0322119eb5f428d90045b71c3e19bf3).

So for all blocks where BIP30 checks are done (old blocks from 2013 and earlier), the loop above will just increment m_input_tail to equal m_inputs.size() and m_inputs will be unused, and every call to FetchCoinFromBase will fall through to PeekCoin.

If this is true, I feel like the "We will only get here for BIP30 checks" comment is misleading and should at least be fixed to describe the situation accurately.

But also if this is true, I feel like it would be better to squash the next commit into this one 2b0d87e8e0322119eb5f428d90045b71c3e19bf3 to make review simpler and avoid needing to reason about this more confusing intermediate state.

Hmm yes this was a side-effect that I overlooked of adding m_input_tail to the first commit. Another option is to not advance the tail unless we find the outpoint we are looking for, like this:

for (auto i{m_input_tail}; m_input_tail < m_inputs.size(); ++i) {
    auto& input{m_inputs[i]};
    // Outputs from earlier txs in the same block are created directly in the cache, so won't be fetched
    // from base. We skip those by scanning for the input prevout that matches the outpoint we are looking for.
    if (input.outpoint != outpoint) continue;
    // We advance the tail since the input is cached and not accessed through this method again.
    ++m_input_tail;
    // We can move the coin since we won't access this input again.
    return std::move(input.coin);
}

Not sure if you'd prefer that or to roll the first 2 commits into 1?

re: #35295 (review)

Another option is to not advance the tail unless we find the outpoint we are looking for

Nice, I think you'd need to use m_input_tail = i instead of ++m_input_tail, but otherwise that seems like a clean solution.

Not sure if you'd prefer that or to roll the first 2 commits into 1?

I would slightly prefer to see the two commits squashed, but I think you should follow your own preference or wait for another opinion, because both options seem reasonable and I'm probably unusually biased in favor of larger commits to reduce code churn.

I updated the first commit to use the above modification along with m_input_tail = i as you suggest.

I decided to keep the first 2 commits separate. Other reviewers seem to have a preference for an initial commit that introduces the smallest working implementation of the inputs queue. I also think the same-block output filtering is not an entirely obvious concept to many reviewers, so it makes sense to split it into its own commit.

Wouldn't these names be still too long on Linux - see #35173?

ps -eL | grep b-
1216097 1216098 ?        00:00:00 b-scheduler
1216097 1216099 ?        00:00:00 b-http_pool_0
1216097 1216100 ?        00:00:00 b-http_pool_1
1216097 1216101 ?        00:00:00 b-http_pool_2
1216097 1216102 ?        00:00:00 b-http_pool_3
1216097 1216103 ?        00:00:00 b-http_pool_4
1216097 1216104 ?        00:00:00 b-http_pool_5
1216097 1216105 ?        00:00:00 b-http_pool_6
1216097 1216106 ?        00:00:00 b-http_pool_7
1216097 1216107 ?        00:00:00 b-http_pool_8
1216097 1216108 ?        00:00:00 b-http_pool_9
1216097 1216109 ?        00:00:00 b-http_pool_10
1216097 1216110 ?        00:00:00 b-http_pool_11
1216097 1216111 ?        00:00:00 b-http_pool_12
1216097 1216112 ?        00:00:00 b-http_pool_13
1216097 1216113 ?        00:00:00 b-http_pool_14
1216097 1216114 ?        00:00:00 b-http_pool_15
1216097 1216115 ?        00:00:00 b-http
1216097 1216116 ?        00:00:05 b-scriptch.0
1216097 1216117 ?        00:00:05 b-scriptch.1
1216097 1216118 ?        00:00:05 b-scriptch.2
1216097 1216119 ?        00:00:05 b-scriptch.3
1216097 1216120 ?        00:00:05 b-scriptch.4
1216097 1216121 ?        00:00:05 b-scriptch.5
1216097 1216122 ?        00:00:05 b-scriptch.6
1216097 1216125 ?        00:00:00 b-prevoutfetch_
1216097 1216126 ?        00:00:00 b-prevoutfetch_
1216097 1216127 ?        00:00:00 b-prevoutfetch_
1216097 1216128 ?        00:00:00 b-prevoutfetch_
1216097 1216129 ?        00:00:00 b-prevoutfetch_
1216097 1216130 ?        00:00:00 b-prevoutfetch_
1216097 1216131 ?        00:00:00 b-prevoutfetch_
1216097 1216132 ?        00:00:00 b-prevoutfetch_
1216097 1216134 ?        00:00:00 b-mapport
1216097 1216135 ?        00:00:00 b-torcontrol
1216097 1216136 ?        00:00:00 b-net
1216097 1216138 ?        00:00:00 b-addcon
1216097 1216139 ?        00:00:00 b-opencon
1216097 1216140 ?        00:00:08 b-msghand

Edit: note that the logs do show the full name on Linux as well:

2026-06-14T12:29:56Z prevoutfetch_pool_4 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_3 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_7 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_2 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_0 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_5 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_1 thread exit
2026-06-14T12:29:56Z prevoutfetch_pool_6 thread exit

Do you have a suggestion for the name?

Updated to fetch.

after https://github.com/bitcoin/bitcoin/pull/35173/changes#diff-bf307d8d34f553323259334512522a03841fd65f81982c3dad83fae143127eb5L115 we don't have the _pool suffix anymore, so the name can probably be something like coinsfetch (i.e. b-coinsfetch_10) - to differentiate it from download threads.

3c9d592 coins: fetch inputs in parallel:

coins.h is included across a large part of the codebase - could we move the method's body to the impl and move the util/threadpool.h include there?

Moved to coins.cpp, and forward declared ThreadPool and CBlock in coins.h.

3c9d592 coins: fetch inputs in parallel:

nit: for an unlikely block with fewer inputs than workers we could cap the task to the input count instead (to avoid needlessly allocating and immediately terminating these futures).

                std::vector<std::function<void()>> tasks(std::min(workers_count, m_inputs.size()), [this] {

I don't think I'll take it since it will make this line slightly more complicated for no real gain (unless benchmarks show this has a measurable gain?).

unless benchmarks show this has a measurable gain

I'm checking it now, will get back to you

Edit:

<details><summary>no change whatsoever</summary>

for DBCACHE in 2000; do \
    COMMITS="dc1c17c0856e3455a0d62f9ffd807c0d14feff62 2d1d7f4f8fc01b025d74c83acf1873fdc4771f5d"; \
    STOP=954459; CC=gcc; CXX=g++; \
    BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
    (echo ""; for c in $COMMITS; do git fetch -q origin "$c" 2>/dev/null || true; git log -1 --pretty='%h %s' "$c" || exit 1; done) && \
    (echo "" && echo "$(date -I) | reindex-chainstate | ${STOP} blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) threads | $(free -h | awk '/^Mem:/{print $2}') RAM | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 1 && echo HDD || echo SSD)"; echo "") && \
    hyperfine \
    --sort command \
    --runs 1 \
    --export-json "$BASE_DIR/rdx-$(sed -E 's/([a-f0-9]{8})[a-f0-9]* ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
    --parameter-list COMMIT ${COMMITS// /,} \
    --prepare "killall -9 bitcoind 2>/dev/null; rm -f ./build/bin/bitcoind; git clean -fxd; git reset --hard {COMMIT} && \
      CC=$CC CXX=$CXX cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind -j1 && \
        ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -printtoconsole=0 && sleep 20 && rm -f $DATA_DIR/debug.log && rm -rfd $DATA_DIR/indexes" \
      --conclude "killall bitcoind || true; sleep 5; \
        grep -q 'height=0' $DATA_DIR/debug.log && \
        grep -q 'Disabling script verification at block [#1](/github-metadata-backup-bitcoin-bitcoin/1/)' $DATA_DIR/debug.log && \
        grep -q 'height=$STOP' $DATA_DIR/debug.log && \
        grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q \"\$(git rev-parse --short=12 {COMMIT})\" && \
        cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-\$(date +%s).log" \
    "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -disablewallet -connect=0 -listen=0 -dnsseed=0 -printtoconsole=0"; \
done

dc1c17c085 doc: add release notes
2d1d7f4f8f suggestion: don't submit more fetch tasks than inputs

2026-07-02 | reindex-chainstate | 954459 blocks | dbcache 2000 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 threads | 62Gi RAM | SSD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=954459 -dbcache=2000 -reindex-chainstate -blocksonly -disablewallet -connect=0 -listen=0 -dnsseed=0 -printtoconsole=0 (COMMIT = dc1c17c0856e3455a0d62f9ffd807c0d14feff62)
  Time (abs ≡):        15245.096 s               [User: 34715.744 s, System: 1218.222 s]
 
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=954459 -dbcache=2000 -reindex-chainstate -blocksonly -disablewallet -connect=0 -listen=0 -dnsseed=0 -printtoconsole=0 (COMMIT = 2d1d7f4f8fc01b025d74c83acf1873fdc4771f5d)
  Time (abs ≡):        15257.427 s               [User: 34748.407 s, System: 1218.114 s]
 
Relative speed comparison
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=954459 -dbcache=2000 -reindex-chainstate -blocksonly -disablewallet -connect=0 -listen=0 -dnsseed=0 -printtoconsole=0 (COMMIT = dc1c17c0856e3455a0d62f9ffd807c0d14feff62)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=954459 -dbcache=2000 -reindex-chainstate -blocksonly -disablewallet -connect=0 -listen=0 -dnsseed=0 -printtoconsole=0 (COMMIT = 2d1d7f4f8fc01b025d74c83acf1873fdc4771f5d)

</details>

3783d64 validation: collect block inputs in CoinsViewOverlay before ConnectBlock:

Doesn't BIP30 make this quadratic now?

re: #35295 (review)

Doesn't BIP30 make this quadratic now?

Good catch, although this is fixed in the next commit aed8687b2690594c65a953fb64e7c46a67828620. IMO this slightly bolsters the case for merging the two commits, but no strong opinion.

I ended up merging the two commits, so when we introduce StartFetching we also do the same block output filtering.

In commit "coins: introduce thread pool in CoinsViewOverlay" (d59725b731e21894bd40a1d1159bdcf6552d0c3a)

Is there a reason for switching from EmplaceCoinInternalDANGER to AddCoin in this commit? Might be good to explain in commit message why this needed now, or if this is just unrelated cleanup. IIUC switching to AddCoin adds the FRESH flag so this does seem better, but it's not clear why the change is here.

I removed touching this. I'm not exactly sure why we modified it, but I don't think it matters at all because we are both filtering same block outputs (so FRESH flag will not apply) and we flush this right after the loop.

I'm not exactly sure why we modified it

I requested exercising the version that's actually used in prod: #35295 (review). I'm fine with both.

In commit "coins: introduce thread pool in CoinsViewOverlay" (d59725b731e21894bd40a1d1159bdcf6552d0c3a)

Argument exceeding MAX_PREVOUTFETCH_THREADS can never actually happen right? If so might suggest simplifying this code and dropping std::min, or switching to an assert, or adding a comment clarifying that this is defensive, because it is confusing to apply a limit multiple places and not be able to easily see where it is actually being applied.

Was requested in #35295 (review). I'm fine with both.

In commit "coins: introduce thread pool in CoinsViewOverlay" (d59725b731e21894bd40a1d1159bdcf6552d0c3a)

It would seem more consistent here to call SpendCoin instead of EmplaceCoinInternalDANGER for consistency with AddCoin below. Would this work? Would seem good to have a code comment explaining reason EmplaceCoinInternalDANGER needs to be used instead if not.

I removed touching this, same as above.

in b1aba878d61473e1e9edabbb2b091fa77fc5721c: yocto-nit: this StartPoolIfNeeded function looks slightly different than the one in the coins_view.cpp fuzz test, could adapt either of them for consistency

in b1aba878d61473e1e9edabbb2b091fa77fc5721c: I think it would be good if at least one node in functional tests is ran with the default setting in CI (and ideally also one with the maximum), in order to exercise the feature with multiple threads. Maybe one test could explicitly set it to a higher number, e.g. in feature_block.py? Could still be discussed and decided in a test-focused follow-up though.

Added 8 threads to feature_block.py.

Nit: hopefully can't happen, but seems more accurate:

    bool AllInputsConsumed() const noexcept { return m_input_tail >= m_inputs.size(); }

Hmm yes this seems more accurate based on the name of the method, but it actually makes the invariant check weaker. If for some reason the tail passed the inputs, we would definitely want to break there and not swallow that error with a true result. Perhaps the name of the method could be changed to better describe this? Not sure I have one right now.

I ran a -reindex-chainstate until 400k blocks in debug (still running), no problems so far

It finished successfully until 950k blocks in debug mode with the new Assumes

in 794cf07ef0ba1f0163cc74a26db980efa42d74be: I was wondering whether it's necessary to set the m_input_{head,tail} variables back to zero as well, but IIUC they should still hold their initial zero values, as ProcessInput hasn't been called yet at this point (no workers submitted to the pool). Would it make sense to assert for head/tail being zero here (either manually or by calling StopFetching, which checks for that)?

Hmm well StartFetching is only called on the main thread, and as you say ProcessInput hasn't been called, so nothing could have incremented m_input_head. But I suppose we could assert here that it has not been incremented.

I'm not sure about m_input_tail though, it is only incremented on main thread, so there's no way to reason it could have been changed from top of the method where it is asserted.

Added a StopFetching() call after we clear the inputs.

0aa7ca7 doc: add release notes:

Seems a bit off to focus on the arg instead of the feature itself. I'd start with the performance improvement and then cover how to configure it if you really want to (not even sure I'd mention it).

0aa7ca7 doc: add release notes:

Seems a bit off to focus on the arg instead of the feature itself. I'd start with the performance improvement and then cover how to configure it if you really want to (not even sure I'd mention it).

I think explicitly mentioning new configuration options in the release notes is quite usual. I agree, though, that starting with an explanation of the improvement might read a bit better. (No big deal, as it can still be refined in the wiki before the release, I guess.)

We're not yet sure people should be fiddling with this option yet, we got mixed results. And seeing how easy it is to misuse dbcache, I think the focus should be on the feature instead of how to tune it.

I think it's perfectly fine for users to fiddle with it and experiment to find best outcomes for their own setup. It's definitely safe to do so.

Sure, they should be able to fiddle with it, but my argument was that the release notes should still describe the feature before the knob:

- Block validation can now prefetch input prevouts from the chainstate database
  in parallel while connecting blocks, speeding up validation when prevouts need
  to be read from disk. A new `-prevoutfetchthreads=<n>` option controls the
  number of prefetch worker threads. The default is 8 threads, up to a maximum
  of 16; set it to 0 to disable parallel prefetching. (#35295)

Updated.

Nit: Does this really need to be a fixture setup? How about making this a function that returns the pool and that can be called by each CoinsViewOverlay?

It was recreated originally for every test, but production uses a single thread pool that is reused. It was changed later to a static pool which wasn't always destructed in time, so now it's a fixture-level instance to have both reuse (for some speed benefit, but mostly to make it more realistic) and proper cleanup.

The way I read InitCache, the overlay now ends up owning the threadpool. Wouldn't it be more realistic to imitate this relationship in the tests?

You mean that the pool is reused because the overlay is reused for later blocks, not because callers keep a separate owner and pass it into many overlays? Is this what you're suggesting instead?

diff --git a/src/test/coinsviewoverlay_tests.cpp b/src/test/coinsviewoverlay_tests.cpp
--- a/src/test/coinsviewoverlay_tests.cpp	(revision 70ca8b3083decab30eeced02a1ced948e3f7fef1)
+++ b/src/test/coinsviewoverlay_tests.cpp	(revision 882e15222b9462861736809797042f45a75e2593)
@@ -22,15 +22,14 @@
 #include <unordered_set>
 #include <vector>
 
-struct CoinsViewOverlaySetup {
-    std::shared_ptr<ThreadPool> m_thread_pool{[] {
-        auto pool{std::make_shared<ThreadPool>("fetch_test")};
-        pool->Start(DEFAULT_PREVOUTFETCH_THREADS);
-        return pool;
-    }()};
-};
-
-namespace {
+namespace {
+
+std::shared_ptr<ThreadPool> MakeStartedThreadPool()
+{
+    auto pool{std::make_shared<ThreadPool>("fetch_test")};
+    pool->Start(DEFAULT_PREVOUTFETCH_THREADS);
+    return pool;
+}
 
 CBlock CreateBlock() noexcept
 {
@@ -100,7 +99,7 @@
 
 } // namespace
 
-BOOST_FIXTURE_TEST_SUITE(coinsviewoverlay_tests, CoinsViewOverlaySetup)
+BOOST_AUTO_TEST_SUITE(coinsviewoverlay_tests)
 
 BOOST_AUTO_TEST_CASE(fetch_inputs_from_db)
 {
@@ -108,7 +107,7 @@
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     PopulateView(block, db);
     CCoinsViewCache main_cache{&db};
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
     const auto& outpoint{block.vtx[1]->vin[0].prevout};
 
@@ -137,7 +136,7 @@
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     CCoinsViewCache main_cache{&db};
     PopulateView(block, main_cache);
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
     CheckCache(block, view);
     BOOST_CHECK(view.AllInputsConsumed());
@@ -159,7 +158,7 @@
     CCoinsViewCache main_cache{&db};
     // Add all inputs as spent already in cache
     PopulateView(block, main_cache, /*spent=*/true);
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
     for (const auto& tx : block.vtx) {
         for (const auto& in : tx->vin) {
@@ -178,7 +177,7 @@
     const auto block{CreateBlock()};
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     CCoinsViewCache main_cache{&db};
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
     for (const auto& tx : block.vtx) {
         for (const auto& in : tx->vin) {
@@ -205,7 +204,7 @@
     const COutPoint outpoint{Txid::FromUint256(uint256::ZERO), 0};
     main_cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, std::move(coin));
 
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
 
     // Non-input fallback hit.
@@ -238,7 +237,7 @@
     }
     BOOST_REQUIRE_GE(fetched_inputs.size(), 2U);
 
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto reset_guard{view.StartFetching(block)};
 
     const auto& out_of_order_input{fetched_inputs[1]};
@@ -262,7 +261,7 @@
     PopulateView(block, alternate_cache);
 
     for (const bool flush : {false, true}) {
-        CoinsViewOverlay view{&main_cache, m_thread_pool};
+        CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
         const auto first_guard{view.StartFetching(block)};
         view.SetBestBlock(uint256::ONE);
         flush ? view.Flush() : view.Sync();
@@ -270,7 +269,7 @@
         CheckCache(block, view);
     }
 
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
     const auto first_guard{view.StartFetching(block)};
     view.SetBackend(alternate_cache);
 
@@ -286,7 +285,8 @@
     CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
     CCoinsViewCache main_cache{&db};
     PopulateView(block, main_cache);
-    CoinsViewOverlay view{&main_cache, m_thread_pool};
+    // Reusing the overlay also reuses its owned thread pool.
+    CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()};
 
     {
         const auto reset_guard{view.StartFetching(block)};

Yes, that is my suggestion

Done.

This probably undoes #35295 (review), so we could reunify the two test suites:

diff --git a/src/test/coinsviewoverlay_tests.cpp b/src/test/coinsviewoverlay_tests.cpp
--- a/src/test/coinsviewoverlay_tests.cpp	(revision 3b54e20ad7540cd8adcebe58d64c99c6c7265d48)
+++ b/src/test/coinsviewoverlay_tests.cpp	(revision 1cff3af24f8f028487d8d3e7eda1bd49aa919ab0)
@@ -268,10 +268,6 @@
     }
 }
 
-BOOST_AUTO_TEST_SUITE_END()
-
-BOOST_AUTO_TEST_SUITE(coinsviewoverlay_tests_noworkers)
-
 // Test that disabled input fetching falls back to normal cache lookups via base->PeekCoin.
 BOOST_AUTO_TEST_CASE(fetch_unstarted_thread_pool)
 {

It's also fine to keep them separate since they're not using MakeStartedThreadPool

Nit: In commit 05695111611f0c3e9ffa6facaa80fc0e173151b7:

I think this is introduced too early, I'm getting an unused function warning:

/home/drgrid/bitcoin/src/test/fuzz/coins_view.cpp:93:6: warning: unused function 'StartPoolIfNeeded' [-Wunused-function]
   93 | void StartPoolIfNeeded() EXCLUSIVE_LOCKS_REQUIRED(!g_thread_pool_mutex)

Moved to the right commit.

In commit "coins: introduce thread pool in CoinsViewOverlay" (05695111611f0c3e9ffa6facaa80fc0e173151b7)

Commit message says this causes a "memory leak when fuzzing". Wondering what is the memory leak? I also think it would be be good to mention the leak in a code comment instead of a commit message so future readers know about it.

Added code comments addressing this.

re: #35295 (review)

In commit "coins: introduce thread pool in CoinsViewOverlay" (f82043af507a2f2caacdae1af6bcacddc8c4876b)

Added code comments addressing this.

Am still confused about a few things:

• "This can cause a memory leak when fuzzing" seems to imply that memory leaks are still possible? Is this right or should "can" be replaced with "would"?
• What actual memory leak was observed? Was it an LSAN error? Was it a false positive?
• Even if per-iteration thread thread pools were used I don't understand what the leak would be because the ThreadPool destructor joins the threads.
• It seems like every other fuzz test that is using thread pools is also using a global thread pool. Using a global pool instead of a per-iteration pool would seem to make sense for performance reasons. So choice of a global pool doesn't seem worth calling out in the commit message. And if the code comment is trying document a memory leak that could happen, it would be better to be specific about what the leak actually looks like.

In commit "coins: introduce thread pool in CoinsViewOverlay" (05695111611f0c3e9ffa6facaa80fc0e173151b7)

What is the rationale for these different settings in different tests? Code comments all 3 places would be helpful to make intent clear

This test launches lots of nodes - there is no need to create so many threadpools.

re: #35295 (review)

This test launches lots of nodes - there is no need to create so many threadpools.

Thanks, this is one of 3 places the setting is overridden, and in each case having a short comment indicating intent would be helpful because when you see tests changing a random setting, it's not obvious if it's being done to make tests more predictable, faster, better for triggering bugs, or other possible reasons. Better if intent is clear and not something you have to guess about.

Added comments to all 3 places where we set this option in the functional tests.

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (df1589dc2081becec63a3b5f52130234a0068e38)

Would be good to explain this with // for SaltedTxidHasher comment. I initially thought this was added to the wrong commit.

In commit "validation: collect block inputs in CoinsViewOverlay before ConnectBlock" (df1589dc2081becec63a3b5f52130234a0068e38)

Would suggest a more detailed message like:

--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3065,7 +3065,8 @@ bool Chainstate::ConnectTip(
             return false;
         }
         if (!Assume(view.AllInputsConsumed())) {
-            LogWarning("Not all prefetched input prevouts were consumed");
+            LogWarning("Internal bug detected: block %s input prefetch queue was not fully consumed (%s %s). Please report this issue here: %s\n",
+                pindexNew->GetBlockHash().ToString(), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
         }
         time_3 = SteadyClock::now();
         m_chainman.time_connect_total += time_3 - time_2;

In commit "coins: stop fetching before mutating base" (03c83f66f597f6bea46105575e1bbc84722b8433)

Seems unreliable to overrride public methods instead of using an internal hook. Scattering StopFetching calls different places with no comments is also confusing because it's not obvious how/why these methods should affect fetching. Using a hook should simplify the code and make the connections more clear. For example:

<details><summary>diff</summary> <p>

--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -264,6 +264,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in
 
 void CCoinsViewCache::Flush(bool reallocate_cache)
 {
+    OnBaseMutate();
     auto cursor{CoinsViewCacheCursor(m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/true)};
     base->BatchWrite(cursor, m_block_hash);
     Assume(m_dirty_count == 0);
@@ -276,6 +277,7 @@ void CCoinsViewCache::Flush(bool reallocate_cache)
 
 void CCoinsViewCache::Sync()
 {
+    OnBaseMutate();
     auto cursor{CoinsViewCacheCursor(m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/false)};
     base->BatchWrite(cursor, m_block_hash);
     Assume(m_dirty_count == 0);
@@ -287,6 +289,7 @@ void CCoinsViewCache::Sync()
 
 void CCoinsViewCache::Reset() noexcept
 {
+    OnBaseMutate();
     cacheCoins.clear();
     cachedCoinsUsage = 0;
     m_dirty_count = 0;
--- a/src/coins.h
+++ b/src/coins.h
@@ -382,11 +382,14 @@ class CCoinsViewBacked : public CCoinsView
 {
 protected:
     CCoinsView* base;
+    //! Called before base view is mutated to allow stopping any threads that
+    //! may be reading from it.
+    virtual void OnBaseMutate() {}
 
 public:
     explicit CCoinsViewBacked(CCoinsView* in_view) : base{Assert(in_view)} {}
 
-    virtual void SetBackend(CCoinsView& in_view) { base = &in_view; }
+    void SetBackend(CCoinsView& in_view) { OnBaseMutate(); base = &in_view; }
 
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override { return base->GetCoin(outpoint); }
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override { return base->PeekCoin(outpoint); }
@@ -497,7 +500,7 @@ public:
      * If reallocate_cache is false, the cache will retain the same memory footprint
      * after flushing and should be destroyed to deallocate.
      */
-    virtual void Flush(bool reallocate_cache = true);
+    void Flush(bool reallocate_cache = true);
 
     /**
      * Push the modifications applied to this cache to its base while retaining
@@ -505,7 +508,7 @@ public:
      * Failure to call this method or Flush() before destruction will cause the changes
      * to be forgotten.
      */
-    virtual void Sync();
+    void Sync();
 
     /**
      * Removes the UTXO with the given outpoint from the cache, if it is
@@ -567,8 +570,7 @@ private:
  * mutating the base cache.
  *
  * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
- * It provides the same interface as CCoinsViewCache. It overrides all methods that mutate base,
- * stopping fetch tasks before calling superclass.
+ * It provides the same interface as CCoinsViewCache.
  * It adds an additional StartFetching method to provide the block.
  *
  * When a block is passed to StartFetching, the inputs of the block are flattened into a vector of InputToFetch
@@ -593,7 +595,8 @@ private:
  *
  * StopFetching() is called before mutating operations (Flush/Sync/Reset/SetBackend). It stops fetching by moving
  * m_input_head to the end of m_inputs (so workers quickly exit), then waits for all futures to complete and clears
- * the per-block state (m_inputs and the head/tail counters).
+ * the per-block state (m_inputs and the head/tail counters). Stopping fetching is ok because there is no realistic
+ * use-case for parallel fetching while updating the base view other than fuzz testing.
  *
  *       Workers advance m_input_head to fetch inputs. Main thread advances m_input_tail to consume.
  *
@@ -713,11 +716,7 @@ private:
     std::vector<std::future<void>> m_futures{};
 
 protected:
-    void Reset() noexcept override
-    {
-        StopFetching();
-        CCoinsViewCache::Reset();
-    }
+    void OnBaseMutate() override { StopFetching(); }
 
 public:
     explicit CoinsViewOverlay(CCoinsView* in_base, std::shared_ptr<ThreadPool> thread_pool,
@@ -734,24 +733,6 @@ public:
 
     //! Verify that all parallel fetched input prevouts have been consumed.
     bool AllInputsConsumed() const noexcept { return m_input_tail == m_inputs.size(); }
-
-    void SetBackend(CCoinsView& view) override
-    {
-        StopFetching();
-        CCoinsViewCache::SetBackend(view);
-    }
-
-    void Flush(bool reallocate_cache = true) override
-    {
-        StopFetching();
-        CCoinsViewCache::Flush(reallocate_cache);
-    }
-
-    void Sync() override
-    {
-        StopFetching();
-        CCoinsViewCache::Sync();
-    }
 };
 
 //! Utility function to add all of a transaction's outputs to a cache.

</p> </details>

Yes, I considered doing this. But if we were going to add this hook, we would also want to add it to CCoinsViewCache::FetchCoinFromBase, which calls base->GetCoin(). However, that is a const method so we can't call that hook in there without making a whole bunch of state mutable. Without that the hook implementation is incomplete.

re: #35295 (review)

Yes, I considered doing this. But if we were going to add this hook, we would also want to add it to CCoinsViewCache::FetchCoinFromBase, which calls base->GetCoin().

I don't think this is true. (Also, even if it were true, I still think we'd be better off adding a few mutables or a strategic const_cast to reduce complexity of these overrides. We all agree this interface would be better if did not rely on mutables, but since it does use them, one of the known drawbacks of mutables is that they are viral and will spread.)

In any case, the simplification I suggested above should be ok in its current form because this->OnBaseMutate() is called when this class is changing the base view (comment says "Called before base view is mutated "). If this class is just calling base->GetCoin() and the base class mutates itself, that's not something this class should assume or know about.

But it would be good to clarify a little bit. Could rename "OnBaseMutate" to "OnMutateBase" and change "before base view is mutated" to "before this class updates the base or writes to it." I don't think it would be necessary, but it would also be reasonable to add a comment to CCoinsViewCache::FetchCoinFromBase saying something like "Depending on the base view, calling base->GetCoin() below could mutate its internal cache and interfere with concurrent readers. So it could be useful to call OnMutateBase() here, but it is not called because don't want to assume a get call is always a mutation, and because there are many other ways for callers to synchronize readers without this."

In commit "validation: add -prevoutfetchthreads configuration option"

We may want to consider using a smaller default number on 32-bit systems. Extra threads can add virtual memory (stack + stl allocations + local allocations) and RSS pressure, probably in the 8-16 MiB range per thread. Doing that 8 times may be non-negligible for configurations where these are limited (and VM usage has a low upper bound on 32-bit).

Alternatively, mention this is doc/reduce-memory.md.

consider using a smaller default number on 32-bit systems

I had the impression we're slowly phasing out 32 bit systems.

Extra threads can add virtual memory

It's why I originally suggested 4 threads instead of 8. I'm fine with both.

I added a mention in doc/reduce-memory.md. We also spawn 16 RPC threads by default, which is mentioned there.

I originally suggested 4 threads instead of 8.

I think 4 is leaving some performance improvement on the table, especially with high-latency disks.

@ryanofsky we can continue the discussion from #35295 (comment) here, so we have a dedicated thread.

I overrode Flush only without overriding Sync and SetBackend, and call Assume(AllInputsConsumed) inside Flush. We do need to call StopFetching before calling the base CCoinsViewCache::Flush(). We can't rely on Reset() since that is called after the guard has left scope after Flush(). Otherwise if a bug is introduced in our validation code that doesn't consume all inputs there will be a data race on base.

I removed the commit introducing the OnMutateBase hooks, and instead in the fuzz tests we don't call Sync or SetBackend on overlays and check first if AllInputsConsumed is true before allowing a call to Flush. This way AllInputsConsumed becomes a fuzz test only method so doesn't have to be called by production callers, and we're not overriding methods nor calling them in the fuzz tests if they're not called in production. Unfortunately I can't see a way of implementing something like #35295 (review) without severely crippling the fuzz tests. Let me know what you think.

re: #35295 (review)

Thanks for the updates. I'm much happier with the state now that there are not as many unnecessary StopFetching calls tucked away in different places.

But the AllInputsConsumed check in Flush does seem misplaced to me because it is trying to check whether the CoinsViewOverlay reads were performant in a Flush function that writes and should be unrelated to fetching. It's also just a weaker check than what I suggested and seems to create problems for the fuzz tests and make them less meaningful because now they have to avoid flush calls and other method calls. The tests now have many "never called in production" comments which raise the question: If these methods are not called in production, and not being tested, why are we still allowing them to be called?

It would seem better to:

• Let the tests to just keep all of their preexisting coverage and just call CancelFetch when necessary
• Stop checking read performance in a Flush method that is supposed to be for writing
• Not let callers bypass this check when they don't Flush

So an updated version of my last commit would be 2decc21e89b9e6f6d9b682570e1611177eab5ed4

Unfortunately I can't see a way of implementing something like #35295 (review) without severely crippling the fuzz tests.

I don't think I understand. My suggestion in #35295 (comment) should add more coverage to the fuzz test making sure they actually exercise the expected code path where inputs are fetched order. An updated version would be: 2202d7133fc9ee6ee3eb87030cbfd03574b4e848

Stop checking read performance in a Flush method that is supposed to be for writing

But we still need to override Flush to stop fetching before writing to base. Otherwise there can be a data race as we write to base while we could still be fetching. So we will still need to call StopFetching(/*cancel=*/false); inside Flush. Am I not seeing a way to avoid this?

Not let callers bypass this check when they don't Flush

If we need to do the above stop fetching in Flush, the only path that does not call Flush is for an invalid block. In that case we need to bypass the check, since invalid blocks can violate the assumptions we make for parallel fetching. Is there another case where we want to do this check but don't call Flush?

My suggestion in #35295 (comment) should add more coverage to the fuzz test making sure they actually exercise the expected code path where inputs are fetched order. An updated version would be: https://github.com/bitcoin/bitcoin/commit/2202d7133fc9ee6ee3eb87030cbfd03574b4e848

Doesn't this make BIP30 and invalid block inputs quadratic?

re: #35295 (review)

Stop checking read performance in a Flush method that is supposed to be for writing

But we still need to override Flush to stop fetching before writing to base. Otherwise there can be a data race as we write to base while we could still be fetching. So we will still need to call StopFetching(/*cancel=*/false); inside Flush. Am I not seeing a way to avoid this?

I feel like the implementation in 2decc21e89b9e6f6d9b682570e1611177eab5ed4 should make this pretty clear (though I'd agree it could be clearer):

• Tests call CancelFetch before they write to the base
• ConnectTip reads all inputs before it writes to the base

But the documentation change in the commit is not very good and mixes up internal implementation details with requirements for callers. Better documentation would describe how the class is intended to be used first:

CCoinsViewCache subclass that asynchronously fetches block input prevouts in parallel during ConnectBlock.

It adds a StartFetching() method it fetches coins in parallel from the base view, using PeekCoin() to avoid mutating the base cache.

It is not safe to write to the base view while fetching is in progress, since writing would interfere with the reads. So before any operation that changes the base view (Flush/Sync/SetBackend), either CancelFetch needs to be called, or all inputs need to have been read.

and describe the implementation details after.

My suggestion in #35295 (comment) should add more coverage to the fuzz test making sure they actually exercise the expected code path where inputs are fetched order. An updated version would be: 2202d7133fc9ee6ee3eb87030cbfd03574b4e848

Doesn't this make BIP30 and invalid block inputs quadratic?

My comment was referring to the more realistic fuzz test coverage added in 2202d7133fc9ee6ee3eb87030cbfd03574b4e848 but I'd agree if the invalid block case could be a DoS concern, or if the BIP30 checks are more expensive than I was assuming, those would be reasons for fully compiling the Assume out of release builds and not trying to log warnings. (A while ago I implemented a SLOWCHECK macro for things like this, but there were not a lot of use-cases).

Overall I think my first suggestion of enforcing the AllInputsConsumed check everywhere ( 2decc21e89b9e6f6d9b682570e1611177eab5ed4) and getting rid of inconsistent StopFetching calls and overrides is more important than my second suggestion of adding a complete InputFetchOrder check (2202d7133fc9ee6ee3eb87030cbfd03574b4e848) and making fuzz test more realistic.

If you don't like these ideas or don't think they are worth looking into, that's perfectly fine. I'm just saying they seem like good changes to me for the reasons stated.

ConnectTip reads all inputs before it writes to the base

My main objection here is that we are just assuming if the block is valid then this has happened, and we can call Flush. If our assumption does not hold in production, then we will have a data race. We need an explicit stop fetching in the happy path. Simply logging a warning in production will not stop the data race. The ResetGuard does not go out of scope before we Flush, so we need to have an explicit call to StopFetching inside Flush.

If you don't like these ideas or don't think they are worth looking into, that's perfectly fine. I'm just saying they seem like good changes to me for the reasons stated.

I definitely think your suggestions for trying to have stricter invariant checking are good and have made this PR better. I am trying to look into them, but I think there is a tension here where we need to assume a valid block to get tighter checks while the CoinsViewOverlay should just be a dumb container and not know anything about validation.

re: #35295 (review)

My main objection here is that we are just assuming if the block is valid then this has happened, and we can call Flush.

If this is your objection, we can continue to make Flush virtual and have it call StopFetching. It doesn't really affect anything else in my suggestion (2decc21e89b9e6f6d9b682570e1611177eab5ed4).

Ultimately, I think think the problem here is just that ResetGuard is not a good abstraction, and it would make more sense for ResetGuard and ReallocateCache and Flush(reallocate_cache) to go away so CCoinsViewCache and CoinsViewOverlay can be simple RAII classes that wrap a cache and a base view and are only responsible for updating the cache and reading from the base view. They should not be responsible for writing to the base view or allocating the cache at all.

ab2a379 coins: fetch inputs in parallel: