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

50cce20013c97e1257cb9f4c9319701a09b0a196 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914775756dcac2d9fa7fe4e4d4e70ba0c test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1eef95c0b626573a9a2e217f4a541a880 test: Validate error messages on fail (Lőrinc)
d5f8d607ab1f8fd9fc17aba4105867b450117be2 test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490a5005d227da75dc8f2d1ab73c6e9d2 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c3818b4138602c127d6a16018aae75687 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cfc79253ffae1527106baa428dd62f39 coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022e6ce4eb3ce526800a6ada3b4a2996d coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc910efa3ed7a6d32e687107248bb5be coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)

Pull request description:

  Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 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 50cce20013c97e1257cb9f4c9319701a09b0a196
  laanwj:
    Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196
  ryanofsky:
    Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.

Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c

1 files changed, 9 insertions, 15 deletions

diff --git a/src/coins.cpp b/src/coins.cpp
index 75d11b4f26..24a102b0bc 100644
--- a/src/coins.cpp
+++ b/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