refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests

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

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
@@ -128,6 +128,21 @@ struct CCoinsCacheEntry
     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.
 
@@ -156,52 +171,43 @@ struct CCoinsCacheEntry
     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
-    {
-        Assume(&self.second == this);
-        if (!m_flags && flags) {
-            m_prev = sentinel.second.m_prev;
-            m_next = &sentinel;
-            sentinel.second.m_prev = &self;
-            m_prev->second.m_next = &self;
-        }
-        m_flags |= flags;
-    }
-    inline void ClearFlags() noexcept
+    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
     {
         if (!m_flags) return;
         m_next->second.m_prev = m_prev;
         m_prev->second.m_next = m_next;
         m_flags = 0;
+        m_prev = m_next = nullptr;
     }
-    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; }
+    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;