[pull] master from bitcoin:master

src/coins.cpp

@@ -55,10 +55,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
         if (auto coin{base->GetCoin(outpoint)}) {
             ret->second.coin = std::move(*coin);
             cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
-            if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
-                // The parent only has an empty entry for this outpoint; we can consider our version as fresh.
-                CCoinsCacheEntry::SetFresh(*ret, m_sentinel);
-            }
+            Assert(!ret->second.coin.IsSpent());
         } else {
             cacheCoins.erase(ret);
             return cacheCoins.end();
@@ -277,7 +274,7 @@ void CCoinsViewCache::Sync()
 void CCoinsViewCache::Uncache(const COutPoint& hash)
 {
     CCoinsMap::iterator it = cacheCoins.find(hash);
-    if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
+    if (it != cacheCoins.end() && !it->second.IsDirty()) {
         cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
         TRACEPOINT(utxocache, uncache,
                hash.hash.data(),
@@ -318,20 +315,19 @@ void CCoinsViewCache::ReallocateCache()
 void CCoinsViewCache::SanityCheck() const
 {
     size_t recomputed_usage = 0;
-    size_t count_flagged = 0;
+    size_t count_dirty = 0;
     for (const auto& [_, entry] : cacheCoins) {
-        unsigned attr = 0;
-        if (entry.IsDirty()) attr |= 1;
-        if (entry.IsFresh()) attr |= 2;
-        if (entry.coin.IsSpent()) attr |= 4;
-        // Only 5 combinations are possible.
-        assert(attr != 2 && attr != 4 && attr != 7);
+        if (entry.coin.IsSpent()) {
+            assert(entry.IsDirty() && !entry.IsFresh()); // A spent coin must be dirty and cannot be fresh
+        } else {
+            assert(entry.IsDirty() || !entry.IsFresh()); // An unspent coin must not be fresh if not dirty
+        }
 
         // Recompute cachedCoinsUsage.
         recomputed_usage += entry.coin.DynamicMemoryUsage();
 
         // Count the number of entries we expect in the linked list.
-        if (entry.IsDirty() || entry.IsFresh()) ++count_flagged;
+        if (entry.IsDirty()) ++count_dirty;
     }
     // Iterate over the linked list of flagged entries.
     size_t count_linked = 0;
@@ -340,11 +336,11 @@ void CCoinsViewCache::SanityCheck() const
         assert(it->second.Next()->second.Prev() == it);
         assert(it->second.Prev()->second.Next() == it);
         // Verify they are actually flagged.
-        assert(it->second.IsDirty() || it->second.IsFresh());
+        assert(it->second.IsDirty());
         // Count the number of entries actually in the list.
         ++count_linked;
     }
-    assert(count_linked == count_flagged);
+    assert(count_linked == count_dirty);
     assert(recomputed_usage == cachedCoinsUsage);
 }
 

src/coins.h

@@ -102,7 +102,6 @@ using CoinsCachePair = std::pair<const COutPoint, CCoinsCacheEntry>;
  * - unspent, FRESH, DIRTY (e.g. a new coin created in the cache)
  * - unspent, not FRESH, DIRTY (e.g. a coin changed in the cache during a reorg)
  * - unspent, not FRESH, not DIRTY (e.g. an unspent coin fetched from the parent cache)
- * - spent, FRESH, not DIRTY (e.g. a spent coin fetched from the parent cache)
  * - spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent)
  */
 struct CCoinsCacheEntry
@@ -117,12 +116,6 @@ struct CCoinsCacheEntry
      * the parent cache for batch writing. This is a performance optimization
      * compared to giving all entries in the cache to the parent and having the
      * parent scan for only modified entries.
-     *
-     * FRESH-but-not-DIRTY coins can not occur in practice, since that would
-     * mean a spent coin exists in the parent CCoinsView and not in the child
-     * CCoinsViewCache. Nevertheless, if a spent coin is retrieved from the
-     * parent cache, the FRESH-but-not-DIRTY coin will be tracked by the linked
-     * list and deleted when Sync or Flush is called on the CCoinsViewCache.
      */
     CoinsCachePair* m_prev{nullptr};
     CoinsCachePair* m_next{nullptr};

src/test/addrman_tests.cpp

@@ -17,6 +17,7 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <cstdint>
 #include <optional>
 #include <string>
 
@@ -106,6 +107,45 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
     BOOST_CHECK(addrman->Size() >= 1);
 }
 
+BOOST_AUTO_TEST_CASE(addrman_penalty_self_announcement)
+{
+    SetMockTime(Now<NodeSeconds>());
+    auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
+
+    const auto base_time{Now<NodeSeconds>() - 10000s};
+    CService addr1 = ResolveService("250.1.1.1", 8333);
+    CNetAddr source1 = ResolveIP("250.1.1.1"); // Same as addr1 - self announcement
+
+    CAddress caddr1(addr1, NODE_NONE);
+    caddr1.nTime = base_time;
+
+    const auto time_penalty{3600s};
+
+    BOOST_CHECK(addrman->Add({caddr1}, source1, time_penalty));
+
+    auto addr_pos1{addrman->FindAddressEntry(caddr1)};
+    BOOST_REQUIRE(addr_pos1.has_value());
+
+    std::vector<CAddress> addresses{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};
+    BOOST_REQUIRE_EQUAL(addresses.size(), 1U);
+
+    BOOST_CHECK(addresses[0].nTime == base_time);
+
+    CService addr2{ResolveService("250.1.1.2", 8333)};
+    CNetAddr source2{ResolveIP("250.1.1.3")}; // Different from addr2 - not self announcement
+
+    CAddress caddr2(addr2, NODE_NONE);
+    caddr2.nTime = base_time;
+
+    BOOST_CHECK(addrman->Add({caddr2}, source2, time_penalty));
+
+    addresses = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt);
+    BOOST_REQUIRE_EQUAL(addresses.size(), 2U);
+
+    CAddress retrieved_addr2{addresses[0]};
+    BOOST_CHECK(retrieved_addr2.nTime == base_time - time_penalty);
+}
+
 BOOST_AUTO_TEST_CASE(addrman_ports)
 {
     auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));

src/test/coins_tests.cpp

@@ -48,11 +48,7 @@ class CCoinsViewTest : public CCoinsView
 
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
     {
-        if (auto it{map_.find(outpoint)}; it != map_.end()) {
-            if (!it->second.IsSpent() || m_rng.randbool()) {
-                return it->second; // TODO spent coins shouldn't be returned
-            }
-        }
+        if (auto it{map_.find(outpoint)}; it != map_.end() && !it->second.IsSpent()) return it->second;
         return std::nullopt;
     }
 

src/test/fuzz/coinscache_sim.cpp

@@ -138,8 +138,6 @@ struct CacheLevel
 /** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB).
  *
  * The initial state consists of the empty UTXO set.
- * Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent.
- * This exercises code paths with spent, non-DIRTY cache entries.
  */
 class CoinsViewBottom final : public CCoinsView
 {
@@ -148,16 +146,13 @@ class CoinsViewBottom final : public CCoinsView
 public:
     std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
     {
-        // TODO GetCoin shouldn't return spent coins
-        if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
+        if (auto it{m_data.find(outpoint)}; it != m_data.end()) {
+            assert(!it->second.IsSpent());
+            return it->second;
+        }
         return std::nullopt;
     }
 
-    bool HaveCoin(const COutPoint& outpoint) const final
-    {
-        return m_data.contains(outpoint);
-    }
-
     uint256 GetBestBlock() const final { return {}; }
     std::vector<uint256> GetHeadBlocks() const final { return {}; }
     std::unique_ptr<CCoinsViewCursor> Cursor() const final { return {}; }
@@ -167,18 +162,20 @@ class CoinsViewBottom final : public CCoinsView
     {
         for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {
             if (it->second.IsDirty()) {
-                if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) {
+                if (it->second.coin.IsSpent()) {
                     m_data.erase(it->first);
-                } else if (cursor.WillErase(*it)) {
-                    m_data[it->first] = std::move(it->second.coin);
                 } else {
-                    m_data[it->first] = it->second.coin;
+                    if (cursor.WillErase(*it)) {
+                        m_data[it->first] = std::move(it->second.coin);
+                    } else {
+                        m_data[it->first] = it->second.coin;
+                    }
                 }
             } else {
                 /* For non-dirty entries being written, compare them with what we have. */
                 auto it2 = m_data.find(it->first);
                 if (it->second.coin.IsSpent()) {
-                    assert(it2 == m_data.end() || it2->second.IsSpent());
+                    assert(it2 == m_data.end());
                 } else {
                     assert(it2 != m_data.end());
                     assert(it->second.coin.out == it2->second.out);
@@ -263,7 +260,7 @@ FUZZ_TARGET(coinscache_sim)
                 auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
                 // Compare results.
                 if (!sim.has_value()) {
-                    assert(!realcoin || realcoin->IsSpent());
+                    assert(!realcoin);
                 } else {
                     assert(realcoin && !realcoin->IsSpent());
                     const auto& simcoin = data.coins[sim->first];
@@ -449,7 +446,7 @@ FUZZ_TARGET(coinscache_sim)
         auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
         auto sim = lookup(outpointidx, 0);
         if (!sim.has_value()) {
-            assert(!realcoin || realcoin->IsSpent());
+            assert(!realcoin);
         } else {
             assert(realcoin && !realcoin->IsSpent());
             assert(realcoin->out == data.coins[sim->first].out);

src/txdb.cpp

@@ -67,7 +67,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size)
 
 std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
 {
-    if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin;
+    if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) {
+        Assert(!coin.IsSpent()); // The UTXO database should never contain spent coins
+        return coin;
+    }
     return std::nullopt;
 }
 

src/wallet/wallet.cpp

@@ -2413,8 +2413,15 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs
         for (const auto& it : erased_txs) {
             const Txid hash{it->first};
             wtxOrdered.erase(it->second.m_it_wtxOrdered);
-            for (const auto& txin : it->second.tx->vin)
-                mapTxSpends.erase(txin.prevout);
+            for (const auto& txin : it->second.tx->vin) {
+                auto range = mapTxSpends.equal_range(txin.prevout);
+                for (auto iter = range.first; iter != range.second; ++iter) {
+                    if (iter->second == hash) {
+                        mapTxSpends.erase(iter);
+                        break;
+                    }
+                }
+            }
             for (unsigned int i = 0; i < it->second.tx->vout.size(); ++i) {
                 m_txos.erase(COutPoint(hash, i));
             }

test/functional/wallet_importprunedfunds.py

@@ -14,6 +14,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_not_equal,
     assert_raises_rpc_error,
     wallet_importprivkey,
 )
@@ -129,6 +130,33 @@ def run_test(self):
         mb.header.nTime += 1  # modify arbitrary block header field to change block hash
         assert_raises_rpc_error(-5, "Block not found in chain", w1.importprunedfunds, rawtxn1, mb.serialize().hex())
 
+        self.log.info("Test removeprunedfunds with conflicting transactions")
+        node = self.nodes[0]
+
+        # Create a transaction
+        utxo = node.listunspent()[0]
+        addr = node.getnewaddress()
+        tx1_id = node.send(outputs=[{addr: 1}], inputs=[utxo])["txid"]
+        tx1_fee = node.gettransaction(tx1_id)["fee"]
+
+        # Create a conflicting tx with a larger fee (tx1_fee is negative)
+        output_value = utxo["amount"] + tx1_fee - Decimal("0.00001")
+        raw_tx2 = node.createrawtransaction(inputs=[utxo], outputs=[{addr: output_value}])
+        signed_tx2 = node.signrawtransactionwithwallet(raw_tx2)
+        tx2_id = node.sendrawtransaction(signed_tx2["hex"])
+        assert_not_equal(tx2_id, tx1_id)
+
+        # Both txs should be in the wallet, tx2 replaced tx1 in mempool
+        assert tx1_id in [tx["txid"] for tx in node.listtransactions()]
+        assert tx2_id in [tx["txid"] for tx in node.listtransactions()]
+
+        # Remove the replaced tx from wallet
+        node.removeprunedfunds(tx1_id)
+
+        # The UTXO should still be considered spent (by tx2)
+        available_utxos = [u["txid"] for u in node.listunspent(minconf=0)]
+        assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx"
+
 
 if __name__ == '__main__':
     ImportPrunedFundsTest(__file__).main()