diff --git a/src/coins.cpp b/src/coins.cpp index c403e006c85c..969b4feae923 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -31,9 +31,7 @@ std::optional CCoinsViewCache::PeekCoin(const COutPoint& outpoint) const CCoinsViewCache::CCoinsViewCache(CCoinsView* in_base, bool deterministic) : CCoinsViewBacked(in_base), 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; @@ -96,9 +94,9 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage())); } it->second.coin = std::move(coin); - CCoinsCacheEntry::SetDirty(*it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it); ++m_dirty_count; - if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); + if (fresh) CCoinsCacheEntry::SetFresh(*it); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACEPOINT(utxocache, add, outpoint.hash.data(), @@ -112,7 +110,7 @@ void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coi const auto mem_usage{coin.DynamicMemoryUsage()}; auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin)); if (inserted) { - CCoinsCacheEntry::SetDirty(*it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it); ++m_dirty_count; cachedCoinsUsage += mem_usage; } @@ -146,7 +144,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - CCoinsCacheEntry::SetDirty(*it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it); ++m_dirty_count; it->second.coin.Clear(); } @@ -188,7 +186,7 @@ void CCoinsViewCache::SetBestBlock(const uint256& in_block_hash) void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in_block_hash) { - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + for (auto it{cursor.Begin()}; it != cursor.End(); it.NextAndMaybeErase()) { if (!it->second.IsDirty()) { // TODO a cursor can only contain dirty entries continue; } @@ -201,20 +199,20 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in // Move the data up and mark it as dirty. CCoinsCacheEntry& entry{itUs->second}; assert(entry.coin.DynamicMemoryUsage() == 0); - if (cursor.WillErase(*it)) { + 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; } - CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*itUs); ++m_dirty_count; cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); // We can mark it FRESH in the parent if it was FRESH in the child // Otherwise it might have just been flushed from the parent's cache // and already exist in the grandparent - if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel); + if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs); } } else { // Found the entry in the parent cache @@ -235,7 +233,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in } else { // A normal modification. Assume(TrySub(cachedCoinsUsage, itUs->second.coin.DynamicMemoryUsage())); - if (cursor.WillErase(*it)) { + 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); @@ -244,7 +242,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); if (!itUs->second.IsDirty()) { - CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*itUs); ++m_dirty_count; } // NOTE: It isn't safe to mark the coin as FRESH in the parent @@ -259,7 +257,7 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& in void CCoinsViewCache::Flush(bool reallocate_cache) { - auto cursor{CoinsViewCacheCursor(m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(m_dirty_count, cacheCoins, /*will_erase=*/true)}; base->BatchWrite(cursor, m_block_hash); Assume(m_dirty_count == 0); cacheCoins.clear(); @@ -271,13 +269,9 @@ void CCoinsViewCache::Flush(bool reallocate_cache) void CCoinsViewCache::Sync() { - auto cursor{CoinsViewCacheCursor(m_dirty_count, m_sentinel, cacheCoins, /*will_erase=*/false)}; + auto cursor{CoinsViewCacheCursor(m_dirty_count, cacheCoins, /*will_erase=*/false)}; base->BatchWrite(cursor, m_block_hash); Assume(m_dirty_count == 0); - 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"); - } } void CCoinsViewCache::Reset() noexcept @@ -346,12 +340,10 @@ void CCoinsViewCache::SanityCheck() const // Count the number of entries we expect in the linked list. if (entry.IsDirty()) ++count_dirty; } - // Iterate over the linked list of flagged entries. + // Iterate over the flagged entries. + auto cursor{CoinsViewCacheCursor(m_dirty_count, cacheCoins, /*will_erase=*/true)}; 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); + for (auto it{cursor.Begin()}; it != cursor.End(); it.Next()) { // Verify they are actually flagged. assert(it->second.IsDirty()); // Count the number of entries actually in the list. diff --git a/src/coins.h b/src/coins.h index ae7f34f46581..70b9183d0218 100644 --- a/src/coins.h +++ b/src/coins.h @@ -109,32 +109,11 @@ using CoinsCachePair = std::pair; struct CCoinsCacheEntry { private: - /** - * These are used to create a doubly linked list of flagged entries. - * They are set in SetDirty, SetFresh, and unset in SetClean. - * 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. - */ - CoinsCachePair* m_prev{nullptr}; - CoinsCachePair* m_next{nullptr}; uint8_t m_flags{0}; - //! Adding a flag requires a reference to the sentinel of the flagged pair linked list. - static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept + static void AddFlags(uint8_t flags, CoinsCachePair& pair) noexcept { Assume(flags & (DIRTY | FRESH)); - if (!pair.second.m_flags) { - Assume(!pair.second.m_prev && !pair.second.m_next); - pair.second.m_prev = sentinel.second.m_prev; - pair.second.m_next = &sentinel; - sentinel.second.m_prev = &pair; - pair.second.m_prev->second.m_next = &pair; - } - Assume(pair.second.m_prev && pair.second.m_next); pair.second.m_flags |= flags; } @@ -169,43 +148,16 @@ struct CCoinsCacheEntry SetClean(); } - static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } - static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); } + static void SetDirty(CoinsCachePair& pair) noexcept { AddFlags(DIRTY, pair); } + static void SetFresh(CoinsCachePair& pair) noexcept { AddFlags(FRESH, pair); } void SetClean() noexcept { - if (!m_flags) return; - m_next->second.m_prev = m_prev; - m_prev->second.m_next = m_next; m_flags = 0; - m_prev = m_next = nullptr; } bool IsDirty() const noexcept { return m_flags & DIRTY; } bool IsFresh() const noexcept { return m_flags & FRESH; } - - //! Only call Next when this entry is DIRTY, FRESH, or both - CoinsCachePair* Next() const noexcept - { - Assume(m_flags); - return m_next; - } - - //! Only call Prev when this entry is DIRTY, FRESH, or both - CoinsCachePair* Prev() const noexcept - { - Assume(m_flags); - return m_prev; - } - - //! Only use this for initializing the linked list sentinel - void SelfRef(CoinsCachePair& pair) noexcept - { - Assume(&pair.second == this); - m_prev = &pair; - m_next = &pair; - // Set sentinel to DIRTY so we can call Next on it - m_flags = DIRTY; - } + bool IsFlagged() const noexcept { return bool(m_flags); } }; /** @@ -247,6 +199,8 @@ class CCoinsViewCursor /** * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache. * + * Wraps an iterator, and it skips over non-flagged entries. + * * 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 @@ -267,38 +221,59 @@ struct CoinsViewCacheCursor //! 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& dirty_count LIFETIMEBOUND, - CoinsCachePair& sentinel LIFETIMEBOUND, CCoinsMap& map LIFETIMEBOUND, bool will_erase) noexcept - : m_dirty_count(dirty_count), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + : m_dirty_count(dirty_count), 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()}; - Assume(TrySub(m_dirty_count, current.second.IsDirty())); - // If we are not going to erase the cache, we must still erase spent entries. - // Otherwise, clear the state of the entry. - if (!m_will_erase) { - if (current.second.coin.IsSpent()) { - assert(current.second.coin.DynamicMemoryUsage() == 0); // scriptPubKey was already cleared in SpendCoin - m_map.erase(current.first); - } else { - current.second.SetClean(); + class Iterator { + private: + CCoinsMap::iterator m_iter; + const CoinsViewCacheCursor& m_cursor; + public: + Iterator(CCoinsMap::iterator iter, const CoinsViewCacheCursor& cursor) : m_iter(std::move(iter)), m_cursor(cursor) {} + inline void SkipOverNonFlagged() noexcept { + while (m_iter != m_cursor.m_map.end() && !m_iter->second.IsFlagged()) { + ++m_iter; } } - return next_entry; + inline void Next() noexcept { + ++m_iter; + SkipOverNonFlagged(); + } + //! Return the next entry after current, possibly erasing current + inline void NextAndMaybeErase() noexcept + { + const auto current{m_iter}; + // go to next + ++m_iter; + SkipOverNonFlagged(); + Assume(TrySub(m_cursor.m_dirty_count, current->second.IsDirty())); + // If we are not going to erase the cache, we must still erase spent entries. + // Otherwise, clear the state of the entry. + if (!m_cursor.m_will_erase) { + if (current->second.coin.IsSpent()) { + assert(current->second.coin.DynamicMemoryUsage() == 0); // scriptPubKey was already cleared in SpendCoin + m_cursor.m_map.erase(current->first); + } else { + current->second.SetClean(); + } + } + } + inline const CCoinsMap::iterator& operator->() const { return m_iter; } + inline bool operator==(Iterator& it2) { return it2.m_iter == m_iter; } + }; + inline Iterator Begin() const noexcept { + auto iter{Iterator(m_map.begin(), *this)}; + iter.SkipOverNonFlagged(); + return iter; } + inline Iterator End() const noexcept { return Iterator(m_map.end(), *this); } - inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } + inline bool WillErase(Iterator& it) const noexcept { return m_will_erase || it->second.coin.IsSpent(); } size_t GetDirtyCount() const noexcept { return m_dirty_count; } size_t GetTotalCount() const noexcept { return m_map.size(); } private: size_t& m_dirty_count; - CoinsCachePair& m_sentinel; CCoinsMap& m_map; bool m_will_erase; }; @@ -361,7 +336,7 @@ class CoinsViewEmpty : public CCoinsView std::vector GetHeadBlocks() const override { return {}; } void BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) override { - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { } + for (auto it{cursor.Begin()}; it != cursor.End(); it.NextAndMaybeErase()) { } } std::unique_ptr Cursor() const override { return {}; } size_t EstimateSize() const override { return 0; } @@ -402,8 +377,6 @@ class CCoinsViewCache : public CCoinsViewBacked */ mutable uint256 m_block_hash; 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 14ccb1c443c4..366a37292afc 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -58,7 +58,7 @@ class CCoinsViewTest : public CoinsViewEmpty void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override { - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){ + for (auto it{cursor.Begin()}; it != cursor.End(); it.NextAndMaybeErase()){ if (it->second.IsDirty()) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; @@ -95,7 +95,6 @@ class CCoinsViewCacheTest : public CCoinsViewCache } CCoinsMap& map() const { return cacheCoins; } - CoinsCachePair& sentinel() const { return m_sentinel; } size_t& usage() const { return cachedCoinsUsage; } size_t& dirty() const { return m_dirty_count; } }; @@ -631,14 +630,14 @@ static void SetCoinsValue(const CAmount value, Coin& coin) } } -static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, const CoinEntry& cache_coin) +static size_t InsertCoinsMapEntry(CCoinsMap& map, const CoinEntry& cache_coin) { CCoinsCacheEntry entry; SetCoinsValue(cache_coin.value, entry.coin); auto [iter, inserted] = map.emplace(OUTPOINT, std::move(entry)); assert(inserted); - if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter, sentinel); - if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter, sentinel); + if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter); + if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter); return iter->second.coin.DynamicMemoryUsage(); } @@ -654,13 +653,11 @@ static MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) { - CoinsCachePair sentinel{}; - sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_coin); + if (cache_coin) InsertCoinsMapEntry(map, *cache_coin); size_t dirty_count{cache_coin && cache_coin->IsDirty()}; - auto cursor{CoinsViewCacheCursor(dirty_count, sentinel, map, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(dirty_count, map, /*will_erase=*/true)}; view.BatchWrite(cursor, {}); BOOST_CHECK_EQUAL(dirty_count, 0U); } @@ -673,7 +670,7 @@ class SingleEntryCacheTest auto base_cache_coin{base_value == ABSENT ? MISSING : CoinEntry{base_value, CoinEntry::State::DIRTY}}; WriteCoinsViewEntry(base, base_cache_coin); if (cache_coin) { - cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin); + cache.usage() += InsertCoinsMapEntry(cache.map(), *cache_coin); cache.dirty() += cache_coin->IsDirty(); } } diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp index 0c208e93dfb4..501b193b0fdb 100644 --- a/src/test/coinscachepair_tests.cpp +++ b/src/test/coinscachepair_tests.cpp @@ -12,27 +12,21 @@ BOOST_AUTO_TEST_SUITE(coinscachepair_tests) static constexpr auto NUM_NODES{4}; -std::list CreatePairs(CoinsCachePair& sentinel) +std::list CreatePairs() { std::list nodes; for (auto i{0}; i < NUM_NODES; ++i) { nodes.emplace_back(); auto node{std::prev(nodes.end())}; - CCoinsCacheEntry::SetDirty(*node, sentinel); + CCoinsCacheEntry::SetDirty(*node); BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh()); - 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; } +/* TODO add test for the cursor skip behaviour BOOST_AUTO_TEST_CASE(linked_list_iteration) { CoinsCachePair sentinel; @@ -194,5 +188,6 @@ BOOST_AUTO_TEST_CASE(linked_list_set_state) 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 c11581d2d3c3..1e2337fd10bd 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -186,8 +186,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co random_mutable_transaction = *opt_mutable_transaction; }, [&] { - CoinsCachePair sentinel{}; - sentinel.second.SelfRef(sentinel); size_t dirty_count{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; @@ -208,11 +206,11 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co bool fresh{!coins_view_cache.PeekCoin(random_out_point) && fuzzed_data_provider.ConsumeBool()}; bool dirty{fresh || fuzzed_data_provider.ConsumeBool()}; auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; - if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); - if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); + if (dirty) CCoinsCacheEntry::SetDirty(*it); + if (fresh) CCoinsCacheEntry::SetFresh(*it); dirty_count += dirty; } - auto cursor{CoinsViewCacheCursor(dirty_count, sentinel, coins_map, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(dirty_count, coins_map, /*will_erase=*/true)}; uint256 best_block{coins_view_cache.GetBestBlock()}; if (fuzzed_data_provider.ConsumeBool()) best_block = ConsumeUInt256(fuzzed_data_provider); // Set best block hash to non-null to satisfy the assertion in CCoinsViewDB::BatchWrite(). diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 15ece2e4af0c..b30ef2baf31c 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -155,12 +155,12 @@ class CoinsViewBottom final : public CoinsViewEmpty void BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { + for (auto it{cursor.Begin()}; it != cursor.End(); it.NextAndMaybeErase()) { if (it->second.IsDirty()) { if (it->second.coin.IsSpent()) { m_data.erase(it->first); } else { - if (cursor.WillErase(*it)) { + 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 a41dfd1657ac..18bf0b06fbe3 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -145,7 +145,7 @@ void CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block } } count++; - it = cursor.NextAndMaybeErase(*it); + it.NextAndMaybeErase(); if (batch.ApproximateSize() > m_options.batch_write_bytes) { LogDebug(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.ApproximateSize() / double(1_MiB));