From 489edb7e0b8612c1c1f5493de9d3083e473aec01 Mon Sep 17 00:00:00 2001 From: Brannon King Date: Tue, 18 Feb 2020 17:08:33 -0700 Subject: [PATCH] Do not try to search only in cache, flush can discard changes Signed-off-by: Anthony Fieroni --- src/coins.cpp | 20 +++++--------------- src/coins.h | 13 +++---------- src/net_processing.cpp | 4 ++-- src/test/coins_tests.cpp | 9 ++++----- src/txdb.cpp | 4 ++-- src/txdb.h | 2 +- src/validation.cpp | 4 ++-- test/functional/mempool_accept.py | 1 + 8 files changed, 20 insertions(+), 37 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 54c8947e1..c0cf6cb2d 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -10,14 +10,9 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } -bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { return false; } +bool CCoinsView::BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { return false; } CCoinsViewCursor *CCoinsView::Cursor() const { return nullptr; } - -bool CCoinsView::HaveCoin(const COutPoint &outpoint) const -{ - Coin coin; - return GetCoin(outpoint, coin); -} +bool CCoinsView::HaveCoin(const COutPoint &outpoint) const { return false; } CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { } bool CCoinsViewBacked::GetCoin(const COutPoint &outpoint, Coin &coin) const { return base->GetCoin(outpoint, coin); } @@ -25,7 +20,7 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { return base->BatchWrite(mapCoins, hashBlock, sync); } +bool CCoinsViewBacked::BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { return base->BatchWrite(mapCoins, hashBlock, sync); } CCoinsViewCursor *CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } @@ -127,11 +122,6 @@ bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const { return (it != cacheCoins.end() && !it->second.coin.IsSpent()); } -bool CCoinsViewCache::HaveCoinInCache(const COutPoint &outpoint) const { - CCoinsMap::const_iterator it = cacheCoins.find(outpoint); - return (it != cacheCoins.end() && !it->second.coin.IsSpent()); -} - uint256 CCoinsViewCache::GetBestBlock() const { if (hashBlock.IsNull()) hashBlock = base->GetBestBlock(); @@ -142,8 +132,8 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool sync) { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) { +bool CCoinsViewCache::BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool sync) { + for (auto it = mapCoins.begin(); it != mapCoins.end(); ++it) { // Ignore non-dirty entries (optimization). if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) { continue; diff --git a/src/coins.h b/src/coins.h index 0bc0831a7..867d51a79 100644 --- a/src/coins.h +++ b/src/coins.h @@ -163,7 +163,7 @@ public: //! Do a bulk modification (multiple Coin changes + BestBlock change). //! The passed mapCoins can be modified. - virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync); + virtual bool BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync); //! Get a cursor to iterate over the whole state virtual CCoinsViewCursor *Cursor() const; @@ -189,7 +189,7 @@ public: uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; + bool BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; CCoinsViewCursor *Cursor() const override; size_t EstimateSize() const override; }; @@ -222,18 +222,11 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; + bool BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; CCoinsViewCursor* Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } - /** - * Check if we have the given utxo already loaded in this cache. - * The semantics are the same as HaveCoin(), but no calls to - * the backing CCoinsView are made. - */ - bool HaveCoinInCache(const COutPoint &outpoint) const; - /** * Return a reference to Coin in the cache, or a pruned one if not found. This is * more efficient than GetCoin. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0be47f2b3..1967490cb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1049,8 +1049,8 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) return recentRejects->contains(inv.hash) || mempool.exists(inv.hash) || - pcoinsTip->HaveCoinInCache(COutPoint(inv.hash, 0)) || // Best effort: only try output 0 and 1 - pcoinsTip->HaveCoinInCache(COutPoint(inv.hash, 1)); + pcoinsTip->HaveCoin(COutPoint(inv.hash, 0)) || // Best effort: only try output 0 and 1 + pcoinsTip->HaveCoin(COutPoint(inv.hash, 1)); } case MSG_BLOCK: case MSG_WITNESS_BLOCK: diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 5658a5fb7..06c4a1c17 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -54,9 +54,9 @@ public: uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool sync) override + bool BatchWrite(const CCoinsMap& mapCoins, const uint256& hashBlock, bool sync) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) { + for (auto it = mapCoins.begin(); it != mapCoins.end(); ++it) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; @@ -65,7 +65,6 @@ public: map_.erase(it->first); } } - mapCoins.erase(it++); } if (!hashBlock.IsNull()) hashBestBlock_ = hashBlock; @@ -186,7 +185,7 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) COutPoint out(txids[InsecureRand32() % txids.size()], 0); int cacheid = InsecureRand32() % stack.size(); stack[cacheid]->Uncache(out); - uncached_an_entry |= !stack[cacheid]->HaveCoinInCache(out); + uncached_an_entry |= !stack[cacheid]->HaveCoin(out); } // Once every 1000 iterations and at the end, verify the full cache. @@ -199,7 +198,7 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) if (coin.IsSpent()) { missed_an_entry = true; } else { - BOOST_CHECK(stack.back()->HaveCoinInCache(entry.first)); + BOOST_CHECK(stack.back()->HaveCoin(entry.first)); found_an_entry = true; } } diff --git a/src/txdb.cpp b/src/txdb.cpp index 54da785cb..7d4bb57c8 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -93,7 +93,7 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { +bool CCoinsViewDB::BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) { size_t count = 0; size_t changed = 0; @@ -105,7 +105,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo db << "INSERT OR REPLACE INTO marker VALUES('head_block', ?)" << hashBlock; auto dbd = db << "DELETE FROM unspent WHERE txID = ? AND txN = ?"; auto dbi = db << "INSERT OR REPLACE INTO unspent VALUES(?,?,?,?,?,?,?)"; - for (auto it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) { + for (auto it = mapCoins.begin(); it != mapCoins.end(); ++it) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { if (it->second.coin.IsSpent()) { // at present the "IsSpent" flag is used for both "spent" and "block going backwards" diff --git a/src/txdb.h b/src/txdb.h index 1a770c588..df2b4098e 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -101,7 +101,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; + bool BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlock, bool sync) override; CCoinsViewCursor *Cursor() const override; size_t EstimateSize() const override; }; diff --git a/src/validation.cpp b/src/validation.cpp index 1b1db18b0..d4de779d0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -505,14 +505,14 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // do all inputs exist? for (const CTxIn& txin : tx.vin) { - if (!pcoinsTip->HaveCoinInCache(txin.prevout)) { + if (!pcoinsTip->HaveCoin(txin.prevout)) { coins_to_uncache.push_back(txin.prevout); } if (!view.HaveCoin(txin.prevout)) { // Are inputs missing because we already have the tx? for (size_t out = 0; out < tx.vout.size(); out++) { // Optimistically just do efficient check of cache for outputs - if (pcoinsTip->HaveCoinInCache(COutPoint(hash, out))) { + if (pcoinsTip->HaveCoin(COutPoint(hash, out))) { return state.Invalid(false, REJECT_DUPLICATE, "txn-already-known"); } } diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 8847777ba..1a6db269f 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -70,6 +70,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): ))['hex'] txid_in_block = node.sendrawtransaction(hexstring=raw_tx_in_block, allowhighfees=True) node.generate(1) + self.mempool_size = 0 self.check_mempool_result( result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': '18: txn-already-known'}], rawtxs=[raw_tx_in_block],