coins/refactor: enforce GetCoin() returns only unspent coins
#34207
Conversation
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.
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>
`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>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34207. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
ACK 712bd86 Tested. |
`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>
712bd86 to
2ee7f9b
Compare
|
re-ACK 2ee7f9b Changes since last ACK: |
|
crACK 2ee7f9b I did not ensure via code review that indeed spent coins are never returned. If that happened, that would be a serious error, the |
Summary
::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.Context
This PR is split out from #33018 to keep that PR focused on removing the
FRESH-but-not-DIRTYcache state.Changes:
CCoinsViewDB::GetCoin()never returns a spent coin.GetCoin()contract by never returning spent coins.CCoinsViewCache::FetchCoin()withAssert(!coin.IsSpent()), drop outdatedspent+FRESHdocs, and tightenSanityCheck()invariants.Behavior is unchanged, it just aligns our tests to exercise valid states.