From e04bb1b4d22e74901ab63ae32c3f510bdf80204f Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:02:49 -0400 Subject: [PATCH 01/10] test: never return true from GetCoins for spent entries --- src/test/coins_tests.cpp | 6 ++---- src/test/fuzz/coinscache_sim.cpp | 9 +++------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index c46144b34b4..4915c3f47db 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -48,10 +48,8 @@ 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; } diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 6000d52fc97..443c830eae1 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,14 +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() && !it->second.IsSpent()) return it->second; return std::nullopt; } bool HaveCoin(const COutPoint& outpoint) const final { - return m_data.count(outpoint); + return GetCoin(outpoint).has_value(); } uint256 GetBestBlock() const final { return {}; } @@ -167,7 +164,7 @@ 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); From f55bac79518bb0e6a2e40d90a53a13a87b05fbeb Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:03:54 -0400 Subject: [PATCH 02/10] coins: coins returned from GetCoin cannot be spent --- src/coins.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..aa95f51e0fa 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -51,10 +51,6 @@ 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); - } } else { cacheCoins.erase(ret); return cacheCoins.end(); From 86107f71265bac28afc31529d38226d9b49c5862 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:07:11 -0400 Subject: [PATCH 03/10] coins: throw errors for non-DIRTY or spent and FRESH coins in BatchWrite There are no code paths which add a non-DIRTY entry to the cursor in BatchWrite, so we can safely make this a logic error and test for it. There are no code paths which add a spent and FRESH coin to the cursor in BatchWrite, so we can safely make this a logic error and test for it. --- src/coins.cpp | 41 ++++++++++++++++++------------------ src/test/coins_tests.cpp | 30 ++++++++++++++------------ src/test/fuzz/coins_view.cpp | 4 +++- 3 files changed, 40 insertions(+), 35 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index aa95f51e0fa..b6422946bb8 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -178,33 +178,32 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) { for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { - // Ignore non-dirty entries (optimization). if (!it->second.IsDirty()) { - continue; + throw std::logic_error("A non-DIRTY coin was returned from the cursor in BatchWrite"); + } + if (it->second.IsFresh() && it->second.coin.IsSpent()) { + throw std::logic_error("A FRESH coin was not removed when it was spent"); } CCoinsMap::iterator itUs = cacheCoins.find(it->first); if (itUs == cacheCoins.end()) { // The parent cache does not have an entry, while the child cache does. - // We can ignore it if it's both spent and FRESH in the child - if (!(it->second.IsFresh() && it->second.coin.IsSpent())) { - // Create the coin in the parent cache, move the data up - // and mark it as dirty. - itUs = cacheCoins.try_emplace(it->first).first; - CCoinsCacheEntry& entry{itUs->second}; - if (cursor.WillErase(*it)) { - // Since this entry will be erased, - // we can move the coin into us instead of copying it - entry.coin = std::move(it->second.coin); - } else { - entry.coin = it->second.coin; - } - cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); - // We can mark it FRESH in the parent if it was FRESH in the child - // Otherwise it might have just been flushed from the parent's cache - // and already exist in the grandparent - if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel); + // Create the coin in the parent cache, move the data up + // and mark it as dirty. + itUs = cacheCoins.try_emplace(it->first).first; + CCoinsCacheEntry& entry{itUs->second}; + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it + entry.coin = std::move(it->second.coin); + } else { + entry.coin = it->second.coin; } + cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); + CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + // We can mark it FRESH in the parent if it was FRESH in the child + // Otherwise it might have just been flushed from the parent's cache + // and already exist in the grandparent + if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel); } else { // Found the entry in the parent cache if (it->second.IsFresh() && !itUs->second.coin.IsSpent()) { diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 4915c3f47db..118fd67a2f8 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -609,6 +609,8 @@ constexpr MaybeCoin VALUE3_DIRTY_FRESH{{VALUE3, CoinEntry::State::DIRTY_FRESH}}; constexpr auto EX_OVERWRITE_UNSPENT{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}; constexpr auto EX_FRESH_MISAPPLIED {"FRESH flag misapplied to coin that exists in parent cache"}; +constexpr auto EX_NON_DIRTY_WRITE{"A non-DIRTY coin was returned from the cursor in BatchWrite"}; +constexpr auto EX_FRESH_AND_SPENT{"A FRESH coin was not removed when it was spent"}; static void SetCoinsValue(const CAmount value, Coin& coin) { @@ -798,7 +800,7 @@ BOOST_AUTO_TEST_CASE(ccoins_write) */ CheckWriteCoins(MISSING, MISSING, MISSING ); CheckWriteCoins(MISSING, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(MISSING, VALUE2_DIRTY, VALUE2_DIRTY ); CheckWriteCoins(MISSING, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); CheckWriteCoins(SPENT_CLEAN, MISSING, SPENT_CLEAN ); @@ -807,13 +809,13 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(SPENT_DIRTY_FRESH, MISSING, SPENT_DIRTY_FRESH ); CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); @@ -829,13 +831,13 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(VALUE1_DIRTY, MISSING, VALUE1_DIRTY ); CheckWriteCoins(VALUE1_DIRTY_FRESH, MISSING, VALUE1_DIRTY_FRESH ); CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_AND_SPENT ); CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); @@ -853,10 +855,12 @@ BOOST_AUTO_TEST_CASE(ccoins_write) for (const MaybeCoin& parent : {MISSING, SPENT_CLEAN, SPENT_DIRTY, SPENT_FRESH, SPENT_DIRTY_FRESH, VALUE1_CLEAN, VALUE1_DIRTY, VALUE1_FRESH, VALUE1_DIRTY_FRESH}) { - for (const MaybeCoin& child : {MISSING, - SPENT_CLEAN, SPENT_FRESH, - VALUE2_CLEAN, VALUE2_FRESH}) { - auto expected{CoinOrError{parent}}; // TODO test failure cases as well + for (const MaybeCoin& child : {MISSING, SPENT_CLEAN, VALUE2_CLEAN}) { + auto expected{CoinOrError{parent}}; + CheckWriteCoins(parent, child, expected); + } + for (const MaybeCoin& child : {SPENT_FRESH, VALUE2_FRESH}) { + auto expected{CoinOrError{EX_NON_DIRTY_WRITE}}; CheckWriteCoins(parent, child, expected); } } diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 9c6aa6e7a1e..470bad1f6c1 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -151,7 +151,9 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) coins_view_cache.BatchWrite(cursor, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); expected_code_path = true; } catch (const std::logic_error& e) { - if (e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"}) { + if (e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"} + || e.what() == std::string{"A FRESH coin was not removed when it was spent"} + || e.what() == std::string{"A non-DIRTY coin was returned from the cursor in BatchWrite"}) { expected_code_path = true; } } From f44fbaea5a3ab9461c8bfc26a8cf841acaeb94f1 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:12:46 -0400 Subject: [PATCH 04/10] doc: update comments to reflect possible code paths --- src/coins.h | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/coins.h b/src/coins.h index 61fb4af6420..2b4f48a5a18 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}; @@ -195,7 +188,7 @@ public: return m_next; } - //! Only call Prev when this entry is DIRTY, FRESH, or both + //! Only call Next when this entry is DIRTY, FRESH, or both CoinsCachePair* Prev() const noexcept { Assume(m_flags); From f9d50f9f5ab58e6a57977efd065a1619508b5656 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:13:44 -0400 Subject: [PATCH 05/10] coins: remove redundant IsDirty checks in BatchWrite It is no longer possible to get non-DIRTY entries in BatchWrite --- src/test/coins_tests.cpp | 14 ++++++-------- src/test/fuzz/coinscache_sim.cpp | 24 ++++++------------------ src/txdb.cpp | 20 +++++++++----------- 3 files changed, 21 insertions(+), 37 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 118fd67a2f8..8738e728c17 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -58,14 +58,12 @@ public: bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){ - if (it->second.IsDirty()) { - // Same optimization used in CCoinsViewDB is to only write dirty entries. - map_[it->first] = it->second.coin; - if (it->second.coin.IsSpent() && m_rng.randrange(3) == 0) { - // Randomly delete empty entries on write. - map_.erase(it->first); - } + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + assert(it->second.IsDirty()); + map_[it->first] = it->second.coin; + if (it->second.coin.IsSpent() && m_rng.randrange(3) == 0) { + // Randomly delete spent entries on write. + map_.erase(it->first); } } if (!hashBlock.IsNull()) diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 443c830eae1..a585ed60a5f 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -163,25 +163,13 @@ public: bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { - if (it->second.IsDirty()) { - 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; - } + assert(it->second.IsDirty()); + 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 { - /* 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()); - } else { - assert(it2 != m_data.end()); - assert(it->second.coin.out == it2->second.out); - assert(it->second.coin.fCoinBase == it2->second.fCoinBase); - assert(it->second.coin.nHeight == it2->second.nHeight); - } + m_data[it->first] = it->second.coin; } } return true; diff --git a/src/txdb.cpp b/src/txdb.cpp index 1622039d63b..f286233a124 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -93,7 +94,6 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { CDBBatch batch(*m_db); size_t count = 0; - size_t changed = 0; assert(!hashBlock.IsNull()); uint256 old_tip = GetBestBlock(); @@ -116,17 +116,15 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB batch.Erase(DB_BEST_BLOCK); batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip)); - for (auto it{cursor.Begin()}; it != cursor.End();) { - if (it->second.IsDirty()) { - CoinEntry entry(&it->first); - if (it->second.coin.IsSpent()) - batch.Erase(entry); - else - batch.Write(entry, it->second.coin); - changed++; + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + Assume(it->second.IsDirty()); + CoinEntry entry(&it->first); + if (it->second.coin.IsSpent()) { + batch.Erase(entry); + } else { + batch.Write(entry, it->second.coin); } count++; - it = cursor.NextAndMaybeErase(*it); if (batch.SizeEstimate() > m_options.batch_write_bytes) { LogDebug(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); @@ -147,7 +145,7 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); bool ret = m_db->WriteBatch(batch); - LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count); + LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs to coin database...", count); return ret; } From 87de3e6c3271be94c733c9bdb6c01ad3c6f85720 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:15:12 -0400 Subject: [PATCH 06/10] coins: remove IsFresh check in Uncache It is not possible for an entry to be FRESH if not already DIRTY. --- src/coins.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index b6422946bb8..30610b45711 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -268,8 +268,7 @@ bool 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 (auto it{cacheCoins.find(hash)}; it != cacheCoins.end() && !it->second.IsDirty()) { cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, uncache, hash.hash.data(), From 2c3e62d4526809e9d2f5a01ff4d049fb6754cce6 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 18 Aug 2024 17:17:11 -0400 Subject: [PATCH 07/10] test: don't check for FRESH but not DIRTY entries in SanityCheck --- src/coins.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 30610b45711..9f8a685c81c 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -315,14 +315,14 @@ void CCoinsViewCache::SanityCheck() const 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); + // Only 4 combinations are possible. + assert(attr != 2 && attr != 4 && attr != 6 && attr != 7); // 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_flagged; } // Iterate over the linked list of flagged entries. size_t count_linked = 0; @@ -331,7 +331,7 @@ 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; } From 147cd505010f0cd53b2ad2bd57cb70e14d68f394 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 31 Aug 2024 14:19:38 -0400 Subject: [PATCH 08/10] coins: assume entry is dirty in Next and Prev --- src/coins.h | 6 ++---- src/test/coinscachepair_tests.cpp | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/coins.h b/src/coins.h index 2b4f48a5a18..d43444a9373 100644 --- a/src/coins.h +++ b/src/coins.h @@ -181,17 +181,15 @@ public: bool IsDirty() const noexcept { return m_flags & DIRTY; } bool IsFresh() const noexcept { return m_flags & FRESH; } - //! Only call Next when this entry is DIRTY, FRESH, or both CoinsCachePair* Next() const noexcept { - Assume(m_flags); + Assume(IsDirty()); return m_next; } - //! Only call Next when this entry is DIRTY, FRESH, or both CoinsCachePair* Prev() const noexcept { - Assume(m_flags); + Assume(IsDirty()); return m_prev; } diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp index 0c208e93dfb..01ab325d9a5 100644 --- a/src/test/coinscachepair_tests.cpp +++ b/src/test/coinscachepair_tests.cpp @@ -154,9 +154,10 @@ BOOST_AUTO_TEST_CASE(linked_list_set_state) BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); - // Check that setting FRESH on new node inserts it after n1 + // Check that setting DIRTY and FRESH on new node inserts it after n1 + CCoinsCacheEntry::SetDirty(n2, sentinel); CCoinsCacheEntry::SetFresh(n2, sentinel); - BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty()); + BOOST_CHECK(n2.second.IsDirty() && n2.second.IsFresh()); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); BOOST_CHECK_EQUAL(n1.second.Next(), &n2); From 4c9c30eda3707f451fb7d5a652159d66ba90d9d7 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 31 Aug 2024 14:20:12 -0400 Subject: [PATCH 09/10] test: simplify coins sanity check --- src/coins.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 9f8a685c81c..4fc06d79cd8 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -311,12 +311,13 @@ void CCoinsViewCache::SanityCheck() const size_t recomputed_usage = 0; size_t count_flagged = 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 4 combinations are possible. - assert(attr != 2 && attr != 4 && attr != 6 && attr != 7); + if (entry.coin.IsSpent()) { + // A spent coin must be dirty and cannot be fresh + assert(entry.IsDirty() && !entry.IsFresh()); + } else { + // An unspent coin must not be fresh if not dirty + assert(entry.IsDirty() || !entry.IsFresh()); + } // Recompute cachedCoinsUsage. recomputed_usage += entry.coin.DynamicMemoryUsage(); From 6956ee9cc76c7815e53fbdee01edc4379d8caafc Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 31 Aug 2024 15:11:21 -0400 Subject: [PATCH 10/10] coins: set all inserted spent coins to fresh A spent coins must be DIRTY, so remove references to spent but not DIRTY coins. The only way a spent coin can be not DIRTY is when creating the CCoinsCacheEntry with an empty coin. This can be made more clear by setting fresh if inserted, instead of checking if an unspent coin is not DIRTY. --- src/coins.cpp | 12 +++++------- src/test/coins_tests.cpp | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 4fc06d79cd8..29579908726 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -79,20 +79,18 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi if (!it->second.coin.IsSpent()) { throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)"); } - // If the coin exists in this cache as a spent coin and is DIRTY, then + // If the coin exists in this cache as a spent coin, then // its spentness hasn't been flushed to the parent cache. We're // re-adding the coin to this cache now but we can't mark it as FRESH. - // If we mark it FRESH and then spend it before the cache is flushed - // we would remove it from this cache and would never flush spentness - // to the parent cache. + // A spent FRESH coin cannot exist in the cache because a FRESH coin + // is simply erased when it is spent. // // Re-adding a spent coin can happen in the case of a re-org (the coin // is 'spent' when the block adding it is disconnected and then // re-added when it is also added in a newly connected block). // - // If the coin doesn't exist in the current cache, or is spent but not - // DIRTY, then it can be marked FRESH. - fresh = !it->second.IsDirty(); + // If the coin doesn't exist in the current cache then it can be marked FRESH. + fresh = inserted; } it->second.coin = std::move(coin); CCoinsCacheEntry::SetDirty(*it, m_sentinel); diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 8738e728c17..facc308dd7f 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -756,7 +756,7 @@ BOOST_AUTO_TEST_CASE(ccoins_add) CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY_FRESH, false); CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, false); CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, true ); CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true );