refactor: Return optional of Coin in GetCoin

Leaving the parameter as well for now.

Co-authored-by: TheCharlatan <[email protected]>

Author: l0rinc

src/coins.cpp

@@ -9,7 +9,7 @@
 #include <random.h>
 #include <util/trace.h>
 
-bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; }
+std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint, Coin& coin) 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; }
@@ -18,11 +18,11 @@ std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
 bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
 {
     Coin coin;
-    return GetCoin(outpoint, coin);
+    return GetCoin(outpoint, coin).has_value();
 }
 
 CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { }
-bool CCoinsViewBacked::GetCoin(const COutPoint &outpoint, Coin &coin) const { return base->GetCoin(outpoint, coin); }
+std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); }
 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(); }
@@ -58,13 +58,14 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
     return ret;
 }
 
-bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
+std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint, Coin& coin) const
+{
     CCoinsMap::const_iterator it = FetchCoin(outpoint);
     if (it != cacheCoins.end()) {
         coin = it->second.coin;
-        return !coin.IsSpent();
+        if (!coin.IsSpent()) return coin;
     }
-    return false;
+    return std::nullopt;
 }
 
 void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possible_overwrite) {
@@ -363,8 +364,8 @@ const Coin& AccessByTxid(const CCoinsViewCache& view, const Txid& txid)
     return coinEmpty;
 }
 
-template <typename Func>
-static bool ExecuteBackedWrapper(Func func, const std::vector<std::function<void()>>& err_callbacks)
+template <typename ReturnType, typename Func>
+static ReturnType ExecuteBackedWrapper(Func func, const std::vector<std::function<void()>>& err_callbacks)
 {
     try {
         return func();
@@ -381,10 +382,12 @@ static bool ExecuteBackedWrapper(Func func, const std::vector<std::function<void
     }
 }
 
-bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const {
-    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
+std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint, Coin& coin) const
+{
+    return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
 }
 
-bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const {
-    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
+bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
+{
+    return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
 }

src/coins.h

@@ -303,11 +303,8 @@ struct CoinsViewCacheCursor
 class CCoinsView
 {
 public:
-    /** Retrieve the Coin (unspent transaction output) for a given outpoint.
-     *  Returns true only when an unspent coin was found, which is returned in coin.
-     *  When false is returned, coin's value is unspecified.
-     */
-    virtual bool GetCoin(const COutPoint &outpoint, Coin &coin) const;
+    //! Retrieve the Coin (unspent transaction output) for a given outpoint.
+    virtual std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const;
 
     //! Just check whether a given outpoint is unspent.
     virtual bool HaveCoin(const COutPoint &outpoint) const;
@@ -344,7 +341,7 @@ class CCoinsViewBacked : public CCoinsView
 
 public:
     CCoinsViewBacked(CCoinsView *viewIn);
-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     std::vector<uint256> GetHeadBlocks() const override;
@@ -384,7 +381,7 @@ class CCoinsViewCache : public CCoinsViewBacked
     CCoinsViewCache(const CCoinsViewCache &) = delete;
 
     // Standard CCoinsView methods
-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     void SetBestBlock(const uint256 &hashBlock);
@@ -514,7 +511,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
         m_err_callbacks.emplace_back(std::move(f));
     }
 
-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
     bool HaveCoin(const COutPoint &outpoint) const override;
 
 private:

src/test/coins_tests.cpp

@@ -44,18 +44,18 @@ 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, Coin& coin) const override
     {
         std::map<COutPoint, Coin>::const_iterator it = map_.find(outpoint);
         if (it == map_.end()) {
-            return false;
+            return std::nullopt;
         }
         coin = it->second;
         if (coin.IsSpent() && m_rng.randbool() == 0) {
-            // Randomly return false in case of an empty entry.
-            return false;
+            // Randomly return std::nullopt in case of an empty entry.
+            return std::nullopt;
         }
-        return true;
+        return coin;
     }
 
     uint256 GetBestBlock() const override { return hashBestBlock_; }

src/test/fuzz/coins_view.cpp

@@ -163,7 +163,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
         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);
+        const bool exists_using_get_coin = coins_view_cache.GetCoin(random_out_point, coin_using_get_coin).has_value();
         if (exists_using_get_coin) {
             assert(coin_using_get_coin == coin_using_access_coin);
         }

src/test/fuzz/coinscache_sim.cpp

@@ -146,14 +146,14 @@ 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, Coin& coin) const final
     {
         auto it = m_data.find(outpoint);
         if (it == m_data.end()) {
-            return false;
+            return std::nullopt;
         } else {
             coin = it->second;
-            return true; // TODO GetCoin shouldn't return spent coins
+            return coin; // TODO GetCoin shouldn't return spent coins
         }
     }
 
@@ -461,7 +461,7 @@ 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 real = bottom.GetCoin(data.outpoints[outpointidx], realcoin);
         auto sim = lookup(outpointidx, 0);
         if (!sim.has_value()) {
             assert(!real || realcoin.IsSpent());

src/txdb.cpp

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

src/txdb.h

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

src/txmempool.cpp

@@ -988,12 +988,13 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
 
 CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
 
-bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
+std::optional<Coin> CCoinsViewMemPool::GetCoin(const COutPoint& outpoint, Coin& coin) 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 true;
+        return coin;
     }
 
     // If an entry in the mempool exists, always return that one, as it's guaranteed to never
@@ -1004,9 +1005,9 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
         if (outpoint.n < ptx->vout.size()) {
             coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false);
             m_non_base_coins.emplace(outpoint);
-            return true;
+            return coin;
         } else {
-            return false;
+            return std::nullopt;
         }
     }
     return base->GetCoin(outpoint, coin);

src/txmempool.h

@@ -851,7 +851,7 @@ class CCoinsViewMemPool : public CCoinsViewBacked
     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. */
-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
+    std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) 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);