From 649cd7a1391a14d03103cfe91a0fa72199ffe2d6 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 21 Jan 2025 14:02:10 -0500 Subject: [PATCH] coins: Keep track of number of dirty entries in `CCoinsViewCache` Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration * Validated in `SanityCheck()` by recomputing from scratch The dirty count is needed because after non-wiping flushes (introduced in #28280 and #28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc --- src/coins.cpp | 26 ++++++++++++++++++++++---- src/coins.h | 16 ++++++++++++---- src/test/coins_tests.cpp | 9 +++++++-- src/test/fuzz/coins_view.cpp | 4 +++- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 2c91063cb31..2e6ac15a5e4 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -96,10 +96,12 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi fresh = !it->second.IsDirty(); } if (!inserted) { + m_dirty_count -= it->second.IsDirty(); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); } it->second.coin = std::move(coin); CCoinsCacheEntry::SetDirty(*it, m_sentinel); + ++m_dirty_count; if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, add, @@ -115,6 +117,7 @@ void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coi auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin)); if (inserted) { CCoinsCacheEntry::SetDirty(*it, m_sentinel); + ++m_dirty_count; cachedCoinsUsage += mem_usage; } } @@ -133,6 +136,7 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { CCoinsMap::iterator it = FetchCoin(outpoint); if (it == cacheCoins.end()) return false; + m_dirty_count -= it->second.IsDirty(); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, spent, outpoint.hash.data(), @@ -147,6 +151,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { cacheCoins.erase(it); } else { CCoinsCacheEntry::SetDirty(*it, m_sentinel); + ++m_dirty_count; it->second.coin.Clear(); } return true; @@ -205,8 +210,9 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha } else { entry.coin = it->second.coin; } - cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + ++m_dirty_count; + cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); // We can mark it FRESH in the parent if it was FRESH in the child // Otherwise it might have just been flushed from the parent's cache // and already exist in the grandparent @@ -225,6 +231,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha if (itUs->second.IsFresh() && it->second.coin.IsSpent()) { // The grandparent cache does not have an entry, and the coin // has been spent. We can just delete it from the parent cache. + m_dirty_count -= itUs->second.IsDirty(); cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); cacheCoins.erase(itUs); } else { @@ -238,7 +245,10 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + if (!itUs->second.IsDirty()) { + CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + ++m_dirty_count; + } // NOTE: It isn't safe to mark the coin as FRESH in the parent // cache. If it already existed and was spent in the parent // cache then marking it FRESH would prevent that spentness @@ -251,21 +261,23 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha } bool CCoinsViewCache::Flush() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/true)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { cacheCoins.clear(); ReallocateCache(); } cachedCoinsUsage = 0; + m_dirty_count = 0; return fOk; } bool CCoinsViewCache::Sync() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/false)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { + Assume(m_dirty_count == 0); if (m_sentinel.second.Next() != &m_sentinel) { /* BatchWrite must clear flags of all entries */ throw std::logic_error("Not all unspent flagged entries were cleared"); @@ -309,6 +321,7 @@ void CCoinsViewCache::ReallocateCache() { // Cache should be empty when we're calling this. assert(cacheCoins.size() == 0); + Assume(m_dirty_count == 0); cacheCoins.~CCoinsMap(); m_cache_coins_memory_resource.~CCoinsMapMemoryResource(); ::new (&m_cache_coins_memory_resource) CCoinsMapMemoryResource{}; @@ -319,6 +332,7 @@ void CCoinsViewCache::SanityCheck() const { size_t recomputed_usage = 0; size_t count_flagged = 0; + size_t dirty_count = 0; for (const auto& [_, entry] : cacheCoins) { unsigned attr = 0; if (entry.IsDirty()) attr |= 1; @@ -330,6 +344,9 @@ void CCoinsViewCache::SanityCheck() const // Recompute cachedCoinsUsage. recomputed_usage += entry.coin.DynamicMemoryUsage(); + // Recompute dirty_count. + dirty_count += entry.IsDirty(); + // Count the number of entries we expect in the linked list. if (entry.IsDirty() || entry.IsFresh()) ++count_flagged; } @@ -346,6 +363,7 @@ void CCoinsViewCache::SanityCheck() const } assert(count_linked == count_flagged); assert(recomputed_usage == cachedCoinsUsage); + assert(dirty_count == m_dirty_count); } static const size_t MIN_TRANSACTION_OUTPUT_WEIGHT = WITNESS_SCALE_FACTOR * ::GetSerializeSize(CTxOut()); diff --git a/src/coins.h b/src/coins.h index 6725d5a51f6..9d12354f8f2 100644 --- a/src/coins.h +++ b/src/coins.h @@ -272,10 +272,11 @@ struct CoinsViewCacheCursor //! 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) {} + size_t& dirty_count LIFETIMEBOUND, + CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_usage(usage), m_dirty_count(dirty_count), 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; } @@ -284,6 +285,7 @@ struct CoinsViewCacheCursor inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept { const auto next_entry{current.second.Next()}; + m_dirty_count -= current.second.IsDirty(); // If we are not going to erase the cache, we must still erase spent entries. // Otherwise, clear the state of the entry. if (!m_will_erase) { @@ -300,6 +302,7 @@ struct CoinsViewCacheCursor inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } private: size_t& m_usage; + size_t& m_dirty_count; CoinsCachePair& m_sentinel; CCoinsMap& m_map; bool m_will_erase; @@ -377,6 +380,8 @@ protected: /* Cached dynamic memory usage for the inner Coin objects. */ mutable size_t cachedCoinsUsage{0}; + /* Running count of dirty Coin cache entries. */ + mutable size_t m_dirty_count{0}; public: CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false); @@ -463,6 +468,9 @@ public: //! Calculate the size of the cache (in number of transaction outputs) unsigned int GetCacheSize() const; + //! Calculate the number of dirty cache entries (transaction outputs) + size_t GetDirtyCount() const noexcept { return m_dirty_count; } + //! Calculate the size of the cache (in bytes) size_t DynamicMemoryUsage() const; diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index d66c62248bb..f6e61b67156 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -100,6 +100,7 @@ public: CCoinsMap& map() const { return cacheCoins; } CoinsCachePair& sentinel() const { return m_sentinel; } size_t& usage() const { return cachedCoinsUsage; } + size_t& dirty() const { return m_dirty_count; } }; } // namespace @@ -666,7 +667,8 @@ static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) 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)}; + size_t dirty_count{cache_coin && cache_coin->IsDirty() ? 1U : 0U}; + auto cursor{CoinsViewCacheCursor(usage, dirty_count, sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } @@ -677,7 +679,10 @@ public: { auto base_cache_coin{base_value == ABSENT ? MISSING : CoinEntry{base_value, CoinEntry::State::DIRTY}}; WriteCoinsViewEntry(base, base_cache_coin); - if (cache_coin) cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin); + if (cache_coin) { + cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin); + cache.dirty() += cache_coin->IsDirty(); + } } CCoinsView root; diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index dacef9a70ad..ef2320167b4 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -132,6 +132,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); size_t usage{0}; + size_t dirty_count{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) @@ -153,10 +154,11 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); usage += it->second.coin.DynamicMemoryUsage(); + dirty_count += dirty; } bool expected_code_path = false; try { - auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(usage, dirty_count, 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().