Merge bitcoin/bitcoin#17487: coins: allow write to disk without cache drop

1d7935b45ac61791399989effc18aece8b368fbb test: add test for coins view flush behavior using Sync() (James O'Beirne)
2c3cbd6c007a588e667751024027462268626fdb test: add use of Sync() to coins tests (James O'Beirne)
6d8affca96c7a34f5f104c5a3122e7420ffc083c test: refactor: clarify the coins simulation (James O'Beirne)
79cedc36afe2e72e42839d861734d73d545d21b8 coins: add Sync() method to allow flush without cacheCoins drop (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):

  Parent PR: #15606
  Issue: #15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

  ---

  In certain circumstances, we may want to flush chainstate data to disk without
  emptying `cacheCoins`, which affects performance. UTXO snapshot
  activation is one such case, as we populate `cacheCoins` with the snapshot
  contents and want to persist immediately afterwards but also enter IBD.

  See also #15265, which makes the case that under normal operation a
  flush-without-erase doesn't necessarily add much benefit. I open this PR
  even in light of the previous discussion because (i) flush-without-erase
  almost certainly provides benefit in the case of snapshot activation (especially
  on spinning disk hardware) and (ii) this diff is fairly small and gives us convenient
  options for more granular cache management without changing existing policy.

  See also #15218.

ACKs for top commit:
  sipa:
    ACK 1d7935b45ac61791399989effc18aece8b368fbb
  achow101:
    ACK 1d7935b45ac61791399989effc18aece8b368fbb
  Sjors:
    tACK 1d7935b45ac61791399989effc18aece8b368fbb

Tree-SHA512: 897583963e98661767d2d09c9a22f6019da24125558cd88770bfe2d017d924f23a9075b729e4b1febdec5b0709a38e8fa1ef94d62aa88650556b06cb4826c845

1 files changed, 230 insertions, 12 deletions

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 31d437081a..92bad8dd2e 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -53,9 +53,9 @@ public:
 
     uint256 GetBestBlock() const override { return hashBestBlock_; }
 
-    bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock) override
+    bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override
     {
-        for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) {
+        for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : ++it) {
             if (it->second.flags & CCoinsCacheEntry::DIRTY) {
                 // Same optimization used in CCoinsViewDB is to only write dirty entries.
                 map_[it->first] = it->second.coin;
@@ -64,7 +64,6 @@ public:
                     map_.erase(it->first);
                 }
             }
-            mapCoins.erase(it++);
         }
         if (!hashBlock.IsNull())
             hashBestBlock_ = hashBlock;
@@ -126,6 +125,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
     bool found_an_entry = false;
     bool missed_an_entry = false;
     bool uncached_an_entry = false;
+    bool flushed_without_erase = false;
 
     // A simple map to track what we expect the cache stack to represent.
     std::map<COutPoint, Coin> result;
@@ -154,9 +154,16 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
             bool test_havecoin_after = InsecureRandBits(2) == 0;
 
             bool result_havecoin = test_havecoin_before ? stack.back()->HaveCoin(COutPoint(txid, 0)) : false;
-            const Coin& entry = (InsecureRandRange(500) == 0) ? AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0));
+
+            // Infrequently, test usage of AccessByTxid instead of AccessCoin - the
+            // former just delegates to the latter and returns the first unspent in a txn.
+            const Coin& entry = (InsecureRandRange(500) == 0) ?
+                AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0));
             BOOST_CHECK(coin == entry);
-            BOOST_CHECK(!test_havecoin_before || result_havecoin == !entry.IsSpent());
+
+            if (test_havecoin_before) {
+                BOOST_CHECK(result_havecoin == !entry.IsSpent());
+            }
 
             if (test_havecoin_after) {
                 bool ret = stack.back()->HaveCoin(COutPoint(txid, 0));
@@ -167,24 +174,29 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
                 Coin newcoin;
                 newcoin.out.nValue = InsecureRand32();
                 newcoin.nHeight = 1;
+
+                // Infrequently test adding unspendable coins.
                 if (InsecureRandRange(16) == 0 && coin.IsSpent()) {
                     newcoin.out.scriptPubKey.assign(1 + InsecureRandBits(6), OP_RETURN);
                     BOOST_CHECK(newcoin.out.scriptPubKey.IsUnspendable());
                     added_an_unspendable_entry = true;
                 } else {
-                    newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); // Random sizes so we can test memory usage accounting
+                    // Random sizes so we can test memory usage accounting
+                    newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0);
                     (coin.IsSpent() ? added_an_entry : updated_an_entry) = true;
                     coin = newcoin;
                 }
-                stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), !coin.IsSpent() || InsecureRand32() & 1);
+                bool is_overwrite = !coin.IsSpent() || InsecureRand32() & 1;
+                stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite);
             } else {
+                // Spend the coin.
                 removed_an_entry = true;
                 coin.Clear();
                 BOOST_CHECK(stack.back()->SpendCoin(COutPoint(txid, 0)));
             }
         }
 
-        // One every 10 iterations, remove a random entry from the cache
+        // Once every 10 iterations, remove a random entry from the cache
         if (InsecureRandRange(10) == 0) {
             COutPoint out(txids[InsecureRand32() % txids.size()], 0);
             int cacheid = InsecureRand32() % stack.size();
@@ -216,7 +228,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
             if (stack.size() > 1 && InsecureRandBool() == 0) {
                 unsigned int flushIndex = InsecureRandRange(stack.size() - 1);
                 if (fake_best_block) stack[flushIndex]->SetBestBlock(InsecureRand256());
-                BOOST_CHECK(stack[flushIndex]->Flush());
+                bool should_erase = InsecureRandRange(4) < 3;
+                BOOST_CHECK(should_erase ? stack[flushIndex]->Flush() : stack[flushIndex]->Sync());
+                flushed_without_erase |= !should_erase;
             }
         }
         if (InsecureRandRange(100) == 0) {
@@ -224,7 +238,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
             if (stack.size() > 0 && InsecureRandBool() == 0) {
                 //Remove the top cache
                 if (fake_best_block) stack.back()->SetBestBlock(InsecureRand256());
-                BOOST_CHECK(stack.back()->Flush());
+                bool should_erase = InsecureRandRange(4) < 3;
+                BOOST_CHECK(should_erase ? stack.back()->Flush() : stack.back()->Sync());
+                flushed_without_erase |= !should_erase;
                 delete stack.back();
                 stack.pop_back();
             }
@@ -260,6 +276,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
     BOOST_CHECK(found_an_entry);
     BOOST_CHECK(missed_an_entry);
     BOOST_CHECK(uncached_an_entry);
+    BOOST_CHECK(flushed_without_erase);
 }
 
 // Run the above simulation for multiple base types.
@@ -589,9 +606,9 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags)
     return inserted.first->second.coin.DynamicMemoryUsage();
 }
 
-void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags)
+void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const COutPoint& outp = OUTPOINT)
 {
-    auto it = map.find(OUTPOINT);
+    auto it = map.find(outp);
     if (it == map.end()) {
         value = ABSENT;
         flags = NO_ENTRY;
@@ -877,4 +894,205 @@ BOOST_AUTO_TEST_CASE(ccoins_write)
                     CheckWriteCoins(parent_value, child_value, parent_value, parent_flags, child_flags, parent_flags);
 }
 
+
+Coin MakeCoin()
+{
+    Coin coin;
+    coin.out.nValue = InsecureRand32();
+    coin.nHeight = InsecureRandRange(4096);
+    coin.fCoinBase = 0;
+    return coin;
+}
+
+
+//! For CCoinsViewCache instances backed by either another cache instance or
+//! leveldb, test cache behavior and flag state (DIRTY/FRESH) by
+//!
+//! 1. Adding a random coin to the child-most cache,
+//! 2. Flushing all caches (without erasing),
+//! 3. Ensure the entry still exists in the cache and has been written to parent,
+//! 4. (if `do_erasing_flush`) Flushing the caches again (with erasing),
+//! 5. (if `do_erasing_flush`) Ensure the entry has been written to the parent and is no longer in the cache,
+//! 6. Spend the coin, ensure it no longer exists in the parent.
+//!
+void TestFlushBehavior(
+    CCoinsViewCacheTest* view,
+    CCoinsViewDB& base,
+    std::vector<CCoinsViewCacheTest*>& all_caches,
+    bool do_erasing_flush)
+{
+    CAmount value;
+    char flags;
+    size_t cache_usage;
+
+    auto flush_all = [&all_caches](bool erase) {
+        // Flush in reverse order to ensure that flushes happen from children up.
+        for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) {
+            auto cache = *i;
+            // hashBlock must be filled before flushing to disk; value is
+            // unimportant here. This is normally done during connect/disconnect block.
+            cache->SetBestBlock(InsecureRand256());
+            erase ? cache->Flush() : cache->Sync();
+        }
+    };
+
+    uint256 txid = InsecureRand256();
+    COutPoint outp = COutPoint(txid, 0);
+    Coin coin = MakeCoin();
+    // Ensure the coins views haven't seen this coin before.
+    BOOST_CHECK(!base.HaveCoin(outp));
+    BOOST_CHECK(!view->HaveCoin(outp));
+
+    // --- 1. Adding a random coin to the child cache
+    //
+    view->AddCoin(outp, Coin(coin), false);
+
+    cache_usage = view->DynamicMemoryUsage();
+    // `base` shouldn't have coin (no flush yet) but `view` should have cached it.
+    BOOST_CHECK(!base.HaveCoin(outp));
+    BOOST_CHECK(view->HaveCoin(outp));
+
+    GetCoinsMapEntry(view->map(), value, flags, outp);
+    BOOST_CHECK_EQUAL(value, coin.out.nValue);
+    BOOST_CHECK_EQUAL(flags, DIRTY|FRESH);
+
+    // --- 2. Flushing all caches (without erasing)
+    //
+    flush_all(/*erase=*/ false);
+
+    // CoinsMap usage should be unchanged since we didn't erase anything.
+    BOOST_CHECK_EQUAL(cache_usage, view->DynamicMemoryUsage());
+
+    // --- 3. Ensuring the entry still exists in the cache and has been written to parent
+    //
+    GetCoinsMapEntry(view->map(), value, flags, outp);
+    BOOST_CHECK_EQUAL(value, coin.out.nValue);
+    BOOST_CHECK_EQUAL(flags, 0);  // Flags should have been wiped.
+
+    // Both views should now have the coin.
+    BOOST_CHECK(base.HaveCoin(outp));
+    BOOST_CHECK(view->HaveCoin(outp));
+
+    if (do_erasing_flush) {
+        // --- 4. Flushing the caches again (with erasing)
+        //
+        flush_all(/*erase=*/ true);
+
+        // Memory usage should have gone down.
+        BOOST_CHECK(view->DynamicMemoryUsage() < cache_usage);
+
+        // --- 5. Ensuring the entry is no longer in the cache
+        //
+        GetCoinsMapEntry(view->map(), value, flags, outp);
+        BOOST_CHECK_EQUAL(value, ABSENT);
+        BOOST_CHECK_EQUAL(flags, NO_ENTRY);
+
+        view->AccessCoin(outp);
+        GetCoinsMapEntry(view->map(), value, flags, outp);
+        BOOST_CHECK_EQUAL(value, coin.out.nValue);
+        BOOST_CHECK_EQUAL(flags, 0);
+    }
+
+    // Can't overwrite an entry without specifying that an overwrite is
+    // expected.
+    BOOST_CHECK_THROW(
+        view->AddCoin(outp, Coin(coin), /*possible_overwrite=*/ false),
+        std::logic_error);
+
+    // --- 6. Spend the coin.
+    //
+    BOOST_CHECK(view->SpendCoin(outp));
+
+    // The coin should be in the cache, but spent and marked dirty.
+    GetCoinsMapEntry(view->map(), value, flags, outp);
+    BOOST_CHECK_EQUAL(value, SPENT);
+    BOOST_CHECK_EQUAL(flags, DIRTY);
+    BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`.
+    BOOST_CHECK(base.HaveCoin(outp));  // But coin should still be unspent in `base`.
+
+    flush_all(/*erase=*/ false);
+
+    // Coin should be considered spent in both views.
+    BOOST_CHECK(!view->HaveCoin(outp));
+    BOOST_CHECK(!base.HaveCoin(outp));
+
+    // Spent coin should not be spendable.
+    BOOST_CHECK(!view->SpendCoin(outp));
+
+    // --- Bonus check: ensure that a coin added to the base view via one cache
+    //     can be spent by another cache which has never seen it.
+    //
+    txid = InsecureRand256();
+    outp = COutPoint(txid, 0);
+    coin = MakeCoin();
+    BOOST_CHECK(!base.HaveCoin(outp));
+    BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
+    BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+
+    all_caches[0]->AddCoin(outp, std::move(coin), false);
+    all_caches[0]->Sync();
+    BOOST_CHECK(base.HaveCoin(outp));
+    BOOST_CHECK(all_caches[0]->HaveCoin(outp));
+    BOOST_CHECK(!all_caches[1]->HaveCoinInCache(outp));
+
+    BOOST_CHECK(all_caches[1]->SpendCoin(outp));
+    flush_all(/*erase=*/ false);
+    BOOST_CHECK(!base.HaveCoin(outp));
+    BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
+    BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+
+    flush_all(/*erase=*/ true); // Erase all cache content.
+
+    // --- Bonus check 2: ensure that a FRESH, spent coin is deleted by Sync()
+    //
+    txid = InsecureRand256();
+    outp = COutPoint(txid, 0);
+    coin = MakeCoin();
+    CAmount coin_val = coin.out.nValue;
+    BOOST_CHECK(!base.HaveCoin(outp));
+    BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
+    BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+
+    // Add and spend from same cache without flushing.
+    all_caches[0]->AddCoin(outp, std::move(coin), false);
+
+    // Coin should be FRESH in the cache.
+    GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp);
+    BOOST_CHECK_EQUAL(value, coin_val);
+    BOOST_CHECK_EQUAL(flags, DIRTY|FRESH);
+
+    // Base shouldn't have seen coin.
+    BOOST_CHECK(!base.HaveCoin(outp));
+
+    BOOST_CHECK(all_caches[0]->SpendCoin(outp));
+    all_caches[0]->Sync();
+
+    // Ensure there is no sign of the coin after spend/flush.
+    GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp);
+    BOOST_CHECK_EQUAL(value, ABSENT);
+    BOOST_CHECK_EQUAL(flags, NO_ENTRY);
+    BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp));
+    BOOST_CHECK(!base.HaveCoin(outp));
+}
+
+BOOST_AUTO_TEST_CASE(ccoins_flush_behavior)
+{
+    // Create two in-memory caches atop a leveldb view.
+    CCoinsViewDB base{"test", /*nCacheSize=*/ 1 << 23, /*fMemory=*/ true, /*fWipe=*/ false};
+    std::vector<CCoinsViewCacheTest*> caches;
+    caches.push_back(new CCoinsViewCacheTest(&base));
+    caches.push_back(new CCoinsViewCacheTest(caches.back()));
+
+    for (CCoinsViewCacheTest* view : caches) {
+        TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ false);
+        TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ true);
+    }
+
+    // Clean up the caches.
+    while (caches.size() > 0) {
+        delete caches.back();
+        caches.pop_back();
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()