coins/refactor: enforce `GetCoin()` returns only unspent coins

l0rinc commented at 11:24 am on January 6, 2026: contributor

This PR is split out from #33018 to keep that PR focused on removing the FRESH-but-not-DIRTY cache state.

Problem

::GetCoin() is an interface for querying the UTXO set, so production implementations should only ever return unspent coins. Tests should mimic this to provide useful feedback.

Fix:

Add a fail-fast assertion that CCoinsViewDB::GetCoin() never returns a spent coin.
Align unit tests and fuzz simulations with the production GetCoin() contract by never returning spent coins.
Replace the unreachable “spent coin returned by parent” handling in CCoinsViewCache::FetchCoin() with Assert(!coin.IsSpent()), drop outdated spent+FRESH docs, and tighten SanityCheck() invariants.

Behavior is unchanged, it just aligns our tests to exercise valid states.

txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins

The chainstate UTXO database only stores unspent outputs; spent entries are removed.

Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface.

ee1e40f580

test: do not return spent coins from `CCoinsViewTest::GetCoin`

Production `GetCoin()` implementations only return unspent coins.

Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`.

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

3e4155fcef

fuzz: keep `coinscache_sim` backend free of spent coins

`CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins.

Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin.

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

eec551aaf1

DrahtBot commented at 11:25 am on January 6, 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/34207.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	andrewtoth, optout21, w0xlt
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34132 (refactor: inline CCoinsViewErrorCatcher into CCoinsViewDB by l0rinc)
#33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
#33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)

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:58 in 712bd86302

54@@ -55,10 +55,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
55         if (auto coin{base->GetCoin(outpoint)}) {
56             ret->second.coin = std::move(*coin);
57             cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
58-            if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
59-                // The parent only has an empty entry for this outpoint; we can consider our version as fresh.
60-                CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
61-            }
62+            Assume(!ret->second.coin.IsSpent());

w0xlt commented at 10:08 pm on January 6, 2026:

Is there a specific reason to use Assert in CCoinsViewDB::GetCoin but Assume here ?

l0rinc commented at 8:33 pm on January 11, 2026:

You’re right, it’s the same assumption, made both of them asserts.

w0xlt commented at 10:40 pm on January 6, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/34207/commits/712bd86302fd395397f6c6ab9bddcffa904c379d

bensig commented at 9:52 pm on January 7, 2026: contributor

ACK 712bd86302fd395397f6c6ab9bddcffa904c379d

Tested.

in src/coins.cpp:321 in 712bd86302

323-        if (entry.IsFresh()) attr |= 2;
324-        if (entry.coin.IsSpent()) attr |= 4;
325-        // Only 5 combinations are possible.
326-        assert(attr != 2 && attr != 4 && attr != 7);
327+        assert(!entry.coin.IsSpent() || entry.IsDirty());
328+        assert(!entry.IsFresh() || (entry.IsDirty() && !entry.coin.IsSpent()));

andrewtoth commented at 8:24 pm on January 11, 2026:

I find https://github.com/bitcoin/bitcoin/commit/bcf4ec0e1f68202fa6535b90f9506c73320ac4b0 to be easier to understand.

l0rinc commented at 8:33 pm on January 11, 2026:

Changed

andrewtoth approved

andrewtoth commented at 8:25 pm on January 11, 2026: contributor

ACK 712bd86302fd395397f6c6ab9bddcffa904c379d

coins: assume `GetCoin` only returns unspent coins

`CCoinsViewCache::FetchCoin()` had special handling for a spent `Coin` returned by the parent view.
Production parents (`CCoinsViewCache` and `CCoinsViewDB`) do not return spent coins, so this path is unreachable.

Replace it with an `Assume(!coin.IsSpent())`, drop outdated documentation about spent+FRESH cache entries, and simplify `SanityCheck()` to assert the remaining possible state invariants.
This is safe because it does not change behavior for valid backends and will fail fast if the `GetCoin()` contract is violated.

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

2ee7f9b259

l0rinc force-pushed on Jan 11, 2026

l0rinc requested review from w0xlt on Jan 11, 2026

andrewtoth commented at 1:15 am on January 16, 2026: contributor

re-ACK 2ee7f9b259059d59e127852ea898b58183604b46

Changes since last ACK:

Assume -> Assert
#34207 (review)

optout21 commented at 3:02 pm on January 16, 2026: contributor

crACK 2ee7f9b259059d59e127852ea898b58183604b46

I did not ensure via code review that indeed spent coins are never returned. If that happened, that would be a serious error, the Assert is warranted. Reviewed code, checked unit tests.

w0xlt commented at 6:55 pm on January 20, 2026: contributor

reACK 2ee7f9b259059d59e127852ea898b58183604b46 Clean removal of dead code handling the impossible “spent coin from parent” scenario.

coins/refactor: enforce GetCoin() returns only unspent coins #34207

Problem

Fix:

Code Coverage & Benchmarks

Reviews

Conflicts

coins/refactor: enforce `GetCoin()` returns only unspent coins #34207