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 1/9] 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 cb09aa2e7a..03ef1a1f0c 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 a2449e1b81..505ca4c41a 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 ec4720966b..f9c226eb52 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 61840f1f09..6460579d94 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 54bfb93b49..9c6aa6e7a1 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; From fc8c282022e6ce4eb3ce526800a6ada3b4a2996d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 31 Oct 2024 13:18:13 +0100 Subject: [PATCH 2/9] coins, refactor: Make AddFlags, SetDirty, SetFresh static This makes the `Assume(&self.second == this)` check redundant Co-authored-by: Ryan Ofsky --- src/coins.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/coins.h b/src/coins.h index 505ca4c41a..ffeaae3ede 100644 --- a/src/coins.h +++ b/src/coins.h @@ -162,20 +162,19 @@ public: //! 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. - void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept + static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { Assume(flags & (DIRTY | FRESH)); - Assume(&pair.second == this); - if (!m_flags) { - m_prev = sentinel.second.m_prev; - m_next = &sentinel; + if (!pair.second.m_flags) { + pair.second.m_prev = sentinel.second.m_prev; + pair.second.m_next = &sentinel; sentinel.second.m_prev = &pair; - m_prev->second.m_next = &pair; + pair.second.m_prev->second.m_next = &pair; } - m_flags |= flags; + pair.second.m_flags |= flags; } - 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); } + static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } + static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); } void SetClean() noexcept { From 6b733699cfc79253ffae1527106baa428dd62f39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 31 Oct 2024 13:33:36 +0100 Subject: [PATCH 3/9] coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers Co-authored-by: Ryan Ofsky --- src/coins.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coins.h b/src/coins.h index ffeaae3ede..1ae99f409a 100644 --- a/src/coins.h +++ b/src/coins.h @@ -166,11 +166,13 @@ public: { 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; } static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } @@ -182,6 +184,7 @@ public: m_next->second.m_prev = m_prev; m_prev->second.m_next = m_next; m_flags = 0; + m_prev = m_next = nullptr; } uint8_t GetFlags() const noexcept { return m_flags; } bool IsDirty() const noexcept { return m_flags & DIRTY; } From 15aaa81c3818b4138602c127d6a16018aae75687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 13 Sep 2024 11:13:28 +0200 Subject: [PATCH 4/9] coins, refactor: Remove direct GetFlags access We don't need so much access to the internals of CCoinsCacheEntry, since many tests are just exercising invalid combinations this way. This implies that `AddFlags` has private access now. --- src/coins.h | 32 ++++++++++----------- src/test/coins_tests.cpp | 4 ++- src/test/coinscachepair_tests.cpp | 48 +++++++++++++++---------------- 3 files changed, 42 insertions(+), 42 deletions(-) diff --git a/src/coins.h b/src/coins.h index 1ae99f409a..61fb4af642 100644 --- a/src/coins.h +++ b/src/coins.h @@ -128,6 +128,21 @@ private: 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 + { + 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; + } + public: Coin coin; // The actual cached data. @@ -159,22 +174,6 @@ public: 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. - static void AddFlags(uint8_t flags, CoinsCachePair& pair, CoinsCachePair& sentinel) 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; - } static void SetDirty(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(DIRTY, pair, sentinel); } static void SetFresh(CoinsCachePair& pair, CoinsCachePair& sentinel) noexcept { AddFlags(FRESH, pair, sentinel); } @@ -186,7 +185,6 @@ public: m_flags = 0; m_prev = m_next = nullptr; } - uint8_t GetFlags() const noexcept { return m_flags; } bool IsDirty() const noexcept { return m_flags & DIRTY; } bool IsFresh() const noexcept { return m_flags & FRESH; } diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index f9c226eb52..e667709418 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -613,7 +613,9 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C } else { value = it->second.coin.out.nValue; } - flags = it->second.GetFlags(); + flags = 0; + if (it->second.IsDirty()) flags |= DIRTY; + if (it->second.IsFresh()) flags |= FRESH; assert(flags != NO_ENTRY); } } diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp index 6460579d94..0c208e93df 100644 --- a/src/test/coinscachepair_tests.cpp +++ b/src/test/coinscachepair_tests.cpp @@ -21,7 +21,7 @@ std::list CreatePairs(CoinsCachePair& sentinel) auto node{std::prev(nodes.end())}; CCoinsCacheEntry::SetDirty(*node, sentinel); - BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh()); BOOST_CHECK_EQUAL(node->second.Next(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node)); @@ -48,7 +48,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) BOOST_CHECK_EQUAL(node, &sentinel); // Check iterating through pairs is identical to iterating through a list - // Clear the flags during iteration + // Clear the state during iteration node = sentinel.second.Next(); for (const auto& expected : nodes) { BOOST_CHECK_EQUAL(&expected, node); @@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) // 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_CHECK(!it->second.IsDirty() && !it->second.IsFresh()); } } @@ -74,8 +74,8 @@ BOOST_AUTO_TEST_CASE(linked_list_iterate_erase) 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 + // Erase the nodes as we iterate through, but don't clear state + // The state 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); @@ -104,10 +104,10 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // 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); + // Also check that state was not altered + BOOST_CHECK(n1->second.IsDirty() && !n1->second.IsFresh()); BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3)); - BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1)); @@ -115,8 +115,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // 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); + // Also check that state was not altered + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4)); BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel); @@ -125,8 +125,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) // 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); + // Also check that state was not altered + BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3)); BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3)); @@ -139,48 +139,48 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel); } -BOOST_AUTO_TEST_CASE(linked_list_add_flags) +BOOST_AUTO_TEST_CASE(linked_list_set_state) { CoinsCachePair sentinel; sentinel.second.SelfRef(sentinel); CoinsCachePair n1; CoinsCachePair n2; - // Check that adding DIRTY flag inserts it into linked list and sets flags + // Check that setting DIRTY inserts it into linked list and sets state CCoinsCacheEntry::SetDirty(n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh()); 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 + // Check that setting FRESH on new node inserts it after n1 CCoinsCacheEntry::SetFresh(n2, sentinel); - BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH); + BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty()); 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 we can add extra flags, but they don't change our position + // Check that we can set extra state, but they don't change our position CCoinsCacheEntry::SetFresh(n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH); + BOOST_CHECK(n1.second.IsDirty() && n1.second.IsFresh()); 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 + // Check that we can clear state then re-set it n1.second.SetClean(); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh()); 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 `SetClean` with 0 flags has no effect + // Calling `SetClean` a second time has no effect n1.second.SetClean(); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0); + BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2); BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2); BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel); @@ -188,7 +188,7 @@ BOOST_AUTO_TEST_CASE(linked_list_add_flags) // Adding DIRTY re-inserts it after n2 CCoinsCacheEntry::SetDirty(n1, sentinel); - BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY); + BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh()); BOOST_CHECK_EQUAL(n2.second.Next(), &n1); BOOST_CHECK_EQUAL(n1.second.Prev(), &n2); BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel); From ca74aa7490a5005d227da75dc8f2d1ab73c6e9d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 16 Sep 2024 13:49:56 +0200 Subject: [PATCH 5/9] test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin --- src/test/coins_tests.cpp | 168 +++++++++++++++++---------------------- 1 file changed, 73 insertions(+), 95 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index e667709418..b90a60f058 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -565,10 +565,23 @@ const static CAmount FAIL = -3; const static CAmount VALUE1 = 100; const static CAmount VALUE2 = 200; const static CAmount VALUE3 = 300; + const static char DIRTY = CCoinsCacheEntry::DIRTY; const static char FRESH = CCoinsCacheEntry::FRESH; const static char NO_ENTRY = -1; +struct CoinEntry { + const CAmount value; + const char flags; + + constexpr CoinEntry(const CAmount v, const char s) : value{v}, flags{s} {} + + bool operator==(const CoinEntry& o) const = default; + friend std::ostream& operator<<(std::ostream& os, const CoinEntry& e) { return os << e.value << ", " << e.flags; } +}; + +using MaybeCoin = std::optional; + const static auto FLAGS = {char(0), FRESH, DIRTY, char(DIRTY | FRESH)}; const static auto CLEAN_FLAGS = {char(0), FRESH}; const static auto ABSENT_FLAGS = {NO_ENTRY}; @@ -585,48 +598,39 @@ static void SetCoinsValue(CAmount value, Coin& coin) } } -static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmount value, char flags) +static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, const CoinEntry& cache_coin) { - if (value == ABSENT) { - assert(flags == NO_ENTRY); + if (cache_coin.value == ABSENT) { + assert(cache_coin.flags == NO_ENTRY); return 0; } - assert(flags != NO_ENTRY); + assert(cache_coin.flags != NO_ENTRY); CCoinsCacheEntry entry; - SetCoinsValue(value, entry.coin); + SetCoinsValue(cache_coin.value, entry.coin); auto inserted = map.emplace(OUTPOINT, std::move(entry)); assert(inserted.second); - if (flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel); - if (flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel); + if (cache_coin.flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel); + if (cache_coin.flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel); return inserted.first->second.coin.DynamicMemoryUsage(); } -void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const COutPoint& outp = OUTPOINT) +MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOINT) { - auto it = map.find(outp); - if (it == map.end()) { - value = ABSENT; - flags = NO_ENTRY; - } else { - if (it->second.coin.IsSpent()) { - value = SPENT; - } else { - value = it->second.coin.out.nValue; - } - flags = 0; - if (it->second.IsDirty()) flags |= DIRTY; - if (it->second.IsFresh()) flags |= FRESH; - assert(flags != NO_ENTRY); + if (auto it{map.find(outp)}; it != map.end()) { + return CoinEntry{ + it->second.coin.IsSpent() ? SPENT : it->second.coin.out.nValue, + static_cast((it->second.IsDirty() ? DIRTY : 0) | (it->second.IsFresh() ? FRESH : 0))}; } + return CoinEntry{ABSENT, NO_ENTRY}; // TODO empty } -void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) +void WriteCoinsViewEntry(CCoinsView& view, const CoinEntry& cache_coin) { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)}; + auto usage{InsertCoinsMapEntry(map, sentinel, cache_coin)}; auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } @@ -634,10 +638,10 @@ void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) class SingleEntryCacheTest { public: - SingleEntryCacheTest(CAmount base_value, CAmount cache_value, char cache_flags) + SingleEntryCacheTest(CAmount base_value, const CoinEntry& cache_coin) { - WriteCoinsViewEntry(base, base_value, base_value == ABSENT ? NO_ENTRY : DIRTY); - cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_value, cache_flags); + WriteCoinsViewEntry(base, CoinEntry{base_value, base_value == ABSENT ? NO_ENTRY : DIRTY}); // TODO complete state should be absent + cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_coin); } CCoinsView root; @@ -645,17 +649,16 @@ public: CCoinsViewCacheTest cache{&base}; }; -static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +static void CheckAccessCoin(CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); + SingleEntryCacheTest test{base_value, cache_coin}; test.cache.AccessCoin(OUTPOINT); test.cache.SelfTest(/*sanity_check=*/false); - - CAmount result_value; - char result_flags; - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); +} +static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +{ + CheckAccessCoin(base_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}); } BOOST_AUTO_TEST_CASE(ccoins_access) @@ -696,18 +699,17 @@ BOOST_AUTO_TEST_CASE(ccoins_access) CheckAccessCoin(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); } -static void CheckSpendCoins(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +static void CheckSpendCoins(CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); + SingleEntryCacheTest test{base_value, cache_coin}; test.cache.SpendCoin(OUTPOINT); test.cache.SelfTest(); - - CAmount result_value; - char result_flags; - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); -}; + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); +} +static void CheckSpendCoins(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) +{ + CheckSpendCoins(base_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}); +} BOOST_AUTO_TEST_CASE(ccoins_spend) { @@ -747,25 +749,22 @@ BOOST_AUTO_TEST_CASE(ccoins_spend) CheckSpendCoins(VALUE1, VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); } -static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount modify_value, CAmount expected_value, char cache_flags, char expected_flags, bool coinbase) +static void CheckAddCoinBase(CAmount base_value, CAmount modify_value, const CoinEntry& cache_coin, const CoinEntry& expected, bool coinbase) { - SingleEntryCacheTest test(base_value, cache_value, cache_flags); - - CAmount result_value; - char result_flags; + SingleEntryCacheTest test{base_value, cache_coin}; try { CTxOut output; output.nValue = modify_value; test.cache.AddCoin(OUTPOINT, Coin(std::move(output), 1, coinbase), coinbase); test.cache.SelfTest(); - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } catch (std::logic_error&) { - result_value = FAIL; - result_flags = NO_ENTRY; + BOOST_CHECK_EQUAL(CoinEntry(FAIL, NO_ENTRY), expected); // TODO empty } - - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); +} +static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount modify_value, CAmount expected_value, char cache_flags, char expected_flags, bool coinbase) +{ + CheckAddCoinBase(base_value, modify_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}, coinbase); } // Simple wrapper for CheckAddCoinBase function above that loops through @@ -810,23 +809,20 @@ BOOST_AUTO_TEST_CASE(ccoins_add) CheckAddCoin(VALUE2, VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true ); } +void CheckWriteCoins(const CoinEntry& parent, const CoinEntry& child, const CoinEntry& expected) +{ + SingleEntryCacheTest test{ABSENT, parent}; + try { + WriteCoinsViewEntry(test.cache, child); + test.cache.SelfTest(/*sanity_check=*/false); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); + } catch (std::logic_error&) { + BOOST_CHECK_EQUAL(CoinEntry(FAIL, NO_ENTRY), expected); // TODO empty + } +} void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected_value, char parent_flags, char child_flags, char expected_flags) { - SingleEntryCacheTest test(ABSENT, parent_value, parent_flags); - - CAmount result_value; - char result_flags; - try { - WriteCoinsViewEntry(test.cache, child_value, child_flags); - test.cache.SelfTest(/*sanity_check=*/false); - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); - } catch (std::logic_error&) { - result_value = FAIL; - result_flags = NO_ENTRY; - } - - BOOST_CHECK_EQUAL(result_value, expected_value); - BOOST_CHECK_EQUAL(result_flags, expected_flags); + CheckWriteCoins(CoinEntry{parent_value, parent_flags}, CoinEntry{child_value, child_flags}, CoinEntry{expected_value, expected_flags}); } BOOST_AUTO_TEST_CASE(ccoins_write) @@ -923,8 +919,6 @@ void TestFlushBehavior( std::vector>& all_caches, bool do_erasing_flush) { - CAmount value; - char flags; size_t cache_usage; size_t cache_size; @@ -958,9 +952,7 @@ void TestFlushBehavior( BOOST_CHECK(!base.HaveCoin(outp)); BOOST_CHECK(view->HaveCoin(outp)); - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, DIRTY|FRESH)); // --- 2. Flushing all caches (without erasing) // @@ -972,9 +964,7 @@ void TestFlushBehavior( // --- 3. Ensuring the entry still exists in the cache and has been written to parent // - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, 0); // Flags should have been wiped. + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, 0)); // Flags should have been wiped. // Both views should now have the coin. BOOST_CHECK(base.HaveCoin(outp)); @@ -992,14 +982,9 @@ void TestFlushBehavior( // --- 5. Ensuring the entry is no longer in the cache // - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, ABSENT); - BOOST_CHECK_EQUAL(flags, NO_ENTRY); - + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(ABSENT, NO_ENTRY)); view->AccessCoin(outp); - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin.out.nValue); - BOOST_CHECK_EQUAL(flags, 0); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, 0)); } // Can't overwrite an entry without specifying that an overwrite is @@ -1013,9 +998,7 @@ void TestFlushBehavior( BOOST_CHECK(view->SpendCoin(outp)); // The coin should be in the cache, but spent and marked dirty. - GetCoinsMapEntry(view->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, SPENT); - BOOST_CHECK_EQUAL(flags, DIRTY); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(SPENT, DIRTY)); BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`. BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`. @@ -1066,10 +1049,7 @@ void TestFlushBehavior( all_caches[0]->AddCoin(outp, std::move(coin), false); // Coin should be FRESH in the cache. - GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, coin_val); - BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); - + BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, DIRTY|FRESH)); // Base shouldn't have seen coin. BOOST_CHECK(!base.HaveCoin(outp)); @@ -1077,9 +1057,7 @@ void TestFlushBehavior( all_caches[0]->Sync(); // Ensure there is no sign of the coin after spend/flush. - GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); - BOOST_CHECK_EQUAL(value, ABSENT); - BOOST_CHECK_EQUAL(flags, NO_ENTRY); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(ABSENT, NO_ENTRY)); BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); BOOST_CHECK(!base.HaveCoin(outp)); } From d5f8d607ab1f8fd9fc17aba4105867b450117be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sun, 15 Sep 2024 20:21:32 +0200 Subject: [PATCH 6/9] test: Group values and states in tests into CoinEntry wrappers Note: that this commit affects the test order, but doesn't change its behavior or coverage otherwise. --- src/test/coins_tests.cpp | 355 +++++++++++++++++++-------------------- 1 file changed, 175 insertions(+), 180 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index b90a60f058..42b704fd4d 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -568,6 +568,7 @@ const static CAmount VALUE3 = 300; const static char DIRTY = CCoinsCacheEntry::DIRTY; const static char FRESH = CCoinsCacheEntry::FRESH; +const static char CLEAN = 0; const static char NO_ENTRY = -1; struct CoinEntry { @@ -582,11 +583,25 @@ struct CoinEntry { using MaybeCoin = std::optional; -const static auto FLAGS = {char(0), FRESH, DIRTY, char(DIRTY | FRESH)}; -const static auto CLEAN_FLAGS = {char(0), FRESH}; -const static auto ABSENT_FLAGS = {NO_ENTRY}; +const static CoinEntry MISSING = CoinEntry{ABSENT, NO_ENTRY}; // TODO std::nullopt +const static CoinEntry FAIL_NO_ENTRY = CoinEntry{FAIL, NO_ENTRY}; // TODO validate error messages -static void SetCoinsValue(CAmount value, Coin& coin) +const static CoinEntry SPENT_DIRTY = CoinEntry{SPENT, DIRTY}; +const static CoinEntry SPENT_DIRTY_FRESH = CoinEntry{SPENT, DIRTY | FRESH}; +const static CoinEntry SPENT_FRESH = CoinEntry{SPENT, FRESH}; +const static CoinEntry SPENT_CLEAN = CoinEntry{SPENT, CLEAN}; +const static CoinEntry VALUE1_DIRTY = CoinEntry{VALUE1, DIRTY}; +const static CoinEntry VALUE1_DIRTY_FRESH = CoinEntry{VALUE1, DIRTY | FRESH}; +const static CoinEntry VALUE1_FRESH = CoinEntry{VALUE1, FRESH}; +const static CoinEntry VALUE1_CLEAN = CoinEntry{VALUE1, CLEAN}; +const static CoinEntry VALUE2_DIRTY = CoinEntry{VALUE2, DIRTY}; +const static CoinEntry VALUE2_DIRTY_FRESH = CoinEntry{VALUE2, DIRTY | FRESH}; +const static CoinEntry VALUE2_FRESH = CoinEntry{VALUE2, FRESH}; +const static CoinEntry VALUE2_CLEAN = CoinEntry{VALUE2, CLEAN}; +const static CoinEntry VALUE3_DIRTY = CoinEntry{VALUE3, DIRTY}; +const static CoinEntry VALUE3_DIRTY_FRESH = CoinEntry{VALUE3, DIRTY | FRESH}; + +static void SetCoinsValue(const CAmount value, Coin& coin) { assert(value != ABSENT); coin.Clear(); @@ -621,7 +636,7 @@ MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOIN it->second.coin.IsSpent() ? SPENT : it->second.coin.out.nValue, static_cast((it->second.IsDirty() ? DIRTY : 0) | (it->second.IsFresh() ? FRESH : 0))}; } - return CoinEntry{ABSENT, NO_ENTRY}; // TODO empty + return MISSING; } void WriteCoinsViewEntry(CCoinsView& view, const CoinEntry& cache_coin) @@ -638,7 +653,7 @@ void WriteCoinsViewEntry(CCoinsView& view, const CoinEntry& cache_coin) class SingleEntryCacheTest { public: - SingleEntryCacheTest(CAmount base_value, const CoinEntry& cache_coin) + SingleEntryCacheTest(const CAmount base_value, const CoinEntry& cache_coin) { WriteCoinsViewEntry(base, CoinEntry{base_value, base_value == ABSENT ? NO_ENTRY : DIRTY}); // TODO complete state should be absent cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_coin); @@ -649,107 +664,99 @@ public: CCoinsViewCacheTest cache{&base}; }; -static void CheckAccessCoin(CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) +static void CheckAccessCoin(const CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) { SingleEntryCacheTest test{base_value, cache_coin}; test.cache.AccessCoin(OUTPOINT); test.cache.SelfTest(/*sanity_check=*/false); BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } -static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) -{ - CheckAccessCoin(base_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}); -} BOOST_AUTO_TEST_CASE(ccoins_access) { /* Check AccessCoin behavior, requesting a coin from a cache view layered on * top of a base view, and checking the resulting entry in the cache after * the access. - * - * Base Cache Result Cache Result - * Value Value Value Flags Flags + * Base Cache Expected */ - CheckAccessCoin(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckAccessCoin(ABSENT, SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(ABSENT, SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(ABSENT, SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(ABSENT, SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(SPENT , ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckAccessCoin(SPENT , SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(SPENT , SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(SPENT , SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(SPENT , SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(SPENT , VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(VALUE1, ABSENT, VALUE1, NO_ENTRY , 0 ); - CheckAccessCoin(VALUE1, SPENT , SPENT , 0 , 0 ); - CheckAccessCoin(VALUE1, SPENT , SPENT , FRESH , FRESH ); - CheckAccessCoin(VALUE1, SPENT , SPENT , DIRTY , DIRTY ); - CheckAccessCoin(VALUE1, SPENT , SPENT , DIRTY|FRESH, DIRTY|FRESH); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, 0 , 0 ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, FRESH , FRESH ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY ); - CheckAccessCoin(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH); + CheckAccessCoin(ABSENT, MISSING, MISSING ); + CheckAccessCoin(ABSENT, SPENT_CLEAN, SPENT_CLEAN ); + CheckAccessCoin(ABSENT, SPENT_FRESH, SPENT_FRESH ); + CheckAccessCoin(ABSENT, SPENT_DIRTY, SPENT_DIRTY ); + CheckAccessCoin(ABSENT, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); + CheckAccessCoin(ABSENT, VALUE2_CLEAN, VALUE2_CLEAN ); + CheckAccessCoin(ABSENT, VALUE2_FRESH, VALUE2_FRESH ); + CheckAccessCoin(ABSENT, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckAccessCoin(ABSENT, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + + CheckAccessCoin(SPENT, MISSING, MISSING ); + CheckAccessCoin(SPENT, SPENT_CLEAN, SPENT_CLEAN ); + CheckAccessCoin(SPENT, SPENT_FRESH, SPENT_FRESH ); + CheckAccessCoin(SPENT, SPENT_DIRTY, SPENT_DIRTY ); + CheckAccessCoin(SPENT, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); + CheckAccessCoin(SPENT, VALUE2_CLEAN, VALUE2_CLEAN ); + CheckAccessCoin(SPENT, VALUE2_FRESH, VALUE2_FRESH ); + CheckAccessCoin(SPENT, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckAccessCoin(SPENT, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + + CheckAccessCoin(VALUE1, MISSING, VALUE1_CLEAN ); + CheckAccessCoin(VALUE1, SPENT_CLEAN, SPENT_CLEAN ); + CheckAccessCoin(VALUE1, SPENT_FRESH, SPENT_FRESH ); + CheckAccessCoin(VALUE1, SPENT_DIRTY, SPENT_DIRTY ); + CheckAccessCoin(VALUE1, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); + CheckAccessCoin(VALUE1, VALUE2_CLEAN, VALUE2_CLEAN ); + CheckAccessCoin(VALUE1, VALUE2_FRESH, VALUE2_FRESH ); + CheckAccessCoin(VALUE1, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckAccessCoin(VALUE1, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); } -static void CheckSpendCoins(CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) +static void CheckSpendCoins(const CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) { SingleEntryCacheTest test{base_value, cache_coin}; test.cache.SpendCoin(OUTPOINT); test.cache.SelfTest(); BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } -static void CheckSpendCoins(CAmount base_value, CAmount cache_value, CAmount expected_value, char cache_flags, char expected_flags) -{ - CheckSpendCoins(base_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}); -} BOOST_AUTO_TEST_CASE(ccoins_spend) { /* Check SpendCoin behavior, requesting a coin from a cache view layered on * top of a base view, spending, and then checking * the resulting entry in the cache after the modification. - * - * Base Cache Result Cache Result - * Value Value Value Flags Flags + * Base Cache Expected */ - CheckSpendCoins(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckSpendCoins(ABSENT, SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(ABSENT, SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(ABSENT, SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(ABSENT, SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(ABSENT, VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(ABSENT, VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(ABSENT, VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(ABSENT, VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(SPENT , ABSENT, ABSENT, NO_ENTRY , NO_ENTRY ); - CheckSpendCoins(SPENT , SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(SPENT , SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(SPENT , VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(SPENT , VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(SPENT , VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(SPENT , VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(VALUE1, ABSENT, SPENT , NO_ENTRY , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , SPENT , 0 , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(VALUE1, SPENT , SPENT , DIRTY , DIRTY ); - CheckSpendCoins(VALUE1, SPENT , ABSENT, DIRTY|FRESH, NO_ENTRY ); - CheckSpendCoins(VALUE1, VALUE2, SPENT , 0 , DIRTY ); - CheckSpendCoins(VALUE1, VALUE2, ABSENT, FRESH , NO_ENTRY ); - CheckSpendCoins(VALUE1, VALUE2, SPENT , DIRTY , DIRTY ); - CheckSpendCoins(VALUE1, VALUE2, ABSENT, DIRTY|FRESH, NO_ENTRY ); + CheckSpendCoins(ABSENT, MISSING, MISSING ); + CheckSpendCoins(ABSENT, SPENT_CLEAN, SPENT_DIRTY); + CheckSpendCoins(ABSENT, SPENT_FRESH, MISSING ); + CheckSpendCoins(ABSENT, SPENT_DIRTY, SPENT_DIRTY); + CheckSpendCoins(ABSENT, SPENT_DIRTY_FRESH, MISSING ); + CheckSpendCoins(ABSENT, VALUE2_CLEAN, SPENT_DIRTY); + CheckSpendCoins(ABSENT, VALUE2_FRESH, MISSING ); + CheckSpendCoins(ABSENT, VALUE2_DIRTY, SPENT_DIRTY); + CheckSpendCoins(ABSENT, VALUE2_DIRTY_FRESH, MISSING ); + + CheckSpendCoins(SPENT, MISSING, MISSING ); + CheckSpendCoins(SPENT, SPENT_CLEAN, SPENT_DIRTY); + CheckSpendCoins(SPENT, SPENT_FRESH, MISSING ); + CheckSpendCoins(SPENT, SPENT_DIRTY, SPENT_DIRTY); + CheckSpendCoins(SPENT, SPENT_DIRTY_FRESH, MISSING ); + CheckSpendCoins(SPENT, VALUE2_CLEAN, SPENT_DIRTY); + CheckSpendCoins(SPENT, VALUE2_FRESH, MISSING ); + CheckSpendCoins(SPENT, VALUE2_DIRTY, SPENT_DIRTY); + CheckSpendCoins(SPENT, VALUE2_DIRTY_FRESH, MISSING ); + + CheckSpendCoins(VALUE1, MISSING, SPENT_DIRTY); + CheckSpendCoins(VALUE1, SPENT_CLEAN, SPENT_DIRTY); + CheckSpendCoins(VALUE1, SPENT_FRESH, MISSING ); + CheckSpendCoins(VALUE1, SPENT_DIRTY, SPENT_DIRTY); + CheckSpendCoins(VALUE1, SPENT_DIRTY_FRESH, MISSING ); + CheckSpendCoins(VALUE1, VALUE2_CLEAN, SPENT_DIRTY); + CheckSpendCoins(VALUE1, VALUE2_FRESH, MISSING ); + CheckSpendCoins(VALUE1, VALUE2_DIRTY, SPENT_DIRTY); + CheckSpendCoins(VALUE1, VALUE2_DIRTY_FRESH, MISSING ); } -static void CheckAddCoinBase(CAmount base_value, CAmount modify_value, const CoinEntry& cache_coin, const CoinEntry& expected, bool coinbase) +static void CheckAddCoin(const CAmount base_value, const CoinEntry& cache_coin, const CAmount modify_value, const CoinEntry& expected, const bool coinbase) { SingleEntryCacheTest test{base_value, cache_coin}; try { @@ -759,54 +766,40 @@ static void CheckAddCoinBase(CAmount base_value, CAmount modify_value, const Coi test.cache.SelfTest(); BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } catch (std::logic_error&) { - BOOST_CHECK_EQUAL(CoinEntry(FAIL, NO_ENTRY), expected); // TODO empty + BOOST_CHECK_EQUAL(FAIL_NO_ENTRY, expected); } } -static void CheckAddCoinBase(CAmount base_value, CAmount cache_value, CAmount modify_value, CAmount expected_value, char cache_flags, char expected_flags, bool coinbase) -{ - CheckAddCoinBase(base_value, modify_value, CoinEntry{cache_value, cache_flags}, CoinEntry{expected_value, expected_flags}, coinbase); -} - -// Simple wrapper for CheckAddCoinBase function above that loops through -// different possible base_values, making sure each one gives the same results. -// This wrapper lets the coins_add test below be shorter and less repetitive, -// while still verifying that the CoinsViewCache::AddCoin implementation -// ignores base values. -template -static void CheckAddCoin(Args&&... args) -{ - for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) - CheckAddCoinBase(base_value, std::forward(args)...); -} BOOST_AUTO_TEST_CASE(ccoins_add) { /* Check AddCoin behavior, requesting a new coin from a cache view, * writing a modification to the coin, and then checking the resulting * entry in the cache after the modification. Verify behavior with the - * AddCoin possible_overwrite argument set to false, and to true. - * - * Cache Write Result Cache Result possible_overwrite - * Value Value Value Flags Flags + * AddCoin coinbase argument set to false, and to true. + * Base Cache Write Expected Coinbase */ - CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY , DIRTY|FRESH, false); - CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, 0 , DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, 0 , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH , DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH , DIRTY|FRESH, true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY , DIRTY , false); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY , DIRTY , true ); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, false); - CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , 0 , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, 0 , DIRTY , true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , FRESH , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, FRESH , DIRTY|FRESH, true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , DIRTY , NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, DIRTY , DIRTY , true ); - CheckAddCoin(VALUE2, VALUE3, FAIL , DIRTY|FRESH, NO_ENTRY , false); - CheckAddCoin(VALUE2, VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true ); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY, true ); + + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, false); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, FAIL_NO_ENTRY, false); + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, FAIL_NO_ENTRY, false); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, FAIL_NO_ENTRY, false); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, FAIL_NO_ENTRY, false); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + } } void CheckWriteCoins(const CoinEntry& parent, const CoinEntry& child, const CoinEntry& expected) @@ -817,81 +810,83 @@ void CheckWriteCoins(const CoinEntry& parent, const CoinEntry& child, const Coin test.cache.SelfTest(/*sanity_check=*/false); BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } catch (std::logic_error&) { - BOOST_CHECK_EQUAL(CoinEntry(FAIL, NO_ENTRY), expected); // TODO empty + BOOST_CHECK_EQUAL(FAIL_NO_ENTRY, expected); } } -void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected_value, char parent_flags, char child_flags, char expected_flags) -{ - CheckWriteCoins(CoinEntry{parent_value, parent_flags}, CoinEntry{child_value, child_flags}, CoinEntry{expected_value, expected_flags}); -} BOOST_AUTO_TEST_CASE(ccoins_write) { /* Check BatchWrite behavior, flushing one entry from a child cache to a * parent cache, and checking the resulting entry in the parent cache * after the write. - * - * Parent Child Result Parent Child Result - * Value Value Value Flags Flags Flags + * Parent Child Expected */ - CheckWriteCoins(ABSENT, ABSENT, ABSENT, NO_ENTRY , NO_ENTRY , NO_ENTRY ); - CheckWriteCoins(ABSENT, SPENT , SPENT , NO_ENTRY , DIRTY , DIRTY ); - CheckWriteCoins(ABSENT, SPENT , ABSENT, NO_ENTRY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(ABSENT, VALUE2, VALUE2, NO_ENTRY , DIRTY , DIRTY ); - CheckWriteCoins(ABSENT, VALUE2, VALUE2, NO_ENTRY , DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(SPENT , ABSENT, SPENT , 0 , NO_ENTRY , 0 ); - CheckWriteCoins(SPENT , ABSENT, SPENT , FRESH , NO_ENTRY , FRESH ); - CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY , NO_ENTRY , DIRTY ); - CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH); - CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY , NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY ); - CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY , DIRTY|FRESH, DIRTY ); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY , DIRTY|FRESH); - CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH, DIRTY|FRESH); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, 0 , NO_ENTRY , 0 ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, FRESH , NO_ENTRY , FRESH ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY , NO_ENTRY , DIRTY ); - CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, SPENT , SPENT , 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , ABSENT, FRESH , DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , SPENT , DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, SPENT , FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); + CheckWriteCoins(MISSING, MISSING, MISSING ); + CheckWriteCoins(MISSING, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(MISSING, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(MISSING, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + CheckWriteCoins(SPENT_CLEAN, MISSING, SPENT_CLEAN ); + CheckWriteCoins(SPENT_FRESH, MISSING, SPENT_FRESH ); + CheckWriteCoins(SPENT_DIRTY, MISSING, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, MISSING, SPENT_DIRTY_FRESH ); + + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, MISSING ); + + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + + CheckWriteCoins(VALUE1_CLEAN, MISSING, VALUE1_CLEAN ); + CheckWriteCoins(VALUE1_FRESH, MISSING, VALUE1_FRESH ); + CheckWriteCoins(VALUE1_DIRTY, MISSING, VALUE1_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, MISSING, VALUE1_DIRTY_FRESH); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); + + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); // The checks above omit cases where the child flags are not DIRTY, since // they would be too repetitive (the parent cache is never updated in these // cases). The loop below covers these cases and makes sure the parent cache // is always left unchanged. - for (const CAmount parent_value : {ABSENT, SPENT, VALUE1}) - for (const CAmount child_value : {ABSENT, SPENT, VALUE2}) - for (const char parent_flags : parent_value == ABSENT ? ABSENT_FLAGS : FLAGS) - for (const char child_flags : child_value == ABSENT ? ABSENT_FLAGS : CLEAN_FLAGS) - CheckWriteCoins(parent_value, child_value, parent_value, parent_flags, child_flags, parent_flags); + for (const CoinEntry parent : {MISSING, + SPENT_CLEAN, SPENT_DIRTY, SPENT_FRESH, SPENT_DIRTY_FRESH, + VALUE1_CLEAN, VALUE1_DIRTY, VALUE1_FRESH, VALUE1_DIRTY_FRESH}) { + for (const CoinEntry child : {MISSING, + SPENT_CLEAN, SPENT_FRESH, + VALUE2_CLEAN, VALUE2_FRESH}) { + auto expected{parent}; // TODO test failure cases as well + CheckWriteCoins(parent, child, expected); + } + } } - struct FlushTest : BasicTestingSetup { Coin MakeCoin() { @@ -964,7 +959,7 @@ void TestFlushBehavior( // --- 3. Ensuring the entry still exists in the cache and has been written to parent // - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, 0)); // Flags should have been wiped. + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CLEAN)); // Flags should have been wiped. // Both views should now have the coin. BOOST_CHECK(base.HaveCoin(outp)); @@ -982,9 +977,9 @@ void TestFlushBehavior( // --- 5. Ensuring the entry is no longer in the cache // - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(ABSENT, NO_ENTRY)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), MISSING); view->AccessCoin(outp); - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, 0)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CLEAN)); } // Can't overwrite an entry without specifying that an overwrite is @@ -998,7 +993,7 @@ void TestFlushBehavior( BOOST_CHECK(view->SpendCoin(outp)); // The coin should be in the cache, but spent and marked dirty. - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(SPENT, DIRTY)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), SPENT_DIRTY); BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`. BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`. @@ -1057,7 +1052,7 @@ void TestFlushBehavior( all_caches[0]->Sync(); // Ensure there is no sign of the coin after spend/flush. - BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(ABSENT, NO_ENTRY)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), MISSING); BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); BOOST_CHECK(!base.HaveCoin(outp)); } From c0b4b2c1eef95c0b626573a9a2e217f4a541a880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 18 Sep 2024 08:18:44 +0200 Subject: [PATCH 7/9] test: Validate error messages on fail The `ccoins_add` and `ccoins_write` tests check the actual exception error messages now instead of just that they fail for the given parameters. This enables us testing different exceptions in a more fine-grained way in later changes. --- src/test/coins_tests.cpp | 261 +++++++++++++++++++-------------------- 1 file changed, 130 insertions(+), 131 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 42b704fd4d..2405cf1cf4 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -15,6 +15,8 @@ #include #include +#include +#include #include #include @@ -559,17 +561,15 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) } const static COutPoint OUTPOINT; -const static CAmount SPENT = -1; -const static CAmount ABSENT = -2; -const static CAmount FAIL = -3; -const static CAmount VALUE1 = 100; -const static CAmount VALUE2 = 200; -const static CAmount VALUE3 = 300; +constexpr CAmount SPENT {-1}; +constexpr CAmount ABSENT{-2}; +constexpr CAmount VALUE1{100}; +constexpr CAmount VALUE2{200}; +constexpr CAmount VALUE3{300}; -const static char DIRTY = CCoinsCacheEntry::DIRTY; -const static char FRESH = CCoinsCacheEntry::FRESH; -const static char CLEAN = 0; -const static char NO_ENTRY = -1; +constexpr char DIRTY{CCoinsCacheEntry::DIRTY}; +constexpr char FRESH{CCoinsCacheEntry::FRESH}; +constexpr char CLEAN{0}; struct CoinEntry { const CAmount value; @@ -581,25 +581,27 @@ struct CoinEntry { friend std::ostream& operator<<(std::ostream& os, const CoinEntry& e) { return os << e.value << ", " << e.flags; } }; -using MaybeCoin = std::optional; +using MaybeCoin = std::optional; +using CoinOrError = std::variant; -const static CoinEntry MISSING = CoinEntry{ABSENT, NO_ENTRY}; // TODO std::nullopt -const static CoinEntry FAIL_NO_ENTRY = CoinEntry{FAIL, NO_ENTRY}; // TODO validate error messages +constexpr MaybeCoin MISSING {std::nullopt}; +constexpr MaybeCoin SPENT_DIRTY {{SPENT, DIRTY}}; +constexpr MaybeCoin SPENT_DIRTY_FRESH {{SPENT, DIRTY | FRESH}}; +constexpr MaybeCoin SPENT_FRESH {{SPENT, FRESH}}; +constexpr MaybeCoin SPENT_CLEAN {{SPENT, CLEAN}}; +constexpr MaybeCoin VALUE1_DIRTY {{VALUE1, DIRTY}}; +constexpr MaybeCoin VALUE1_DIRTY_FRESH{{VALUE1, DIRTY | FRESH}}; +constexpr MaybeCoin VALUE1_FRESH {{VALUE1, FRESH}}; +constexpr MaybeCoin VALUE1_CLEAN {{VALUE1, CLEAN}}; +constexpr MaybeCoin VALUE2_DIRTY {{VALUE2, DIRTY}}; +constexpr MaybeCoin VALUE2_DIRTY_FRESH{{VALUE2, DIRTY | FRESH}}; +constexpr MaybeCoin VALUE2_FRESH {{VALUE2, FRESH}}; +constexpr MaybeCoin VALUE2_CLEAN {{VALUE2, CLEAN}}; +constexpr MaybeCoin VALUE3_DIRTY {{VALUE3, DIRTY}}; +constexpr MaybeCoin VALUE3_DIRTY_FRESH{{VALUE3, DIRTY | FRESH}}; -const static CoinEntry SPENT_DIRTY = CoinEntry{SPENT, DIRTY}; -const static CoinEntry SPENT_DIRTY_FRESH = CoinEntry{SPENT, DIRTY | FRESH}; -const static CoinEntry SPENT_FRESH = CoinEntry{SPENT, FRESH}; -const static CoinEntry SPENT_CLEAN = CoinEntry{SPENT, CLEAN}; -const static CoinEntry VALUE1_DIRTY = CoinEntry{VALUE1, DIRTY}; -const static CoinEntry VALUE1_DIRTY_FRESH = CoinEntry{VALUE1, DIRTY | FRESH}; -const static CoinEntry VALUE1_FRESH = CoinEntry{VALUE1, FRESH}; -const static CoinEntry VALUE1_CLEAN = CoinEntry{VALUE1, CLEAN}; -const static CoinEntry VALUE2_DIRTY = CoinEntry{VALUE2, DIRTY}; -const static CoinEntry VALUE2_DIRTY_FRESH = CoinEntry{VALUE2, DIRTY | FRESH}; -const static CoinEntry VALUE2_FRESH = CoinEntry{VALUE2, FRESH}; -const static CoinEntry VALUE2_CLEAN = CoinEntry{VALUE2, CLEAN}; -const static CoinEntry VALUE3_DIRTY = CoinEntry{VALUE3, DIRTY}; -const static CoinEntry VALUE3_DIRTY_FRESH = CoinEntry{VALUE3, DIRTY | FRESH}; +constexpr auto EX_OVERWRITE_UNSPENT{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}; +constexpr auto EX_FRESH_MISAPPLIED {"FRESH flag misapplied to coin that exists in parent cache"}; static void SetCoinsValue(const CAmount value, Coin& coin) { @@ -615,18 +617,13 @@ static void SetCoinsValue(const CAmount value, Coin& coin) static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, const CoinEntry& cache_coin) { - if (cache_coin.value == ABSENT) { - assert(cache_coin.flags == NO_ENTRY); - return 0; - } - assert(cache_coin.flags != NO_ENTRY); CCoinsCacheEntry entry; SetCoinsValue(cache_coin.value, entry.coin); - auto inserted = map.emplace(OUTPOINT, std::move(entry)); - assert(inserted.second); - if (cache_coin.flags & DIRTY) CCoinsCacheEntry::SetDirty(*inserted.first, sentinel); - if (cache_coin.flags & FRESH) CCoinsCacheEntry::SetFresh(*inserted.first, sentinel); - return inserted.first->second.coin.DynamicMemoryUsage(); + auto [iter, inserted] = map.emplace(OUTPOINT, std::move(entry)); + assert(inserted); + if (cache_coin.flags & DIRTY) CCoinsCacheEntry::SetDirty(*iter, sentinel); + if (cache_coin.flags & FRESH) CCoinsCacheEntry::SetFresh(*iter, sentinel); + return iter->second.coin.DynamicMemoryUsage(); } MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOINT) @@ -639,13 +636,13 @@ MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOIN return MISSING; } -void WriteCoinsViewEntry(CCoinsView& view, const CoinEntry& cache_coin) +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}; - auto usage{InsertCoinsMapEntry(map, sentinel, cache_coin)}; + auto usage{cache_coin ? InsertCoinsMapEntry(map, sentinel, *cache_coin) : 0}; auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } @@ -653,10 +650,11 @@ void WriteCoinsViewEntry(CCoinsView& view, const CoinEntry& cache_coin) class SingleEntryCacheTest { public: - SingleEntryCacheTest(const CAmount base_value, const CoinEntry& cache_coin) + SingleEntryCacheTest(const CAmount base_value, const MaybeCoin& cache_coin) { - WriteCoinsViewEntry(base, CoinEntry{base_value, base_value == ABSENT ? NO_ENTRY : DIRTY}); // TODO complete state should be absent - cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_coin); + auto base_cache_coin{base_value == ABSENT ? MISSING : CoinEntry{base_value, DIRTY}}; + WriteCoinsViewEntry(base, base_cache_coin); + if (cache_coin) cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin); } CCoinsView root; @@ -664,7 +662,7 @@ public: CCoinsViewCacheTest cache{&base}; }; -static void CheckAccessCoin(const CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) +static void CheckAccessCoin(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) { SingleEntryCacheTest test{base_value, cache_coin}; test.cache.AccessCoin(OUTPOINT); @@ -710,7 +708,7 @@ BOOST_AUTO_TEST_CASE(ccoins_access) CheckAccessCoin(VALUE1, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); } -static void CheckSpendCoins(const CAmount base_value, const CoinEntry& cache_coin, const CoinEntry& expected) +static void CheckSpendCoins(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) { SingleEntryCacheTest test{base_value, cache_coin}; test.cache.SpendCoin(OUTPOINT); @@ -756,17 +754,17 @@ BOOST_AUTO_TEST_CASE(ccoins_spend) CheckSpendCoins(VALUE1, VALUE2_DIRTY_FRESH, MISSING ); } -static void CheckAddCoin(const CAmount base_value, const CoinEntry& cache_coin, const CAmount modify_value, const CoinEntry& expected, const bool coinbase) +static void CheckAddCoin(const CAmount base_value, const MaybeCoin& cache_coin, const CAmount modify_value, const CoinOrError& expected, const bool coinbase) { SingleEntryCacheTest test{base_value, cache_coin}; - try { - CTxOut output; - output.nValue = modify_value; - test.cache.AddCoin(OUTPOINT, Coin(std::move(output), 1, coinbase), coinbase); + bool possible_overwrite{coinbase}; + auto add_coin{[&] { test.cache.AddCoin(OUTPOINT, Coin{CTxOut{modify_value, CScript{}}, 1, coinbase}, possible_overwrite); }}; + if (auto* expected_coin{std::get_if(&expected)}) { + add_coin(); test.cache.SelfTest(); - BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); - } catch (std::logic_error&) { - BOOST_CHECK_EQUAL(FAIL_NO_ENTRY, expected); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), *expected_coin); + } else { + BOOST_CHECK_EXCEPTION(add_coin(), std::logic_error, HasReason(std::get(expected))); } } @@ -776,41 +774,42 @@ BOOST_AUTO_TEST_CASE(ccoins_add) * writing a modification to the coin, and then checking the resulting * entry in the cache after the modification. Verify behavior with the * AddCoin coinbase argument set to false, and to true. - * Base Cache Write Expected Coinbase + * Base Cache Write Expected Coinbase */ for (auto base_value : {ABSENT, SPENT, VALUE1}) { - CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY_FRESH, false); - CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, MISSING, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY_FRESH, false); - CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); - CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); - CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, false); - CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); - CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, false); + CheckAddCoin(base_value, SPENT_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, false); + CheckAddCoin(base_value, SPENT_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); - CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, FAIL_NO_ENTRY, false); - CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, FAIL_NO_ENTRY, false); - CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); - CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, FAIL_NO_ENTRY, false); - CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, VALUE3_DIRTY, true ); - CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, FAIL_NO_ENTRY, false); - CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_CLEAN, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_DIRTY, VALUE3, VALUE3_DIRTY, true ); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, EX_OVERWRITE_UNSPENT, false); + CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH, true ); } } -void CheckWriteCoins(const CoinEntry& parent, const CoinEntry& child, const CoinEntry& expected) +void CheckWriteCoins(const MaybeCoin& parent, const MaybeCoin& child, const CoinOrError& expected) { SingleEntryCacheTest test{ABSENT, parent}; - try { - WriteCoinsViewEntry(test.cache, child); + auto write_coins{[&] { WriteCoinsViewEntry(test.cache, child); }}; + if (auto* expected_coin{std::get_if(&expected)}) { + write_coins(); test.cache.SelfTest(/*sanity_check=*/false); - BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); - } catch (std::logic_error&) { - BOOST_CHECK_EQUAL(FAIL_NO_ENTRY, expected); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), *expected_coin); + } else { + BOOST_CHECK_EXCEPTION(write_coins(), std::logic_error, HasReason(std::get(expected))); } } @@ -821,67 +820,67 @@ BOOST_AUTO_TEST_CASE(ccoins_write) * after the write. * Parent Child Expected */ - CheckWriteCoins(MISSING, MISSING, MISSING ); - CheckWriteCoins(MISSING, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, MISSING ); - CheckWriteCoins(MISSING, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckWriteCoins(MISSING, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); - CheckWriteCoins(SPENT_CLEAN, MISSING, SPENT_CLEAN ); - CheckWriteCoins(SPENT_FRESH, MISSING, SPENT_FRESH ); - CheckWriteCoins(SPENT_DIRTY, MISSING, SPENT_DIRTY ); - CheckWriteCoins(SPENT_DIRTY_FRESH, MISSING, SPENT_DIRTY_FRESH ); + CheckWriteCoins(MISSING, MISSING, MISSING ); + CheckWriteCoins(MISSING, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(MISSING, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(MISSING, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(MISSING, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_CLEAN, MISSING, SPENT_CLEAN ); + CheckWriteCoins(SPENT_FRESH, MISSING, SPENT_FRESH ); + CheckWriteCoins(SPENT_DIRTY, MISSING, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, MISSING, SPENT_DIRTY_FRESH ); - CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, SPENT_DIRTY ); - CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, MISSING ); - CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, SPENT_DIRTY ); - CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_FRESH, SPENT_DIRTY_FRESH, MISSING ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, SPENT_DIRTY_FRESH, SPENT_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH, MISSING ); - CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); - CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); - CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); - CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); - CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); - CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_CLEAN, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY, VALUE2_DIRTY_FRESH, VALUE2_DIRTY ); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(SPENT_DIRTY_FRESH, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH ); - CheckWriteCoins(VALUE1_CLEAN, MISSING, VALUE1_CLEAN ); - CheckWriteCoins(VALUE1_FRESH, MISSING, VALUE1_FRESH ); - CheckWriteCoins(VALUE1_DIRTY, MISSING, VALUE1_DIRTY ); - CheckWriteCoins(VALUE1_DIRTY_FRESH, MISSING, VALUE1_DIRTY_FRESH); - CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); - CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY, MISSING ); - CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_CLEAN, MISSING, VALUE1_CLEAN ); + CheckWriteCoins(VALUE1_FRESH, MISSING, VALUE1_FRESH ); + CheckWriteCoins(VALUE1_DIRTY, MISSING, VALUE1_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, MISSING, VALUE1_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY, SPENT_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY, MISSING ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, SPENT_DIRTY_FRESH, EX_FRESH_MISAPPLIED); - CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); - CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); - CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH); - CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY_FRESH, FAIL_NO_ENTRY ); + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_CLEAN, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_FRESH, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckWriteCoins(VALUE1_DIRTY, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); + CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); // The checks above omit cases where the child flags are not DIRTY, since // they would be too repetitive (the parent cache is never updated in these // cases). The loop below covers these cases and makes sure the parent cache // is always left unchanged. - for (const CoinEntry parent : {MISSING, - SPENT_CLEAN, SPENT_DIRTY, SPENT_FRESH, SPENT_DIRTY_FRESH, - VALUE1_CLEAN, VALUE1_DIRTY, VALUE1_FRESH, VALUE1_DIRTY_FRESH}) { - for (const CoinEntry child : {MISSING, - SPENT_CLEAN, SPENT_FRESH, - VALUE2_CLEAN, VALUE2_FRESH}) { - auto expected{parent}; // TODO test failure cases as well + for (const MaybeCoin& parent : {MISSING, + SPENT_CLEAN, SPENT_DIRTY, SPENT_FRESH, SPENT_DIRTY_FRESH, + VALUE1_CLEAN, VALUE1_DIRTY, VALUE1_FRESH, VALUE1_DIRTY_FRESH}) { + for (const MaybeCoin& child : {MISSING, + SPENT_CLEAN, SPENT_FRESH, + VALUE2_CLEAN, VALUE2_FRESH}) { + auto expected{CoinOrError{parent}}; // TODO test failure cases as well CheckWriteCoins(parent, child, expected); } } @@ -977,7 +976,7 @@ void TestFlushBehavior( // --- 5. Ensuring the entry is no longer in the cache // - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), MISSING); + BOOST_CHECK(!GetCoinsMapEntry(view->map(), outp)); view->AccessCoin(outp); BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CLEAN)); } @@ -1052,7 +1051,7 @@ void TestFlushBehavior( all_caches[0]->Sync(); // Ensure there is no sign of the coin after spend/flush. - BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), MISSING); + BOOST_CHECK(!GetCoinsMapEntry(all_caches[0]->map(), outp)); BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); BOOST_CHECK(!base.HaveCoin(outp)); } From 0a159f0914775756dcac2d9fa7fe4e4d4e70ba0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 17 Sep 2024 17:52:41 +0200 Subject: [PATCH 8/9] test, refactor: Remove remaining unbounded flags from coins_tests --- src/test/coins_tests.cpp | 77 ++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 2405cf1cf4..0e6348f844 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -567,38 +567,47 @@ constexpr CAmount VALUE1{100}; constexpr CAmount VALUE2{200}; constexpr CAmount VALUE3{300}; -constexpr char DIRTY{CCoinsCacheEntry::DIRTY}; -constexpr char FRESH{CCoinsCacheEntry::FRESH}; -constexpr char CLEAN{0}; - struct CoinEntry { - const CAmount value; - const char flags; + enum class State { CLEAN, DIRTY, FRESH, DIRTY_FRESH }; - constexpr CoinEntry(const CAmount v, const char s) : value{v}, flags{s} {} + const CAmount value; + const State state; + + constexpr CoinEntry(const CAmount v, const State s) : value{v}, state{s} {} bool operator==(const CoinEntry& o) const = default; - friend std::ostream& operator<<(std::ostream& os, const CoinEntry& e) { return os << e.value << ", " << e.flags; } + friend std::ostream& operator<<(std::ostream& os, const CoinEntry& e) { return os << e.value << ", " << e.state; } + + constexpr bool IsDirtyFresh() const { return state == State::DIRTY_FRESH; } + constexpr bool IsDirty() const { return state == State::DIRTY || IsDirtyFresh(); } + constexpr bool IsFresh() const { return state == State::FRESH || IsDirtyFresh(); } + + static constexpr State ToState(const bool is_dirty, const bool is_fresh) { + if (is_dirty && is_fresh) return State::DIRTY_FRESH; + if (is_dirty) return State::DIRTY; + if (is_fresh) return State::FRESH; + return State::CLEAN; + } }; using MaybeCoin = std::optional; using CoinOrError = std::variant; constexpr MaybeCoin MISSING {std::nullopt}; -constexpr MaybeCoin SPENT_DIRTY {{SPENT, DIRTY}}; -constexpr MaybeCoin SPENT_DIRTY_FRESH {{SPENT, DIRTY | FRESH}}; -constexpr MaybeCoin SPENT_FRESH {{SPENT, FRESH}}; -constexpr MaybeCoin SPENT_CLEAN {{SPENT, CLEAN}}; -constexpr MaybeCoin VALUE1_DIRTY {{VALUE1, DIRTY}}; -constexpr MaybeCoin VALUE1_DIRTY_FRESH{{VALUE1, DIRTY | FRESH}}; -constexpr MaybeCoin VALUE1_FRESH {{VALUE1, FRESH}}; -constexpr MaybeCoin VALUE1_CLEAN {{VALUE1, CLEAN}}; -constexpr MaybeCoin VALUE2_DIRTY {{VALUE2, DIRTY}}; -constexpr MaybeCoin VALUE2_DIRTY_FRESH{{VALUE2, DIRTY | FRESH}}; -constexpr MaybeCoin VALUE2_FRESH {{VALUE2, FRESH}}; -constexpr MaybeCoin VALUE2_CLEAN {{VALUE2, CLEAN}}; -constexpr MaybeCoin VALUE3_DIRTY {{VALUE3, DIRTY}}; -constexpr MaybeCoin VALUE3_DIRTY_FRESH{{VALUE3, DIRTY | FRESH}}; +constexpr MaybeCoin SPENT_DIRTY {{SPENT, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin SPENT_DIRTY_FRESH {{SPENT, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin SPENT_FRESH {{SPENT, CoinEntry::State::FRESH}}; +constexpr MaybeCoin SPENT_CLEAN {{SPENT, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE1_DIRTY {{VALUE1, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE1_DIRTY_FRESH{{VALUE1, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin VALUE1_FRESH {{VALUE1, CoinEntry::State::FRESH}}; +constexpr MaybeCoin VALUE1_CLEAN {{VALUE1, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE2_DIRTY {{VALUE2, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE2_DIRTY_FRESH{{VALUE2, CoinEntry::State::DIRTY_FRESH}}; +constexpr MaybeCoin VALUE2_FRESH {{VALUE2, CoinEntry::State::FRESH}}; +constexpr MaybeCoin VALUE2_CLEAN {{VALUE2, CoinEntry::State::CLEAN}}; +constexpr MaybeCoin VALUE3_DIRTY {{VALUE3, CoinEntry::State::DIRTY}}; +constexpr MaybeCoin VALUE3_DIRTY_FRESH{{VALUE3, CoinEntry::State::DIRTY_FRESH}}; constexpr auto EX_OVERWRITE_UNSPENT{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}; constexpr auto EX_FRESH_MISAPPLIED {"FRESH flag misapplied to coin that exists in parent cache"}; @@ -621,22 +630,22 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, cons SetCoinsValue(cache_coin.value, entry.coin); auto [iter, inserted] = map.emplace(OUTPOINT, std::move(entry)); assert(inserted); - if (cache_coin.flags & DIRTY) CCoinsCacheEntry::SetDirty(*iter, sentinel); - if (cache_coin.flags & FRESH) CCoinsCacheEntry::SetFresh(*iter, sentinel); + if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter, sentinel); + if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter, sentinel); return iter->second.coin.DynamicMemoryUsage(); } -MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOINT) +static MaybeCoin GetCoinsMapEntry(const CCoinsMap& map, const COutPoint& outp = OUTPOINT) { if (auto it{map.find(outp)}; it != map.end()) { return CoinEntry{ it->second.coin.IsSpent() ? SPENT : it->second.coin.out.nValue, - static_cast((it->second.IsDirty() ? DIRTY : 0) | (it->second.IsFresh() ? FRESH : 0))}; + CoinEntry::ToState(it->second.IsDirty(), it->second.IsFresh())}; } return MISSING; } -void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) +static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); @@ -652,7 +661,7 @@ class SingleEntryCacheTest public: SingleEntryCacheTest(const CAmount base_value, const MaybeCoin& cache_coin) { - auto base_cache_coin{base_value == ABSENT ? MISSING : CoinEntry{base_value, DIRTY}}; + 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); } @@ -800,7 +809,7 @@ BOOST_AUTO_TEST_CASE(ccoins_add) } } -void CheckWriteCoins(const MaybeCoin& parent, const MaybeCoin& child, const CoinOrError& expected) +static void CheckWriteCoins(const MaybeCoin& parent, const MaybeCoin& child, const CoinOrError& expected) { SingleEntryCacheTest test{ABSENT, parent}; auto write_coins{[&] { WriteCoinsViewEntry(test.cache, child); }}; @@ -870,7 +879,7 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY, VALUE2_DIRTY_FRESH ); CheckWriteCoins(VALUE1_DIRTY_FRESH, VALUE2_DIRTY_FRESH, EX_FRESH_MISAPPLIED); - // The checks above omit cases where the child flags are not DIRTY, since + // The checks above omit cases where the child state is not DIRTY, since // they would be too repetitive (the parent cache is never updated in these // cases). The loop below covers these cases and makes sure the parent cache // is always left unchanged. @@ -946,7 +955,7 @@ void TestFlushBehavior( BOOST_CHECK(!base.HaveCoin(outp)); BOOST_CHECK(view->HaveCoin(outp)); - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, DIRTY|FRESH)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::DIRTY_FRESH)); // --- 2. Flushing all caches (without erasing) // @@ -958,7 +967,7 @@ void TestFlushBehavior( // --- 3. Ensuring the entry still exists in the cache and has been written to parent // - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CLEAN)); // Flags should have been wiped. + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); // State should have been wiped. // Both views should now have the coin. BOOST_CHECK(base.HaveCoin(outp)); @@ -978,7 +987,7 @@ void TestFlushBehavior( // BOOST_CHECK(!GetCoinsMapEntry(view->map(), outp)); view->AccessCoin(outp); - BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CLEAN)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); } // Can't overwrite an entry without specifying that an overwrite is @@ -1043,7 +1052,7 @@ void TestFlushBehavior( all_caches[0]->AddCoin(outp, std::move(coin), false); // Coin should be FRESH in the cache. - BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, DIRTY|FRESH)); + BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, CoinEntry::State::DIRTY_FRESH)); // Base shouldn't have seen coin. BOOST_CHECK(!base.HaveCoin(outp)); From 50cce20013c97e1257cb9f4c9319701a09b0a196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 18 Sep 2024 09:00:52 +0200 Subject: [PATCH 9/9] test, refactor: Compact ccoins_access and ccoins_spend Also added an extra check for `AccessCoin` in `CheckAccessCoin` to make sure its result is also validated. --- src/test/coins_tests.cpp | 83 +++++++++++++--------------------------- 1 file changed, 26 insertions(+), 57 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 0e6348f844..c46144b34b 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -674,7 +674,8 @@ public: static void CheckAccessCoin(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) { SingleEntryCacheTest test{base_value, cache_coin}; - test.cache.AccessCoin(OUTPOINT); + auto& coin = test.cache.AccessCoin(OUTPOINT); + BOOST_CHECK_EQUAL(coin.IsSpent(), !test.cache.GetCoin(OUTPOINT)); test.cache.SelfTest(/*sanity_check=*/false); BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), expected); } @@ -684,37 +685,21 @@ BOOST_AUTO_TEST_CASE(ccoins_access) /* Check AccessCoin behavior, requesting a coin from a cache view layered on * top of a base view, and checking the resulting entry in the cache after * the access. - * Base Cache Expected + * Base Cache Expected */ - CheckAccessCoin(ABSENT, MISSING, MISSING ); - CheckAccessCoin(ABSENT, SPENT_CLEAN, SPENT_CLEAN ); - CheckAccessCoin(ABSENT, SPENT_FRESH, SPENT_FRESH ); - CheckAccessCoin(ABSENT, SPENT_DIRTY, SPENT_DIRTY ); - CheckAccessCoin(ABSENT, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); - CheckAccessCoin(ABSENT, VALUE2_CLEAN, VALUE2_CLEAN ); - CheckAccessCoin(ABSENT, VALUE2_FRESH, VALUE2_FRESH ); - CheckAccessCoin(ABSENT, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckAccessCoin(ABSENT, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckAccessCoin(base_value, MISSING, base_value == VALUE1 ? VALUE1_CLEAN : MISSING); - CheckAccessCoin(SPENT, MISSING, MISSING ); - CheckAccessCoin(SPENT, SPENT_CLEAN, SPENT_CLEAN ); - CheckAccessCoin(SPENT, SPENT_FRESH, SPENT_FRESH ); - CheckAccessCoin(SPENT, SPENT_DIRTY, SPENT_DIRTY ); - CheckAccessCoin(SPENT, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); - CheckAccessCoin(SPENT, VALUE2_CLEAN, VALUE2_CLEAN ); - CheckAccessCoin(SPENT, VALUE2_FRESH, VALUE2_FRESH ); - CheckAccessCoin(SPENT, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckAccessCoin(SPENT, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + CheckAccessCoin(base_value, SPENT_CLEAN, SPENT_CLEAN ); + CheckAccessCoin(base_value, SPENT_FRESH, SPENT_FRESH ); + CheckAccessCoin(base_value, SPENT_DIRTY, SPENT_DIRTY ); + CheckAccessCoin(base_value, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); - CheckAccessCoin(VALUE1, MISSING, VALUE1_CLEAN ); - CheckAccessCoin(VALUE1, SPENT_CLEAN, SPENT_CLEAN ); - CheckAccessCoin(VALUE1, SPENT_FRESH, SPENT_FRESH ); - CheckAccessCoin(VALUE1, SPENT_DIRTY, SPENT_DIRTY ); - CheckAccessCoin(VALUE1, SPENT_DIRTY_FRESH, SPENT_DIRTY_FRESH ); - CheckAccessCoin(VALUE1, VALUE2_CLEAN, VALUE2_CLEAN ); - CheckAccessCoin(VALUE1, VALUE2_FRESH, VALUE2_FRESH ); - CheckAccessCoin(VALUE1, VALUE2_DIRTY, VALUE2_DIRTY ); - CheckAccessCoin(VALUE1, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + CheckAccessCoin(base_value, VALUE2_CLEAN, VALUE2_CLEAN ); + CheckAccessCoin(base_value, VALUE2_FRESH, VALUE2_FRESH ); + CheckAccessCoin(base_value, VALUE2_DIRTY, VALUE2_DIRTY ); + CheckAccessCoin(base_value, VALUE2_DIRTY_FRESH, VALUE2_DIRTY_FRESH); + } } static void CheckSpendCoins(const CAmount base_value, const MaybeCoin& cache_coin, const MaybeCoin& expected) @@ -730,37 +715,21 @@ BOOST_AUTO_TEST_CASE(ccoins_spend) /* Check SpendCoin behavior, requesting a coin from a cache view layered on * top of a base view, spending, and then checking * the resulting entry in the cache after the modification. - * Base Cache Expected + * Base Cache Expected */ - CheckSpendCoins(ABSENT, MISSING, MISSING ); - CheckSpendCoins(ABSENT, SPENT_CLEAN, SPENT_DIRTY); - CheckSpendCoins(ABSENT, SPENT_FRESH, MISSING ); - CheckSpendCoins(ABSENT, SPENT_DIRTY, SPENT_DIRTY); - CheckSpendCoins(ABSENT, SPENT_DIRTY_FRESH, MISSING ); - CheckSpendCoins(ABSENT, VALUE2_CLEAN, SPENT_DIRTY); - CheckSpendCoins(ABSENT, VALUE2_FRESH, MISSING ); - CheckSpendCoins(ABSENT, VALUE2_DIRTY, SPENT_DIRTY); - CheckSpendCoins(ABSENT, VALUE2_DIRTY_FRESH, MISSING ); + for (auto base_value : {ABSENT, SPENT, VALUE1}) { + CheckSpendCoins(base_value, MISSING, base_value == VALUE1 ? SPENT_DIRTY : MISSING); - CheckSpendCoins(SPENT, MISSING, MISSING ); - CheckSpendCoins(SPENT, SPENT_CLEAN, SPENT_DIRTY); - CheckSpendCoins(SPENT, SPENT_FRESH, MISSING ); - CheckSpendCoins(SPENT, SPENT_DIRTY, SPENT_DIRTY); - CheckSpendCoins(SPENT, SPENT_DIRTY_FRESH, MISSING ); - CheckSpendCoins(SPENT, VALUE2_CLEAN, SPENT_DIRTY); - CheckSpendCoins(SPENT, VALUE2_FRESH, MISSING ); - CheckSpendCoins(SPENT, VALUE2_DIRTY, SPENT_DIRTY); - CheckSpendCoins(SPENT, VALUE2_DIRTY_FRESH, MISSING ); + CheckSpendCoins(base_value, SPENT_CLEAN, SPENT_DIRTY); + CheckSpendCoins(base_value, SPENT_FRESH, MISSING ); + CheckSpendCoins(base_value, SPENT_DIRTY, SPENT_DIRTY); + CheckSpendCoins(base_value, SPENT_DIRTY_FRESH, MISSING ); - CheckSpendCoins(VALUE1, MISSING, SPENT_DIRTY); - CheckSpendCoins(VALUE1, SPENT_CLEAN, SPENT_DIRTY); - CheckSpendCoins(VALUE1, SPENT_FRESH, MISSING ); - CheckSpendCoins(VALUE1, SPENT_DIRTY, SPENT_DIRTY); - CheckSpendCoins(VALUE1, SPENT_DIRTY_FRESH, MISSING ); - CheckSpendCoins(VALUE1, VALUE2_CLEAN, SPENT_DIRTY); - CheckSpendCoins(VALUE1, VALUE2_FRESH, MISSING ); - CheckSpendCoins(VALUE1, VALUE2_DIRTY, SPENT_DIRTY); - CheckSpendCoins(VALUE1, VALUE2_DIRTY_FRESH, MISSING ); + CheckSpendCoins(base_value, VALUE2_CLEAN, SPENT_DIRTY); + CheckSpendCoins(base_value, VALUE2_FRESH, MISSING ); + CheckSpendCoins(base_value, VALUE2_DIRTY, SPENT_DIRTY); + CheckSpendCoins(base_value, VALUE2_DIRTY_FRESH, MISSING ); + } } static void CheckAddCoin(const CAmount base_value, const MaybeCoin& cache_coin, const CAmount modify_value, const CoinOrError& expected, const bool coinbase)