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

@@ -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; }
 

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);
     }
 }

src/test/coinscachepair_tests.cpp

@@ -21,7 +21,7 @@ std::list<CoinsCachePair> 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
+    // Erase the nodes as we iterate through, but don't clear state
-    // The flags will be cleared by the CCoinsCacheEntry's destructor
+    // 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
+    // Also check that state was not altered
-    BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+    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
+    // Also check that state was not altered
-    BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+    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
+    // Also check that state was not altered
-    BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+    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);