coins: make cache freshness imply dirtiness and remove invalid test states#34864
Open
l0rinc wants to merge 7 commits intobitcoin:masterfrom
Open
coins: make cache freshness imply dirtiness and remove invalid test states#34864l0rinc wants to merge 7 commits intobitcoin:masterfrom
l0rinc wants to merge 7 commits intobitcoin:masterfrom
Conversation
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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()`. Co-authored-by: Lőrinc <[email protected]>
`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 so callers cannot accidentally preserve an old fresh state by passing `false`. 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 <[email protected]>
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. |
This was referenced Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-freshBatchWrite()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.