[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.

Type	Reviewers
ACK	kwsantiago, chrisguida
Concept NACK	l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

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); -> Consider BOOST_CHECK_EXCEPTION(..., std::logic_error, HasReason("Attempted to overwrite an unspent coin (when possible_overwrite is false)")) to verify the specific error message instead of only 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.

chrisguida commented at 10:43 PM on July 6, 2026:

#34645 is a crash in the coins_view_overlay fuzz target, which was caused by #34165 (which introduces CoinsViewOverlay), which isn't part of this backport (#34164 is). I confirmed both are absent from this branch, and the coins fuzzers here don't define any custom CCoinsViewCache/BatchWrite override, so this m_dirty_count decrement can't underflow and the Assume(m_dirty_count == 0) in Flush()/Sync() can't be hit. So #34655 isn't needed for this backport.

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

validation: Use dirty entry count in flush warnings and disk space checks

Changes flush warnings to use the actual number of dirty entries being written rather than total cache size or memory usage:
* Moves warning from `FlushStateToDisk` to `CCoinsViewDB::BatchWrite` so it applies to both regular flushes and `AssumeUTXO` snapshot writes
* Changes threshold from `WARN_FLUSH_COINS_SIZE` (1 GiB) to `WARN_FLUSH_COINS_COUNT` (10M entries), approximately equivalent - this also helps with the confusion caused by UTXO size difference on-disk vs in-memory
* Moves benchmark logging to `BatchWrite` where the actual disk I/O occurs to make sure AssumeUTXO also warns
* Uses dirty count for disk space check (48 bytes per entry estimate)
* Removes redundant `changed` counter since `dirty_count` is now tracked

This ensures users are warned appropriately even when only a fraction of the cache is dirty, and provides accurate warnings during `AssumeUTXO` loads.

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

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

b53119b01c

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.

c1acaf2eda

luke-jr force-pushed on May 11, 2026

achow101 commented at 10:51 PM on May 11, 2026: member

Can you explain why these should be backported?

luke-jr commented at 4:07 PM on May 15, 2026: member

It fixes bugs.

kwsantiago commented at 12:32 AM on June 10, 2026: none

tested ACK c1acaf2eda082a37ffb4e24777b042026eef214c

Checked the 6 commits against their merged-master versions via the Rebased-From hashes and they line up. The extra dirty_count_tag commit looks like a sensible guard, since the cursor's first arg went from size_t& usage to size_t& dirty_count (same type, easy to mis-wire on a partial apply).

Built on Linux (gcc 13 + clang 18) and ran coins_tests (including the 3 new cases) plus coinscachepair, validation_flush, validation_chainstate, validation_chainstatemanager, txvalidationcache and miner_tests, all green. Also did ASan+UBSan libFuzzer runs of coins_view (~1.96M execs), coins_view_db (~392k) and coinscache_sim with no crashes. Figured that was worth doing since this PR removes the CCoinsViewCache UBSan suppressions. @luke-jr On the #34645/#34655 question: this still has the older harness with independent dirty/fresh bools, but the #34645 repro goes through MutationGuardCoinsViewCache, which isn't in this backport, and I couldn't get it to crash in the runs above, so it doesn't look exposed here. #34655's Assume(TrySub(...)) change might be worth pulling in anyway but doesn't seem like a blocker.

chrisguida commented at 10:36 PM on July 6, 2026: none

tACK https://github.com/bitcoin/bitcoin/pull/35226/commits/c1acaf2eda082a37ffb4e24777b042026eef214c

My read of this backport is that it fixes a fairly serious accounting bug which was introduced by #28233 which changed periodic flushes to Sync the dbcache rather than Flush it. While this was a legitimate improvement (significant speedup for connecting blocks), the change caused the total cache size and the number of dirty entries a flush actually writes to diverge much more widely than before, and various places in the code weren't updated to match.

The worst bug this caused, that I can find, is that the node will sometimes shut itself down, thinking that it's out of disk space, when it isn't: the disk-space check reserves room as if the whole cache were about to be written, but only the dirty coins actually get written, so it demands far more free space than the flush needs and wrongly decides the disk is too low. I reproduced this by creating a node with a small disk, a large dbcache (=2000), and a 2-second DATABASE_FLUSH_INTERVAL, causing the node to shut itself down at only 28% disk usage. After running the same test using this backport, the bug is no longer present. So I can recommend this backport for merge into v29 on this basis alone.

Another thing it fixes is a spammy log line ("Flushing large (N GiB) UTXO set to disk, it may take several minutes") that fires on every full flush with a large cache even when few entries are dirty. Less serious, but certainly annoying for people wanting to stay on v29 and maintain a large dbcache.

Both of these bugs are fixed in b53119b01ce86daba1c1a12a15442311baaffc83 (the last commit of #33512, whose purpose is to introduce and use m_dirty_count in disk space checks and logging) (cleanly backported from afb1bc120e which was released in v31).

The rest of the PRs in this backport appear to be test hardening for this fix (#32313, #32602) or useful features that it relies on to minimize the diff with v29 (#34207, #34164, #32279).

The final commit isn't a backport, but it's useful for anyone trying to rebase on top of the change; it makes an existing CoinsViewCacheCursor(usage, …) call fail to compile rather than silently bind usage to the new dirty_count parameter, forcing an explicit migration.

I also verified that the backport is faithful to the original and all commits build clean, so this is good to go.

l0rinc commented at 12:47 AM on July 7, 2026: contributor

I'm surprised by the sudden interest here - we don't usually backport features, optimizations, or low-severity bugs to a branch about to sunset: "the threshold for backporting a change to an older major version increases as it ages".

The mentioned over-reservation bug only triggers when free space drops below the cache-sized reservation. Your 28% usage with -dbcache=2000 implies you created a ~5 GB disk. I don't think anything under ~20 GB can safely hold the UTXO set: m_assumed_chain_state_size alone is 14 GB. The node shutting down is expected in that case.

But the good news is that anyone who wants these accounting improvements (plus the many other fixes and performance gains merged since) can just upgrade to v30 - I hear it's a good release.

Concept NACK - I don't think it's worth the effort here. But it's open source, so feel free to use these changes in Knots.

achow101 commented at 1:58 AM on July 7, 2026: member

It fixes bugs.

Not all bug fixes are backported, especially to branches that are nearing their end of life.

The changes being backported in this PR do not seem to fall into the category of things that we would usually backport at this point in a release's lifecycle. It doesn't seem like this is fixing any major issue in 29.x that people would actually run into.

achow101 closed this on Jul 7, 2026

chrisguida commented at 2:57 AM on July 7, 2026: none

Your 28% usage with -dbcache=2000 implies you created a ~5 GB disk. I don't think anything under ~20 GB can safely hold the UTXO set: m_assumed_chain_state_size alone is 14 GB. The node shutting down is expected in that case.

This was on regtest, not mainnet