diff --git a/src/coins.cpp b/src/coins.cpp index 3d3e63fa4b8..0a40564d76f 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -379,12 +379,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); @@ -402,7 +403,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. } diff --git a/src/coins.h b/src/coins.h index 6fcf1092929..643a007490a 100644 --- a/src/coins.h +++ b/src/coins.h @@ -592,12 +592,16 @@ public: }; /** - * 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 @@ -607,17 +611,16 @@ public: * 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), @@ -741,6 +744,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(); @@ -769,6 +775,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/coinsviewoverlay_tests.cpp b/src/test/coinsviewoverlay_tests.cpp index b63807da31e..e44bcf6fefc 100644 --- a/src/test/coinsviewoverlay_tests.cpp +++ b/src/test/coinsviewoverlay_tests.cpp @@ -202,6 +202,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. diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 0cc38d8a878..d6dc19c8009 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -197,7 +197,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 @@ -375,7 +375,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(); 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(); }