refactor: Rely on returned value of GetCoin instead of parameter

Also removed the unused coin parameter of GetCoin.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

src/coins.cpp

@@ -9,7 +9,7 @@
 #include <random.h>
 #include <util/trace.h>
 
-std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint, Coin& coin) const { return std::nullopt; }
+std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
 uint256 CCoinsView::GetBestBlock() const { return uint256(); }
 std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
 bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; }
@@ -17,12 +17,11 @@ std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
 
 bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
 {
-    Coin coin;
+    return GetCoin(outpoint).has_value();
-    return GetCoin(outpoint, coin).has_value();
 }
 
 CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { }
-std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); }
+std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint) const { return base->GetCoin(outpoint); }
 bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->HaveCoin(outpoint); }
 uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
 std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); }
@@ -45,26 +44,24 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const {
 CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
     const auto [ret, inserted] = cacheCoins.try_emplace(outpoint);
     if (inserted) {
-        if (!base->GetCoin(outpoint, ret->second.coin)) {
+        if (auto coin{base->GetCoin(outpoint)}) {
+            ret->second.coin = std::move(*coin);
+            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);
+            }
+        } else {
             cacheCoins.erase(ret);
             return cacheCoins.end();
         }
-        if (ret->second.coin.IsSpent()) {
-            // 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);
-        }
-        cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
     }
     return ret;
 }
 
-std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint, Coin& coin) const
+std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint) const
 {
-    CCoinsMap::const_iterator it = FetchCoin(outpoint);
+    if (auto it{FetchCoin(outpoint)}; it != cacheCoins.end() && !it->second.coin.IsSpent()) return it->second.coin;
-    if (it != cacheCoins.end()) {
-        coin = it->second.coin;
-        if (!coin.IsSpent()) return coin;
-    }
     return std::nullopt;
 }
 
@@ -382,9 +379,9 @@ static ReturnType ExecuteBackedWrapper(Func func, const std::vector<std::functio
     }
 }
 
-std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint, Coin& coin) const
+std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint) const
 {
-    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
+    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks);
 }
 
 bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const

src/coins.h

@@ -304,7 +304,7 @@ class CCoinsView
 {
 public:
     //! Retrieve the Coin (unspent transaction output) for a given outpoint.
-    virtual std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const;
+    virtual std::optional<Coin> GetCoin(const COutPoint& outpoint) const;
 
     //! Just check whether a given outpoint is unspent.
     virtual bool HaveCoin(const COutPoint &outpoint) const;
@@ -341,7 +341,7 @@ protected:
 
 public:
     CCoinsViewBacked(CCoinsView *viewIn);
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     std::vector<uint256> GetHeadBlocks() const override;
@@ -381,7 +381,7 @@ public:
     CCoinsViewCache(const CCoinsViewCache &) = delete;
 
     // Standard CCoinsView methods
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     void SetBestBlock(const uint256 &hashBlock);
@@ -511,7 +511,7 @@ public:
         m_err_callbacks.emplace_back(std::move(f));
     }
 
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
 
 private:

src/node/coin.cpp

@@ -16,10 +16,11 @@ void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
     LOCK2(cs_main, node.mempool->cs);
     CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip();
     CCoinsViewMemPool mempool_view(&chain_view, *node.mempool);
-    for (auto& coin : coins) {
+    for (auto& [outpoint, coin] : coins) {
-        if (!mempool_view.GetCoin(coin.first, coin.second)) {
+        if (auto c{mempool_view.GetCoin(outpoint)}) {
-            // Either the coin is not in the CCoinsViewCache or is spent. Clear it.
+            coin = std::move(*c);
-            coin.second.Clear();
+        } else {
+            coin.Clear(); // Either the coin is not in the CCoinsViewCache or is spent
         }
     }
 }

src/node/interfaces.cpp

@@ -352,9 +352,7 @@ public:
     std::optional<Coin> getUnspentOutput(const COutPoint& output) override
     {
         LOCK(::cs_main);
-        Coin coin;
+        return chainman().ActiveChainstate().CoinsTip().GetCoin(output);
-        if (chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin)) return coin;
-        return {};
     }
     TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override
     {

src/rest.cpp

@@ -870,10 +870,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
     {
         auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
             for (const COutPoint& vOutPoint : vOutPoints) {
-                Coin coin;
+                auto coin = !mempool || !mempool->isSpent(vOutPoint) ? view.GetCoin(vOutPoint) : std::nullopt;
-                bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
+                hits.push_back(coin.has_value());
-                hits.push_back(hit);
+                if (coin) outs.emplace_back(std::move(*coin));
-                if (hit) outs.emplace_back(std::move(coin));
             }
             active_height = chainman.ActiveHeight();
             active_hash = chainman.ActiveTip()->GetBlockHash();

src/rpc/blockchain.cpp

@@ -1140,35 +1140,32 @@ static RPCHelpMan gettxout()
     if (!request.params[2].isNull())
         fMempool = request.params[2].get_bool();
 
-    Coin coin;
     Chainstate& active_chainstate = chainman.ActiveChainstate();
     CCoinsViewCache* coins_view = &active_chainstate.CoinsTip();
 
+    std::optional<Coin> coin;
     if (fMempool) {
         const CTxMemPool& mempool = EnsureMemPool(node);
         LOCK(mempool.cs);
         CCoinsViewMemPool view(coins_view, mempool);
-        if (!view.GetCoin(out, coin) || mempool.isSpent(out)) {
+        if (!mempool.isSpent(out)) coin = view.GetCoin(out);
-            return UniValue::VNULL;
-        }
     } else {
-        if (!coins_view->GetCoin(out, coin)) {
+        coin = coins_view->GetCoin(out);
-            return UniValue::VNULL;
-        }
     }
+    if (!coin) return UniValue::VNULL;
 
     const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(coins_view->GetBestBlock());
     ret.pushKV("bestblock", pindex->GetBlockHash().GetHex());
-    if (coin.nHeight == MEMPOOL_HEIGHT) {
+    if (coin->nHeight == MEMPOOL_HEIGHT) {
         ret.pushKV("confirmations", 0);
     } else {
-        ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin.nHeight + 1));
+        ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin->nHeight + 1));
     }
-    ret.pushKV("value", ValueFromAmount(coin.out.nValue));
+    ret.pushKV("value", ValueFromAmount(coin->out.nValue));
     UniValue o(UniValue::VOBJ);
-    ScriptToUniv(coin.out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
+    ScriptToUniv(coin->out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
     ret.pushKV("scriptPubKey", std::move(o));
-    ret.pushKV("coinbase", (bool)coin.fCoinBase);
+    ret.pushKV("coinbase", (bool)coin->fCoinBase);
 
     return ret;
 },

src/test/coins_tests.cpp

@@ -44,18 +44,14 @@ class CCoinsViewTest : public CCoinsView
 public:
     CCoinsViewTest(FastRandomContext& rng) : m_rng{rng} {}
 
-    std::optional<Coin> 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 (auto it{map_.find(outpoint)}; it != map_.end()) {
-        if (it == map_.end()) {
+            if (!it->second.IsSpent() || m_rng.randbool()) {
-            return std::nullopt;
+                return it->second; // TODO spent coins shouldn't be returned
+            }
         }
-        coin = it->second;
+        return std::nullopt;
-        if (coin.IsSpent() && m_rng.randbool() == 0) {
-            // Randomly return std::nullopt in case of an empty entry.
-            return std::nullopt;
-        }
-        return coin;
     }
 
     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;
+        if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {
-        const bool exists_using_get_coin = coins_view_cache.GetCoin(random_out_point, coin_using_get_coin).has_value();
+            assert(*coin == coin_using_access_coin);
-        if (exists_using_get_coin) {
+            assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin);
-            assert(coin_using_get_coin == coin_using_access_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 (auto coin{backend_coins_view.GetCoin(random_out_point)}) {
-        if (backend_coins_view.GetCoin(random_out_point, coin_using_backend_get_coin)) {
             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

@@ -146,15 +146,11 @@ class CoinsViewBottom final : public CCoinsView
     std::map<COutPoint, Coin> m_data;
 
 public:
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const final
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
     {
-        auto it = m_data.find(outpoint);
+        // TODO GetCoin shouldn't return spent coins
-        if (it == m_data.end()) {
+        if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
-            return std::nullopt;
+        return std::nullopt;
-        } else {
-            coin = it->second;
-            return coin; // TODO GetCoin shouldn't return spent coins
-        }
     }
 
     bool HaveCoin(const COutPoint& outpoint) const final
@@ -265,17 +261,16 @@ FUZZ_TARGET(coinscache_sim)
                 // Look up in simulation data.
                 auto sim = lookup(outpointidx);
                 // Look up in real caches.
-                Coin realcoin;
+                auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
-                auto real = caches.back()->GetCoin(data.outpoints[outpointidx], realcoin);
                 // 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->out == simcoin.out);
-                    assert(realcoin.fCoinBase == simcoin.fCoinBase);
+                    assert(realcoin->fCoinBase == simcoin.fCoinBase);
-                    assert(realcoin.nHeight == sim->second);
+                    assert(realcoin->nHeight == sim->second);
                 }
             },
 
@@ -460,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;
+        auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
-        auto real = bottom.GetCoin(data.outpoints[outpointidx], realcoin);
         auto sim = lookup(outpointidx, 0);
         if (!sim.has_value()) {
-            assert(!real || realcoin.IsSpent());
+            assert(!realcoin || realcoin->IsSpent());
         } else {
-            assert(real && !realcoin.IsSpent());
+            assert(realcoin && !realcoin->IsSpent());
-            assert(realcoin.out == data.coins[sim->first].out);
+            assert(realcoin->out == data.coins[sim->first].out);
-            assert(realcoin.fCoinBase == data.coins[sim->first].fCoinBase);
+            assert(realcoin->fCoinBase == data.coins[sim->first].fCoinBase);
-            assert(realcoin.nHeight == sim->second);
+            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;
+        auto coin{amount_view.GetCoin(outpoint).value()};
-        Assert(amount_view.GetCoin(outpoint, c));
+        return coin.out.nValue;
-        return c.out.nValue;
     };
 
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)

src/test/util/setup_common.cpp

@@ -432,9 +432,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,
+        // Use GetCoin to properly populate utxo_to_spend
-        Coin utxo_to_spend;
+        auto utxo_to_spend{coins_cache.GetCoin(outpoint_to_spend).value()};
-        assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend));
         input_coins.insert({outpoint_to_spend, utxo_to_spend});
         inputs_amount += utxo_to_spend.out.nValue;
     }

src/txdb.cpp

@@ -65,10 +65,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size)
     }
 }
 
-std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint, Coin& coin) const
+std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
 {
-    if (m_db->Read(CoinEntry(&outpoint), coin)) return coin;
+    if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin;
-    else return std::nullopt;
+    return std::nullopt;
 }
 
 bool CCoinsViewDB::HaveCoin(const COutPoint &outpoint) const {

src/txdb.h

@@ -57,7 +57,7 @@ protected:
 public:
     explicit CCoinsViewDB(DBParams db_params, CoinsViewOptions options);
 
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     std::vector<uint256> GetHeadBlocks() const override;

src/txmempool.cpp

@@ -988,13 +988,12 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
 
 CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
 
-std::optional<Coin> CCoinsViewMemPool::GetCoin(const COutPoint& outpoint, Coin& coin) const
+std::optional<Coin> CCoinsViewMemPool::GetCoin(const COutPoint& outpoint) const
 {
     // Check to see if the inputs are made available by another tx in the package.
     // These Coins would not be available in the underlying CoinsView.
     if (auto it = m_temp_added.find(outpoint); it != m_temp_added.end()) {
-        coin = it->second;
+        return it->second;
-        return coin;
     }
 
     // If an entry in the mempool exists, always return that one, as it's guaranteed to never
@@ -1003,14 +1002,13 @@ std::optional<Coin> CCoinsViewMemPool::GetCoin(const COutPoint& outpoint, Coin&
     CTransactionRef ptx = mempool.get(outpoint.hash);
     if (ptx) {
         if (outpoint.n < ptx->vout.size()) {
-            coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false);
+            Coin coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false);
             m_non_base_coins.emplace(outpoint);
             return coin;
-        } else {
-            return std::nullopt;
         }
+        return std::nullopt;
     }
-    return base->GetCoin(outpoint, coin);
+    return base->GetCoin(outpoint);
 }
 
 void CCoinsViewMemPool::PackageAddTransaction(const CTransactionRef& tx)

src/txmempool.h

@@ -851,7 +851,7 @@ public:
     CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn);
     /** GetCoin, returning whether it exists and is not spent. Also updates m_non_base_coins if the
      * coin is not fetched from base. */
-    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     /** Add the coins created by this transaction. These coins are only temporarily stored in
      * m_temp_added and cannot be flushed to the back end. Only used for package validation. */
     void PackageAddTransaction(const CTransactionRef& tx);

src/validation.cpp

@@ -180,18 +180,14 @@ std::optional<std::vector<int>> CalculatePrevHeights(
     std::vector<int> prev_heights;
     prev_heights.resize(tx.vin.size());
     for (size_t i = 0; i < tx.vin.size(); ++i) {
-        const CTxIn& txin = tx.vin[i];
+        if (auto coin{coins.GetCoin(tx.vin[i].prevout)}) {
-        Coin coin;
+            prev_heights[i] = coin->nHeight == MEMPOOL_HEIGHT
-        if (!coins.GetCoin(txin.prevout, coin)) {
+                              ? tip.nHeight + 1 // Assume all mempool transaction confirm in the next block.
+                              : coin->nHeight;
+        } else {
             LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
             return std::nullopt;
         }
-        if (coin.nHeight == MEMPOOL_HEIGHT) {
-            // Assume all mempool transaction confirm in the next block.
-            prev_heights[i] = tip.nHeight + 1;
-        } else {
-            prev_heights[i] = coin.nHeight;
-        }
     }
     return prev_heights;
 }