Do not try to search only in cache, flush can discard changes

Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>

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<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
-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 { return false; }
-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); }
@@ -25,7 +20,7 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->
 uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
 std::vector<uint256> 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) {
+bool CCoinsViewCache::BatchWrite(const CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool sync) {
-    for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) {
+    for (auto it = mapCoins.begin(); it != mapCoins.end(); ++it) {
         // Ignore non-dirty entries (optimization).
         if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) {
             continue;

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<uint256> 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.

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->HaveCoin(COutPoint(inv.hash, 0)) || // Best effort: only try output 0 and 1
-                   pcoinsTip->HaveCoinInCache(COutPoint(inv.hash, 1));
+                   pcoinsTip->HaveCoin(COutPoint(inv.hash, 1));
         }
     case MSG_BLOCK:
     case MSG_WITNESS_BLOCK:

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;
                 }
             }

src/txdb.cpp

@@ -93,7 +93,7 @@ std::vector<uint256> 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"

src/txdb.h

@@ -101,7 +101,7 @@ public:
     bool HaveCoin(const COutPoint &outpoint) const override;
     uint256 GetBestBlock() const override;
     std::vector<uint256> 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;
 };

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");
                     }
                 }

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],