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 return false without overwriting caller-owned output, while preserving the already-unchanged cache accounting. 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. Production callers retain their behavior: checked inputs remain spendable, disconnects report unclean state, and replay ignores repeated deletes.

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 and AI policy 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:

#35793 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35793.svg"></sub> (Implement BIP 54 (Consensus Cleanup) without mainnet activation by darosior)
#35569 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35569.svg"></sub> (Encapsulation for CTransaction by purpleKarrot)

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 SetDirty call

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.

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

sedited requested review from optout21 on Jul 24, 2026

DrahtBot added the label Needs rebase on Aug 11, 2026

l0rinc force-pushed on Aug 11, 2026

DrahtBot removed the label Needs rebase on Aug 12, 2026

optout21 commented at 6:56 AM on September 7, 2026: contributor

reACK f27eba862da60c11e5e1e70250cc267f4a1b0a52

No issues. A net reduction in complexity around coins view flags (my previous reivew had more details).

test: characterize coin cache state handling

Limit fixtures to entries that are dirty whenever they are fresh, and prepare the fuzz harness to avoid spent fresh entries and duplicate insertion accounting.
Record that repeated spends report success and overwrite caller-owned output, while retaining the existing state and accounting checks.

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

c19068f1fe

coins: pass freshness through `SetDirty()`

Fresh entries must also be dirty.
Require the resulting freshness at every `SetDirty()` call and remove the bare `SetFresh()` helper, so callers cannot create a fresh entry without linking it into the dirty list.

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

2444634014

coins: assert dirty `BatchWrite()` cursor entries

`CoinsViewCacheCursor` iterates the dirty-entry list.
Treat cursor entries as dirty instead of checking them again, while retaining assertions of the cursor contract.

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

6a6c290079

coins: reject spent FRESH entries in BatchWrite()

Spending a fresh entry erases it locally because the parent never knew it existed.
Reject a spent fresh entry before merging it into the parent cache instead of treating it as a valid state.

b7f35ad836

refactor: inline dirty-list insertion

`SetDirty()` is the only remaining caller of `AddFlags()`.
Inline the helper so the representation change is confined to the entry's state management.

28ead9c57f

coins: derive `DIRTY` from linked list membership

List membership already identifies dirty entries, so the separate `DIRTY` bit duplicates state.
Store only freshness and require it to be preserved while an entry remains dirty.

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

90685bd555

coins: fail `SpendCoin()` on already-spent entries

Return false without overwriting caller-owned output when an entry is already cached as spent.
Cache accounting was already unchanged for reachable spent entries.
Production callers retain their behavior: checked inputs remain spendable, disconnects report unclean state, and replay ignores repeated deletes.
The rejected spend no longer emits the `utxocache:spent` tracepoint.

c14d63a15c

coins: require checking `SpendCoin()` results

Mark `SpendCoin()` as `[[nodiscard]]` so accidental ignored results are rejected.
The mempool consistency check requires successful spends, while replay explicitly ignores repeated deletes after an interrupted coins DB flush.

6fd59bf706

l0rinc force-pushed on Sep 30, 2026

l0rinc commented at 10:33 PM on September 30, 2026: contributor

Rebased to make sure related changes are included and applied some AI review cleanup suggestions.