Merge #10581: Simplify return values of GetCoin/HaveCoin(InCache)

Summary:
21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille)

Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b

Backport of Core PR10581
bitcoin/bitcoin#10581

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3620

src/coins.cpp

@@ -14,9 +14,6 @@
 bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const {
     return false;
 }
-bool CCoinsView::HaveCoin(const COutPoint &outpoint) const {
-    return false;
-}
 uint256 CCoinsView::GetBestBlock() const {
     return uint256();
 }
@@ -29,6 +26,10 @@ bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
 CCoinsViewCursor *CCoinsView::Cursor() const {
     return nullptr;
 }
+bool CCoinsView::HaveCoin(const COutPoint &outpoint) const {
+    Coin coin;
+    return GetCoin(outpoint, coin);
+}
 
 CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) {}
 bool CCoinsViewBacked::GetCoin(const COutPoint &outpoint, Coin &coin) const {
@@ -98,7 +99,7 @@ bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
         return false;
     }
     coin = it->second.coin;
-    return true;
+    return !coin.IsSpent();
 }
 
 void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin coin,

src/coins.h

@@ -138,11 +138,14 @@ class CCoinsViewCursor {
 /** Abstract view on the open txout dataset. */
 class CCoinsView {
 public:
-    //! Retrieve the Coin (unspent transaction output) for a given outpoint.
+    /**
+     * 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;
 
-    //! Just check whether we have data for a given outpoint.
-    //! This may (but cannot always) return true for spent outputs.
+    //! Just check whether a given outpoint is unspent.
     virtual bool HaveCoin(const COutPoint &outpoint) const;
 
     //! Retrieve the block hash whose state this CCoinsView currently represents

src/test/coins_tests.cpp

@@ -51,11 +51,6 @@ class CCoinsViewTest : public CCoinsView {
         return true;
     }
 
-    bool HaveCoin(const COutPoint &outpoint) const override {
-        Coin coin;
-        return GetCoin(outpoint, coin);
-    }
-
     uint256 GetBestBlock() const override { return hashBestBlock_; }
 
     bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override {
@@ -150,11 +145,28 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) {
             // txid we're going to modify in this iteration.
             TxId txid = txids[InsecureRandRange(txids.size())];
             Coin &coin = result[COutPoint(txid, 0)];
+            // Determine whether to test HaveCoin before or after Access* (or
+            // both). As these functions can influence each other's behaviour by
+            // pulling things into the cache, all combinations are tested.
+            bool test_havecoin_before = InsecureRandBits(2) == 0;
+            bool test_havecoin_after = InsecureRandBits(2) == 0;
+
+            bool result_havecoin =
+                test_havecoin_before
+                    ? stack.back()->HaveCoin(COutPoint(txid, 0))
+                    : false;
             const Coin &entry =
                 (InsecureRandRange(500) == 0)
                     ? AccessByTxid(*stack.back(), txid)
                     : stack.back()->AccessCoin(COutPoint(txid, 0));
             BOOST_CHECK(coin == entry);
+            BOOST_CHECK(!test_havecoin_before ||
+                        result_havecoin == !entry.IsSpent());
+
+            if (test_havecoin_after) {
+                bool ret = stack.back()->HaveCoin(COutPoint(txid, 0));
+                BOOST_CHECK(ret == !entry.IsSpent());
+            }
 
             if (InsecureRandRange(5) == 0 || coin.IsSpent()) {
                 CTxOut txout;
@@ -663,7 +675,7 @@ BOOST_AUTO_TEST_CASE(coin_access) {
     CheckAccessCoin(ABSENT, VALUE2, VALUE2, FRESH, FRESH);
     CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY, DIRTY);
     CheckAccessCoin(ABSENT, VALUE2, VALUE2, DIRTY | FRESH, DIRTY | FRESH);
-    CheckAccessCoin(PRUNED, ABSENT, PRUNED, NO_ENTRY, FRESH);
+    CheckAccessCoin(PRUNED, ABSENT, ABSENT, NO_ENTRY, NO_ENTRY);
     CheckAccessCoin(PRUNED, PRUNED, PRUNED, 0, 0);
     CheckAccessCoin(PRUNED, PRUNED, PRUNED, FRESH, FRESH);
     CheckAccessCoin(PRUNED, PRUNED, PRUNED, DIRTY, DIRTY);

src/txmempool.cpp

@@ -1027,12 +1027,7 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
         }
         return false;
     }
-
-    return base->GetCoin(outpoint, coin) && !coin.IsSpent();
-}
-
-bool CCoinsViewMemPool::HaveCoin(const COutPoint &outpoint) const {
-    return mempool.exists(outpoint) || base->HaveCoin(outpoint);
+    return base->GetCoin(outpoint, coin);
 }
 
 size_t CTxMemPool::DynamicMemoryUsage() const {

src/txmempool.h

@@ -785,7 +785,6 @@ class CCoinsViewMemPool : public CCoinsViewBacked {
 public:
     CCoinsViewMemPool(CCoinsView *baseIn, const CTxMemPool &mempoolIn);
     bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
-    bool HaveCoin(const COutPoint &outpoint) const override;
 };
 
 // We want to sort transactions by coin age priority