coins: tighten cache entry state invariants #34864

l0rinc commented at 12:01 PM on March 19, 2026: contributor

Problem: The coins cache still exposed and tested states that production code should not create: standalone FRESH entries, non-dirty BatchWrite() cursor entries, and spent FRESH entries. That makes the representation harder to reason about, because CCoinsCacheEntry stores a separate DIRTY bit even though dirty entries are already tracked in the linked list.

Fix: This revives and splits the relevant parts of #30673 and #33018. The series passes freshness through SetDirty(), removes fresh-only test states, treats BatchWrite() cursor entries as dirty by construction, rejects spent FRESH entries, removes the bare SetFresh() helper, and derives dirtiness from linked-list membership. The final commits also tighten SpendCoin() around the same cache-state contract: repeated spends of an entry already cached as spent now fail without changing cache accounting or caller-owned output, and callers must either check the result or explicitly discard it. coinscache_sim now checks the real SpendCoin() result against its simulation model, and the no-op repeated-spend path no longer emits the utxocache:spent tracepoint because no spend occurs.

DrahtBot added the label UTXO Db and Indexes on Mar 19, 2026

DrahtBot commented at 12:02 PM on March 19, 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/34864.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	andrewtoth
Stale ACK	optout21

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35587 (Remove boost as a unit test runner by rustaceanrob)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)

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:235 in 745d974961 outdated

 230 | +                CCoinsCacheEntry::SetDirty(*itUs, m_sentinel, it->second.IsFresh());
 231 |                  ++m_dirty_count;
 232 |                  cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
 233 | -                // We can mark it FRESH in the parent if it was FRESH in the child
 234 | -                // Otherwise it might have just been flushed from the parent's cache
 235 | -                // and already exist in the grandparent

andrewtoth commented at 6:37 PM on April 18, 2026:

Why remove this comment? It's still relevant and provides valuable context.

l0rinc commented at 7:44 PM on April 19, 2026:

Restored above the new SetDirtycall

in src/txdb.cpp:160 in c4ddb1cebb

 156 | @@ -160,7 +157,7 @@ void CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashB
 157 |  
 158 |      LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0));
 159 |      m_db->WriteBatch(batch);
 160 | -    LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...", (unsigned int)dirty_count, (unsigned int)count);
 161 | +    LogDebug(BCLog::COINDB, "Committed %u transaction outputs to coin database...", dirty_count);

andrewtoth commented at 6:43 PM on April 18, 2026:

The changed adjective could be left here. Existing transaction outputs that are deleted are changed. The log now can be conflated with committing only new transactions outputs.

l0rinc commented at 7:44 PM on April 19, 2026:

Added changed back

in src/coins.h:179 in 9073d71e91 outdated

 168 | +        pair.second.m_flags |= DIRTY | (fresh ? FRESH : 0);
 169 |      }
 170 |  
 171 |      void SetClean() noexcept
 172 |      {
 173 | -        if (!m_flags) return;

andrewtoth commented at 6:46 PM on April 18, 2026:

Why is this touched here?

l0rinc commented at 7:44 PM on April 19, 2026:

Just for symmetry with SetDirty

in src/coins.h:149 in 5c30d62df4 outdated

 175 |              pair.second.m_prev->second.m_next = &pair;
 176 |          }
 177 | -        Assume(pair.second.m_prev && pair.second.m_next);
 178 | -        pair.second.m_flags |= DIRTY | (fresh ? FRESH : 0);
 179 | +        Assume(pair.second.IsDirty());
 180 | +        pair.second.m_fresh = fresh;

andrewtoth commented at 6:59 PM on April 18, 2026:

This is a behavior change, and the commit message says:

Make `SetDirty()` take the resulting freshness explicitly so callers cannot accidentally preserve an old fresh state by passing `false`.

Why do we want to set a fresh entry to not-fresh? If an entry is fresh, it should stay fresh until cleaned or erased as before.

l0rinc commented at 7:44 PM on April 19, 2026:

This is a behavior change

Why do we want to set a fresh entry to not-fresh?

Added Assume(!pair.second.m_fresh || fresh) so the API can't silently clear FRESH, thanks

andrewtoth commented at 6:59 PM on April 18, 2026: contributor

Concept ACK

Thanks for picking this up.

l0rinc force-pushed on Apr 19, 2026

l0rinc commented at 8:10 PM on April 19, 2026: contributor

Adjusted the code and commit messages, thanks.

git range-diff 745d97496188d50ad3e1a7173bb30d363e97b9c8..5c30d62df48656399e48fdda52d4c6cbd4471789 000efb0bdf928195ddc168be80741c55708b047a..fa9683af5f74d81c4907ad81d888a94ef2e880be

optout21 commented at 4:26 PM on April 23, 2026: contributor

ACK fa9683af5f74d81c4907ad81d888a94ef2e880be

My perspective on the benefit of this change is as follows: The CCoinsCacheEntry class is capable of expressing several combinations of properties, some of which are invalid and never used. They are invalid logically, and enforced by convention only. Post-change the invalid states cannot be represented. This change is a code maintainability benefit, because any code change could potentially break the conventions. It also results in slightly simpler code.

The relevant state space dimensions are:

The "dirty" and "fresh" properties were represented as independent bools (stored in a bitfield), however, of the 2x2 combinations the fresh-but-not-dirty is invalid (because new entries in the cache that are not present in an underlying level must always be saved). This combination is not used.
A dirty (or fresh) entry must be placed in the linked list of dirty elements. The dirty flag and the existence of the linked list pointers may contradict.

Post change:

Dirtyness is implicitly represented by being included in the linked list, not by an explicit flag.
Freshness is represented as a single bool flag, and the setter methods have been changed to prevent the invalid state. Asserts have been also added.

The essence of the change is concentrated to the CCoinsCacheEntry class, and is relatively simple. However, auxiliary changes are needed to ensure/assert the call sites and the tests. Moreover, the change is broken up into smaller consistent steps.

An overview of the commits (in chronological order):

SetFresh is being folded into SetDirty with a fresh flag. At the relevant call site the "freshness" information is already available.
Tests are adjusted to not use the invalid fresh-but-not-dirty state.
In BatchWrite drop the conditional for invalid non-dirty cases, replace them with assert.
Also in BatchWrite, for the invalid fresh-but-spent case instead of being ignored it is handled as an exception.
Drop SetFresh.
Small refactor to fold the now-single-use AddFlags into SetDirty
Perform the internal representation change (Dirtiness is derived from the linked list pointers, and freshness is stored as a bool. Extra asserts are added to checks for preconditions. Also, the default value for the fresh option on SetDirty is removed (so its usage is more explicit)).

I have reimplemented part of the changes independently. I've reviewed the code, and found no issues. (Note: as the PR had only light review so far, further changes are likely.)

DrahtBot requested review from andrewtoth on Apr 23, 2026

ptrinh commented at 8:00 AM on June 24, 2026: none

Tested the coins fuzz targets on this branch (commit fa9683af5f), since the PR turns the previously-handled FRESH+spent cases in BatchWrite() into hard asserts/throws and the safety of that rests on those states never occurring in practice.

Setup: arm64 macOS (Apple M-series), Homebrew clang 22, -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer,address,undefined. Started from an empty corpus (no qa-assets seed), libFuzzer, 2h per target:

coins_view: 3,230,233 execs, peak cov 2961 edges / 14028 features
coinscache_sim: 1,443,489 execs, peak cov 1700 edges / 12217 features

No crashes, OOMs, sanitizer errors, or hits on the new BatchWrite() logic_error. The coins unit suites (coins_tests, coinscachepair_tests, coinsviewoverlay_tests, coinstatsindex_tests) also pass on this branch.

Caveat: this was an unseeded 2h run, so it's shallower than a seeded run against the qa-assets corpus would be; happy to do a longer seeded run if useful. I also read through the BatchWrite/SetDirty/IsDirty changes and the fresh-implies-dirty invariant looks internally consistent.

coins: pass freshness through `SetDirty()` in production

Production code already knows when a dirty entry should also be fresh.
Thread that state through `SetDirty(..., fresh)` in `AddCoin()` and the inserted-entry path in `BatchWrite()` while test-only callers still use `SetFresh()` directly.

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

18e7474e37

test: remove fresh-only states from `coins_tests`

Production code no longer creates standalone `FRESH` entries.
Remove the `State::FRESH` cases from `coins_tests.cpp` so the unit tests stop constructing cache states that production code cannot reach.
This keeps the test helper aligned with reachable cache states before the following `BatchWrite()` cleanup asserts that every cursor entry is dirty.

99b2346942

coins: assert `BatchWrite()` cursor entries are `DIRTY`

`CoinsViewCacheCursor` is constructed with a dirty count and iterates the dirty-entry list.
`CCoinsViewCache::BatchWrite()` and `CCoinsViewDB::BatchWrite()` can therefore treat cursor entries as dirty instead of checking `IsDirty()` again.
Remove the redundant branches, simplify the committed-count log, and keep the cursor contract local with `assert(it->second.IsDirty())`.

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

dea6583cbb

coins: reject spent `FRESH` entries in `BatchWrite()`

A spent `FRESH` entry is inconsistent with how the cache writes to its parent.
Spending a fresh entry erases it locally because the parent never knew it existed, so `BatchWrite()` should treat that state as a logic error.
Update the `ccoins_write` table to cover the failure and keep `coins_view` fuzz-generated entries within that invariant.

8fdb9fa33e

coins: remove the bare `SetFresh()` helper

Fresh entries must also be dirty.
Remove the bare `SetFresh()` helper and pass freshness through `SetDirty()` at the remaining call sites so the type no longer exposes a way to create standalone `FRESH` entries.

a53b634f33

coins: inline `AddFlags()` into `SetDirty()`

`SetDirty()` is now the only helper that adds an entry to the dirty list.
Inline `AddFlags()` there so the following representation change only needs to update `SetDirty()` and `SetClean()`.

fefaf41d28

coins: derive `DIRTY` from linked list membership

`CCoinsCacheEntry` only needs to store whether a dirty entry is also fresh.
After the previous cleanup, list membership already identifies dirty entries, so keeping a separate `DIRTY` bit duplicates state.
Replace `m_flags` with `m_fresh`, derive `IsDirty()` from the linked-list pointers, and require callers to pass the resulting freshness explicitly when marking an entry dirty.
Update comments and pair tests to cover the new representation.

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

b4e8af104d

coins: preserve cache state on repeated spends

Spending an outpoint that is already cached as spent should behave like spending any other outpoint without an unspent output: return false and leave caller-owned output unchanged.
Previously, `CCoinsViewCache::SpendCoin()` only checked whether `FetchCoin()` returned an entry, so a valid spent cache entry could be treated as successfully spent and overwrite `moveout` with an empty `Coin`.
Return false before touching `moveout` or cache accounting, after assuming the spent entry has the valid dirty-not-fresh state.
Extend the existing `ccoins_spend` coverage to check the return value, unchanged spent entry, cache accounting, and `moveout` preservation on a failed spend.
Remove the spent-clean and spent-fresh spend rows, since those cache states violate the invariant this commit now checks.
Let the `coinscache_sim` fuzzer compare `SpendCoin()` results against its unspent-set model.

bba737601a

coins: require `SpendCoin()` callers to inspect result

`SpendCoin()` returns a bool because callers have different contracts: some handle a missing unspent output, some can prove the spend must succeed, and replay code can intentionally ignore repeated deletes.
Document the return value and mark it `[[nodiscard]]` so accidental ignored spends are rejected.
`CTxMemPool::check()` already proves each input exists with `CheckTxInputs()`, so it now checks that each later spend succeeds while still applying the spend unconditionally.
`RollforwardBlock()` casts the result to void because replaying partially applied block effects after an interrupted coins DB flush intentionally treats repeated deletes as no-ops.

4812de53e9

l0rinc force-pushed on Jul 5, 2026

l0rinc renamed this:
~~coins: make cache freshness imply dirtiness and remove invalid test states~~
coins: tighten cache entry state invariants
on Jul 5, 2026

l0rinc commented at 6:01 AM on July 5, 2026: contributor

Rebased and added two commits tightening SpendCoin() return-value handling