Merge bitcoin/bitcoin#30849: refactor: migrate bool GetCoin to return optional<Coin>

4feaa28728 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c2 refactor: Remove unrealistic simulation state (Lőrinc)

Pull request description:

  While reviewing [the removal of the unreachable combinations from the Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).

  Browsing the Coin cache refactoring history revealed that migrating `bool GetCoin` to `optional<Coin> GetCoin` was [already proposed a few times before](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).

  This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well.
  The PR is done in multiple small steps, first swapping the new `optional` return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.

  Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.

ACKs for top commit:
  andrewtoth:
    re-ACK 4feaa28728
  achow101:
    ACK 4feaa28728
  laanwj:
    Code review ACK 4feaa28728
  theStack:
    Code-review ACK 4feaa28728

Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7

src/test/coins_tests.cpp

@@ -44,18 +44,14 @@ class CCoinsViewTest : public CCoinsView
 public:
     CCoinsViewTest(FastRandomContext& rng) : m_rng{rng} {}
 
-    [[nodiscard]] bool GetCoin(const COutPoint& outpoint, Coin& coin) const override
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
     {
-        std::map<COutPoint, Coin>::const_iterator it = map_.find(outpoint);
-        if (it == map_.end()) {
-            return false;
+        if (auto it{map_.find(outpoint)}; it != map_.end()) {
+            if (!it->second.IsSpent() || m_rng.randbool()) {
+                return it->second; // TODO spent coins shouldn't be returned
+            }
         }
-        coin = it->second;
-        if (coin.IsSpent() && m_rng.randbool() == 0) {
-            // Randomly return false in case of an empty entry.
-            return false;
-        }
-        return true;
+        return std::nullopt;
     }
 
     uint256 GetBestBlock() const override { return hashBestBlock_; }

src/test/fuzz/coins_view.cpp

@@ -162,22 +162,20 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
         const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN);
         const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point);
         const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point);
-        Coin coin_using_get_coin;
-        const bool exists_using_get_coin = coins_view_cache.GetCoin(random_out_point, coin_using_get_coin);
-        if (exists_using_get_coin) {
-            assert(coin_using_get_coin == coin_using_access_coin);
+        if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {
+            assert(*coin == coin_using_access_coin);
+            assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin);
+        } else {
+            assert(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin);
         }
-        assert((exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin && exists_using_get_coin) ||
-               (!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin && !exists_using_get_coin));
         // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent.
         const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point);
         if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) {
             assert(exists_using_have_coin);
         }
-        Coin coin_using_backend_get_coin;
-        if (backend_coins_view.GetCoin(random_out_point, coin_using_backend_get_coin)) {
+        if (auto coin{backend_coins_view.GetCoin(random_out_point)}) {
             assert(exists_using_have_coin_in_backend);
-            // Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
+            // Note we can't assert that `coin_using_get_coin == *coin` because the coin in
             // the cache may have been modified but not yet flushed.
         } else {
             assert(!exists_using_have_coin_in_backend);

src/test/fuzz/coinscache_sim.cpp

@@ -137,9 +137,8 @@ struct CacheLevel
 
 /** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB).
  *
- * The initial state consists of the empty UTXO set, though coins whose output index
- * is 3 (mod 5) always have GetCoin() succeed (but returning an IsSpent() coin unless a UTXO
- * exists). Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent.
+ * The initial state consists of the empty UTXO set.
+ * Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent.
  * This exercises code paths with spent, non-DIRTY cache entries.
  */
 class CoinsViewBottom final : public CCoinsView
@@ -147,19 +146,11 @@ class CoinsViewBottom final : public CCoinsView
     std::map<COutPoint, Coin> m_data;
 
 public:
-    bool GetCoin(const COutPoint& outpoint, Coin& coin) const final
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
     {
-        auto it = m_data.find(outpoint);
-        if (it == m_data.end()) {
-            if ((outpoint.n % 5) == 3) {
-                coin.Clear();
-                return true;
-            }
-            return false;
-        } else {
-            coin = it->second;
-            return true;
-        }
+        // TODO GetCoin shouldn't return spent coins
+        if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
+        return std::nullopt;
     }
 
     bool HaveCoin(const COutPoint& outpoint) const final
@@ -270,17 +261,16 @@ FUZZ_TARGET(coinscache_sim)
                 // Look up in simulation data.
                 auto sim = lookup(outpointidx);
                 // Look up in real caches.
-                Coin realcoin;
-                auto real = caches.back()->GetCoin(data.outpoints[outpointidx], realcoin);
+                auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
                 // Compare results.
                 if (!sim.has_value()) {
-                    assert(!real || realcoin.IsSpent());
+                    assert(!realcoin || realcoin->IsSpent());
                 } else {
-                    assert(real && !realcoin.IsSpent());
+                    assert(realcoin && !realcoin->IsSpent());
                     const auto& simcoin = data.coins[sim->first];
-                    assert(realcoin.out == simcoin.out);
-                    assert(realcoin.fCoinBase == simcoin.fCoinBase);
-                    assert(realcoin.nHeight == sim->second);
+                    assert(realcoin->out == simcoin.out);
+                    assert(realcoin->fCoinBase == simcoin.fCoinBase);
+                    assert(realcoin->nHeight == sim->second);
                 }
             },
 
@@ -465,16 +455,15 @@ FUZZ_TARGET(coinscache_sim)
 
     // Compare the bottom coinsview (not a CCoinsViewCache) with sim_cache[0].
     for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) {
-        Coin realcoin;
-        bool real = bottom.GetCoin(data.outpoints[outpointidx], realcoin);
+        auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
         auto sim = lookup(outpointidx, 0);
         if (!sim.has_value()) {
-            assert(!real || realcoin.IsSpent());
+            assert(!realcoin || realcoin->IsSpent());
         } else {
-            assert(real && !realcoin.IsSpent());
-            assert(realcoin.out == data.coins[sim->first].out);
-            assert(realcoin.fCoinBase == data.coins[sim->first].fCoinBase);
-            assert(realcoin.nHeight == sim->second);
+            assert(realcoin && !realcoin->IsSpent());
+            assert(realcoin->out == data.coins[sim->first].out);
+            assert(realcoin->fCoinBase == data.coins[sim->first].fCoinBase);
+            assert(realcoin->nHeight == sim->second);
         }
     }
 }

src/test/fuzz/tx_pool.cpp

@@ -214,9 +214,8 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
     // Helper to query an amount
     const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};
     const auto GetAmount = [&](const COutPoint& outpoint) {
-        Coin c;
-        Assert(amount_view.GetCoin(outpoint, c));
-        return c.out.nValue;
+        auto coin{amount_view.GetCoin(outpoint).value()};
+        return coin.out.nValue;
     };
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)

src/test/util/setup_common.cpp

@@ -433,9 +433,8 @@ std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransactio
     std::map<COutPoint, Coin> input_coins;
     CAmount inputs_amount{0};
     for (const auto& outpoint_to_spend : inputs) {
-        // - Use GetCoin to properly populate utxo_to_spend,
-        Coin utxo_to_spend;
-        assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend));
+        // Use GetCoin to properly populate utxo_to_spend
+        auto utxo_to_spend{coins_cache.GetCoin(outpoint_to_spend).value()};
         input_coins.insert({outpoint_to_spend, utxo_to_spend});
         inputs_amount += utxo_to_spend.out.nValue;
     }