coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`

l0rinc commented at 5:33 pm on April 20, 2025: contributor

Split out of #32296 (comment) since this needed more complicated production code changes.

The changes ensure cachedCoinsUsage remains balanced throughout all coin operations and that the sanitizers will catch future violations.

The change was tested with the related fuzz test, and asserted before/after each cachedCoinsUsage change (in production code and fuzz) that the calculations are still correct by recalculating it 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}

This part wasn’t committed to the code.

DrahtBot commented at 5:33 pm on April 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32313.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32128 (Draft: CCoinMap Experiments by martinus)
#31132 (validation: fetch block inputs on parallel threads 10% faster IBD by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/coins.cpp:82 in 4bd4805217 outdated

75@@ -76,13 +76,13 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
76     bool inserted;
77     std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>());
78     bool fresh = false;
79+    if (!possible_overwrite && !it->second.coin.IsSpent()) { // check before any other partial state change
80+        throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)");
81+    }
82     if (!inserted) {

andrewtoth commented at 5:36 pm on April 20, 2025:

Would it be simpler to just move this if block below the if block that throws, instead of pulling the throw out?

l0rinc commented at 8:57 pm on April 20, 2025:

If you think that’s better, I don’t mind - done

in src/coins.cpp:115 in 4bd4805217 outdated

114-    cachedCoinsUsage += coin.DynamicMemoryUsage();
115-    auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
116-    if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
117+void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin)
118+{
119+    auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint));

andrewtoth commented at 5:41 pm on April 20, 2025:

I think this would be simpler if we kept the original behavior of moving the coin here as well, and then only increment the cachedCoinsUsage if the coin is inserted.

In practice it’s not possible to get !inserted unless the assume utxo payload was generated incorrectly.

l0rinc commented at 8:57 pm on April 20, 2025:

In practice it’s not possible to get !inserted unless the assume utxo payload was generated incorrectly.

As far as I can see that’s not a guarantee, but I’ve also noted it in the commit message that currently this is the case.

kept the original behavior of moving the coin here as well

Sure, if you think that’s better - changed!

in src/coins.cpp:204 in 4bd4805217 outdated

200@@ -195,6 +201,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha
201                 // and mark it as dirty.
202                 itUs = cacheCoins.try_emplace(it->first).first;
203                 CCoinsCacheEntry& entry{itUs->second};
204+                cachedCoinsUsage -= entry.coin.DynamicMemoryUsage();

andrewtoth commented at 5:43 pm on April 20, 2025:

The entry has an empty coin here, it was just created. So this is just subtracting zero.

l0rinc commented at 8:57 pm on April 20, 2025:

Thanks, I’ll add an assert in that case to explain why this branch wasn’t symmetric with the other one.

andrewtoth commented at 11:23 pm on May 5, 2025:

I don’t think we should add an assert here to explain. The comment right above says Create the coin in the parent cache.

l0rinc commented at 7:14 am on May 6, 2025:

Doesn’t the presence of the original cache-usage-update indicate that the comment isn’t enough?

andrewtoth commented at 2:05 pm on May 6, 2025:

Sorry, what is the original cache-usage-update? AFAICT this line was added in this PR, and there is no cache-usage-update here?

l0rinc commented at 2:30 pm on May 6, 2025:

I prefer code over comments (the comment can be wrong, the code validates) - and since the usual pattern is basically:

0cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
1if (cursor.WillErase(*it)) {
2    itUs->second.coin = std::move(it->second.coin);
3} else {
4    itUs->second.coin = it->second.coin;
5}
6cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();

it makes sense to explain (via code) why here we don’t need the first part:

0assert(entry.coin.DynamicMemoryUsage() == 0); // i.e. we didn't forget the symmetry, it's on purpose
1if (cursor.WillErase(*it)) {
2    entry.coin = std::move(it->second.coin);
3} else {
4    entry.coin = it->second.coin;
5}
6cachedCoinsUsage += entry.coin.DynamicMemoryUsage();

Do you still think we don’t need it?

l0rinc force-pushed on Apr 20, 2025

l0rinc renamed this:
~~RFC: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`~~
coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`
on Apr 20, 2025

DrahtBot added the label UTXO Db and Indexes on Apr 20, 2025

l0rinc marked this as ready for review on Apr 20, 2025

DrahtBot added the label CI failed on Apr 28, 2025

test: drop leftover UBSan suppressions from `CCoinsViewCache` a1fe87eb66

coins: check unspent‑overwrite before `cachedCoinsUsage` change in `AddCoin` 02dbefb9a9

coins: assert just-created-coin has no dynamic memory usage

This was added for the two branches to be symmetric and to explain why this branch is missing a subtraction.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

0189ab0939

coins: spent erase in `CoinsViewCacheCursor` must not change usage

Assert that spent coins already have zero dynamic size, and remove unused leftover counter

a7f8b5f13b

coins: reset `cachedCoinsUsage` in Flush() only after the map is cleared

Prevents the counter from dropping to 0 while cacheCoins still holds objects

2923d0055d

coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` inserts

Guarantees counter stays balanced both on insert and on in‑place replacement.
Note that this is currently only called from AssumeUTXO code where it should only insert.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

a0468a881e

l0rinc commented at 9:50 am on April 29, 2025: contributor

Rebased to allow CI running the new tests.

l0rinc force-pushed on Apr 29, 2025

l0rinc renamed this:
~~coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`~~
coins: fix `cachedCoinsUsage` usages in `CCoinsViewCache`
on Apr 29, 2025

l0rinc renamed this:
~~coins: fix `cachedCoinsUsage` usages in `CCoinsViewCache`~~
coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`
on Apr 29, 2025

DrahtBot removed the label CI failed on Apr 29, 2025

coins: fix cachedCoinsUsage accounting in CCoinsViewCache #32313

Code Coverage & Benchmarks

Reviews

Conflicts

coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache` #32313