diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 4c9e81a354..ea5937d6ca 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -88,6 +88,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 e6e34afcd8..5e0ad8de48 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; } // ELEMENTS: bool CCoinsView::IsPeginSpent(const std::pair &outpoint) const { return false; } @@ -29,7 +29,7 @@ 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(); } // ELEMENTS: @@ -38,7 +38,9 @@ bool CCoinsViewBacked::IsPeginSpent(const std::pair &outpoin 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; @@ -63,7 +65,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; @@ -106,10 +108,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(), @@ -121,10 +123,10 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { cachedCoinsUsage += coin.DynamicMemoryUsage(); - cacheCoins.emplace( - std::piecewise_construct, - std::forward_as_tuple(native_key(outpoint)), - std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY)); + auto [it, inserted] = cacheCoins.try_emplace(native_key(outpoint), 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) { @@ -151,10 +153,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; @@ -199,13 +201,11 @@ bool CCoinsViewCache::IsPeginSpent(const std::pair &outpoint CCoinsMap::iterator it = cacheCoins.find(outpoint); if (it == cacheCoins.end()) { - bool inserted; - std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, - std::forward_as_tuple(outpoint), std::tuple<>()); + it = cacheCoins.try_emplace(outpoint).first; it->second.peginSpent = base->IsPeginSpent(outpoint); - it->second.flags |= CCoinsCacheEntry::PEGIN; + it->second.AddFlags(CCoinsCacheEntry::PEGIN, *it, m_sentinel); if (!it->second.peginSpent) - it->second.flags |= CCoinsCacheEntry::FRESH; + it->second.AddFlags(CCoinsCacheEntry::FRESH, *it, m_sentinel); } return it->second.peginSpent; } @@ -227,14 +227,12 @@ void CCoinsViewCache::SetPeginSpent(const std::pair &outpoin return; if (it == cacheCoins.end()) { - bool inserted; - std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, - std::forward_as_tuple(outpoint), std::tuple<>()); + it = cacheCoins.try_emplace(outpoint).first; if (!hadSpent) - it->second.flags = CCoinsCacheEntry::FRESH; + it->second.AddFlags(CCoinsCacheEntry::FRESH, *it, m_sentinel); } it->second.peginSpent = fSpent; - it->second.flags |= CCoinsCacheEntry::PEGIN | CCoinsCacheEntry::DIRTY; + it->second.AddFlags(CCoinsCacheEntry::PEGIN | CCoinsCacheEntry::DIRTY, *it, m_sentinel); } // END ELEMENTS @@ -250,52 +248,53 @@ 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; } // ELEMENTS: - bool fIsPegin = it->second.flags & CCoinsCacheEntry::PEGIN; + bool fIsPegin = it->second.GetFlags() & CCoinsCacheEntry::PEGIN; CCoinsMap::iterator itUs = cacheCoins.find(it->first); if (itUs == cacheCoins.end()) { - // The parent cache does not have an entry, while the child cache does + // The parent cache does not have an entry, while the child cache does. // We can ignore it if it's both FRESH and {pruned, spent pegin} in the child - if (!((it->second.flags & CCoinsCacheEntry::FRESH) && + if (!(it->second.IsFresh() && (( fIsPegin && !it->second.peginSpent) || (!fIsPegin && 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]; - entry.flags = CCoinsCacheEntry::DIRTY; + itUs = cacheCoins.try_emplace(it->first).first; + CCoinsCacheEntry& entry{itUs->second}; if (fIsPegin) { entry.peginSpent = it->second.peginSpent; - entry.flags |= CCoinsCacheEntry::PEGIN; } - 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 entry.coin = std::move(it->second.coin); } else { entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); + uint8_t parent_flags = CCoinsCacheEntry::DIRTY; + if (fIsPegin) { + parent_flags |= CCoinsCacheEntry::PEGIN; + } + entry.AddFlags(parent_flags, *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 @@ -303,7 +302,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) && + if (itUs->second.IsFresh() && ((fIsPegin && !it->second.peginSpent) || (!fIsPegin && 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. @@ -312,10 +311,9 @@ 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; @@ -325,7 +323,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn itUs->second.peginSpent = it->second.peginSpent; } 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 @@ -338,12 +336,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; @@ -352,16 +348,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; @@ -370,7 +362,7 @@ bool CCoinsViewCache::Sync() void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(native_key(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, it->first.second.hash.data(), @@ -414,17 +406,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 87625032ca..f188d305a4 100644 --- a/src/coins.h +++ b/src/coins.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -86,6 +87,9 @@ public: } }; +struct CCoinsCacheEntry; +using CoinsCachePair = std::pair; + /** * A Coin in one level of the coins database caching hierarchy. * @@ -103,8 +107,29 @@ public: */ 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; // ELEMENTS: bool peginSpent; @@ -135,9 +160,59 @@ struct CCoinsCacheEntry PEGIN = (1 << 2), }; - 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; + } }; /** @@ -152,8 +227,8 @@ using CCoinsMap = std::unordered_map, - PoolAllocator, - sizeof(std::pair) + sizeof(void*) * 4>>; + PoolAllocator>; using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; @@ -176,6 +251,62 @@ private: 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 { @@ -203,8 +334,8 @@ public: 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; @@ -230,7 +361,7 @@ public: 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; // ELEMENTS: @@ -251,6 +382,8 @@ protected: */ 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. */ @@ -269,7 +402,7 @@ public: 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 6b7e1abb78..5f3be71d72 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,10 +55,10 @@ public: 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.second] = 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 @@ public: } 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; } }; @@ -577,7 +581,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); @@ -585,10 +589,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(); } @@ -604,17 +608,20 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C } else { value = it->second.coin.out.nValue.GetAmount(); } - 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 @@ -623,7 +630,7 @@ public: 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; @@ -635,7 +642,7 @@ static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount exp { SingleEntryCacheTest test(base_value, cache_value, cache_flags); test.cache.AccessCoin(OUTPOINT.second); - test.cache.SelfTest(); + test.cache.SelfTest(/*sanity_check=*/false); CAmount result_value; char result_flags; @@ -804,7 +811,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; @@ -917,6 +924,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 0000000000..61840f1f09 --- /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 23e5b96f50..eeba88da87 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 { @@ -137,19 +140,24 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) coins_cache_entry.coin = *opt_coin; } // ELEMENTS - if (coins_cache_entry.flags & CCoinsCacheEntry::PEGIN) { + if (flags & CCoinsCacheEntry::PEGIN) { const std::optional genhash = ConsumeDeserializable(fuzzed_data_provider); if (genhash) { coins_cache_entry.peginSpent = fuzzed_data_provider.ConsumeBool(); - coins_map.emplace(std::pair(*genhash, random_out_point), std::move(coins_cache_entry)); + auto it{coins_map.emplace(std::pair(*genhash, random_out_point), std::move(coins_cache_entry)).first}; + it->second.AddFlags(flags, *it, sentinel); + usage += it->second.coin.DynamicMemoryUsage(); } } else { - coins_map.emplace(std::pair(uint256(), random_out_point), std::move(coins_cache_entry)); + auto it{coins_map.emplace(std::pair(uint256(), 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 2b1bc1694b..8d257a88ab 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -172,13 +172,13 @@ public: 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.second.n % 5) != 4) { m_data.erase(it->first.second); - } else if (erase) { + } else if (cursor.WillErase(*it)) { m_data[it->first.second] = std::move(it->second.coin); } else { m_data[it->first.second] = it->second.coin; diff --git a/src/test/pegin_spent_tests.cpp b/src/test/pegin_spent_tests.cpp index 90c8737180..1bbc2a93b0 100644 --- a/src/test/pegin_spent_tests.cpp +++ b/src/test/pegin_spent_tests.cpp @@ -31,18 +31,24 @@ public: return CCoinsView::IsPeginSpent(outpoint); } + CoinsCachePair m_written_sentinel{}; CCoinsMapMemoryResource resource; CCoinsMap mapCoinsWritten{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) override { + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override { mapCoinsWritten.clear(); - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) { - mapCoinsWritten[it->first] = it->second; + m_written_sentinel.second.SelfRef(m_written_sentinel); + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + auto [dest_it, inserted] = mapCoinsWritten.try_emplace(it->first); + dest_it->second.coin = it->second.coin; + dest_it->second.peginSpent = it->second.peginSpent; + dest_it->second.AddFlags(it->second.GetFlags(), *dest_it, m_written_sentinel); } - //mapCoinsWritten = mapCoins; - return CCoinsView::BatchWrite(mapCoins, hashBlock, erase); + return true; } - CCoinsViewTester() {} + CCoinsViewTester() { + m_written_sentinel.second.SelfRef(m_written_sentinel); + } }; BOOST_AUTO_TEST_CASE(PeginSpent_validity) @@ -77,40 +83,61 @@ BOOST_AUTO_TEST_CASE(PeginSpent_validity) CCoinsMapMemoryResource resource; CCoinsMap mapCoins{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - CCoinsCacheEntry entry; + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); + size_t usage{0}; std::pair outpoint3(std::make_pair(GetRandHash(), COutPoint(Txid::FromUint256(GetRandHash()), 42))); //Attempt batch write of non-dirty pegin, no effect - entry.flags = CCoinsCacheEntry::PEGIN; - entry.peginSpent = true; - mapCoins.insert(std::make_pair(outpoint3, entry)); - coinsCache.BatchWrite(mapCoins, uint256()); + { + auto [it, inserted] = mapCoins.try_emplace(outpoint3); + it->second.peginSpent = true; + it->second.AddFlags(CCoinsCacheEntry::PEGIN, *it, sentinel); + } + { + auto cursor{CoinsViewCacheCursor(usage, sentinel, mapCoins, /*will_erase=*/true)}; + coinsCache.BatchWrite(cursor, uint256()); + } //Check for effect coins.IsPeginSpentCalled = false; BOOST_CHECK(!coinsCache.IsPeginSpent(outpoint3)); BOOST_CHECK(coins.IsPeginSpentCalled); - BOOST_CHECK(mapCoins.size() == 0); + mapCoins.clear(); + sentinel.second.SelfRef(sentinel); //Write again with pegin, dirty && fresh flags, but unspent. No effect. - entry.peginSpent = false; - entry.flags |= CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH; - mapCoins.insert(std::make_pair(outpoint3, entry)); - coinsCache.BatchWrite(mapCoins, uint256()); + { + auto [it, inserted] = mapCoins.try_emplace(outpoint3); + it->second.peginSpent = false; + it->second.AddFlags(CCoinsCacheEntry::PEGIN | CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH, *it, sentinel); + } + { + auto cursor{CoinsViewCacheCursor(usage, sentinel, mapCoins, /*will_erase=*/true)}; + coinsCache.BatchWrite(cursor, uint256()); + } //Check for effect coins.IsPeginSpentCalled = false; BOOST_CHECK(!coinsCache.IsPeginSpent(outpoint3)); BOOST_CHECK(coins.IsPeginSpentCalled); - BOOST_CHECK(mapCoins.size() == 0); + mapCoins.clear(); + sentinel.second.SelfRef(sentinel); //Re-mark as spent. It's super effective. - entry.peginSpent = true; - mapCoins.insert(std::make_pair(outpoint3, entry)); - coinsCache.BatchWrite(mapCoins, uint256()); + { + auto [it, inserted] = mapCoins.try_emplace(outpoint3); + it->second.peginSpent = true; + it->second.AddFlags(CCoinsCacheEntry::PEGIN | CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH, *it, sentinel); + } + { + auto cursor{CoinsViewCacheCursor(usage, sentinel, mapCoins, /*will_erase=*/true)}; + coinsCache.BatchWrite(cursor, uint256()); + } //Check for effect coins.IsPeginSpentCalled = false; BOOST_CHECK(coinsCache.IsPeginSpent(outpoint3)); BOOST_CHECK(!coins.IsPeginSpentCalled); - BOOST_CHECK(mapCoins.size() == 0); + mapCoins.clear(); + sentinel.second.SelfRef(sentinel); //Add an entry we never IsPeginSpent'd first (ie added to cache via SetPeginSpent) std::pair outpoint4(std::make_pair(GetRandHash(), COutPoint(Txid::FromUint256(GetRandHash()), 42))); @@ -120,26 +147,33 @@ BOOST_AUTO_TEST_CASE(PeginSpent_validity) BOOST_CHECK_EQUAL(coins.mapCoinsWritten.size(), 0U); coinsCache.Flush(); BOOST_CHECK_EQUAL(coins.mapCoinsWritten.size(), 4U); - BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint].peginSpent, true); - BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint2].flags, CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint2].GetFlags(), CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint2].peginSpent, false); - BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint3].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint3].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint3].peginSpent, true); - BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint4].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint4].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins.mapCoinsWritten[outpoint3].peginSpent, true); // CCoinsViewCache should lose outpoint2 in BatchWrite logic CCoinsViewTester coins2; CCoinsViewCache coinsCache2(&coins2); - BOOST_CHECK(coinsCache2.BatchWrite(coins.mapCoinsWritten, uint256())); + { + size_t written_usage{0}; + for (auto& [key, entry] : coins.mapCoinsWritten) { + written_usage += entry.coin.DynamicMemoryUsage(); + } + auto cursor{CoinsViewCacheCursor(written_usage, coins.m_written_sentinel, coins.mapCoinsWritten, /*will_erase=*/true)}; + BOOST_CHECK(coinsCache2.BatchWrite(cursor, uint256())); + } coinsCache2.Flush(); BOOST_CHECK_EQUAL(coins2.mapCoinsWritten.size(), 3U); - BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint].peginSpent, true); - BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint3].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint3].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint3].peginSpent, true); - BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint4].flags, CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); + BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint4].GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH | CCoinsCacheEntry::PEGIN); BOOST_CHECK_EQUAL(coins2.mapCoinsWritten[outpoint4].peginSpent, true); } diff --git a/src/txdb.cpp b/src/txdb.cpp index 95797a1990..cc17f041db 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -99,7 +99,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; @@ -125,10 +125,10 @@ 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()) { // ELEMENTS: - if (it->second.flags & CCoinsCacheEntry::PEGIN) { + if (it->second.GetFlags() & CCoinsCacheEntry::PEGIN) { if (!it->second.peginSpent) { batch.Erase(std::make_pair(DB_PEGIN_FLAG, it->first)); } else { @@ -148,7 +148,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 284d34d546..7c72167941 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -63,7 +63,7 @@ public: 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; // ELEMENTS: bool IsPeginSpent(const std::pair &outpoint) const override; diff --git a/src/validation.cpp b/src/validation.cpp index 7c2f89e883..01b4aeb34a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3318,7 +3318,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.")); }