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.