validation: fetch block inputs 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

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?

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);
}

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};

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);
            }