diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..29579908726 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(); @@ -83,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); @@ -182,33 +176,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()) { @@ -273,8 +266,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(), @@ -317,18 +309,19 @@ 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 5 combinations are possible. - assert(attr != 2 && attr != 4 && 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(); // 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; @@ -337,7 +330,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; } diff --git a/src/coins.h b/src/coins.h index 61fb4af6420..d43444a9373 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}; @@ -188,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 Prev 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/coins_tests.cpp b/src/test/coins_tests.cpp index c46144b34b4..facc308dd7f 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; } @@ -60,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()) @@ -611,6 +607,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) { @@ -758,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 ); @@ -800,7 +798,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 ); @@ -809,13 +807,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 ); @@ -831,13 +829,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); @@ -855,10 +853,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/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); 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; } } diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 6000d52fc97..a585ed60a5f 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 {}; } @@ -166,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() && (it->first.n % 5) != 4) { - 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; }