Merge bitcoin/bitcoin#30906: refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests

50cce20 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0 test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c test: Validate error messages on fail (Lőrinc)
d5f8d60 test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b73369 coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282 coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498e coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)

Pull request description:

  Similarly to bitcoin/bitcoin#30849, this cleanup is intended to de-risk bitcoin/bitcoin#30673 (comment) by simplifying the coin cache public interface.

  `CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).

  Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.

  This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.

  The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.

ACKs for top commit:
  andrewtoth:
    Code Review ACK 50cce20
  laanwj:
    Code review ACK 50cce20
  ryanofsky:
    Code review ACK 50cce20. Looks good! Thanks for the followups.

Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c

Author: ryanofsky

src/coins.cpp

@@ -53,7 +53,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);
@@ -99,7 +99,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();
     TRACEPOINT(utxocache, add,
            outpoint.hash.data(),
@@ -111,13 +112,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) {
@@ -147,7 +143,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;
@@ -207,13 +203,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
@@ -241,7 +235,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;