coins: make cache freshness imply dirtiness and remove invalid test states #34864

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

Problem

The coins cache implementation still carries logic and representation for states that production code does not create, especially standalone fresh entries and BatchWrite() inputs that are not dirty. The unit tests and fuzz harnesses were also constructing those states directly.

That reduces confidence in the results: instead of checking realistic cache behavior, parts of the test suite are checking behavior around impossible states. It also becomes a recurring source of test-only failures whenever production invariants are tightened.

Fix

This series first makes the production invariant explicit by setting freshness through the same method that marks entries dirty. That makes the dependency clear at the call sites instead of relying on a separate freshness step.

With that in place, the series updates the tests so they stop constructing fresh-only states before the stricter BatchWrite() checks land. It then asserts that every cache entry reaching BatchWrite() is dirty, rejects spent-and-fresh BatchWrite() entries as logic errors, removes the bare freshness helper, and simplifies the internal representation so dirtiness is derived from linked-list membership instead of a stored flag bit.

The final step makes the dirty-marking API take the resulting freshness explicitly. That avoids callers accidentally preserving an old fresh state when they mean to clear it.

Notes

This revives Andrew’s earlier work in #30673 and #33018.

A spent-and-fresh entry is not just unusual, it is inconsistent with how the cache works. If a fresh entry is spent, it should be erased locally because the parent view never knew it existed. Writing it upward as spent is therefore a logic error, and the series makes that boundary explicit in BatchWrite().

Another subtle point is that making freshness explicit changes the contract of the dirty-marking helper. The caller must pass the resulting freshness of the entry, not just whether the current operation introduced freshness. That matters in paths where an entry may already be fresh before the current update.

Finally, once fresh-only states are removed from tests, dirtiness can be derived from linked-list membership without losing information. That lets the cache entry store only whether an entry is fresh, while the linked list itself becomes the source of truth for whether an entry is dirty.

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.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21
Concept ACK	andrewtoth

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:

#35078 (validation: merge PeekCoin into GetCoin by l0rinc)
#34320 (coins: remove redundant and confusing CCoinsViewDB::HaveCoin by l0rinc)
#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Avoid setting FRESH for an outpoint that already exists unspent in the parent view. -> Avoid setting FRESH for an outpoint that already exists and is unspent in the parent view. [“already exists unspent” is awkward/broken phrasing and slightly unclear without inferring the missing “and is”].

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.

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()` instead of setting `FRESH` in a second step.

This sets up the later cleanup, making the production invariant explicit before the tests are adjusted, and shows that bare `FRESH` entries are only coming from test code.

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

000efb0bdf

test: remove fresh-only states from `coins_tests`

Production code no longer introduces 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 the reachable cache states before the following `BatchWrite()` cleanup asserts that every cursor entry is dirty.

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

f9eecca2d7

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

Now that production code no longer creates fresh-only linked-list entries, every cursor entry reaching `BatchWrite()` is already dirty.
Replace the old skip logic with assertions and unconditional processing in the cache, db, test, and fuzz backends.

Simplify the coin DB log line to match the new invariant.

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

58e10baaa4

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

A spent `FRESH` entry is the remaining impossible `BatchWrite()` state.
Spending a fresh entry erases it instead of writing it to the parent, so `CCoinsViewCache::BatchWrite()` should treat that case as a logic error.

Update the `ccoins_write` table to cover the failure explicitly.

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

07593767ad

coins: remove the bare `SetFresh()` helper

The earlier commits make the production invariant explicit and enforce the impossible `BatchWrite()` states.
Remove the bare `SetFresh()` helper and update the unit tests and the `coins_view` fuzz harness to use `SetDirty(..., fresh)` directly.

This drops the remaining fresh-only test states so later cleanups can stop modeling a standalone `FRESH` flag.

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

e20ba7d69b

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

The next commit changes how `CCoinsCacheEntry` stores dirtiness and freshness.
Move the linked-list update logic into `SetDirty()` first so that the representation change only needs to touch one helper and `SetClean()`.
Mirror the wrap-if guard shape in `SetClean()` for symmetry with the new `SetDirty()`.

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

f9d668f02f

coins: derive `DIRTY` from linked list membership

`CCoinsCacheEntry` only needs to store whether an entry is fresh.
Now that `AddFlags()` is inlined into `SetDirty()`, dirtiness can be derived from membership in the dirty-entry list instead of a separate bit in `m_flags`.

Make `SetDirty()` take the resulting freshness explicitly and assert that an already-fresh entry is never cleared back to not-fresh.
Replace `m_flags` with `m_fresh`, derive `IsDirty()` from the linked-list pointers, and update the nearby comments, docstrings, and pair tests to match.

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

fa9683af5f

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