laantungir
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

coins: fix `cachedCoinsUsage` accounting to prevent underflow 

Move the `cachedCoinsUsage` subtraction in `AddCoin()` after the `possible_overwrite` check, because throwing before the assignment used to decrement the counter without changing the entry, which corrupted the accounting and caused underflow in later operations.
In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared (only happening in tests currently since `BatchWrite` always returns `true` in production code), so failed flushes leave the map and its accounting in sync.

With these changes the counter remains consistent across success and exception paths, so drop the temporary underflow guards added earlier and finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This commit is contained in:
Lőrinc 2025-04-18 11:23:27 +02:00
parent bf1f9907d4
commit 33a8150315
2 changed files with 4 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
src/coins.cpp
View File

@ -76,10 +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) {
Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
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)");
@ -99,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);
@ -134,7 +133,6 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool
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(),
@ -228,12 +226,10 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
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,
@ -261,8 +257,8 @@ bool CCoinsViewCache::Flush() {
if (fOk) {
cacheCoins.clear();
ReallocateCache();
cachedCoinsUsage = 0;
}
cachedCoinsUsage = 0;
return fOk;
}
@ -283,7 +279,6 @@ void CCoinsViewCache::Uncache(const COutPoint& hash)
{
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(),

5
test/sanitizer_suppressions/ubsan
View File

@ -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/