From cd0498eabc910efa3ed7a6d32e687107248bb5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 13 Sep 2024 10:58:16 +0200 Subject: [PATCH] coins, refactor: Split up AddFlags to remove invalid states CCoinsCacheEntry provided general access to its internal flags state, even though in reality it could only be clean, fresh, dirty or fresh|dirty. After it got dirtied we couldn't set the state back to clean by AddFlags(0) - tests were explicitly checking against that. This commit cleans up the public interface to make this distinction cleaner and invalid behavior impossible instead of just checked by tests. This includes the removal of redundant `inline` qualifiers (we're inside a struct). Also renamed `self` to `pair` to simplify the upcoming commits. Also modernized `EmplaceCoinInternalDANGER` since it was already modified. Co-authored-by: Andrew Toth --- src/coins.cpp | 24 +++++++---------- src/coins.h | 44 ++++++++++++++++++------------- src/test/coins_tests.cpp | 3 ++- src/test/coinscachepair_tests.cpp | 41 +++++++--------------------- src/test/fuzz/coins_view.cpp | 6 +++-- 5 files changed, 50 insertions(+), 68 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index cb09aa2e7aa..03ef1a1f0cc 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins // The parent only has an empty entry for this outpoint; we can consider our version as fresh. - ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); + CCoinsCacheEntry::SetFresh(*ret, m_sentinel); } } else { cacheCoins.erase(ret); @@ -95,7 +95,8 @@ 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, m_sentinel); + CCoinsCacheEntry::SetDirty(*it, m_sentinel); + if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); cachedCoinsUsage += it->second.coin.DynamicMemoryUsage(); TRACE5(utxocache, add, outpoint.hash.data(), @@ -107,13 +108,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) { cachedCoinsUsage += coin.DynamicMemoryUsage(); - auto [it, inserted] = cacheCoins.emplace( - std::piecewise_construct, - std::forward_as_tuple(std::move(outpoint)), - std::forward_as_tuple(std::move(coin))); - if (inserted) { - it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); - } + auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin)); + if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel); } void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { @@ -143,7 +139,7 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { if (it->second.IsFresh()) { cacheCoins.erase(it); } else { - it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel); + CCoinsCacheEntry::SetDirty(*it, m_sentinel); it->second.coin.Clear(); } return true; @@ -203,13 +199,11 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha entry.coin = it->second.coin; } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); - entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*itUs, m_sentinel); // We can mark it FRESH in the parent if it was FRESH in the child // Otherwise it might have just been flushed from the parent's cache // and already exist in the grandparent - if (it->second.IsFresh()) { - entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel); - } + if (it->second.IsFresh()) CCoinsCacheEntry::SetFresh(*itUs, m_sentinel); } } else { // Found the entry in the parent cache @@ -237,7 +231,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha itUs->second.coin = it->second.coin; } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); - itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel); + CCoinsCacheEntry::SetDirty(*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 a2449e1b815..505ca4c41a8 100644 --- a/src/coins.h +++ b/src/coins.h @@ -110,7 +110,7 @@ struct CCoinsCacheEntry private: /** * These are used to create a doubly linked list of flagged entries. - * They are set in AddFlags and unset in ClearFlags. + * 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 @@ -156,52 +156,58 @@ public: explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {} ~CCoinsCacheEntry() { - ClearFlags(); + SetClean(); } //! 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 + void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { - Assume(&self.second == this); - if (!m_flags && flags) { + Assume(flags & (DIRTY | FRESH)); + Assume(&pair.second == this); + if (!m_flags) { m_prev = sentinel.second.m_prev; m_next = &sentinel; - sentinel.second.m_prev = &self; - m_prev->second.m_next = &self; + sentinel.second.m_prev = &pair; + m_prev->second.m_next = &pair; } m_flags |= flags; } - inline void ClearFlags() noexcept + static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(DIRTY, pair, sentinel); } + static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { pair.second.AddFlags(FRESH, pair, sentinel); } + + void SetClean() 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; } + uint8_t GetFlags() const noexcept { return m_flags; } + 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 - inline CoinsCachePair* Next() const noexcept { + 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 { + 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 + void SelfRef(CoinsCachePair& pair) noexcept { - Assume(&self.second == this); - m_prev = &self; - m_next = &self; + Assume(&pair.second == this); + m_prev = &pair; + m_next = &pair; // Set sentinel to DIRTY so we can call Next on it m_flags = DIRTY; } @@ -279,13 +285,13 @@ struct CoinsViewCacheCursor { 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. + // Otherwise, clear the state of 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(); + current.second.SetClean(); } } return next_entry; diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index ec4720966b0..f9c226eb52c 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -596,7 +596,8 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmo SetCoinsValue(value, entry.coin); auto inserted = map.emplace(OUTPOINT, std::move(entry)); assert(inserted.second); - inserted.first->second.AddFlags(flags, *inserted.first, sentinel); + if (flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel); + if (flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel); return inserted.first->second.coin.DynamicMemoryUsage(); } diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp index 61840f1f092..6460579d945 100644 --- a/src/test/coinscachepair_tests.cpp +++ b/src/test/coinscachepair_tests.cpp @@ -19,7 +19,7 @@ std::list CreatePairs(CoinsCachePair& sentinel) nodes.emplace_back(); auto node{std::prev(nodes.end())}; - node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel); + CCoinsCacheEntry::SetDirty(*node, sentinel); BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); @@ -53,7 +53,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) for (const auto& expected : nodes) { BOOST_CHECK_EQUAL(&expected, node); auto next = node->second.Next(); - node->second.ClearFlags(); + node->second.SetClean(); node = next; } BOOST_CHECK_EQUAL(node, &sentinel); @@ -146,14 +146,8 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags) 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); + CCoinsCacheEntry::SetDirty(n1, sentinel); BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel); @@ -161,23 +155,15 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags) 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); + CCoinsCacheEntry::SetFresh(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); + CCoinsCacheEntry::SetFresh(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); @@ -185,30 +171,23 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags) BOOST_CHECK_EQUAL(n2.second.Prev(), &n1); // Check that we can clear flags then re-add them - n1.second.ClearFlags(); + n1.second.SetClean(); 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(); + // Check that calling `SetClean` with 0 flags has no effect + n1.second.SetClean(); 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); + // Adding DIRTY re-inserts it after n2 + CCoinsCacheEntry::SetDirty(n1, sentinel); BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); BOOST_CHECK_EQUAL(n2.second.Next(), &n1); BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 54bfb93b491..9c6aa6e7a1e 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -128,7 +128,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - const auto flags{fuzzed_data_provider.ConsumeIntegral()}; + const auto dirty{fuzzed_data_provider.ConsumeBool()}; + const auto fresh{fuzzed_data_provider.ConsumeBool()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -140,7 +141,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) coins_cache_entry.coin = *opt_coin; } auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; - it->second.AddFlags(flags, *it, sentinel); + if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); + if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false;