From 67cff8bec9094e968f36d351fb2e38c9bf563757 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 1/4] refactor: assert newly-created parent cache entry has zero memory usage During `BatchWrite`, the parent entry is created under a guard that guarantees insertion, so the new `Coin` is default-constructed and empty. Assert this invariant to document why there is no `cachedCoinsUsage` decrement before the assignment at this site. Co-authored-by: Andrew Toth --- src/coins.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..59c7d67c445 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 From 39cf8bb3d0d9ee84544d161bf66d90d5e2a1a140 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 2/4] 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(). From d7c9d6c2914aadd711544908d0fad8857a809c72 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 3/4] coins: fix `cachedCoinsUsage` accounting to prevent underflow Move the `cachedCoinsUsage` subtract in `AddCoin()` to after the `possible_overwrite` check. Previously a throw before assignment decremented the counter without changing the entry, which corrupted accounting and later underflowed. In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared. In production `BatchWrite()` returns `true`, so this mostly affects tests. On failure, leave the counter unchanged to keep it in sync with the cache. The existing `Flush()` workaround in fuzzing was also removed now that the source of the problem was fixed, so the fuzzer no longer needs `coins_view_cache.Flush()` to realign `cachedCoinsUsage` after an exception. Replace the prior `expected_code_path` tracking with direct assertions. The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed. With these changes the counter stays consistent across success and exception paths, so we can finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue. Included a unit test as well, attempting to add a different coin to the same outpoint without allowing overwrites and make sure it throws. We use `SelfTest()` to validates accounting, and check that the cache remains usable. Co-authored-by: Ryan Ofsky Co-authored-by: w0xlt --- src/coins.cpp | 8 ++++---- src/test/coins_tests.cpp | 18 ++++++++++++++++++ src/test/fuzz/coins_view.cpp | 22 ++++++---------------- test/sanitizer_suppressions/ubsan | 5 ----- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 28408481e46..42e83dab8c3 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); @@ -254,8 +254,8 @@ bool CCoinsViewCache::Flush() { if (fOk) { cacheCoins.clear(); ReallocateCache(); + cachedCoinsUsage = 0; } - cachedCoinsUsage = 0; return fOk; } diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index f3926ef4044..46b1e2eb955 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -1085,4 +1085,22 @@ BOOST_AUTO_TEST_CASE(coins_resource_is_used) PoolResourceTester::CheckAllDataAccountedFor(resource); } +BOOST_AUTO_TEST_CASE(ccoins_addcoin_exception_keeps_usage_balanced) +{ + CCoinsView root; + CCoinsViewCacheTest cache{&root}; + + const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()}; + + const Coin coin1{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false}; + cache.AddCoin(outpoint, Coin{coin1}, /*possible_overwrite=*/false); + cache.SelfTest(); + + const Coin coin2{CTxOut{m_rng.randrange(20), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 2)}, 2, false}; + BOOST_CHECK_THROW(cache.AddCoin(outpoint, Coin{coin2}, /*possible_overwrite=*/false), std::logic_error); + cache.SelfTest(); + + BOOST_CHECK(cache.AccessCoin(outpoint) == coin1); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index e8ba655e54c..2b3557fffed 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -59,25 +59,15 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend if (random_coin.IsSpent()) { return; } - Coin coin = random_coin; - bool expected_code_path = false; - const bool possible_overwrite = fuzzed_data_provider.ConsumeBool(); + COutPoint outpoint{random_out_point}; + Coin coin{random_coin}; + const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()}; try { - coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite); - expected_code_path = true; + coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite); } catch (const std::logic_error& e) { - if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) { - assert(!possible_overwrite); - expected_code_path = true; - // AddCoin() decreases cachedCoinsUsage by the memory usage of the old coin at the beginning and - // increases it by the value of the new coin at the end. If it throws in the process, the value - // of cachedCoinsUsage would have been incorrectly decreased, leading to an underflow later on. - // To avoid this, use Flush() to reset the value of cachedCoinsUsage in sync with the cacheCoins - // mapping. - (void)coins_view_cache.Flush(); - } + assert(e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}); + assert(!possible_overwrite); } - assert(expected_code_path); }, [&] { (void)coins_view_cache.Flush(); diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 77aed1aa8a1..0151f9d0253 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -47,11 +47,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: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 24d861da7894add47747eff69dd3fc71fbcdd7d0 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 4/4] coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert `EmplaceCoinInternalDANGER()` incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key), inflating the counter. This is mostly reachable in tests today since `AssumeUTXO` does not overwrite. Increment only on successful insert, and capture `coin.DynamicMemoryUsage()` before the move so accounting uses the correct value. Fuzz: add an `EmplaceCoinInternalDANGER` path to exercise insert-only accounting. Unit test: emplace two different coins at the same outpoint (with different `DynamicMemoryUsage()`), verify `SelfTest()` passes and `AccessCoin(outpoint)` returns the first coin. Co-authored-by: Andrew Toth Co-authored-by: w0xlt --- src/coins.cpp | 7 +++++-- src/test/coins_tests.cpp | 18 ++++++++++++++++++ src/test/fuzz/coins_view.cpp | 16 ++++++++++------ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 42e83dab8c3..090d36dd041 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -111,9 +111,12 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi } void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { - cachedCoinsUsage += coin.DynamicMemoryUsage(); + const auto mem_usage{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 += mem_usage; + } } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 46b1e2eb955..a1152d245f7 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -1103,4 +1103,22 @@ BOOST_AUTO_TEST_CASE(ccoins_addcoin_exception_keeps_usage_balanced) BOOST_CHECK(cache.AccessCoin(outpoint) == coin1); } +BOOST_AUTO_TEST_CASE(ccoins_emplace_duplicate_keeps_usage_balanced) +{ + CCoinsView root; + CCoinsViewCacheTest cache{&root}; + + const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()}; + + const Coin coin1{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false}; + cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin1}); + cache.SelfTest(); + + const Coin coin2{CTxOut{m_rng.randrange(20), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 2)}, 2, false}; + cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin2}); + cache.SelfTest(); + + BOOST_CHECK(cache.AccessCoin(outpoint) == coin1); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 2b3557fffed..c687065423a 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -61,12 +61,16 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend } COutPoint outpoint{random_out_point}; Coin coin{random_coin}; - const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()}; - try { - coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite); - } catch (const std::logic_error& e) { - assert(e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}); - assert(!possible_overwrite); + if (fuzzed_data_provider.ConsumeBool()) { + const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()}; + try { + coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite); + } catch (const std::logic_error& e) { + assert(e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}); + assert(!possible_overwrite); + } + } else { + coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin)); } }, [&] {