From 330022993fb96b3b776e562f1de6696d381e6524 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 15 Jul 2026 13:23:51 -0400 Subject: [PATCH 1/5] coins: filter coinbase txid from parallel input fetching A non-segwit invalid block could spend its own coinbase output. In that case we would want to skip fetching the coinbase prevout since it would already be in the CoinsViewOverlay's cache and would cause block validation to revert to synchronous fetching. Co-authored-by: Pieter Wuille --- src/coins.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coins.cpp b/src/coins.cpp index 7bb05f68c1a..39c9c12dbee 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -372,12 +372,13 @@ CCoinsViewCache::ResetGuard CoinsViewOverlay::StartFetching(const CBlock& block Assert(m_inputs.empty()); Assert(m_input_head.load(std::memory_order_relaxed) == 0); Assert(m_input_tail == 0); - if (const auto workers_count{m_thread_pool->WorkersCount()}; workers_count > 0) { + if (const auto workers_count{m_thread_pool->WorkersCount()}; workers_count > 0 && block.vtx.size() > 1) { // 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 // directly in the cache from the tx that creates them, so they will not be requested from a base view. std::unordered_set earlier_txids; earlier_txids.reserve(block.vtx.size()); + earlier_txids.emplace(block.vtx[0]->GetHash()); for (const auto& tx : block.vtx | std::views::drop(1)) { for (const auto& input : tx->vin) { if (!earlier_txids.contains(input.prevout.hash)) m_inputs.emplace_back(input.prevout); From 2ffaa6e6a7db239306309a859b7e8aed478a810d Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 15 Jul 2026 13:24:11 -0400 Subject: [PATCH 2/5] coins: delete Sync and SetBackend on CoinsViewOverlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither is called in production code, and both would write to or swap the base view, which is unsafe while workers are still fetching. Hide the non-virtual base class methods with deleted ones. Also fix a doubled comment and reuse the existing overlay pointer in the coins_view fuzz target. Co-authored-by: Lőrinc --- src/coins.h | 4 ++++ src/test/fuzz/coins_view.cpp | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coins.h b/src/coins.h index 6e8643ccc25..5bf4c07e7af 100644 --- a/src/coins.h +++ b/src/coins.h @@ -733,6 +733,10 @@ public: CCoinsViewCache::Flush(reallocate_cache); } + //! Swapping the backend or writing through to it with Sync() is not supported while fetching. + void SetBackend(CCoinsView&) = delete; + void Sync() = delete; + //! Verify that all parallel fetched input prevouts have been consumed. bool AllInputsConsumed() const noexcept { return m_input_tail == m_inputs.size(); } }; diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 5e1b08ade52..343eb993f0d 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -204,7 +204,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co coins_view_cache.Uncache(random_out_point); }, [&] { - if (overlay) return; // // CoinsViewOverlay::SetBackend() is never called in production code + if (overlay) return; // CoinsViewOverlay::SetBackend() is never called in production code const bool use_original_backend{fuzzed_data_provider.ConsumeBool()}; if (use_original_backend && backend_coins_view != original_backend) { // FRESH flags valid against the empty backend may be invalid @@ -382,7 +382,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent. std::optional coin_in_backend; bool exists_using_have_coin_in_backend; - if (dynamic_cast(&coins_view_cache)) { + if (overlay) { // 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(); From d552c52b081c4c0853086bcdce38d67fd02f2c67 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 15 Jul 2026 13:24:24 -0400 Subject: [PATCH 3/5] coins: log error reason when prevout fetch submission fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lőrinc --- src/coins.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coins.cpp b/src/coins.cpp index 39c9c12dbee..83eefb51958 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -396,7 +396,7 @@ CCoinsViewCache::ResetGuard CoinsViewOverlay::StartFetching(const CBlock& block // Submit can fail if a shared owner of the thread pool outside of this class calls Stop() or // Interrupt() on a different thread after we call WorkersCount() above. In that case parallel // fetching will not make progress, so we clear the inputs to fall back to single threaded fetching. - LogWarning("Failed to submit prevout fetch tasks; falling back to single-threaded fetching for this block."); + LogWarning("Failed to submit prevout fetch tasks (%s); falling back to single-threaded fetching for this block.", SubmitErrorString(futures.error())); m_inputs.clear(); StopFetching(); // Assert nothing changed if we failed to start tasks. } From 5292386b785a0a133368b5429a2363cf53e29e00 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 15 Jul 2026 13:24:57 -0400 Subject: [PATCH 4/5] doc: improve CoinsViewOverlay documentation Document the single main thread requirement, clarify how FetchCoinFromBase consumes fetched inputs, and note why Reset must stop fetching. Also explain a no-op StartFetching call in the unit tests. Co-authored-by: Ryan Ofsky --- src/coins.h | 30 +++++++++++++++++------------ src/test/coinsviewoverlay_tests.cpp | 2 ++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/coins.h b/src/coins.h index 5bf4c07e7af..15f1f34c4bd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -556,12 +556,16 @@ private: }; /** - * CCoinsViewCache subclass that asynchronously fetches most block input prevouts in parallel during ConnectBlock without - * mutating the base cache. + * CCoinsViewCache subclass that asynchronously fetches most block input prevouts in parallel during ConnectBlock + * without mutating the base cache. This is achieved by fetching coins from the base view using PeekCoin() instead of + * GetCoin(), so intermediate CCoinsViewCache layers are not filled. * - * 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 adds an additional StartFetching method to provide the block. + * Used during ConnectBlock() as an ephemeral, resettable top-level view that is flushed only on success, so invalid + * blocks don't pollute the underlying cache. + * + * While this class uses threads internally to fetch coins, externally it is only safe to call its methods from a + * single "main" thread. It assumes StartFetching, StopFetching, FetchCoinFromBase, Flush and Reset will all only be + * called from the main thread. * * When a block is passed to StartFetching, the inputs of the block are flattened into a vector of InputToFetch * objects. StartFetching then submits worker tasks to a ThreadPool and keeps the returned futures alive until fetching @@ -571,17 +575,16 @@ private: * m_inputs vector at a time. Workers race to claim inputs, so they may fetch elements in any order. If the fetched * index is greater than or equal to the size of m_inputs, no more inputs can be fetched and false is returned. * - * The worker claims the InputToFetch at this index, fetches the coin from the base cache and moves it into the + * The worker claims the InputToFetch at this index, fetches the coin with base->PeekCoin() and moves it into the * InputToFetch object. The ready flag is then set with a release memory order. This allows the ready flag to be * used as a memory fence, guaranteeing the coin being written to the object will have happened before another * thread tests the flag with an acquire memory order. - * This assumes all base->PeekCoin() paths are safe for concurrent readers and do not mutate lower cache layers. + * This assumes all base->PeekCoin() paths are safe for concurrent readers. * - * When a coin is requested from the cache on the main thread and is not already in cacheCoins map, FetchCoinFromBase - * checks whether the next unconsumed entry in m_inputs has the requested outpoint. On a match, m_input_tail is advanced - * and the entry's ready flag is waited on with an acquire memory order until a worker has finished fetching it. The - * coin is then moved out and returned. Since the main thread is the only consumer of validation results, it blocks - * on the specific input it needs rather than racing workers for other inputs. + * The main thread is the only consumer of the fetched coins. FetchCoinFromBase is called when a coin is requested on + * the main thread and is not already in the cache. It checks whether the next unconsumed entry in m_inputs has the + * requested outpoint. On a match, m_input_tail is advanced and the entry's ready flag is waited on with an acquire + * memory order until a worker has finished fetching it. The coin is then moved out and returned. * * StopFetching() is called in Flush() and in Reset() (the per-block teardown) so workers stop before the block they * reference goes away. It stops fetching by moving m_input_head to the end of m_inputs (so workers quickly exit), @@ -705,6 +708,9 @@ private: std::vector> m_futures{}; protected: + //! StopFetching must be called here for two reasons: InputToFetch objects hold references to the + //! block's outpoints, so they must not outlive the block being connected; and when connecting a + //! block fails, workers must not keep fetching inputs for the block that was abandoned. void Reset() noexcept override { StopFetching(); diff --git a/src/test/coinsviewoverlay_tests.cpp b/src/test/coinsviewoverlay_tests.cpp index d6403752b0e..e6ea2217598 100644 --- a/src/test/coinsviewoverlay_tests.cpp +++ b/src/test/coinsviewoverlay_tests.cpp @@ -203,6 +203,8 @@ BOOST_AUTO_TEST_CASE(access_non_input_coins) main_cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, std::move(coin)); CoinsViewOverlay view{&main_cache, MakeStartedThreadPool()}; + // The block has no non-coinbase transactions, so this fetches nothing and only creates the + // reset guard. All lookups below use the fallback path. const auto reset_guard{view.StartFetching(block)}; // Non-input fallback hit. From 8e4b7ab7258aa8497ef90847e495e81d984b99d4 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 15 Jul 2026 13:25:06 -0400 Subject: [PATCH 5/5] fuzz: use per-level fetch scopes in coinscache_sim Keep one StartFetching guard per cache level instead of a single guard for the top level, so overlays continue fetching while new cache levels are added on top. Tear the guards down top down, since resetting a lower cache while an upper overlay's workers read through it would cause a data race. Co-authored-by: Ryan Ofsky --- src/test/fuzz/coinscache_sim.cpp | 34 +++++++++++++++++++------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 3e5d29a2c24..f7862503905 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -226,8 +226,8 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< CoinsViewBottom bottom; /** Real CCoinsViewCache objects. */ std::vector> caches; - /** Long-lived StartFetching guard (nullptr unless corresponding level is a CoinsViewOverlay). */ - std::unique_ptr overlay_fetch_scope; + /** Long-lived StartFetching guards, parallel to `caches` (entries are nullptr unless corresponding level is a CoinsViewOverlay). */ + std::vector> fetch_scopes; /** Simulated cache data (sim_caches[0] matches bottom, sim_caches[i+1] matches caches[i]). */ CacheLevel sim_caches[MAX_CACHES + 1]; /** Current height in the simulation. */ @@ -265,6 +265,12 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< } }; + /** Helper creating a fetch scope for the top cache (which must be a CoinsViewOverlay). */ + const auto make_fetch_scope{[&] { + auto& overlay{static_cast(*caches.back())}; + return std::make_unique(overlay, data.block); + }}; + // Main simulation loop: read commands from the fuzzer input, and apply them // to both the real cache stack and the simulation. FuzzedDataProvider provider(buffer.data(), buffer.size()); @@ -275,8 +281,10 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< // Make sure there is always at least one CCoinsViewCache. if (caches.empty()) { caches.emplace_back(new CCoinsViewCache(&bottom, /*deterministic=*/true)); + fetch_scopes.emplace_back(); sim_caches[caches.size()].Wipe(); } + assert(caches.size() == fetch_scopes.size()); // Execute command. CallOneOf( @@ -403,17 +411,13 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< [&]() { // Add a cache level (if not already at the max). if (caches.size() != MAX_CACHES) { - if (overlay_fetch_scope) { - overlay_fetch_scope.reset(); - sim_caches[caches.size()].Wipe(); - } // Apply to real caches. if (provider.ConsumeBool()) { caches.emplace_back(new CCoinsViewCache(&*caches.back(), /*deterministic=*/true)); + fetch_scopes.emplace_back(); } else { caches.emplace_back(new CoinsViewOverlay(&*caches.back(), g_thread_pool, /*deterministic=*/true)); - auto& overlay{static_cast(*caches.back())}; - overlay_fetch_scope = std::make_unique(overlay, data.block); + fetch_scopes.emplace_back(make_fetch_scope()); } // Apply to simulation data. sim_caches[caches.size()].Wipe(); @@ -423,7 +427,7 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< [&]() { // Remove a cache level. // Apply to real caches (this reduces caches.size(), implicitly doing the same on the simulation data). caches.back()->SanityCheck(); - overlay_fetch_scope.reset(); + fetch_scopes.pop_back(); caches.pop_back(); }, @@ -440,7 +444,7 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< }, [&]() { // Sync. - if (overlay_fetch_scope) return; // CoinsViewOverlay::Sync() is never called in production + if (fetch_scopes.back()) return; // CoinsViewOverlay::Sync() is never called in production // Apply to simulation data (note that in our simulation, syncing and flushing is the same thing). flush(); // Apply to real caches. @@ -450,10 +454,9 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< [&]() { // Reset. sim_caches[caches.size()].Wipe(); // Apply to real caches. Optionally start fetching again. - if (overlay_fetch_scope && provider.ConsumeBool()) { - overlay_fetch_scope.reset(); - auto& overlay{static_cast(*caches.back())}; - overlay_fetch_scope = std::make_unique(overlay, data.block); + if (fetch_scopes.back() && provider.ConsumeBool()) { + fetch_scopes.back().reset(); // Stop fetching before starting again. + fetch_scopes.back() = make_fetch_scope(); } else { (void)caches.back()->CreateResetGuard(); } @@ -514,4 +517,7 @@ FUZZ_TARGET(coinscache_sim, .init = [] { static auto setup{MakeNoLogFileContext< assert(realcoin->nHeight == sim->second); } } + + // Tear down the fetch scopes top down. Otherwise lower level could reset while upper level is reading from it. + while (!fetch_scopes.empty()) fetch_scopes.pop_back(); }