Merge bitcoin/bitcoin#33602: [IBD] coins: reduce lookups in dbcache layer propagation

0ac969cddf validation: don't reallocate cache for short-lived CCoinsViewCache (Lőrinc) c8f5e446dc coins: reduce lookups in dbcache layer propagation (Lőrinc) Pull request description: This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043) ### Summary Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path. On a different path, these caches were recreated needlessly for every block connection. ### Fix for double fetch This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (not used in production, only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations. This change is a minimal version of [bitcoin/bitcoin@`723c49b` (#32128)](723c49b63b) and draws simplification ideas [bitcoin/bitcoin@`ae76ec7` (#30673)](ae76ec7bcf) and https://github.com/bitcoin/bitcoin/pull/30326. ### Fix for temporary cache recreation Related to parent cache propagation, the second commit makes it possible to avoid destructuring-recreating-destructuring of these short-live parent caches created for each new block. A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway. This change was based on a subset of https://github.com/bitcoin/bitcoin/pull/28945, the original authors and relevant commenters were added as coauthors to this version. ----- Reindex-chainstate indicates ~1% speedup. <details> <summary>Details</summary> ```python COMMITS="647cdb4f7e8041affed887e2325ee03a91078bb1 0b0c3293ffd75afb27dadc0b28426b40132a8c6b"; \ STOP=909090; DBCACHE=4500; \ CC=gcc; CXX=g++; \ BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \ (echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \ hyperfine \ --sort command \ --runs 2 \ --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \ --parameter-list COMMIT ${COMMITS// /,} \ --prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind && \ ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20" \ --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \ "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0" 647cdb4f7e Merge bitcoin/bitcoin#33311: net: Quiet down logging when router doesn't support natpmp/pcp 0b0c3293ff validation: don't reallocate cache for short-lived CCoinsViewCache Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e) Time (mean ± σ): 16233.508 s ± 9.501 s [User: 19064.578 s, System: 951.672 s] Range (min … max): 16226.790 s … 16240.226 s 2 runs Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b) Time (mean ± σ): 16039.626 s ± 17.284 s [User: 18870.130 s, System: 950.722 s] Range (min … max): 16027.405 s … 16051.848 s 2 runs Relative speed comparison 1.01 ± 0.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e) 1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b) ``` </details> ACKs for top commit: optout21: utACK 0ac969cddf achow101: ACK 0ac969cddf andrewtoth: utACK 0ac969cddf sedited: ACK 0ac969cddf Tree-SHA512: 9fcc3f1a8314368576a4fba96ca72665527eaa3a97964ab5b39491757f3527147d134f79a5c3456f76c1330c7ef862989d23f764236c5e2563be89a81c1cee47
2026-01-19 23:03:45 +01:00 · 2025-12-10 15:02:25 -08:00
parent c1f0a89d9c 0ac969cddf
commit c2975f26d6
5 changed files with 20 additions and 26 deletions
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -185,18 +185,16 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) {

 bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) {
    for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {
-        // Ignore non-dirty entries (optimization).
-        if (!it->second.IsDirty()) {
+        if (!it->second.IsDirty()) { // TODO a cursor can only contain dirty entries
            continue;
        }
-        CCoinsMap::iterator itUs = cacheCoins.find(it->first);
-        if (itUs == cacheCoins.end()) {
-            // The parent cache does not have an entry, while the child cache does.
-            // We can ignore it if it's both spent and FRESH in the child
-            if (!(it->second.IsFresh() && it->second.coin.IsSpent())) {
-                // Create the coin in the parent cache, move the data up
-                // and mark it as dirty.
-                itUs = cacheCoins.try_emplace(it->first).first;
+        auto [itUs, inserted]{cacheCoins.try_emplace(it->first)};
+        if (inserted) {
+            if (it->second.IsFresh() && it->second.coin.IsSpent()) {
+                cacheCoins.erase(itUs); // TODO fresh coins should have been removed at spend
+            } else {
+                // The parent cache does not have an entry, while the child cache does.
+                // Move the data up and mark it as dirty.
                CCoinsCacheEntry& entry{itUs->second};
                assert(entry.coin.DynamicMemoryUsage() == 0);
                if (cursor.WillErase(*it)) {
@@ -251,12 +249,14 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
    return true;
 }

-bool CCoinsViewCache::Flush() {
+bool CCoinsViewCache::Flush(bool will_reuse_cache) {
    auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/true)};
    bool fOk = base->BatchWrite(cursor, hashBlock);
    if (fOk) {
        cacheCoins.clear();
-        ReallocateCache();
+        if (will_reuse_cache) {
+            ReallocateCache();
+        }
        cachedCoinsUsage = 0;
    }
    return fOk;
--- a/src/coins.h
+++ b/src/coins.h
@@ -439,9 +439,11 @@ public:
     * Push the modifications applied to this cache to its base and wipe local state.
     * Failure to call this method or Sync() before destruction will cause the changes
     * to be forgotten.
+     * If will_reuse_cache is false, the cache will retain the same memory footprint
+     * after flushing and should be destroyed to deallocate.
     * If false is returned, the state of this cache (and its backing view) will be undefined.
     */
-    bool Flush();
+    bool Flush(bool will_reuse_cache = true);

    /**
     * Push the modifications applied to this cache to its base while retaining
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -74,7 +74,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
                }
            },
            [&] {
-                (void)coins_view_cache.Flush();
+                (void)coins_view_cache.Flush(/*will_reuse_cache=*/fuzzed_data_provider.ConsumeBool());
            },
            [&] {
                (void)coins_view_cache.Sync();
--- a/src/test/fuzz/coinscache_sim.cpp
+++ b/src/test/fuzz/coinscache_sim.cpp
@@ -392,7 +392,7 @@ FUZZ_TARGET(coinscache_sim)
                // Apply to simulation data.
                flush();
                // Apply to real caches.
-                caches.back()->Flush();
+                caches.back()->Flush(/*will_reuse_cache=*/provider.ConsumeBool());
            },

            [&]() { // Sync.
@@ -402,14 +402,6 @@ FUZZ_TARGET(coinscache_sim)
                caches.back()->Sync();
            },

-            [&]() { // Flush + ReallocateCache.
-                // Apply to simulation data.
-                flush();
-                // Apply to real caches.
-                caches.back()->Flush();
-                caches.back()->ReallocateCache();
-            },
-
            [&]() { // GetCacheSize
                (void)caches.back()->GetCacheSize();
            },
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2966,7 +2966,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
            LogError("DisconnectTip(): DisconnectBlock %s failed\n", pindexDelete->GetBlockHash().ToString());
            return false;
        }
-        bool flushed = view.Flush();
+        bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope
        assert(flushed);
    }
    LogDebug(BCLog::BENCH, "- Disconnect block: %.2fms\n",
@@ -3101,7 +3101,7 @@ bool Chainstate::ConnectTip(
                 Ticks<MillisecondsDouble>(time_3 - time_2),
                 Ticks<SecondsDouble>(m_chainman.time_connect_total),
                 Ticks<MillisecondsDouble>(m_chainman.time_connect_total) / m_chainman.num_blocks_total);
-        bool flushed = view.Flush();
+        bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope
        assert(flushed);
    }
    const auto time_4{SteadyClock::now()};
@@ -4895,7 +4895,7 @@ bool Chainstate::ReplayBlocks()
    }

    cache.SetBestBlock(pindexNew->GetBlockHash());
-    cache.Flush();
+    cache.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope
    m_chainman.GetNotifications().progress(bilingual_str{}, 100, false);
    return true;
 }