diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e2d6f94b2491e..3a6d56c9e1a54 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -85,6 +85,7 @@ BITCOIN_TESTS =\ test/checkqueue_tests.cpp \ test/cluster_linearize_tests.cpp \ test/coins_tests.cpp \ + test/coinscachepair_tests.cpp \ test/coinstatsindex_tests.cpp \ test/common_url_tests.cpp \ test/compilerbug_tests.cpp \ diff --git a/src/coins.cpp b/src/coins.cpp index a4e4d4ad322c2..91e02331f51bb 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -12,7 +12,7 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } -bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; } +bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; } std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const @@ -27,14 +27,16 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); } +bool CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return base->BatchWrite(cursor, hashBlock); } std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) : CCoinsViewBacked(baseIn), m_deterministic(deterministic), cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource) -{} +{ + m_sentinel.second.SelfRef(m_sentinel); +} size_t CCoinsViewCache::DynamicMemoryUsage() const { return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage; @@ -51,7 +53,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const if (ret->second.coin.IsSpent()) { // The parent only has an empty entry for this outpoint; we can consider our // version as fresh. - ret->second.flags = CCoinsCacheEntry::FRESH; + ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); } cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); return ret; @@ -93,10 +95,10 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi // // 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.flags & CCoinsCacheEntry::DIRTY); + fresh = !it->second.IsDirty(); } it->second.coin = std::move(coin); - it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0); + it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, add, outpoint.hash.data(), @@ -108,10 +110,13 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { cachedCoinsUsage += coin.DynamicMemoryUsage(); - cacheCoins.emplace( + auto [it, inserted] = cacheCoins.emplace( std::piecewise_construct, std::forward_as_tuple(std::move(outpoint)), - std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY)); + std::forward_as_tuple(std::move(coin))); + if (inserted) { + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); + } } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { @@ -138,10 +143,10 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (moveout) { *moveout = std::move(it->second.coin); } - if (it->second.flags & CCoinsCacheEntry::FRESH) { + if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.flags |= CCoinsCacheEntry::DIRTY; + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); it->second.coin.Clear(); } return true; @@ -178,42 +183,40 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) { - for (CCoinsMap::iterator it = mapCoins.begin(); - it != mapCoins.end(); - it = erase ? mapCoins.erase(it) : std::next(it)) { +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.flags & CCoinsCacheEntry::DIRTY)) { + if (!it->second.IsDirty()) { continue; } 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.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) { + if (!(it->second.IsFresh() && it->second.coin.IsSpent())) { // Create the coin in the parent cache, move the data up // and mark it as dirty. - CCoinsCacheEntry& entry = cacheCoins[it->first]; - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + 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(); - entry.flags = CCoinsCacheEntry::DIRTY; + entry.AddFlags(CCoinsCacheEntry::DIRTY, *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.flags & CCoinsCacheEntry::FRESH) { - entry.flags |= CCoinsCacheEntry::FRESH; + if (it->second.IsFresh()) { + entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel); } } } else { // Found the entry in the parent cache - if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coin.IsSpent()) { + if (it->second.IsFresh() && !itUs->second.coin.IsSpent()) { // The coin was marked FRESH in the child cache, but the coin // exists in the parent cache. If this ever happens, it means // the FRESH flag was misapplied and there is a logic error in @@ -221,7 +224,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn throw std::logic_error("FRESH flag misapplied to coin that exists in parent cache"); } - if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coin.IsSpent()) { + if (itUs->second.IsFresh() && it->second.coin.IsSpent()) { // The grandparent cache does not have an entry, and the coin // has been spent. We can just delete it from the parent cache. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); @@ -229,16 +232,15 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } else { // A normal modification. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it itUs->second.coin = std::move(it->second.coin); } else { itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.flags |= CCoinsCacheEntry::DIRTY; + itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); // NOTE: It isn't safe to mark the coin as FRESH in the parent // cache. If it already existed and was spent in the parent // cache then marking it FRESH would prevent that spentness @@ -251,12 +253,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } bool CCoinsViewCache::Flush() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true); + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { - if (!cacheCoins.empty()) { - /* BatchWrite must erase all cacheCoins elements when erase=true. */ - throw std::logic_error("Not all cached coins were erased"); - } + cacheCoins.clear(); ReallocateCache(); } cachedCoinsUsage = 0; @@ -265,16 +265,12 @@ bool CCoinsViewCache::Flush() { bool CCoinsViewCache::Sync() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false); - // Instead of clearing `cacheCoins` as we would in Flush(), just clear the - // FRESH/DIRTY flags of any coin that isn't spent. - for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) { - if (it->second.coin.IsSpent()) { - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); - it = cacheCoins.erase(it); - } else { - it->second.flags = 0; - ++it; + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + bool fOk = base->BatchWrite(cursor, hashBlock); + if (fOk) { + if (m_sentinel.second.Next() != &m_sentinel) { + /* BatchWrite must clear flags of all entries */ + throw std::logic_error("Not all unspent flagged entries were cleared"); } } return fOk; @@ -283,7 +279,7 @@ bool CCoinsViewCache::Sync() void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(hash); - if (it != cacheCoins.end() && it->second.flags == 0) { + if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) { cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, uncache, hash.hash.data(), @@ -324,17 +320,33 @@ void CCoinsViewCache::ReallocateCache() void CCoinsViewCache::SanityCheck() const { size_t recomputed_usage = 0; + size_t count_flagged = 0; for (const auto& [_, entry] : cacheCoins) { unsigned attr = 0; - if (entry.flags & CCoinsCacheEntry::DIRTY) attr |= 1; - if (entry.flags & CCoinsCacheEntry::FRESH) attr |= 2; + 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); // 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; + } + // Iterate over the linked list of flagged entries. + size_t count_linked = 0; + for (auto it = m_sentinel.second.Next(); it != &m_sentinel; it = it->second.Next()) { + // Verify linked list integrity. + 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()); + // Count the number of entries actually in the list. + ++count_linked; } + assert(count_linked == count_flagged); assert(recomputed_usage == cachedCoinsUsage); } diff --git a/src/coins.h b/src/coins.h index 76e64b641d241..78b8eddacd79b 100644 --- a/src/coins.h +++ b/src/coins.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -86,6 +87,9 @@ class Coin } }; +struct CCoinsCacheEntry; +using CoinsCachePair = std::pair; + /** * A Coin in one level of the coins database caching hierarchy. * @@ -103,8 +107,29 @@ class Coin */ struct CCoinsCacheEntry { +private: + /** + * These are used to create a doubly linked list of flagged entries. + * They are set in AddFlags and unset in ClearFlags. + * A flagged entry is any entry that is either DIRTY, FRESH, or both. + * + * DIRTY entries are tracked so that only modified entries can be passed to + * 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}; + uint8_t m_flags{0}; + +public: Coin coin; // The actual cached data. - unsigned char flags; enum Flags { /** @@ -127,9 +152,59 @@ struct CCoinsCacheEntry FRESH = (1 << 1), }; - CCoinsCacheEntry() : flags(0) {} - explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {} - CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {} + CCoinsCacheEntry() noexcept = default; + explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} + ~CCoinsCacheEntry() + { + ClearFlags(); + } + + //! Adding a flag also requires a self reference to the pair that contains + //! this entry in the CCoinsCache map and a reference to the sentinel of the + //! flagged pair linked list. + inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept + { + Assume(&self.second == this); + if (!m_flags && flags) { + m_prev = sentinel.second.m_prev; + m_next = &sentinel; + sentinel.second.m_prev = &self; + m_prev->second.m_next = &self; + } + m_flags |= flags; + } + inline void ClearFlags() noexcept + { + if (!m_flags) return; + m_next->second.m_prev = m_prev; + m_prev->second.m_next = m_next; + m_flags = 0; + } + inline uint8_t GetFlags() const noexcept { return m_flags; } + inline bool IsDirty() const noexcept { return m_flags & DIRTY; } + inline bool IsFresh() const noexcept { return m_flags & FRESH; } + + //! Only call Next when this entry is DIRTY, FRESH, or both + inline CoinsCachePair* Next() const noexcept { + Assume(m_flags); + return m_next; + } + + //! Only call Prev when this entry is DIRTY, FRESH, or both + inline CoinsCachePair* Prev() const noexcept { + Assume(m_flags); + return m_prev; + } + + //! Only use this for initializing the linked list sentinel + inline void SelfRef(CoinsCachePair& self) noexcept + { + Assume(&self.second == this); + m_prev = &self; + m_next = &self; + // Set sentinel to DIRTY so we can call Next on it + m_flags = DIRTY; + } }; /** @@ -144,8 +219,8 @@ using CCoinsMap = std::unordered_map, - PoolAllocator, - sizeof(std::pair) + sizeof(void*) * 4>>; + PoolAllocator>; using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; @@ -168,6 +243,62 @@ class CCoinsViewCursor uint256 hashBlock; }; +/** + * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache. + * + * This is a helper struct to encapsulate the diverging logic between a non-erasing + * CCoinsViewCache::Sync and an erasing CCoinsViewCache::Flush. This allows the receiver + * of CCoinsView::BatchWrite to iterate through the flagged entries without knowing + * the caller's intent. + * + * However, the receiver can still call CoinsViewCacheCursor::WillErase to see if the + * caller will erase the entry after BatchWrite returns. If so, the receiver can + * perform optimizations such as moving the coin out of the CCoinsCachEntry instead + * of copying it. + */ +struct CoinsViewCacheCursor +{ + //! If will_erase is not set, iterating through the cursor will erase spent coins from the map, + //! and other coins will be unflagged (removing them from the linked list). + //! If will_erase is set, the underlying map and linked list will not be modified, + //! as the caller is expected to wipe the entire map anyway. + //! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set. + //! Calling CCoinsMap::clear() afterwards is faster because a CoinsCachePair cannot be coerced back into a + //! CCoinsMap::iterator to be erased, and must therefore be looked up again by key in the CCoinsMap before being erased. + CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND, + CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_usage(usage), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + + inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } + inline CoinsCachePair* End() const noexcept { return &m_sentinel; } + + //! Return the next entry after current, possibly erasing current + inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept + { + const auto next_entry{current.second.Next()}; + // If we are not going to erase the cache, we must still erase spent entries. + // Otherwise clear the flags on the entry. + if (!m_will_erase) { + if (current.second.coin.IsSpent()) { + m_usage -= current.second.coin.DynamicMemoryUsage(); + m_map.erase(current.first); + } else { + current.second.ClearFlags(); + } + } + return next_entry; + } + + inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } +private: + size_t& m_usage; + CoinsCachePair& m_sentinel; + CCoinsMap& m_map; + bool m_will_erase; +}; + /** Abstract view on the open txout dataset. */ class CCoinsView { @@ -191,8 +322,8 @@ class CCoinsView virtual std::vector GetHeadBlocks() const; //! Do a bulk modification (multiple Coin changes + BestBlock change). - //! The passed mapCoins can be modified. - virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true); + //! The passed cursor is used to iterate through the coins. + virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock); //! Get a cursor to iterate over the whole state virtual std::unique_ptr Cursor() const; @@ -218,7 +349,7 @@ class CCoinsViewBacked : public CCoinsView uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override; size_t EstimateSize() const override; }; @@ -237,6 +368,8 @@ class CCoinsViewCache : public CCoinsViewBacked */ mutable uint256 hashBlock; mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{}; + /* The starting sentinel of the flagged entry circular doubly linked list. */ + mutable CoinsCachePair m_sentinel; mutable CCoinsMap cacheCoins; /* Cached dynamic memory usage for the inner Coin objects. */ @@ -255,7 +388,7 @@ class CCoinsViewCache : public CCoinsViewBacked bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index a992e2fa0331c..fb929a2b0e0b5 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,10 +55,10 @@ class CCoinsViewTest : public CCoinsView uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + 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() && InsecureRandRange(3) == 0) { @@ -78,7 +78,7 @@ class CCoinsViewCacheTest : public CCoinsViewCache public: explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsViewCache(_base) {} - void SelfTest() const + void SelfTest(bool sanity_check = true) const { // Manually recompute the dynamic usage of the whole data, and compare it. size_t ret = memusage::DynamicUsage(cacheCoins); @@ -89,9 +89,13 @@ class CCoinsViewCacheTest : public CCoinsViewCache } BOOST_CHECK_EQUAL(GetCacheSize(), count); BOOST_CHECK_EQUAL(DynamicMemoryUsage(), ret); + if (sanity_check) { + SanityCheck(); + } } CCoinsMap& map() const { return cacheCoins; } + CoinsCachePair& sentinel() const { return m_sentinel; } size_t& usage() const { return cachedCoinsUsage; } }; @@ -576,7 +580,7 @@ static void SetCoinsValue(CAmount value, Coin& coin) } } -static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) +static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmount value, char flags) { if (value == ABSENT) { assert(flags == NO_ENTRY); @@ -584,10 +588,10 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) } assert(flags != NO_ENTRY); CCoinsCacheEntry entry; - entry.flags = flags; SetCoinsValue(value, entry.coin); auto inserted = map.emplace(OUTPOINT, std::move(entry)); assert(inserted.second); + inserted.first->second.AddFlags(flags, *inserted.first, sentinel); return inserted.first->second.coin.DynamicMemoryUsage(); } @@ -603,17 +607,20 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C } else { value = it->second.coin.out.nValue; } - flags = it->second.flags; + flags = it->second.GetFlags(); assert(flags != NO_ENTRY); } } void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) { + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - InsertCoinsMapEntry(map, value, flags); - BOOST_CHECK(view.BatchWrite(map, {})); + auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)}; + auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; + BOOST_CHECK(view.BatchWrite(cursor, {})); } class SingleEntryCacheTest @@ -622,7 +629,7 @@ class SingleEntryCacheTest SingleEntryCacheTest(CAmount base_value, CAmount cache_value, char cache_flags) { WriteCoinsViewEntry(base, base_value, base_value == ABSENT ? NO_ENTRY : DIRTY); - cache.usage() += InsertCoinsMapEntry(cache.map(), cache_value, cache_flags); + cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_value, cache_flags); } CCoinsView root; @@ -634,7 +641,7 @@ static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount exp { SingleEntryCacheTest test(base_value, cache_value, cache_flags); test.cache.AccessCoin(OUTPOINT); - test.cache.SelfTest(); + test.cache.SelfTest(/*sanity_check=*/false); CAmount result_value; char result_flags; @@ -803,7 +810,7 @@ void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected char result_flags; try { WriteCoinsViewEntry(test.cache, child_value, child_flags); - test.cache.SelfTest(); + test.cache.SelfTest(/*sanity_check=*/false); GetCoinsMapEntry(test.cache.map(), result_value, result_flags); } catch (std::logic_error&) { result_value = FAIL; @@ -916,6 +923,7 @@ void TestFlushBehavior( // Flush in reverse order to ensure that flushes happen from children up. for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) { auto& cache = *i; + cache->SanityCheck(); // hashBlock must be filled before flushing to disk; value is // unimportant here. This is normally done during connect/disconnect block. cache->SetBestBlock(InsecureRand256()); diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp new file mode 100644 index 0000000000000..61840f1f0925b --- /dev/null +++ b/src/test/coinscachepair_tests.cpp @@ -0,0 +1,219 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include + +BOOST_AUTO_TEST_SUITE(coinscachepair_tests) + +static constexpr auto NUM_NODES{4}; + +std::list CreatePairs(CoinsCachePair& sentinel) +{ + std::list nodes; + for (auto i{0}; i < NUM_NODES; ++i) { + nodes.emplace_back(); + + auto node{std::prev(nodes.end())}; + node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel); + + BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node)); + + if (i > 0) { + BOOST_CHECK_EQUAL(std::prev(node)->second.Next(), &(*node)); + BOOST_CHECK_EQUAL(node->second.Prev(), &(*std::prev(node))); + } + } + return nodes; +} + +BOOST_AUTO_TEST_CASE(linked_list_iteration) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Check iterating through pairs is identical to iterating through a list + auto node{sentinel.second.Next()}; + for (const auto& expected : nodes) { + BOOST_CHECK_EQUAL(&expected, node); + node = node->second.Next(); + } + BOOST_CHECK_EQUAL(node, &sentinel); + + // Check iterating through pairs is identical to iterating through a list + // Clear the flags during iteration + node = sentinel.second.Next(); + for (const auto& expected : nodes) { + BOOST_CHECK_EQUAL(&expected, node); + auto next = node->second.Next(); + node->second.ClearFlags(); + node = next; + } + BOOST_CHECK_EQUAL(node, &sentinel); + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); + + // Delete the nodes from the list to make sure there are no dangling pointers + for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) { + BOOST_CHECK_EQUAL(it->second.GetFlags(), 0); + } +} + +BOOST_AUTO_TEST_CASE(linked_list_iterate_erase) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Check iterating through pairs is identical to iterating through a list + // Erase the nodes as we iterate through, but don't clear flags + // The flags will be cleared by the CCoinsCacheEntry's destructor + auto node{sentinel.second.Next()}; + for (auto expected{nodes.begin()}; expected != nodes.end(); expected = nodes.erase(expected)) { + BOOST_CHECK_EQUAL(&(*expected), node); + node = node->second.Next(); + } + BOOST_CHECK_EQUAL(node, &sentinel); + + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); +} + +BOOST_AUTO_TEST_CASE(linked_list_random_deletion) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + auto nodes{CreatePairs(sentinel)}; + + // Create linked list sentinel->n1->n2->n3->n4->sentinel + auto n1{nodes.begin()}; + auto n2{std::next(n1)}; + auto n3{std::next(n2)}; + auto n4{std::next(n3)}; + + // Delete n2 + // sentinel->n1->n3->n4->sentinel + nodes.erase(n2); + // Check that n1 now points to n3, and n3 still points to n4 + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); + BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1)); + + // Delete n1 + // sentinel->n3->n4->sentinel + nodes.erase(n1); + // Check that sentinel now points to n3, and n3 still points to n4 + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); + BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel); + + // Delete n4 + // sentinel->n3->sentinel + nodes.erase(n4); + // Check that sentinel still points to n3, and n3 points to sentinel + // Also check that flags were not altered + BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); + BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3)); + + // Delete n3 + // sentinel->sentinel + nodes.erase(n3); + // Check that sentinel's next and prev are itself + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); +} + +BOOST_AUTO_TEST_CASE(linked_list_add_flags) +{ + CoinsCachePair sentinel; + sentinel.second.SelfRef(sentinel); + CoinsCachePair n1; + CoinsCachePair n2; + + // Check that adding 0 flag has no effect + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); + + // Check that adding DIRTY flag inserts it into linked list and sets flags + n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); + + // Check that adding FRESH flag on new node inserts it after n1 + n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel); + BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + + // Check that adding 0 flag has no effect, and doesn't change position + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + + // Check that we can add extra flags, but they don't change our position + n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH); + BOOST_CHECK_EQUAL(n1.second.Next(), &n2); + BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1); + BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); + + // Check that we can clear flags then re-add them + n1.second.ClearFlags(); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // Check that calling `ClearFlags` with 0 flags has no effect + n1.second.ClearFlags(); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // Adding 0 still has no effect + n1.second.AddFlags(0, n1, sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel); + + // But adding DIRTY re-inserts it after n2 + n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel); + BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK_EQUAL(n2.second.Next(), &n1); + BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); + BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); + BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 41c971c237b9d..368c69819a0e8 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -120,12 +120,15 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) random_mutable_transaction = *opt_mutable_transaction; }, [&] { + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); + size_t usage{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - coins_cache_entry.flags = fuzzed_data_provider.ConsumeIntegral(); + const auto flags{fuzzed_data_provider.ConsumeIntegral()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -136,11 +139,14 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) } coins_cache_entry.coin = *opt_coin; } - coins_map.emplace(random_out_point, std::move(coins_cache_entry)); + auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; + it->second.AddFlags(flags, *it, sentinel); + usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; try { - coins_view_cache.BatchWrite(coins_map, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); + auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + 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"}) { diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 648e96b4a05b2..8e717e96b4ed8 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -172,13 +172,13 @@ class CoinsViewBottom final : public CCoinsView std::unique_ptr Cursor() const final { return {}; } size_t EstimateSize() const final { return m_data.size(); } - bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { - for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + 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 (erase) { + } else if (cursor.WillErase(*it)) { m_data[it->first] = std::move(it->second.coin); } else { m_data[it->first] = it->second.coin; diff --git a/src/txdb.cpp b/src/txdb.cpp index e4a4b3bf72a01..3865692b6a6b6 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -88,7 +88,7 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { +bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; @@ -114,8 +114,8 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo batch.Erase(DB_BEST_BLOCK); batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip)); - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + for (auto it{cursor.Begin()}; it != cursor.End();) { + if (it->second.IsDirty()) { CoinEntry entry(&it->first); if (it->second.coin.IsSpent()) batch.Erase(entry); @@ -124,7 +124,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo changed++; } count++; - it = erase ? mapCoins.erase(it) : std::next(it); + it = cursor.NextAndMaybeErase(*it); if (batch.SizeEstimate() > m_options.batch_write_bytes) { LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); diff --git a/src/txdb.h b/src/txdb.h index c9af0a091ec68..e0acb09e98a93 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -63,7 +63,7 @@ class CCoinsViewDB final : public CCoinsView bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override; //! Whether an unsupported database format is used. diff --git a/src/validation.cpp b/src/validation.cpp index f7250778fcb3f..219e72f1b15bd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2902,7 +2902,7 @@ bool Chainstate::FlushStateToDisk( return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!")); } // Flush the chainstate (which may refer to block index entries). - const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical || fFlushForPrune}; + const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical}; if (empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) { return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database.")); }