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
cachedCoinsUsagebefore thepossible_overwritevalidation - 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
cachedCoinsUsageto 0, even whenBatchWrite()failed - Left the map populated while the counter read zero
- Impact: Test-only (production
BatchWrite()returnstrue), but broke accounting consistency
3. Cursor redundant usage tracking
CoinsViewCacheCursor::NextAndMaybeErase()subtracted usage when erasing spent entries- However,
SpendCoin()already decremented and cleared thescriptPubKey, leavingDynamicMemoryUsage()at 0 - Impact: Redundant code that obscured actual accounting behavior
4. EmplaceCoinInternalDANGER() double-counting
- Incremented
cachedCoinsUsageeven whentry_emplacedid 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:
0MAKEJOBS="-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.
0bool CCoinsViewCache::CacheUsageValid() const
1{
2 size_t actual{0};
3 for (auto& entry : cacheCoins | std::views::values) actual += entry.coin.DynamicMemoryUsage();
4 return actual == cachedCoinsUsage;
5}
or
0diff --git a/src/coins.cpp b/src/coins.cpp
1--- a/src/coins.cpp (revision fd3b1a7f4bb2ac527f23d4eb4cfa40a3215906e5)
2+++ b/src/coins.cpp (revision 872a05633bfdbd06ad82190d7fe34b42d13ebfe9)
3@@ -96,6 +96,7 @@
4 fresh = !it->second.IsDirty();
5 }
6 if (!inserted) {
7+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
8 cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
9 }
10 it->second.coin = std::move(coin);
11@@ -133,6 +134,7 @@
12 bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
13 CCoinsMap::iterator it = FetchCoin(outpoint);
14 if (it == cacheCoins.end()) return false;
15+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
16 cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
17 TRACEPOINT(utxocache, spent,
18 outpoint.hash.data(),
19@@ -226,10 +228,12 @@
20 if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
21 // The grandparent cache does not have an entry, and the coin
22 // has been spent. We can just delete it from the parent cache.
23+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
24 cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
25 cacheCoins.erase(itUs);
26 } else {
27 // A normal modification.
28+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
29 cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
30 if (cursor.WillErase(*it)) {
31 // Since this entry will be erased,
32@@ -279,6 +283,7 @@
33 {
34 CCoinsMap::iterator it = cacheCoins.find(hash);
35 if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
36+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
37 cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
38 TRACEPOINT(utxocache, uncache,
39 hash.hash.data(),
