From 5c2f073e1e09218ea2e68853b5b857a6ad354a6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sun, 20 Apr 2025 16:47:51 +0200 Subject: [PATCH] refactor: remove redundant usage tracking from `CoinsViewCacheCursor` When a coin is spent via `SpendCoin()`, `cachedCoinsUsage` is already decremented and the coin's `scriptPubKey` is cleared, so `DynamicMemoryUsage()` is `0`. `CoinsViewCacheCursor::NextAndMaybeErase()` was subtracting usage again when erasing spent entries. Replace it with an assert that documents spent coins have zero dynamic memory usage by the time the cursor encounters them. Remove the now-unnecessary `usage` reference from the cursor's constructor and member variables. --- src/coins.cpp | 4 ++-- src/coins.h | 12 +++++------- src/test/coins_tests.cpp | 4 ++-- src/test/fuzz/coins_view.cpp | 4 +--- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 59c7d67c445..28408481e46 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -249,7 +249,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha } bool CCoinsViewCache::Flush() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/true)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { cacheCoins.clear(); @@ -261,7 +261,7 @@ bool CCoinsViewCache::Flush() { bool CCoinsViewCache::Sync() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/false)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { if (m_sentinel.second.Next() != &m_sentinel) { diff --git a/src/coins.h b/src/coins.h index 6725d5a51f6..2fcc764a3fd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -271,11 +271,10 @@ struct CoinsViewCacheCursor //! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set. //! Calling CCoinsMap::clear() afterwards is faster because a CoinsCachePair cannot be coerced back into a //! CCoinsMap::iterator to be erased, and must therefore be looked up again by key in the CCoinsMap before being erased. - CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND, - CoinsCachePair& sentinel LIFETIMEBOUND, - CCoinsMap& map LIFETIMEBOUND, - bool will_erase) noexcept - : m_usage(usage), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + CoinsViewCacheCursor(CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } inline CoinsCachePair* End() const noexcept { return &m_sentinel; } @@ -288,7 +287,7 @@ struct CoinsViewCacheCursor // Otherwise, clear the state of the entry. if (!m_will_erase) { if (current.second.coin.IsSpent()) { - m_usage -= current.second.coin.DynamicMemoryUsage(); + assert(current.second.coin.DynamicMemoryUsage() == 0); // scriptPubKey was already cleared in SpendCoin m_map.erase(current.first); } else { current.second.SetClean(); @@ -299,7 +298,6 @@ struct CoinsViewCacheCursor inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } private: - size_t& m_usage; CoinsCachePair& m_sentinel; CCoinsMap& m_map; bool m_will_erase; diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 6ce0c7996f8..f3926ef4044 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -662,8 +662,8 @@ static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - auto usage{cache_coin ? InsertCoinsMapEntry(map, sentinel, *cache_coin) : 0}; - auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; + if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_coin); + auto cursor{CoinsViewCacheCursor(sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index dacef9a70ad..e8ba655e54c 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -131,7 +131,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend [&] { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); - size_t usage{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) @@ -152,11 +151,10 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); - usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; try { - auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(sentinel, coins_map, /*will_erase=*/true)}; uint256 best_block{coins_view_cache.GetBestBlock()}; if (fuzzed_data_provider.ConsumeBool()) best_block = ConsumeUInt256(fuzzed_data_provider); // Set best block hash to non-null to satisfy the assertion in CCoinsViewDB::BatchWrite().