Merge bitcoin/bitcoin#34165: coins: don't mutate main cache when connecting block

cae6d895f8 fuzz: add target for CoinsViewOverlay (Andrew Toth)
86eda88c8e fuzz: move backend mutating block to end of coins_view (Andrew Toth)
89824fb27b fuzz: pass coins_view_cache to TestCoinsView in coins_view (Andrew Toth)
73e99a5966 coins: don't mutate main cache when connecting block (Andrew Toth)
67c0d1798e coins: introduce CoinsViewOverlay (Andrew Toth)
69b01af0eb coins: add PeekCoin() (Andrew Toth)

Pull request description:

  This is a slightly modified version of the first few commits of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.

  When accessing coins via the `CCoinsViewCache`, methods like `GetCoin` can call `FetchCoin` which actually mutate `cacheCoins` internally to cache entries when they are pulled from the backing db. This is generally a performance improvement for single threaded access patterns, but it precludes us from accessing entries in a `CCoinsViewCache` from multiple threads without a lock.

  Another aspect is that when we use the resettable `CCoinsViewCache` view backed by the main cache for use in `ConnectBlock()`, we will insert entries into the main cache even if the block is determined to be invalid. This is not the biggest concern, since an invalid block requires proof-of-work. But, an attacker could craft multiple invalid blocks to fill the main cache. This would make us `Flush` the cache more often than necessary. Obviously this would be very expensive to do on mainnet.

  Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads coins without mutating the underlying cache via `FetchCoin()`.

  Add `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`.

  This is the foundation for async input fetching, where worker threads must not mutate shared state.

ACKs for top commit:
  l0rinc:
    ACK cae6d895f8
  sipa:
    reACK cae6d895f8
  sedited:
    Re-ACK cae6d895f8
  willcl-ark:
    ACK cae6d895f8
  vasild:
    Cursory ACK cae6d895f8
  ryanofsky:
    Code review ACK cae6d895f8. PR is basically back to the form I had acked the first time, implementing `PeekCoin()` by calling `GetCoin()`. This is not ideal because `PeekCoin()` is not supposed to modify caches and `GetCoin()` does that, but it at least avoids problems of the subsequent approach tried where `GetCoin()` calls `PeekCoin` and would result in bugs when subclasses implement `GetCoin` forgetting to override `PeekCoin`. Hopefully #34124 can clean all of this by making relevant methods pure virtual.

Tree-SHA512: a81a98e60ca9e47454933ad879840cc226cb3b841bc36a4b746c34b350e07c546cdb5ddc55ec1ff66cf65d1ec503d22201d3dc12d4e82a8f4d386ccc52ba6441

src/test/CMakeLists.txt

@@ -33,6 +33,7 @@ add_executable(test_bitcoin
   coins_tests.cpp
   coinscachepair_tests.cpp
   coinstatsindex_tests.cpp
+  coinsviewoverlay_tests.cpp
   common_url_tests.cpp
   compress_tests.cpp
   crypto_tests.cpp

src/test/coins_tests.cpp

@@ -1171,4 +1171,25 @@ BOOST_AUTO_TEST_CASE(ccoins_reset_guard)
     BOOST_CHECK_EQUAL(cache.GetDirtyCount(), 0U);
 }
 
+BOOST_AUTO_TEST_CASE(ccoins_peekcoin)
+{
+    CCoinsViewTest base{m_rng};
+
+    // Populate the base view with a coin.
+    const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
+    const Coin coin{CTxOut{m_rng.randrange(10), CScript{}}, 1, false};
+    {
+        CCoinsViewCache cache{&base};
+        cache.AddCoin(outpoint, Coin{coin}, /*possible_overwrite=*/false);
+        cache.Flush();
+    }
+
+    // Verify PeekCoin can read through the cache stack without mutating the intermediate cache.
+    CCoinsViewCacheTest main_cache{&base};
+    const auto fetched{main_cache.PeekCoin(outpoint)};
+    BOOST_CHECK(fetched.has_value());
+    BOOST_CHECK(*fetched == coin);
+    BOOST_CHECK(!main_cache.HaveCoinInCache(outpoint));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/coinsviewoverlay_tests.cpp

@@ -0,0 +1,165 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <coins.h>
+#include <primitives/block.h>
+#include <primitives/transaction.h>
+#include <primitives/transaction_identifier.h>
+#include <txdb.h>
+#include <uint256.h>
+#include <util/byte_units.h>
+#include <util/hasher.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <cstdint>
+#include <cstring>
+#include <ranges>
+
+BOOST_AUTO_TEST_SUITE(coinsviewoverlay_tests)
+
+namespace {
+
+CBlock CreateBlock() noexcept
+{
+    static constexpr auto NUM_TXS{100};
+    CBlock block;
+    CMutableTransaction coinbase;
+    coinbase.vin.emplace_back();
+    block.vtx.push_back(MakeTransactionRef(coinbase));
+
+    for (const auto i : std::views::iota(1, NUM_TXS)) {
+        CMutableTransaction tx;
+        Txid txid{Txid::FromUint256(uint256(i))};
+        tx.vin.emplace_back(txid, 0);
+        block.vtx.push_back(MakeTransactionRef(tx));
+    }
+
+    return block;
+}
+
+void PopulateView(const CBlock& block, CCoinsView& view, bool spent = false)
+{
+    CCoinsViewCache cache{&view};
+    cache.SetBestBlock(uint256::ONE);
+
+    for (const auto& tx : block.vtx | std::views::drop(1)) {
+        for (const auto& in : tx->vin) {
+            Coin coin{};
+            if (!spent) coin.out.nValue = 1;
+            cache.EmplaceCoinInternalDANGER(COutPoint{in.prevout}, std::move(coin));
+        }
+    }
+
+    cache.Flush();
+}
+
+void CheckCache(const CBlock& block, const CCoinsViewCache& cache)
+{
+    uint32_t counter{0};
+
+    for (const auto& tx : block.vtx) {
+        if (tx->IsCoinBase()) {
+            BOOST_CHECK(!cache.HaveCoinInCache(tx->vin[0].prevout));
+        } else {
+            for (const auto& in : tx->vin) {
+                const auto& outpoint{in.prevout};
+                const auto& first{cache.AccessCoin(outpoint)};
+                const auto& second{cache.AccessCoin(outpoint)};
+                BOOST_CHECK_EQUAL(&first, &second);
+                ++counter;
+                BOOST_CHECK(cache.HaveCoinInCache(outpoint));
+            }
+        }
+    }
+    BOOST_CHECK_EQUAL(cache.GetCacheSize(), counter);
+}
+
+} // namespace
+
+BOOST_AUTO_TEST_CASE(fetch_inputs_from_db)
+{
+    const auto block{CreateBlock()};
+    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
+    PopulateView(block, db);
+    CCoinsViewCache main_cache{&db};
+    CoinsViewOverlay view{&main_cache};
+    const auto& outpoint{block.vtx[1]->vin[0].prevout};
+
+    BOOST_CHECK(view.HaveCoin(outpoint));
+    BOOST_CHECK(view.GetCoin(outpoint).has_value());
+    BOOST_CHECK(!main_cache.HaveCoinInCache(outpoint));
+
+    CheckCache(block, view);
+    // Check that no coins have been moved up to main cache from db
+    for (const auto& tx : block.vtx) {
+        for (const auto& in : tx->vin) {
+            BOOST_CHECK(!main_cache.HaveCoinInCache(in.prevout));
+        }
+    }
+
+    view.SetBestBlock(uint256::ONE);
+    BOOST_CHECK(view.SpendCoin(outpoint));
+    view.Flush();
+    BOOST_CHECK(!main_cache.PeekCoin(outpoint).has_value());
+}
+
+BOOST_AUTO_TEST_CASE(fetch_inputs_from_cache)
+{
+    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};
+    CheckCache(block, view);
+
+    const auto& outpoint{block.vtx[1]->vin[0].prevout};
+    view.SetBestBlock(uint256::ONE);
+    BOOST_CHECK(view.SpendCoin(outpoint));
+    view.Flush();
+    BOOST_CHECK(!main_cache.PeekCoin(outpoint).has_value());
+}
+
+// Test for the case where a block spends coins that are spent in the cache, but
+// the spentness has not been flushed to the db.
+BOOST_AUTO_TEST_CASE(fetch_no_double_spend)
+{
+    const auto block{CreateBlock()};
+    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};
+    PopulateView(block, db);
+    CCoinsViewCache main_cache{&db};
+    // Add all inputs as spent already in cache
+    PopulateView(block, main_cache, /*spent=*/true);
+    CoinsViewOverlay view{&main_cache};
+    for (const auto& tx : block.vtx) {
+        for (const auto& in : tx->vin) {
+            const auto& c{view.AccessCoin(in.prevout)};
+            BOOST_CHECK(c.IsSpent());
+            BOOST_CHECK(!view.HaveCoin(in.prevout));
+            BOOST_CHECK(!view.GetCoin(in.prevout));
+        }
+    }
+    // Coins are not added to the view, even though they exist unspent in the parent db
+    BOOST_CHECK_EQUAL(view.GetCacheSize(), 0);
+}
+
+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};
+    for (const auto& tx : block.vtx) {
+        for (const auto& in : tx->vin) {
+            const auto& c{view.AccessCoin(in.prevout)};
+            BOOST_CHECK(c.IsSpent());
+            BOOST_CHECK(!view.HaveCoin(in.prevout));
+            BOOST_CHECK(!view.GetCoin(in.prevout));
+        }
+    }
+    BOOST_CHECK_EQUAL(view.GetCacheSize(), 0);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+

src/test/fuzz/coins_view.cpp

@@ -18,10 +18,13 @@
 #include <util/hasher.h>
 
 #include <cassert>
+#include <algorithm>
 #include <cstdint>
+#include <functional>
 #include <limits>
 #include <memory>
 #include <optional>
+#include <ranges>
 #include <stdexcept>
 #include <string>
 #include <utility>
@@ -35,6 +38,62 @@ bool operator==(const Coin& a, const Coin& b)
     if (a.IsSpent() && b.IsSpent()) return true;
     return a.fCoinBase == b.fCoinBase && a.nHeight == b.nHeight && a.out == b.out;
 }
+
+/**
+ * MutationGuardCoinsViewCache asserts that nothing mutates cacheCoins until
+ * BatchWrite is called. It keeps a snapshot of the cacheCoins state, which it
+ * uses for the assertion in BatchWrite. After the call to the superclass
+ * CCoinsViewCache::BatchWrite returns, it recomputes the snapshot at that
+ * moment.
+ */
+class MutationGuardCoinsViewCache final : public CCoinsViewCache
+{
+private:
+    struct CacheCoinSnapshot {
+        COutPoint outpoint;
+        bool dirty{false};
+        bool fresh{false};
+        Coin coin;
+        bool operator==(const CacheCoinSnapshot&) const = default;
+    };
+
+    std::vector<CacheCoinSnapshot> ComputeCacheCoinsSnapshot() const
+    {
+        std::vector<CacheCoinSnapshot> snapshot;
+        snapshot.reserve(cacheCoins.size());
+
+        for (const auto& [outpoint, entry] : cacheCoins) {
+            snapshot.emplace_back(outpoint, entry.IsDirty(), entry.IsFresh(), entry.coin);
+        }
+
+        std::ranges::sort(snapshot, std::less<>{}, &CacheCoinSnapshot::outpoint);
+        return snapshot;
+    }
+
+    mutable std::vector<CacheCoinSnapshot> m_expected_snapshot{ComputeCacheCoinsSnapshot()};
+
+public:
+    void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override
+    {
+        // Nothing must modify cacheCoins other than BatchWrite.
+        assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot);
+        try {
+            CCoinsViewCache::BatchWrite(cursor, block_hash);
+        } catch (const std::logic_error& e) {
+            // This error is thrown if the cursor contains a fresh entry for an outpoint that we already have a fresh
+            // entry for. This can happen if the fuzzer calls AddCoin -> Flush -> AddCoin -> Flush on the child cache.
+            // There's not an easy way to prevent the fuzzer from reaching this, so we handle it here.
+            // Since it is thrown in the middle of the write, we reset our own state and iterate through
+            // the cursor so the caller's state is also reset.
+            assert(e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"});
+            Reset();
+            for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {}
+        }
+        m_expected_snapshot = ComputeCacheCoinsSnapshot();
+    }
+
+    using CCoinsViewCache::CCoinsViewCache;
+};
 } // namespace
 
 void initialize_coins_view()
@@ -42,11 +101,10 @@ void initialize_coins_view()
     static const auto testing_setup = MakeNoLogFileContext<>();
 }
 
-void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view, bool is_db)
+void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& coins_view_cache, CCoinsView& backend_coins_view, bool is_db)
 {
     bool good_data{true};
 
-    CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
     if (is_db) coins_view_cache.SetBestBlock(uint256::ONE);
     COutPoint random_out_point;
     Coin random_coin;
@@ -180,31 +238,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
             });
     }
 
-    {
-        const Coin& coin_using_access_coin = coins_view_cache.AccessCoin(random_out_point);
-        const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN);
-        const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point);
-        const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point);
-        if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {
-            assert(*coin == coin_using_access_coin);
-            assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin);
-        } else {
-            assert(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin);
-        }
-        // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent.
-        const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point);
-        if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) {
-            assert(exists_using_have_coin);
-        }
-        if (auto coin{backend_coins_view.GetCoin(random_out_point)}) {
-            assert(exists_using_have_coin_in_backend);
-            // Note we can't assert that `coin_using_get_coin == *coin` because the coin in
-            // the cache may have been modified but not yet flushed.
-        } else {
-            assert(!exists_using_have_coin_in_backend);
-        }
-    }
-
     {
         bool expected_code_path = false;
         try {
@@ -222,8 +255,10 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
     }
 
     {
-        std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
-        assert(is_db == !!coins_view_cursor);
+        if (is_db) {
+            std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
+            assert(!!coins_view_cursor);
+        }
         (void)backend_coins_view.EstimateSize();
         (void)backend_coins_view.GetBestBlock();
         (void)backend_coins_view.GetHeadBlocks();
@@ -308,13 +343,39 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
                 (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
             });
     }
+
+    {
+        const Coin& coin_using_access_coin = coins_view_cache.AccessCoin(random_out_point);
+        const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN);
+        const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point);
+        const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point);
+        if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {
+            assert(*coin == coin_using_access_coin);
+            assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin);
+        } else {
+            assert(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin);
+        }
+        // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent.
+        const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point);
+        if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) {
+            assert(exists_using_have_coin);
+        }
+        if (auto coin{backend_coins_view.GetCoin(random_out_point)}) {
+            assert(exists_using_have_coin_in_backend);
+            // Note we can't assert that `coin_using_get_coin == *coin` because the coin in
+            // the cache may have been modified but not yet flushed.
+        } else {
+            assert(!exists_using_have_coin_in_backend);
+        }
+    }
 }
 
 FUZZ_TARGET(coins_view, .init = initialize_coins_view)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     CCoinsView backend_coins_view;
-    TestCoinsView(fuzzed_data_provider, backend_coins_view, /*is_db=*/false);
+    CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
+    TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_coins_view, /*is_db=*/false);
 }
 
 FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
@@ -325,6 +386,20 @@ FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
         .cache_bytes = 1_MiB,
         .memory_only = true,
     };
-    CCoinsViewDB coins_db{std::move(db_params), CoinsViewOptions{}};
-    TestCoinsView(fuzzed_data_provider, coins_db, /*is_db=*/true);
+    CCoinsViewDB backend_coins_view{std::move(db_params), CoinsViewOptions{}};
+    CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
+    TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_coins_view, /*is_db=*/true);
+}
+
+// Creates a CoinsViewOverlay and a MutationGuardCoinsViewCache as the base.
+// This allows us to exercise all methods on a CoinsViewOverlay, while also
+// ensuring that nothing can mutate the underlying cache until Flush or Sync is
+// called.
+FUZZ_TARGET(coins_view_overlay, .init = initialize_coins_view)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    CCoinsView backend_base_coins_view;
+    MutationGuardCoinsViewCache backend_cache{&backend_base_coins_view, /*deterministic=*/true};
+    CoinsViewOverlay coins_view_cache{&backend_cache, /*deterministic=*/true};
+    TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_cache, /*is_db=*/false);
 }

src/test/fuzz/coinscache_sim.cpp

@@ -257,7 +257,9 @@ FUZZ_TARGET(coinscache_sim)
                 // Look up in simulation data.
                 auto sim = lookup(outpointidx);
                 // Look up in real caches.
-                auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
+                auto realcoin = provider.ConsumeBool() ?
+                    caches.back()->PeekCoin(data.outpoints[outpointidx]) :
+                    caches.back()->GetCoin(data.outpoints[outpointidx]);
                 // Compare results.
                 if (!sim.has_value()) {
                     assert(!realcoin);
@@ -372,7 +374,11 @@ FUZZ_TARGET(coinscache_sim)
             [&]() { // Add a cache level (if not already at the max).
                 if (caches.size() != MAX_CACHES) {
                     // Apply to real caches.
-                    caches.emplace_back(new CCoinsViewCache(&*caches.back(), /*deterministic=*/true));
+                    if (provider.ConsumeBool()) {
+                        caches.emplace_back(new CCoinsViewCache(&*caches.back(), /*deterministic=*/true));
+                    } else {
+                        caches.emplace_back(new CoinsViewOverlay(&*caches.back(), /*deterministic=*/true));
+                    }
                     // Apply to simulation data.
                     sim_caches[caches.size()].Wipe();
                 }

src/test/validation_chainstate_tests.cpp

@@ -3,10 +3,12 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 //
 #include <chainparams.h>
+#include <consensus/amount.h>
 #include <consensus/validation.h>
 #include <node/kernel_notifications.h>
 #include <random.h>
 #include <rpc/blockchain.h>
+#include <script/script.h>
 #include <sync.h>
 #include <test/util/chainstate.h>
 #include <test/util/coins.h>
@@ -63,6 +65,30 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(connect_tip_does_not_cache_inputs_on_failed_connect, TestChain100Setup)
+{
+    Chainstate& chainstate{Assert(m_node.chainman)->ActiveChainstate()};
+
+    COutPoint outpoint;
+    {
+        LOCK(cs_main);
+        outpoint = AddTestCoin(m_rng, chainstate.CoinsTip());
+        chainstate.CoinsTip().Flush(/*reallocate_cache=*/false);
+    }
+
+    CMutableTransaction tx;
+    tx.vin.emplace_back(outpoint);
+    tx.vout.emplace_back(MAX_MONEY, CScript{} << OP_TRUE);
+
+    const auto tip{WITH_LOCK(cs_main, return chainstate.m_chain.Tip()->GetBlockHash())};
+    const CBlock block{CreateBlock({tx}, CScript{} << OP_TRUE, chainstate)};
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(std::make_shared<CBlock>(block), true, true, nullptr));
+
+    LOCK(cs_main);
+    BOOST_CHECK_EQUAL(tip, chainstate.m_chain.Tip()->GetBlockHash()); // block rejected
+    BOOST_CHECK(!chainstate.CoinsTip().HaveCoinInCache(outpoint));    // input not cached
+}
+
 //! Test UpdateTip behavior for both active and background chainstates.
 //!
 //! When run on the background chainstate, UpdateTip should do a subset