From ee1e40f58000921e95f08bcb199a452eb5c4d9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 3 Jan 2026 20:26:04 +0100 Subject: [PATCH 1/4] 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. --- src/txdb.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/txdb.cpp b/src/txdb.cpp index 5d61388b078..141c31c5d51 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -67,7 +67,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size) std::optional 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; } From 3e4155fcefe0aafcc9cb84640e303e05477605a3 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:26:33 +0100 Subject: [PATCH 2/4] test: do not return spent coins from `CCoinsViewTest::GetCoin` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/test/coins_tests.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 6396fce60ac..63024054fa6 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -48,11 +48,7 @@ public: std::optional 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; } From eec551aaf1dff4cccc15e486d5618a8a44d8314c Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:28:38 +0100 Subject: [PATCH 3/4] fuzz: keep `coinscache_sim` backend free of spent coins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/test/fuzz/coinscache_sim.cpp | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index f57c25210e3..c6102c2a65c 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -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 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 GetHeadBlocks() const final { return {}; } std::unique_ptr 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); From 2ee7f9b259059d59e127852ea898b58183604b46 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 Jan 2026 20:28:55 +0100 Subject: [PATCH 4/4] coins: assume `GetCoin` only returns unspent coins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/coins.cpp | 26 +++++++++++--------------- src/coins.h | 7 ------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 7f2ffc38efa..ccd6d605ba6 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -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); } diff --git a/src/coins.h b/src/coins.h index 6da53829996..66d712be34d 100644 --- a/src/coins.h +++ b/src/coins.h @@ -102,7 +102,6 @@ using CoinsCachePair = std::pair; * - 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};