From df34a94e57659c31873c26c86a8de5a032400061 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:11:16 -0400 Subject: [PATCH 01/14] refactor: encapsulate flags access for dirty and fresh checks No behavior change. This prepares moving the cache entry flags field to private access. Co-Authored-By: l0rinc --- src/coins.cpp | 20 ++++++++++---------- src/coins.h | 3 +++ src/test/coins_tests.cpp | 2 +- src/test/fuzz/coinscache_sim.cpp | 2 +- src/txdb.cpp | 2 +- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index a4e4d4ad322..ecc435e6e7d 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -93,7 +93,7 @@ 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); @@ -138,7 +138,7 @@ 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; @@ -183,14 +183,14 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(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]; @@ -207,13 +207,13 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn // 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) { + if (it->second.IsFresh()) { entry.flags |= CCoinsCacheEntry::FRESH; } } } 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 +221,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(); @@ -283,7 +283,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(), @@ -326,8 +326,8 @@ void CCoinsViewCache::SanityCheck() const size_t recomputed_usage = 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); diff --git a/src/coins.h b/src/coins.h index 76e64b641d2..11ff7f4bced 100644 --- a/src/coins.h +++ b/src/coins.h @@ -130,6 +130,9 @@ struct CCoinsCacheEntry 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) {} + + inline bool IsDirty() const noexcept { return flags & DIRTY; } + inline bool IsFresh() const noexcept { return flags & FRESH; } }; /** diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index a992e2fa033..3d415b3db42 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -58,7 +58,7 @@ public: bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override { for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + 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) { diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 648e96b4a05..8c815e65ab5 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -175,7 +175,7 @@ public: bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final { for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + if (it->second.IsDirty()) { if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) { m_data.erase(it->first); } else if (erase) { diff --git a/src/txdb.cpp b/src/txdb.cpp index e4a4b3bf72a..d4b0186356f 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -115,7 +115,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip)); for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) { - if (it->second.flags & CCoinsCacheEntry::DIRTY) { + if (it->second.IsDirty()) { CoinEntry entry(&it->first); if (it->second.coin.IsSpent()) batch.Erase(entry); From 9715d3bf1e4013476539f1523a6b54d2055c32c6 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:19:17 -0400 Subject: [PATCH 02/14] refactor: encapsulate flags get access for all other checks No behavior change. This prepares moving the cache entry flags field to private access. --- src/coins.h | 1 + src/test/coins_tests.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coins.h b/src/coins.h index 11ff7f4bced..df4d7a78831 100644 --- a/src/coins.h +++ b/src/coins.h @@ -131,6 +131,7 @@ struct CCoinsCacheEntry explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {} CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {} + inline unsigned char GetFlags() const noexcept { return flags; } inline bool IsDirty() const noexcept { return flags & DIRTY; } inline bool IsFresh() const noexcept { return flags & FRESH; } }; diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 3d415b3db42..1502595cbb5 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -603,7 +603,7 @@ 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); } } From 8737c0cefa6ec49a4d17d9bef9e5e1a7990af1ac Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:27:48 -0400 Subject: [PATCH 03/14] refactor: encapsulate flags setting with AddFlags and ClearFlags No behavior change. This prepares moving the cache entry flags field to private access. --- src/coins.cpp | 14 +++++++------- src/coins.h | 5 +++++ src/test/coins_tests.cpp | 2 +- src/test/fuzz/coins_view.cpp | 2 +- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index ecc435e6e7d..8c32752a3aa 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -51,7 +51,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); } cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); return ret; @@ -96,7 +96,7 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi 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)); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, add, outpoint.hash.data(), @@ -141,7 +141,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.flags |= CCoinsCacheEntry::DIRTY; + it->second.AddFlags(CCoinsCacheEntry::DIRTY); it->second.coin.Clear(); } return true; @@ -203,12 +203,12 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - entry.flags = CCoinsCacheEntry::DIRTY; + entry.AddFlags(CCoinsCacheEntry::DIRTY); // 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()) { - entry.flags |= CCoinsCacheEntry::FRESH; + entry.AddFlags(CCoinsCacheEntry::FRESH); } } } else { @@ -238,7 +238,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.flags |= CCoinsCacheEntry::DIRTY; + itUs->second.AddFlags(CCoinsCacheEntry::DIRTY); // 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 @@ -273,7 +273,7 @@ bool CCoinsViewCache::Sync() cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); it = cacheCoins.erase(it); } else { - it->second.flags = 0; + it->second.ClearFlags(); ++it; } } diff --git a/src/coins.h b/src/coins.h index df4d7a78831..b85c316ee51 100644 --- a/src/coins.h +++ b/src/coins.h @@ -131,6 +131,11 @@ struct CCoinsCacheEntry explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {} CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {} + inline void AddFlags(unsigned char flags) noexcept { this->flags |= flags; } + inline void ClearFlags() noexcept + { + flags = 0; + } inline unsigned char GetFlags() const noexcept { return flags; } inline bool IsDirty() const noexcept { return flags & DIRTY; } inline bool IsFresh() const noexcept { return flags & FRESH; } diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 1502595cbb5..8ac972c4e0d 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -584,7 +584,7 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) } assert(flags != NO_ENTRY); CCoinsCacheEntry entry; - entry.flags = flags; + entry.AddFlags(flags); SetCoinsValue(value, entry.coin); auto inserted = map.emplace(OUTPOINT, std::move(entry)); assert(inserted.second); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 41c971c237b..5f942eba671 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -125,7 +125,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - coins_cache_entry.flags = fuzzed_data_provider.ConsumeIntegral(); + coins_cache_entry.AddFlags(fuzzed_data_provider.ConsumeIntegral()); if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { From 4e4fb4cbabcc10e723534f5f80f3e3cf09f6ca50 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:41:01 -0400 Subject: [PATCH 04/14] refactor: disallow setting flags in CCoinsCacheEntry constructors No behavior change because any entries that are added in EmplaceCoinInternalDANGER have DIRTY assigned to them after, and if they are not inserted then they will not be modified as before. This prepares moving the cache entry flags field to private access. Co-Authored-By: Martin Leitner-Ankerl --- src/coins.cpp | 7 +++++-- src/coins.h | 7 +++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 8c32752a3aa..8e8edcca636 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -108,10 +108,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); + } } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { diff --git a/src/coins.h b/src/coins.h index b85c316ee51..bb1c17a8874 100644 --- a/src/coins.h +++ b/src/coins.h @@ -104,7 +104,7 @@ public: struct CCoinsCacheEntry { Coin coin; // The actual cached data. - unsigned char flags; + unsigned char flags{0}; enum Flags { /** @@ -127,9 +127,8 @@ 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_)) {} inline void AddFlags(unsigned char flags) noexcept { this->flags |= flags; } inline void ClearFlags() noexcept From f08faeade2f99ae1de6f3c481697541cc16186c7 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:44:25 -0400 Subject: [PATCH 05/14] refactor: move flags to private uint8_t and rename to m_flags No behavior change. This prepares to add CCoinsCacheEntrys to a doubly linked list when a flag is added. --- src/coins.h | 15 +++++++++------ src/test/fuzz/coins_view.cpp | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/coins.h b/src/coins.h index bb1c17a8874..22b01901e6b 100644 --- a/src/coins.h +++ b/src/coins.h @@ -103,8 +103,11 @@ public: */ struct CCoinsCacheEntry { +private: + uint8_t m_flags{0}; + +public: Coin coin; // The actual cached data. - unsigned char flags{0}; enum Flags { /** @@ -130,14 +133,14 @@ struct CCoinsCacheEntry CCoinsCacheEntry() noexcept = default; explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} - inline void AddFlags(unsigned char flags) noexcept { this->flags |= flags; } + inline void AddFlags(uint8_t flags) noexcept { m_flags |= flags; } inline void ClearFlags() noexcept { - flags = 0; + m_flags = 0; } - inline unsigned char GetFlags() const noexcept { return flags; } - inline bool IsDirty() const noexcept { return flags & DIRTY; } - inline bool IsFresh() const noexcept { return flags & FRESH; } + 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; } }; /** diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 5f942eba671..1f24d78dca3 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -125,7 +125,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - coins_cache_entry.AddFlags(fuzzed_data_provider.ConsumeIntegral()); + coins_cache_entry.AddFlags(fuzzed_data_provider.ConsumeIntegral()); if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { From 75f36d241d2a344c5c4ce2c80250bdde91b3295e Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 16:57:42 -0400 Subject: [PATCH 06/14] refactor: add CoinsCachePair alias --- src/coins.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coins.h b/src/coins.h index 22b01901e6b..95baf3f700d 100644 --- a/src/coins.h +++ b/src/coins.h @@ -86,6 +86,9 @@ public: } }; +struct CCoinsCacheEntry; +using CoinsCachePair = std::pair; + /** * A Coin in one level of the coins database caching hierarchy. * @@ -155,8 +158,8 @@ using CCoinsMap = std::unordered_map, - PoolAllocator, - sizeof(std::pair) + sizeof(void*) * 4>>; + PoolAllocator>; using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; From 8bd3959feaa0e71585bc5e0976f584fb06ee6d14 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 17:14:33 -0400 Subject: [PATCH 07/14] refactor: require self and sentinel parameters for AddFlags No behavior change. Prepares for adding the CoinsCachePairs to a linked list when flagged. --- src/coins.cpp | 21 ++++++++++++--------- src/coins.h | 18 +++++++++++++++++- src/test/coins_tests.cpp | 11 +++++++---- src/test/fuzz/coins_view.cpp | 7 +++++-- 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 8e8edcca636..739cc7bc3bd 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -34,7 +34,9 @@ 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.AddFlags(CCoinsCacheEntry::FRESH); + ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); } cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); return ret; @@ -96,7 +98,7 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi fresh = !it->second.IsDirty(); } it->second.coin = std::move(coin); - it->second.AddFlags(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(), @@ -113,7 +115,7 @@ void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coi std::forward_as_tuple(std::move(outpoint)), std::forward_as_tuple(std::move(coin))); if (inserted) { - it->second.AddFlags(CCoinsCacheEntry::DIRTY); + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); } } @@ -144,7 +146,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.AddFlags(CCoinsCacheEntry::DIRTY); + it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); it->second.coin.Clear(); } return true; @@ -196,7 +198,8 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn 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]; + itUs = cacheCoins.try_emplace(it->first).first; + CCoinsCacheEntry& entry{itUs->second}; if (erase) { // The `move` call here is purely an optimization; we rely on the // `mapCoins.erase` call in the `for` expression to actually remove @@ -206,12 +209,12 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - entry.AddFlags(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.IsFresh()) { - entry.AddFlags(CCoinsCacheEntry::FRESH); + entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel); } } } else { @@ -241,7 +244,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.AddFlags(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 diff --git a/src/coins.h b/src/coins.h index 95baf3f700d..8cdee3763b6 100644 --- a/src/coins.h +++ b/src/coins.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -136,7 +137,14 @@ public: CCoinsCacheEntry() noexcept = default; explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} - inline void AddFlags(uint8_t flags) noexcept { m_flags |= flags; } + //! 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); + m_flags |= flags; + } inline void ClearFlags() noexcept { m_flags = 0; @@ -144,6 +152,12 @@ public: 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 use this for initializing the linked list sentinel + inline void SelfRef(CoinsCachePair& self) noexcept + { + Assume(&self.second == this); + } }; /** @@ -251,6 +265,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. */ diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 8ac972c4e0d..4fe560df956 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -92,6 +92,7 @@ public: } CCoinsMap& map() const { return cacheCoins; } + CoinsCachePair& sentinel() const { return m_sentinel; } size_t& usage() const { return cachedCoinsUsage; } }; @@ -576,7 +577,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 +585,10 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) } assert(flags != NO_ENTRY); CCoinsCacheEntry entry; - entry.AddFlags(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(); } @@ -610,9 +611,11 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C 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); + InsertCoinsMapEntry(map, sentinel, value, flags); BOOST_CHECK(view.BatchWrite(map, {})); } @@ -622,7 +625,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; diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 1f24d78dca3..8b7592f28ca 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -120,12 +120,14 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) random_mutable_transaction = *opt_mutable_transaction; }, [&] { + CoinsCachePair sentinel{}; + sentinel.second.SelfRef(sentinel); 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.AddFlags(fuzzed_data_provider.ConsumeIntegral()); + const auto flags{fuzzed_data_provider.ConsumeIntegral()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -136,7 +138,8 @@ 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); } bool expected_code_path = false; try { From 58b7ed156d5993b69375bb455b03bd8b17f64fa4 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 19:49:40 -0400 Subject: [PATCH 08/14] coins: call ClearFlags in CCoinsCacheEntry destructor No behavior change. Prepares for flags adding CCoinsCacheEntrys to a linked list which need to be removed on destruction. --- src/coins.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coins.h b/src/coins.h index 8cdee3763b6..09de25fc637 100644 --- a/src/coins.h +++ b/src/coins.h @@ -136,6 +136,10 @@ public: 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 From 24ce37cb867b95e86d9fd4e50858d64ee8a59abf Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 20:05:14 -0400 Subject: [PATCH 09/14] coins: track flagged cache entries in linked list No visible behavior change. This commit tracks the flagged entries internally but the list is not iterated by anything. Co-Authored-By: Pieter Wuille Co-Authored-By: l0rinc --- src/coins.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/coins.h b/src/coins.h index 09de25fc637..cf14107ce3e 100644 --- a/src/coins.h +++ b/src/coins.h @@ -108,6 +108,24 @@ using CoinsCachePair = std::pair; 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: @@ -147,20 +165,45 @@ public: 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; } }; From a14edada8a051e280af6fedd5130be40247e2d7a Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 28 Jun 2024 20:12:19 -0400 Subject: [PATCH 10/14] test: add cache entry linked list tests Co-Authored-By: l0rinc --- src/Makefile.test.include | 1 + src/test/coins_tests.cpp | 10 +- src/test/coinscachepair_tests.cpp | 219 ++++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 src/test/coinscachepair_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e2d6f94b249..3a6d56c9e1a 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/test/coins_tests.cpp b/src/test/coins_tests.cpp index 4fe560df956..a8af21c6878 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -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,6 +89,9 @@ public: } BOOST_CHECK_EQUAL(GetCacheSize(), count); BOOST_CHECK_EQUAL(DynamicMemoryUsage(), ret); + if (sanity_check) { + SanityCheck(); + } } CCoinsMap& map() const { return cacheCoins; } @@ -637,7 +640,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; @@ -806,7 +809,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; @@ -919,6 +922,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 00000000000..61840f1f092 --- /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() From 7825b8b9aeceb4ff607650cdc9c49e5de9c7719f Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 17 Jul 2024 23:11:48 -0400 Subject: [PATCH 11/14] coins: pass linked list of flagged entries to BatchWrite BatchWrite now iterates through the linked list of flagged entries instead of the entire coinsCache map. Co-Authored-By: l0rinc --- src/coins.cpp | 35 ++++++++++++++------------------ src/coins.h | 32 +++++++++++++++++++++++++---- src/test/coins_tests.cpp | 9 ++++---- src/test/fuzz/coins_view.cpp | 5 ++++- src/test/fuzz/coinscache_sim.cpp | 6 +++--- src/txdb.cpp | 6 +++--- src/txdb.h | 2 +- 7 files changed, 59 insertions(+), 36 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 739cc7bc3bd..92444fb0172 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,7 +27,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(); } @@ -183,10 +183,8 @@ 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.IsDirty()) { continue; @@ -200,10 +198,9 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn // and mark it as dirty. itUs = cacheCoins.try_emplace(it->first).first; CCoinsCacheEntry& entry{itUs->second}; - 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; @@ -235,10 +232,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; @@ -257,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; @@ -271,7 +265,8 @@ bool CCoinsViewCache::Flush() { bool CCoinsViewCache::Sync() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false); + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + bool fOk = base->BatchWrite(cursor, hashBlock); // 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(); ) { diff --git a/src/coins.h b/src/coins.h index cf14107ce3e..e1ab6413925 100644 --- a/src/coins.h +++ b/src/coins.h @@ -243,6 +243,30 @@ private: uint256 hashBlock; }; +/** + * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache. + */ +struct CoinsViewCacheCursor +{ + 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; } + + inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept { return current.second.Next(); } + + inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase; } +private: + size_t& m_usage; + CoinsCachePair& m_sentinel; + CCoinsMap& m_map; + bool m_will_erase; +}; + /** Abstract view on the open txout dataset. */ class CCoinsView { @@ -266,8 +290,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; @@ -293,7 +317,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; }; @@ -332,7 +356,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 a8af21c6878..fb929a2b0e0 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,9 +55,9 @@ 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)) { + 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; @@ -618,8 +618,9 @@ void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - InsertCoinsMapEntry(map, sentinel, 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 diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 8b7592f28ca..368c69819a0 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -122,6 +122,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) [&] { 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) @@ -140,10 +141,12 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) } 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 8c815e65ab5..8e717e96b4e 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)) { + 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 d4b0186356f..3865692b6a6 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,7 +114,7 @@ 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();) { + for (auto it{cursor.Begin()}; it != cursor.End();) { if (it->second.IsDirty()) { CoinEntry entry(&it->first); if (it->second.coin.IsSpent()) @@ -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 c9af0a091ec..e0acb09e98a 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; //! Whether an unsupported database format is used. From 05cf4e18758618bb493d26014d3a9c89bf28d898 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 17 Jul 2024 23:13:07 -0400 Subject: [PATCH 12/14] coins: move Sync logic to CoinsViewCacheCursor Erase spent cache entries and clear flags of unspent entries inside the BatchWrite loop, instead of an additional loop after BatchWrite. Co-Authored-By: Pieter Wuille --- src/coins.cpp | 13 ++++--------- src/coins.h | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 92444fb0172..0bcb4a30f6b 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -267,15 +267,10 @@ bool CCoinsViewCache::Sync() { auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; bool fOk = base->BatchWrite(cursor, hashBlock); - // 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.ClearFlags(); - ++it; + 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; diff --git a/src/coins.h b/src/coins.h index e1ab6413925..78b8eddacd7 100644 --- a/src/coins.h +++ b/src/coins.h @@ -245,9 +245,26 @@ private: /** * 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, @@ -257,9 +274,24 @@ struct CoinsViewCacheCursor inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } inline CoinsCachePair* End() const noexcept { return &m_sentinel; } - inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept { return current.second.Next(); } + //! 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; } + inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } private: size_t& m_usage; CoinsCachePair& m_sentinel; From 0e8918755f725b6269ed2be5a0b46f1611233515 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 16 Jul 2024 10:37:44 -0400 Subject: [PATCH 13/14] Add linked-list test to CCoinsViewCache::SanityCheck --- src/coins.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/coins.cpp b/src/coins.cpp index 0bcb4a30f6b..91e02331f51 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -320,6 +320,7 @@ 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.IsDirty()) attr |= 1; @@ -330,7 +331,22 @@ void CCoinsViewCache::SanityCheck() const // 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); } From 589db872e116779ab9cae693171ac8a8c02d9923 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 17 Jul 2024 23:20:32 -0400 Subject: [PATCH 14/14] validation: don't erase coins cache on prune flushes --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index d6b3924cda0..9ee44bc675c 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.")); }