[draft] TXO check (BIP30) optimization in coins cache

src/coins.cpp

@@ -170,6 +170,19 @@ bool CCoinsViewCache::HaveCoin(const COutPoint& outpoint) const
     return (it != cacheCoins.end() && !it->second.coin.IsSpent());
 }
 
+/**
+ * Check for the existence of a coin in cache and in base.
+ * Doesn't add it to the cache if found.
+ */
+bool CCoinsViewCache::HaveCoinDontCache(const COutPoint& outpoint) const
+{
+    if (const auto it{cacheCoins.find(outpoint)}; it != cacheCoins.end()) {
+        return !it->second.coin.IsSpent();
+    } else {
+        return base->HaveCoinDontCache(outpoint);
+    }
+}
+
 bool CCoinsViewCache::HaveCoinInCache(const COutPoint &outpoint) const {
     CCoinsMap::const_iterator it = cacheCoins.find(outpoint);
     return (it != cacheCoins.end() && !it->second.coin.IsSpent());
@@ -403,6 +416,11 @@ bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
     return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
 }
 
+bool CCoinsViewErrorCatcher::HaveCoinDontCache(const COutPoint& outpoint) const
+{
+    return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoinDontCache(outpoint); }, m_err_callbacks);
+}
+
 std::optional<Coin> CCoinsViewErrorCatcher::PeekCoin(const COutPoint& outpoint) const
 {
     return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::PeekCoin(outpoint); }, m_err_callbacks);

src/coins.h

@@ -319,9 +319,15 @@ class CCoinsView
     virtual std::optional<Coin> PeekCoin(const COutPoint& outpoint) const = 0;
 
     //! Just check whether a given outpoint is unspent.
-    //! May populate the cache. Use PeekCoin() to perform a non-caching lookup.
+    //! May populate the cache. Use HaveCoinDontCache() to perform a non-caching lookup.
     virtual bool HaveCoin(const COutPoint& outpoint) const = 0;
 
+    //! Check for the existence of a coin, similar to HaveCoin(), but with
+    //! the expectation that it does not exist.
+    //! It doesn't cache (in case the item is found), and this makes it more
+    //! efficient (no need to retrieve the item from lower cache).
+    virtual bool HaveCoinDontCache(const COutPoint &outpoint) const = 0;
+
     //! Retrieve the block hash whose state this CCoinsView currently represents
     virtual uint256 GetBestBlock() const = 0;
 
@@ -357,6 +363,7 @@ class CoinsViewEmpty : public CCoinsView
     std::optional<Coin> GetCoin(const COutPoint&) const override { return {}; }
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override { return GetCoin(outpoint); }
     bool HaveCoin(const COutPoint& outpoint) const override { return !!GetCoin(outpoint); }
+    bool HaveCoinDontCache(const COutPoint& outpoint) const override { return !!GetCoin(outpoint); }
     uint256 GetBestBlock() const override { return {}; }
     std::vector<uint256> GetHeadBlocks() const override { return {}; }
     void BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) override
@@ -381,6 +388,7 @@ class CCoinsViewBacked : public CCoinsView
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override { return base->GetCoin(outpoint); }
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override { return base->PeekCoin(outpoint); }
     bool HaveCoin(const COutPoint& outpoint) const override { return base->HaveCoin(outpoint); }
+    bool HaveCoinDontCache(const COutPoint& outpoint) const override { return base->HaveCoinDontCache(outpoint); }
     uint256 GetBestBlock() const override { return base->GetBestBlock(); }
     std::vector<uint256> GetHeadBlocks() const override { return base->GetHeadBlocks(); }
     void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override { base->BatchWrite(cursor, block_hash); }
@@ -432,6 +440,7 @@ class CCoinsViewCache : public CCoinsViewBacked
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint& outpoint) const override;
+    bool HaveCoinDontCache(const COutPoint& outpoint) const override;
     uint256 GetBestBlock() const override;
     void SetBestBlock(const uint256& block_hash);
     void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override;
@@ -605,6 +614,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
 
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint& outpoint) const override;
+    bool HaveCoinDontCache(const COutPoint& outpoint) const override;
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override;
 
 private:

src/txdb.cpp

@@ -89,6 +89,11 @@ bool CCoinsViewDB::HaveCoin(const COutPoint& outpoint) const
     return m_db->Exists(CoinEntry(&outpoint));
 }
 
+bool CCoinsViewDB::HaveCoinDontCache(const COutPoint& outpoint) const
+{
+    return m_db->Exists(CoinEntry(&outpoint));
+}
+
 uint256 CCoinsViewDB::GetBestBlock() const {
     uint256 hashBestChain;
     if (!m_db->Read(DB_BEST_BLOCK, hashBestChain))

src/txdb.h

@@ -43,6 +43,7 @@ class CCoinsViewDB final : public CCoinsView
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
     std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override;
     bool HaveCoin(const COutPoint& outpoint) const override;
+    bool HaveCoinDontCache(const COutPoint& outpoint) const override;
     uint256 GetBestBlock() const override;
     std::vector<uint256> GetHeadBlocks() const override;
     void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& block_hash) override;

src/validation.cpp

@@ -2464,7 +2464,10 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     if (fEnforceBIP30 || pindex->nHeight >= BIP34_IMPLIES_BIP30_LIMIT) {
         for (const auto& tx : block.vtx) {
             for (size_t o = 0; o < tx->vout.size(); o++) {
-                if (view.HaveCoin(COutPoint(tx->GetHash(), o))) {
+                // Note: the likely case is that the new TXO doesn't exist,
+                // so it doesn't make sense to try to cache it.
+                // It will result in a cache miss (a miss for an non-existent item).
+                if (view.HaveCoinDontCache(COutPoint(tx->GetHash(), o))) {
                     state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30",
                                   "tried to overwrite transaction");
                 }