cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-31 02:31:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
2ee7f9b259059d59e127852ea898b58183604b46 coins: assume `GetCoin` only returns unspent coins (Andrew Toth)
eec551aaf1dff4cccc15e486d5618a8a44d8314c fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth)
3e4155fcefe0aafcc9cb84640e303e05477605a3 test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth)
ee1e40f58000921e95f08bcb199a452eb5c4d9b2 txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins (Lőrinc)

Pull request description:

  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.

ACKs for top commit:
  andrewtoth:
    re-ACK 2ee7f9b259059d59e127852ea898b58183604b46
  optout21:
    crACK 2ee7f9b259059d59e127852ea898b58183604b46
  achow101:
    ACK 2ee7f9b259059d59e127852ea898b58183604b46
  w0xlt:
    reACK 2ee7f9b259059d59e127852ea898b58183604b46

Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
This commit is contained in:
Ava Chow 2026-01-28 14:29:17 -08:00
commit 75ec9001ce
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
5 changed files with 29 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/coins.cpp
View File

@ -55,10 +55,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
if (auto coin{base->GetCoin(outpoint)}) {
ret->second.coin = std::move(*coin);
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
// The parent only has an empty entry for this outpoint; we can consider our version as fresh.
CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
}
Assert(!ret->second.coin.IsSpent());
} else {
cacheCoins.erase(ret);
return cacheCoins.end();
@ -277,7 +274,7 @@ void CCoinsViewCache::Sync()
void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
if (it != cacheCoins.end() && !it->second.IsDirty()) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, uncache,
hash.hash.data(),
@ -318,20 +315,19 @@ void CCoinsViewCache::ReallocateCache()
void CCoinsViewCache::SanityCheck() const
{
size_t recomputed_usage = 0;
size_t count_flagged = 0;
size_t count_dirty = 0;
for (const auto& [_, entry] : cacheCoins) {
unsigned attr = 0;
if (entry.IsDirty()) attr |= 1;
if (entry.IsFresh()) attr |= 2;
if (entry.coin.IsSpent()) attr |= 4;
// Only 5 combinations are possible.
assert(attr != 2 && attr != 4 && attr != 7);
if (entry.coin.IsSpent()) {
assert(entry.IsDirty() && !entry.IsFresh()); // A spent coin must be dirty and cannot be fresh
} else {
assert(entry.IsDirty() || !entry.IsFresh()); // An unspent coin must not be fresh if not dirty
}
// Recompute cachedCoinsUsage.
recomputed_usage += entry.coin.DynamicMemoryUsage();
// Count the number of entries we expect in the linked list.
if (entry.IsDirty() || entry.IsFresh()) ++count_flagged;
if (entry.IsDirty()) ++count_dirty;
}
// Iterate over the linked list of flagged entries.
size_t count_linked = 0;
@ -340,11 +336,11 @@ void CCoinsViewCache::SanityCheck() const
assert(it->second.Next()->second.Prev() == it);
assert(it->second.Prev()->second.Next() == it);
// Verify they are actually flagged.
assert(it->second.IsDirty() || it->second.IsFresh());
assert(it->second.IsDirty());
// Count the number of entries actually in the list.
++count_linked;
}
assert(count_linked == count_flagged);
assert(count_linked == count_dirty);
assert(recomputed_usage == cachedCoinsUsage);
}

7
src/coins.h
View File

@ -102,7 +102,6 @@ using CoinsCachePair = std::pair<const COutPoint, CCoinsCacheEntry>;
* - unspent, FRESH, DIRTY (e.g. a new coin created in the cache)
* - unspent, not FRESH, DIRTY (e.g. a coin changed in the cache during a reorg)
* - unspent, not FRESH, not DIRTY (e.g. an unspent coin fetched from the parent cache)
* - spent, FRESH, not DIRTY (e.g. a spent coin fetched from the parent cache)
* - spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent)
*/
struct CCoinsCacheEntry
@ -117,12 +116,6 @@ private:
* the parent cache for batch writing. This is a performance optimization
* compared to giving all entries in the cache to the parent and having the
* parent scan for only modified entries.
*
* FRESH-but-not-DIRTY coins can not occur in practice, since that would
* mean a spent coin exists in the parent CCoinsView and not in the child
* CCoinsViewCache. Nevertheless, if a spent coin is retrieved from the
* parent cache, the FRESH-but-not-DIRTY coin will be tracked by the linked
* list and deleted when Sync or Flush is called on the CCoinsViewCache.
*/
CoinsCachePair* m_prev{nullptr};
CoinsCachePair* m_next{nullptr};

6
src/test/coins_tests.cpp
View File

@ -48,11 +48,7 @@ public:
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
{
if (auto it{map_.find(outpoint)}; it != map_.end()) {
if (!it->second.IsSpent() || m_rng.randbool()) {
return it->second; // TODO spent coins shouldn't be returned
}
}
if (auto it{map_.find(outpoint)}; it != map_.end() && !it->second.IsSpent()) return it->second;
return std::nullopt;
}

29
src/test/fuzz/coinscache_sim.cpp
View File

@ -138,8 +138,6 @@ struct CacheLevel
/** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB).
*
* The initial state consists of the empty UTXO set.
* Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent.
* This exercises code paths with spent, non-DIRTY cache entries.
*/
class CoinsViewBottom final : public CCoinsView
{
@ -148,16 +146,13 @@ class CoinsViewBottom final : public CCoinsView
public:
std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
{
// TODO GetCoin shouldn't return spent coins
if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
if (auto it{m_data.find(outpoint)}; it != m_data.end()) {
assert(!it->second.IsSpent());
return it->second;
}
return std::nullopt;
}
bool HaveCoin(const COutPoint& outpoint) const final
{
return m_data.contains(outpoint);
}
uint256 GetBestBlock() const final { return {}; }
std::vector<uint256> GetHeadBlocks() const final { return {}; }
std::unique_ptr<CCoinsViewCursor> Cursor() const final { return {}; }
@ -167,18 +162,20 @@ public:
{
for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {
if (it->second.IsDirty()) {
if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) {
if (it->second.coin.IsSpent()) {
m_data.erase(it->first);
} else if (cursor.WillErase(*it)) {
m_data[it->first] = std::move(it->second.coin);
} else {
m_data[it->first] = it->second.coin;
if (cursor.WillErase(*it)) {
m_data[it->first] = std::move(it->second.coin);
} else {
m_data[it->first] = it->second.coin;
}
}
} else {
/* For non-dirty entries being written, compare them with what we have. */
auto it2 = m_data.find(it->first);
if (it->second.coin.IsSpent()) {
assert(it2 == m_data.end() || it2->second.IsSpent());
assert(it2 == m_data.end());
} else {
assert(it2 != m_data.end());
assert(it->second.coin.out == it2->second.out);
@ -263,7 +260,7 @@ FUZZ_TARGET(coinscache_sim)
auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
// Compare results.
if (!sim.has_value()) {
assert(!realcoin || realcoin->IsSpent());
assert(!realcoin);
} else {
assert(realcoin && !realcoin->IsSpent());
const auto& simcoin = data.coins[sim->first];
@ -449,7 +446,7 @@ FUZZ_TARGET(coinscache_sim)
auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
auto sim = lookup(outpointidx, 0);
if (!sim.has_value()) {
assert(!realcoin || realcoin->IsSpent());
assert(!realcoin);
} else {
assert(realcoin && !realcoin->IsSpent());
assert(realcoin->out == data.coins[sim->first].out);

5
src/txdb.cpp
View File

@ -67,7 +67,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size)
std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
{
if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin;
if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) {
Assert(!coin.IsSpent()); // The UTXO database should never contain spent coins
return coin;
}
return std::nullopt;
}