Simplify return values of GetCoin/HaveCoin(InCache)

This removes the possibility for GetCoin/HaveCoin/HaveCoinInCache to return
true while the respective coin is spent. By doing it across all calls, some
extra checks can be eliminated.

coins_tests is modified to call HaveCoin sometimes before and sometimes
after AccessCoin. A further change is needed because the semantics for
GetCoin slightly changed, causing a pruned entry in the parent cache to not
be pulled into the child in FetchCoin.

5 files changed, 28 insertions, 19 deletions

diff --git a/src/coins.cpp b/src/coins.cpp
index f8df835e9f..3113b7755d 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -11,11 +11,15 @@
 #include <assert.h>
 
 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(); }
 bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return false; }
 CCoinsViewCursor *CCoinsView::Cursor() const { return 0; }
 
+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 { return base->GetCoin(outpoint, coin); }
@@ -55,7 +59,7 @@ bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
     CCoinsMap::const_iterator it = FetchCoin(outpoint);
     if (it != cacheCoins.end()) {
         coin = it->second.coin;
-        return true;
+        return !coin.IsSpent();
     }
     return false;
 }
diff --git a/src/coins.h b/src/coins.h
index 4774c9f6a6..077545a55b 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -145,11 +145,13 @@ private:
 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
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 622b157621..e24431528a 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -50,12 +50,6 @@ public:
         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
@@ -147,8 +141,22 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test)
         {
             uint256 txid = txids[InsecureRandRange(txids.size())]; // txid we're going to modify in this iteration.
             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()) {
                 Coin newcoin;
@@ -628,7 +636,7 @@ BOOST_AUTO_TEST_CASE(ccoins_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      );
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 8deb703d2e..dcfc5ffde0 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -903,11 +903,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 {
diff --git a/src/txmempool.h b/src/txmempool.h
index 7ca3b18a1e..78ac3c209b 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -684,8 +684,7 @@ protected:
 
 public:
     CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn);
-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const;
-    bool HaveCoin(const COutPoint &outpoint) const;
+    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
 };
 
 /**