[29.x] Backport coins accounting fixes (and their dependencies) #35226

luke-jr commented at 7:46 PM on May 6, 2026: member

This backports:

Part of #32279 (only extracting STATIC_SIZE const to prevector)
#32602
#32313
#34207
#34164
#33512

And adds a tag to the CoinsViewCacheCursor constructor to avoid silent conflicts.

refactor: extract `STATIC_SIZE` constant to prevector

Co-authored-by: Anthony Towns <aj@erisian.com.au>

Github-Pull: #32279
Rebased-From: 756da2a994c5f6cf552f025bab5d60dae0582b09

930048a844

fuzz: avoid underflow in coins_view target

Github-Pull: bitcoin/bitcoin#32602
Rebased-From: 56d878c4650cc46ce54d1d79db054ac44b486605

d64fa73635

fuzz: move the coins_view target's body into a standalone function

We'll reuse it for a target where the coins view is a DB.

Github-Pull: bitcoin/bitcoin#32602
Rebased-From: 46e14630f7fe8e2d51adc18a4f50345eb26970c7

dfa4157181

fuzz: add a target for the coins database

It reuses the logic from the `coins_view` target, except it uses an
in-memory CCoinsViewDB as the backend.

Note `CCoinsViewDB` will assert the best block hash is never null, so we
slightly modify the coins_view fuzz logic to take care of this.

Github-Pull: bitcoin/bitcoin#32602
Rebased-From: cfc42ae5b7ef6d3892907cfe36dc3ae923495d37

c2a0d5a6fd

refactor: assert newly-created parent cache entry has zero memory usage

During `BatchWrite`, the parent entry is created under a guard that guarantees insertion, so the new `Coin` is default-constructed and empty.
Assert this invariant to document why there is no `cachedCoinsUsage` decrement before the assignment at this site.

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

Github-Pull: bitcoin/bitcoin#32313
Rebased-From: 67cff8bec9094e968f36d351fb2e38c9bf563757

bd6d840b0f

refactor: remove redundant usage tracking from `CoinsViewCacheCursor`

When a coin is spent via `SpendCoin()`, `cachedCoinsUsage` is already decremented and the coin's `scriptPubKey` is cleared, so `DynamicMemoryUsage()` is `0`.

`CoinsViewCacheCursor::NextAndMaybeErase()` was subtracting usage again when erasing spent entries.
Replace it with an assert that documents spent coins have zero dynamic memory usage by the time the cursor encounters them.

Remove the now-unnecessary `usage` reference from the cursor's constructor and member variables.

Github-Pull: bitcoin/bitcoin#32313
Rebased-From: 39cf8bb3d0d9ee84544d161bf66d90d5e2a1a140

787de911de

coins: fix `cachedCoinsUsage` accounting to prevent underflow

Move the `cachedCoinsUsage` subtract in `AddCoin()` to after the `possible_overwrite` check.
Previously a throw before assignment decremented the counter without changing the entry, which corrupted accounting and later underflowed.

In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared. In production `BatchWrite()` returns `true`, so this mostly affects tests. On failure, leave the counter unchanged to keep it in sync with the cache.

The existing `Flush()` workaround in fuzzing was also removed now that the source of the problem was fixed, so the fuzzer no longer needs `coins_view_cache.Flush()` to realign `cachedCoinsUsage` after an exception.
Replace the prior `expected_code_path` tracking with direct assertions. The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed.

With these changes the counter stays consistent across success and exception paths, so we can finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue.

Included a unit test as well, attempting to add a different coin to the same outpoint without allowing overwrites and make sure it throws.
We use `SelfTest()` to validates accounting, and check that the cache remains usable.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <woltx@protonmail.com>

Github-Pull: bitcoin/bitcoin#32313
Rebased-From: d7c9d6c2914aadd711544908d0fad8857a809c72

2fccba8e69

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

`EmplaceCoinInternalDANGER()` incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key), inflating the counter.
This is mostly reachable in tests today since `AssumeUTXO` does not overwrite.

Increment only on successful insert, and capture `coin.DynamicMemoryUsage()` before the move so accounting uses the correct value.

Fuzz: add an `EmplaceCoinInternalDANGER` path to exercise insert-only accounting.
Unit test: emplace two different coins at the same outpoint (with different `DynamicMemoryUsage()`), verify `SelfTest()` passes and `AccessCoin(outpoint)` returns the first coin.

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

Github-Pull: bitcoin/bitcoin#32313
Rebased-From: 24d861da7894add47747eff69dd3fc71fbcdd7d0

18d54ddfed

txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins

The chainstate UTXO database only stores unspent outputs; spent entries are removed.

Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface.

Github-Pull: bitcoin/bitcoin#34207
Rebased-From: ee1e40f58000921e95f08bcb199a452eb5c4d9b2

b497675b50

test: do not return spent coins from `CCoinsViewTest::GetCoin`

Production `GetCoin()` implementations only return unspent coins.

Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin/bitcoin#34207
Rebased-From: 3e4155fcefe0aafcc9cb84640e303e05477605a3

1429807f14

fuzz: keep `coinscache_sim` backend free of spent coins

`CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins.

Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin/bitcoin#34207
Rebased-From: eec551aaf1dff4cccc15e486d5618a8a44d8314c

54386ddbd0

coins: assume `GetCoin` only returns unspent coins

`CCoinsViewCache::FetchCoin()` had special handling for a spent `Coin` returned by the parent view.
Production parents (`CCoinsViewCache` and `CCoinsViewDB`) do not return spent coins, so this path is unreachable.

Replace it with an `Assume(!coin.IsSpent())`, drop outdated documentation about spent+FRESH cache entries, and simplify `SanityCheck()` to assert the remaining possible state invariants.
This is safe because it does not change behavior for valid backends and will fail fast if the `GetCoin()` contract is violated.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin/bitcoin#34207
Rebased-From: 2ee7f9b259059d59e127852ea898b58183604b46

fec18ac7ab

coins: add Reset on CCoinsViewCache

Add a Reset() method to CCoinsViewCache that clears cacheCoins,
cachedCoinsUsage, and hashBlock without flushing to the base view.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: sedited <seb.kung@gmail.com>

Github-Pull: bitcoin/bitcoin#34164
Rebased-From: 8dd9200fc9b0d263f8f75943ce581a925d061378

18292486bb

coins: use hashBlock setter internally for CCoinsViewCache methods

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin/bitcoin#34164
Rebased-From: 041758f5eda5725daad4ae20f66c7d19ba02d063

bb8e5d87d4

coins: introduce CCoinsViewCache::ResetGuard

CCoinsViewCache::CreateResetGuard returns a guard that calls
Reset on the cache when the guard goes out of scope.
This RAII pattern ensures the cache is always properly reset
when it leaves current scope.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: sedited <seb.kung@gmail.com>

Github-Pull: bitcoin/bitcoin#34164
Rebased-From: 8fb6043231ea396aaa1165b36b082c89e10fcafd

3ef3b3824d

validation: reuse same CCoinsViewCache for every ConnectBlock call

Add m_connect_block_view to ChainState's CoinsViews.
Call CreateResetGuard inside ConnectTip to ensure the view
is Reset after each block, avoiding repeated memory allocations.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin/bitcoin#34164
Rebased-From: 44b4ee194d3bdccd86cf5e151b2fc1479aabbb6c

5e19298d2c

fuzz: call `EmplaceCoinInternalDANGER` as well in `SimulationTest`

Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify it remains correct as we modify it in a future commit.

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

Github-Pull: bitcoin/bitcoin#33512
Rebased-From: 7e52b1b945c4137e0fb05715090635ce82ed04b3

17b63a3b93

coins: Keep track of number of dirty entries in `CCoinsViewCache`

Adds `m_dirty_count` member to track the running count of dirty cache entries as follows:
* Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty`
* Decremented when dirty entries are removed or cleaned
* Passed through `CoinsViewCacheCursor` and updated during iteration

The dirty count is needed because after non-wiping flushes (introduced in #28280 and #28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading.

Updates all test code to properly initialize and maintain the dirty count.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>

Github-Pull: bitcoin/bitcoin#33512
Rebased-From: b413491a1cdd9a51f2aa10b775650f54f6785e3e

2e19f4b935

DrahtBot added the label Backport on May 6, 2026

DrahtBot commented at 7:46 PM on May 6, 2026: 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/35226.

Reviews

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

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

Coin{CTxOut{...}, 1, false} in src/test/coins_tests.cpp
Coin{CTxOut{...}, 2, false} in src/test/coins_tests.cpp
Coin{CTxOut{...}, 1, false} in src/test/coins_tests.cpp

Possible places where comparison-specific test macros should replace generic comparisons:

[src/test/coins_tests.cpp] BOOST_CHECK_THROW(cache.AddCoin(outpoint, Coin{coin2}, /*possible_overwrite=*/false), std::logic_error); -> use BOOST_CHECK_EXCEPTION(..., std::logic_error, HasReason("Attempted to overwrite an unspent coin (when possible_overwrite is false)")) to verify the expected message, not just the exception type.

in src/coins.h:285 in a1325d087f outdated

 281 | @@ -284,11 +282,12 @@ struct CoinsViewCacheCursor
 282 |      inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept
 283 |      {
 284 |          const auto next_entry{current.second.Next()};
 285 | +        m_dirty_count -= current.second.IsDirty();

luke-jr commented at 7:55 PM on May 6, 2026:

Someone familiar with #34645 and #34655 should probably review this.

coins: Add tag for CoinsViewCacheCursor constructor to avoid silent conflicts

The first parameter was previously `size_t& usage` which could be silently confused with the new `dirty_count`.
By adding a tag, we ensure code must explicitly declare the constructor signature.

52917cb4c7

in src/coins.h:263 in a1325d087f

 257 | @@ -264,18 +258,22 @@ class CCoinsViewCursor
 258 |   */
 259 |  struct CoinsViewCacheCursor
 260 |  {
 261 | +    //! CoinsViewCacheCursor's constructor used to take a `usage` reference in exactly the same way as `dirty_count`, so to avoid a silent misuse, the tag is required
 262 | +    struct dirty_count_tag_t {};
 263 | +    static inline constexpr dirty_count_tag_t dirty_count_tag;

l0rinc commented at 6:29 AM on May 7, 2026:

    static inline constexpr dirty_count_tag_t dirty_count_tag{};

luke-jr force-pushed on May 7, 2026