mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-01-20 07:09:15 +01:00
f76e1ae389
24d861da78coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert (Lőrinc)d7c9d6c291coins: fix `cachedCoinsUsage` accounting to prevent underflow (Lőrinc)39cf8bb3d0refactor: remove redundant usage tracking from `CoinsViewCacheCursor` (Lőrinc)67cff8bec9refactor: assert newly-created parent cache entry has zero memory usage (Lőrinc) Pull request description: ### Summary This PR fixes `cachedCoinsUsage` accounting bugs in `CCoinsViewCache` that caused UBSan `unsigned-integer-overflow` violations during testing. The issues stemmed from incorrect decrement timing in `AddCoin()`, unconditional reset in `Flush()` on failure, and incorrect increment in `EmplaceCoinInternalDANGER()` when insertion fails. ### Problems Fixed **1. `AddCoin()` underflow on exception** - Previously decremented `cachedCoinsUsage` *before* the `possible_overwrite` validation - If validation threw, the map entry remained unchanged but counter was decremented - This corrupted accounting and later caused underflow - **Impact**: Test-only in current codebase, but unsound accounting that could affect future changes **2. `Flush()` accounting drift on failure** - Unconditionally reset `cachedCoinsUsage` to 0, even when `BatchWrite()` failed - Left the map populated while the counter read zero - **Impact**: Test-only (production `BatchWrite()` returns `true`), but broke accounting consistency **3. Cursor redundant usage tracking** - `CoinsViewCacheCursor::NextAndMaybeErase()` subtracted usage when erasing spent entries - However, `SpendCoin()` already decremented and cleared the `scriptPubKey`, leaving `DynamicMemoryUsage()` at 0 - **Impact**: Redundant code that obscured actual accounting behavior **4. `EmplaceCoinInternalDANGER()` double-counting** - Incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key) - Inflated the counter on duplicate attempts - **Impact**: Mostly test-reachable (AssumeUTXO doesn't overwrite in production), but incorrect accounting ### Testing To reproduce the historical UBSan failures on the referenced baseline and to verify the fix, run: ``` MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh ``` The change was tested with the related unit and fuzz test, and asserted before/after each `cachedCoinsUsage` change (in production code and fuzz) that the calculations are still correct by recalculating them from scratch. <details> <summary>Details</summary> ```C++ bool CCoinsViewCache::CacheUsageValid() const { size_t actual{0}; for (auto& entry : cacheCoins | std::views::values) actual += entry.coin.DynamicMemoryUsage(); return actual == cachedCoinsUsage; } ``` or ```patch diff --git a/src/coins.cpp b/src/coins.cpp --- a/src/coins.cpp(revision fd3b1a7f4bb2ac527f23d4eb4cfa40a3215906e5) +++ b/src/coins.cpp(revision 872a05633bfdbd06ad82190d7fe34b42d13ebfe9) @@ -96,6 +96,7 @@ fresh = !it->second.IsDirty(); } if (!inserted) { + Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage()); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); } it->second.coin = std::move(coin); @@ -133,6 +134,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { CCoinsMap::iterator it = FetchCoin(outpoint); if (it == cacheCoins.end()) return false; + Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage()); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, spent, outpoint.hash.data(), @@ -226,10 +228,12 @@ 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. + Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage()); cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); cacheCoins.erase(itUs); } else { // A normal modification. + Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage()); cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); if (cursor.WillErase(*it)) { // Since this entry will be erased, @@ -279,6 +283,7 @@ { CCoinsMap::iterator it = cacheCoins.find(hash); if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) { + Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage()); cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, uncache, hash.hash.data(), ``` </details> ACKs for top commit: optout21: reACK24d861da78andrewtoth: ACK24d861da78sipa: ACK24d861da78w0xlt: ACK24d861da78Tree-SHA512: ff1b756b46220f278ab6c850626a0f376bed64389ef7f66a95c994e1c7cceec1d1843d2b24e8deabe10e2bdade2a274d9654ac60eb2b9bf471a71db8a2ff496c