cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-31 10:41:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#32313: coins: fix cachedCoinsUsage accounting in CCoinsViewCache

Browse Source 
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
This commit is contained in:
merge-script 2025-10-15 09:48:04 -04:00
commit f76e1ae389
No known key found for this signature in database
GPG Key ID: BA03F4DBE0C63FB4
5 changed files with 66 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/coins.cpp
View File

@ -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);
@ -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) {
@ -195,6 +198,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
@ -248,19 +252,19 @@ 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();
ReallocateCache();
cachedCoinsUsage = 0;
}
cachedCoinsUsage = 0;
return fOk;
}
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) {

12
src/coins.h
View File

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

40
src/test/coins_tests.cpp
View File

@ -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, {}));
}
@ -1085,4 +1085,40 @@ 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_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()

30
src/test/fuzz/coins_view.cpp
View File

@ -59,25 +59,19 @@ 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();
try {
coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite);
expected_code_path = true;
} catch (const std::logic_error& e) {
if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) {
COutPoint outpoint{random_out_point};
Coin coin{random_coin};
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);
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();
}
} else {
coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));
}
assert(expected_code_path);
},
[&] {
(void)coins_view_cache.Flush();
@ -131,7 +125,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 +145,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().

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/