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;