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.

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](/bitcoin-bitcoin/contributor/return/) true if an input prevout was fetched
     * [@return](/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?

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.