mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-02-21 12:59:12 +00:00
f76e1ae389
24d861da7894add47747eff69dd3fc71fbcdd7d0 coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert (Lőrinc)
d7c9d6c2914aadd711544908d0fad8857a809c72 coins: fix `cachedCoinsUsage` accounting to prevent underflow (Lőrinc)
39cf8bb3d0d9ee84544d161bf66d90d5e2a1a140 refactor: remove redundant usage tracking from `CoinsViewCacheCursor` (Lőrinc)
67cff8bec9094e968f36d351fb2e38c9bf563757 refactor: 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:
reACK 24d861da7894add47747eff69dd3fc71fbcdd7d0
andrewtoth:
ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0
sipa:
ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0
w0xlt:
ACK 24d861da78
Tree-SHA512: ff1b756b46220f278ab6c850626a0f376bed64389ef7f66a95c994e1c7cceec1d1843d2b24e8deabe10e2bdade2a274d9654ac60eb2b9bf471a71db8a2ff496c