wallet: track mempool conflicts

Behavior changes are:
- if a tx has a mempool conflict, the wallet will not attempt to
  rebroadcast it
- if a txo is spent by a mempool-conflicted tx, that txo is no
  longer considered spent

Author: ishaanam

src/wallet/transaction.h

@@ -48,7 +48,7 @@ struct TxStateBlockConflicted {
     int conflicting_block_height;
 
     explicit TxStateBlockConflicted(const uint256& block_hash, int height) : conflicting_block_hash(block_hash), conflicting_block_height(height) {}
-    std::string toString() const { return strprintf("Conflicted (block=%s, height=%i)", conflicting_block_hash.ToString(), conflicting_block_height); }
+    std::string toString() const { return strprintf("BlockConflicted (block=%s, height=%i)", conflicting_block_hash.ToString(), conflicting_block_height); }
 };
 
 //! State of transaction not confirmed or conflicting with a known block and
@@ -258,6 +258,14 @@ class CWalletTx
     CTransactionRef tx;
     TxState m_state;
 
+    // Set of mempool transactions that conflict
+    // directly with the transaction, or that conflict
+    // with an ancestor transaction. This set will be
+    // empty if state is InMempool or Confirmed, but
+    // can be nonempty if state is Inactive or
+    // BlockConflicted.
+    std::set<Txid> mempool_conflicts;
+
     template<typename Stream>
     void Serialize(Stream& s) const
     {
@@ -335,9 +343,10 @@ class CWalletTx
     void updateState(interfaces::Chain& chain);
 
     bool isAbandoned() const { return state<TxStateInactive>() && state<TxStateInactive>()->abandoned; }
+    bool isMempoolConflicted() const { return !mempool_conflicts.empty(); }
     bool isBlockConflicted() const { return state<TxStateBlockConflicted>(); }
     bool isInactive() const { return state<TxStateInactive>(); }
-    bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !isConfirmed(); }
+    bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !isMempoolConflicted() && !isConfirmed(); }
     bool isConfirmed() const { return state<TxStateConfirmed>(); }
     const Txid& GetHash() const LIFETIMEBOUND { return tx->GetHash(); }
     const Wtxid& GetWitnessHash() const LIFETIMEBOUND { return tx->GetWitnessHash(); }

src/wallet/wallet.cpp

@@ -753,7 +753,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
         const auto mit = mapWallet.find(wtxid);
         if (mit != mapWallet.end()) {
             const auto& wtx = mit->second;
-            if (!wtx.isAbandoned() && !wtx.isBlockConflicted())
+            if (!wtx.isAbandoned() && !wtx.isBlockConflicted() && !wtx.isMempoolConflicted())
                 return true; // Spent
         }
     }
@@ -1360,7 +1360,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
 void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
     // Do not flush the wallet here for performance reasons
     WalletBatch batch(GetDatabase(), false);
+    RecursiveUpdateTxState(&batch, tx_hash, try_updating_state);
+}
 
+void CWallet::RecursiveUpdateTxState(WalletBatch* batch, const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
     std::set<uint256> todo;
     std::set<uint256> done;
 
@@ -1377,7 +1380,7 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt
         TxUpdate update_state = try_updating_state(wtx);
         if (update_state != TxUpdate::UNCHANGED) {
             wtx.MarkDirty();
-            batch.WriteTx(wtx);
+            if (batch) batch->WriteTx(wtx);
             // Iterate over all its outputs, and update those tx states as well (if applicable)
             for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) {
                 std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(COutPoint(Txid::FromUint256(now), i));
@@ -1418,6 +1421,20 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
     if (it != mapWallet.end()) {
         RefreshMempoolStatus(it->second, chain());
     }
+
+    const Txid& txid = tx->GetHash();
+
+    for (const CTxIn& tx_in : tx->vin) {
+        // For each wallet transaction spending this prevout..
+        for (auto range = mapTxSpends.equal_range(tx_in.prevout); range.first != range.second; range.first++) {
+            const uint256& spent_id = range.first->second;
+            // Skip the recently added tx
+            if (spent_id == txid) continue;
+            RecursiveUpdateTxState(/*batch=*/nullptr, spent_id, [&txid](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+                return wtx.mempool_conflicts.insert(txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
+            });
+        }
+    }
 }
 
 void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
@@ -1455,6 +1472,21 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
         // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
         SyncTransaction(tx, TxStateInactive{});
     }
+
+    const Txid& txid = tx->GetHash();
+
+    for (const CTxIn& tx_in : tx->vin) {
+        // Iterate over all wallet transactions spending txin.prev
+        // and recursively mark them as no longer conflicting with
+        // txid
+        for (auto range = mapTxSpends.equal_range(tx_in.prevout); range.first != range.second; range.first++) {
+            const uint256& spent_id = range.first->second;
+
+            RecursiveUpdateTxState(/*batch=*/nullptr, spent_id, [&txid](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+                return wtx.mempool_conflicts.erase(txid) ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
+            });
+        }
+    }
 }
 
 void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& block)

src/wallet/wallet.h

@@ -364,6 +364,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 
     /** Mark a transaction (and its in-wallet descendants) as a particular tx state. */
     void RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void RecursiveUpdateTxState(WalletBatch* batch, const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /** Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */
     void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

test/functional/wallet_abandonconflict.py

@@ -232,7 +232,11 @@ def run_test(self):
         balance = newbalance
 
         # Invalidate the block with the double spend. B & C's 10 BTC outputs should no longer be available
-        self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
+        blk = self.nodes[0].getbestblockhash()
+        # mine 10 blocks so that when the blk is invalidated, the transactions are not
+        # returned to the mempool
+        self.generate(self.nodes[1], 10)
+        self.nodes[0].invalidateblock(blk)
         assert_equal(alice.gettransaction(txAB1)["confirmations"], 0)
         newbalance = alice.getbalance()
         assert_equal(newbalance, balance - Decimal("20"))

test/functional/wallet_conflicts.py

@@ -174,9 +174,9 @@ def test_mempool_conflict(self):
         # broadcast tx2, replaces tx1 in mempool
         tx2_txid = alice.sendrawtransaction(tx2)
 
-        # Check that unspent[0] is still not available because the wallet does not know that the tx spending it has a mempool conflicted
-        assert_equal(alice.listunspent(), [])
-        assert_equal(alice.getbalance(), 0)
+        # Check that unspent[0] is now available because the transaction spending it has been replaced in the mempool
+        assert_equal(alice.listunspent(), [unspents[0]])
+        assert_equal(alice.getbalance(), 25)
 
         self.log.info("Test scenario where a mempool conflict is removed")
 
@@ -262,8 +262,8 @@ def test_mempool_and_block_conflicts(self):
         assert tx2_txid in bob.getrawmempool()
         assert tx1_conflict_txid in bob.getrawmempool()
 
-        # check that the tx2 unspent is still not available because the wallet does not know that the tx spending it has a mempool conflict
-        assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0)
+        # check that tx3 is now conflicted, so the output from tx2 can now be spent
+        assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("24.99990000"))
 
         # we will be disconnecting this block in the future
         alice.sendrawtransaction(tx2_conflict)
@@ -293,7 +293,7 @@ def test_mempool_and_block_conflicts(self):
         assert_equal(bob.gettransaction(tx3_txid)["confirmations"], 0)
 
         bob.sendrawtransaction(raw_tx2)
-        assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0)
+        assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("24.99990000"))
 
         # create a conflict to previous tx (also spends unspents[2]), but don't broadcast, sends funds back to alice
         raw_tx = alice.createrawtransaction(inputs=[unspents[2]], outputs=[{alice.getnewaddress() : 24.99}])