From 58b87ed97a95228ae3e41406b62d3c24ea89d2f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sun, 20 Apr 2025 15:30:14 +0200 Subject: [PATCH 1/6] test: drop leftover UBSan suppressions from `CCoinsViewCache` --- test/sanitizer_suppressions/ubsan | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index acdae7415d2..bfe7e45d188 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -46,9 +46,6 @@ unsigned-integer-overflow:CRollingBloomFilter::insert unsigned-integer-overflow:RollingBloomHash unsigned-integer-overflow:CCoinsViewCache::AddCoin unsigned-integer-overflow:CCoinsViewCache::BatchWrite -unsigned-integer-overflow:CCoinsViewCache::DynamicMemoryUsage -unsigned-integer-overflow:CCoinsViewCache::SpendCoin -unsigned-integer-overflow:CCoinsViewCache::Uncache unsigned-integer-overflow:CompressAmount unsigned-integer-overflow:DecompressAmount unsigned-integer-overflow:crypto/ From 8d11c5b383ddaec9e52343eba9c226ce18bdcac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 18 Apr 2025 23:21:02 +0200 Subject: [PATCH 2/6] =?UTF-8?q?coins:=20check=20unspent=E2=80=91overwrite?= =?UTF-8?q?=20before=20`cachedCoinsUsage`=20change=20in=20`AddCoin`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/coins.cpp | 6 +++--- test/sanitizer_suppressions/ubsan | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..ed910ea6578 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -76,9 +76,6 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi bool inserted; std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>()); bool fresh = false; - if (!inserted) { - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); - } if (!possible_overwrite) { if (!it->second.coin.IsSpent()) { throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)"); @@ -98,6 +95,9 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi // DIRTY, then it can be marked FRESH. fresh = !it->second.IsDirty(); } + if (!inserted) { + cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + } it->second.coin = std::move(coin); CCoinsCacheEntry::SetDirty(*it, m_sentinel); if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index bfe7e45d188..47c33f02a3d 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -44,7 +44,6 @@ unsigned-integer-overflow:arith_uint256.h unsigned-integer-overflow:CBloomFilter::Hash unsigned-integer-overflow:CRollingBloomFilter::insert unsigned-integer-overflow:RollingBloomHash -unsigned-integer-overflow:CCoinsViewCache::AddCoin unsigned-integer-overflow:CCoinsViewCache::BatchWrite unsigned-integer-overflow:CompressAmount unsigned-integer-overflow:DecompressAmount From ba38dad50266c7830f19c08f81f816a7919c1f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 18 Apr 2025 22:02:08 +0200 Subject: [PATCH 3/6] coins: assert just-created-coin has no dynamic memory usage This was added for the two branches to be symmetric and to explain why this branch is missing a subtraction. Co-authored-by: Andrew Toth --- src/coins.cpp | 1 + test/sanitizer_suppressions/ubsan | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coins.cpp b/src/coins.cpp index ed910ea6578..bbab0fb2eac 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -195,6 +195,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha // and mark it as dirty. itUs = cacheCoins.try_emplace(it->first).first; CCoinsCacheEntry& entry{itUs->second}; + assert(entry.coin.DynamicMemoryUsage() == 0); if (cursor.WillErase(*it)) { // Since this entry will be erased, // we can move the coin into us instead of copying it diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 47c33f02a3d..10ca1f084bd 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -44,7 +44,6 @@ unsigned-integer-overflow:arith_uint256.h unsigned-integer-overflow:CBloomFilter::Hash unsigned-integer-overflow:CRollingBloomFilter::insert unsigned-integer-overflow:RollingBloomHash -unsigned-integer-overflow:CCoinsViewCache::BatchWrite unsigned-integer-overflow:CompressAmount unsigned-integer-overflow:DecompressAmount unsigned-integer-overflow:crypto/ From 6210b702912504a9ca3cb9b78ab540ce9745ed9b 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 4/6] coins: spent erase in `CoinsViewCacheCursor` must not change usage Assert that spent coins already have zero dynamic size, and remove unused leftover counter --- 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 bbab0fb2eac..8e3afc7ff62 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 61fb4af6420..ed5c2e2f0fc 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 c46144b34b4..bc46de15adf 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -651,8 +651,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 9c6aa6e7a1e..27fa44e064d 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -122,7 +122,6 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) [&] { 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) @@ -143,11 +142,10 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) 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)}; coins_view_cache.BatchWrite(cursor, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); expected_code_path = true; } catch (const std::logic_error& e) { From 79bd941da6105080fb52fe2c1a2b1995ac8201ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 18 Apr 2025 11:23:27 +0200 Subject: [PATCH 5/6] coins: reset `cachedCoinsUsage` in Flush() only after the map is cleared Prevents the counter from dropping to 0 while cacheCoins still holds objects --- src/coins.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coins.cpp b/src/coins.cpp index 8e3afc7ff62..42e83dab8c3 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -254,8 +254,8 @@ bool CCoinsViewCache::Flush() { if (fOk) { cacheCoins.clear(); ReallocateCache(); + cachedCoinsUsage = 0; } - cachedCoinsUsage = 0; return fOk; } From 763be0ac98338d023d12749387b4ab1cf792069a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 18 Apr 2025 11:24:30 +0200 Subject: [PATCH 6/6] coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` inserts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guarantees counter stays balanced both on insert and on in‑place replacement. Note that this is currently only called from AssumeUTXO code where it should only insert. Co-authored-by: Andrew Toth --- src/coins.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 42e83dab8c3..bf858791fdc 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -111,9 +111,11 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi } void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { - cachedCoinsUsage += coin.DynamicMemoryUsage(); auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin)); - if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel); + if (inserted) { + CCoinsCacheEntry::SetDirty(*it, m_sentinel); + cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); + } } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {