diff --git a/src/coins.cpp b/src/coins.cpp index 38eeee6d1..3ca8afe52 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -12,14 +12,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); } @@ -27,7 +22,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(); } @@ -129,11 +124,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(); @@ -144,8 +134,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 bdfff0ed1..e894e18fa 100644 --- a/src/coins.h +++ b/src/coins.h @@ -173,7 +173,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; @@ -199,7 +199,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; }; @@ -232,18 +232,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 ab26e7e7b..9a9c6849b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1300,8 +1300,8 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) return recentRejects->contains(inv.hash) || mempool.exists(inv.hash) || - coins_cache.HaveCoinInCache(COutPoint(inv.hash, 0)) || // Best effort: only try output 0 and 1 - coins_cache.HaveCoinInCache(COutPoint(inv.hash, 1)); + coins_cache.HaveCoin(COutPoint(inv.hash, 0)) || // Best effort: only try output 0 and 1 + coins_cache.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 637b15dac..cbd62a9be 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,9 +55,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; @@ -66,7 +66,6 @@ public: map_.erase(it->first); } } - mapCoins.erase(it++); } if (!hashBlock.IsNull()) hashBestBlock_ = hashBlock; @@ -187,7 +186,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. @@ -200,7 +199,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 6db2ed63a..7ac718238 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -87,7 +87,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; @@ -109,7 +109,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo 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();) { + 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" @@ -129,10 +129,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo } changed++; } - count++; - auto itOld = it++; - mapCoins.erase(itOld); - if (crash_simulate && count % 200000 == 0) { + if (crash_simulate && ++count % 200000 == 0) { static FastRandomContext rng; if (rng.randrange(crash_simulate) == 0) { LogPrintf("Simulating a crash. Goodbye.\n"); diff --git a/src/txdb.h b/src/txdb.h index 94cdab58f..c99d91467 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -81,7 +81,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 e19c26e65..7bf440a65 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -676,32 +676,29 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - LockPoints lp; m_view.SetBackend(m_viewmempool); CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip(); // do all inputs exist? for (const CTxIn& txin : tx.vin) { - if (!coins_cache.HaveCoinInCache(txin.prevout)) { + if (!coins_cache.HaveCoin(txin.prevout)) coins_to_uncache.push_back(txin.prevout); - } // Note: this call may add txin.prevout to the coins cache // (coins_cache.cacheCoins) by way of FetchCoin(). It should be removed // later (via coins_to_uncache) if this tx turns out to be invalid. if (!m_view.HaveCoin(txin.prevout)) { // Are inputs missing because we already have the tx? - for (size_t out = 0; out < tx.vout.size(); out++) { + for (size_t out = 0; out < tx.vout.size(); out++) // Optimistically just do efficient check of cache for outputs - if (coins_cache.HaveCoinInCache(COutPoint(hash, out))) { + if (coins_cache.HaveCoin(COutPoint(hash, out))) return state.Invalid(ValidationInvalidReason::TX_CONFLICT, false, REJECT_DUPLICATE, "txn-already-known"); - } - } + // Otherwise assume this might be an orphan tx for which we just haven't seen parents yet - if (pfMissingInputs) { + if (pfMissingInputs) *pfMissingInputs = true; - } + return false; // fMissingInputs and !state.IsInvalid() is used to detect this condition, don't set state.Invalid() } } diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 35463a998..f20eeeab6 100644 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -71,11 +71,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework): txid_in_block = node.sendrawtransaction(hexstring=raw_tx_in_block, maxfeerate=0) node.generate(1) self.mempool_size = 0 - # FIXME: it returns missing-inputs since HaveCoinInCache returns false - # self.check_mempool_result( - # result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': '18: txn-already-known'}], - # rawtxs=[raw_tx_in_block], - # ) + self.check_mempool_result( + result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': '18: txn-already-known'}], + rawtxs=[raw_tx_in_block], + ) self.log.info('A transaction not in the mempool') fee = 0.00000700