Merge bitcoin/bitcoin#34358: wallet: fix removeprunedfunds bug with conflicting transactions

1f60ca360eb83fa7982b1aac402eaaf477294197 wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande)

Pull request description:

  `removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
  The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again.

  The added test should fail on master.

ACKs for top commit:
  achow101:
    ACK 1f60ca360eb83fa7982b1aac402eaaf477294197
  fjahr:
    tACK 1f60ca360eb83fa7982b1aac402eaaf477294197
  furszy:
    utACK 1f60ca360eb83fa7982b1aac402eaaf477294197
  vasild:
    ACK 1f60ca360eb83fa7982b1aac402eaaf477294197

Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347

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.messages import (
 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 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
         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()