#34864 coins: tighten cache entry state invariants
https://github.com/bitcoin/bitcoin/pull/34864 · · +150/-192 in 9 files, 9 commits · labels: UTXO Db and Indexes
Goal
- Prevent unreachable and contradictory states in the UTXO cache
- Protect consensus-critical cache synchronization from logic bugs around coin spending and updates
This PR refactors `CCoinsCacheEntry` so that dirty status is derived from membership in the dirty-entry linked list rather than a separate flag, making fresh-without-dirty states unrepresentable. It removes unreachable fresh-only states across production and test code, asserts that `BatchWrite()` cursor entries are dirty, rejects spent `FRESH` entries, and tightens `SpendCoin()` to return false without mutating cache state when attempting to spend an already-spent entry while marking the return value `[[nodiscard]]`.
Problem: `CCoinsCacheEntry` tracked dirty state using both a bit flag and linked-list pointers, allowing contradictory or unreachable entry states (such as fresh but not dirty) to be represented and tested. In addition, `SpendCoin()` handled repeated spends on already-spent entries inconsistently, mutating output without checking for prior spends.
Category: Validation (#21 of 48)
P3 · cleanup
- P3 because it hardens internal UTXO cache state invariants against logic errors
- Does not fix an active bug or affect user-facing behavior
Tightening UTXO cache invariants and eliminating redundant state flags reduces the risk of logic errors in consensus-critical cache synchronization, but does not fix an active bug or affect user-facing behavior.
Membership: Modifies core UTXO cache classes (CCoinsViewCache, CCoinsCacheEntry, CCoinsViewDB) and validation calling code in validation.cpp and txdb.cpp.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1
Reviewability: Ready
- Worthwhile to review now; code is clean, CI passes, and all reviewer feedback has been addressed
The code is clean, CI passes, and the latest push has a matching reACK with no unresolved review questions.
Author status: Active; addressed all inline comments and rebased cleanly.
Resolved concerns:
- andrewtoth noted that `SetDirty()` should not silently clear freshness when passing `fresh=false`; author added `Assume(!pair.second.m_fresh || fresh)`.
- andrewtoth requested keeping descriptive wording in log messages and retaining relevant comments, which the author restored.
Agreement: Strong
- Broad agreement on making invalid cache states unrepresentable
- Support for maintainability benefits from making invalid states impossible (optout21)
- Concept approval without stated reasons (andrewtoth)
- Verified by running coins fuzz targets without errors or crashes (ptrinh)
Strong: thorough ACK from optout21, concept ACK from andrewtoth, and clean fuzz testing from ptrinh
Reviewers support making invalid cache states unrepresentable, with optout21 providing a detailed analysis and reACKing the latest head, and ptrinh verifying with fuzz runs.
- optout21: 'Post-change the invalid states cannot be represented. This change is a code maintainability benefit... ACK fa9683... reACK f27eba'
- andrewtoth: 'Concept ACK. Thanks for picking this up.'
- ptrinh: 'Tested the coins fuzz targets on this branch... No crashes, OOMs, sanitizer errors, or hits on the new BatchWrite() logic_error.'
Review verdicts (DrahtBot): 1
- ACK: optout21
- Concept ACK: andrewtoth
Files
146 lines under test/bench/ci.
- src/coins.h +49/-62
- src/test/coins_tests.cpp +31/-49
- src/coins.cpp +29/-32
- src/test/fuzz/coinscache_sim.cpp +11/-22
- src/test/coinscachepair_tests.cpp +10/-8
- src/txdb.cpp +7/-10
- src/test/fuzz/coins_view.cpp +8/-7
- src/txmempool.cpp +4/-1
- src/validation.cpp +1/-1
Card
This PR tightens coins cache invariants by deriving dirty status directly from dirty linked-list membership rather than separate flags, making fresh-without-dirty states impossible to represent. It eliminates unreachable test states, asserts cursor dirty invariants in BatchWrite(), and makes SpendCoin() return values nodiscard while leaving cache state untouched on repeated spends. The change reduces the state space in the consensus-critical UTXO cache, has concept support from andrewtoth, fuzz verification by ptrinh, and an ACK from optout21.