From 21180ff73436e198b6828c312ddfd0a1195447b2 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 13 Jun 2017 12:17:30 -0700 Subject: [PATCH] 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. --- src/coins.cpp | 8 ++++++-- src/coins.h | 8 +++++--- src/test/coins_tests.cpp | 22 +++++++++++++++------- src/txmempool.cpp | 6 +----- src/txmempool.h | 3 +-- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index f8df835e9..3113b7755 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -11,11 +11,15 @@ #include 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 4774c9f6a..077545a55 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 622b15762..e24431528 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 8deb703d2..dcfc5ffde 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 7ca3b18a1..78ac3c209 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; }; /**