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

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

@@ -758,7 +758,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
             // check for the inverse condition and only consider spending
             // transactions in known, potentially active states.
             const auto& wtx = mit->second;
-            if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
+            if (!wtx.isAbandoned() && !wtx.isMempoolConflicted() && !wtx.isBlockConflicted())
                 return true; // Spent
         }
     }
@@ -1363,8 +1363,12 @@ 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) {
+    // Do not flush the wallet here for performance reasons
 
     std::set<uint256> todo;
     std::set<uint256> done;
@@ -1382,7 +1386,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));
@@ -1423,6 +1427,26 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
     if (it != mapWallet.end()) {
         RefreshMempoolStatus(it->second, chain());
     }
+
+    const Txid& conflict_txid = tx->GetHash();
+
+    for (const CTxIn& tx_in : tx->vin) {
+        auto range =  mapTxSpends.equal_range(tx_in.prevout);
+
+        // For each wallet transaction spending this prevout...
+        for (auto _it = range.first; _it != range.second; ++_it) {
+            // if this tx is the same as the tx which was just added to the mempool, continue
+            if (_it->second == conflict_txid) continue;
+
+            auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+                if (!wtx.mempool_conflicts.insert(conflict_txid).second) return TxUpdate::UNCHANGED;
+
+                return TxUpdate::CHANGED;
+            };
+
+            RecursiveUpdateTxState(nullptr, _it->second, try_updating_state);
+        }
+    }
 }
 
 void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
@@ -1460,6 +1484,25 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
         // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
         SyncTransaction(tx, TxStateInactive{});
     }
+
+    const Txid& conflict_txid = tx->GetHash();
+
+    for (const CTxIn& tx_in : tx->vin) {
+        auto range =  mapTxSpends.equal_range(tx_in.prevout);
+
+        // For each wallet transaction spending this prevout...
+        for (auto _it = range.first; _it != range.second; ++_it) {
+
+            auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+                // Remove the previously conflicting transaction from this wtx's entry
+                if (!wtx.mempool_conflicts.erase(conflict_txid)) return TxUpdate::UNCHANGED;
+
+                return TxUpdate::CHANGED;
+            };
+
+            RecursiveUpdateTxState(nullptr, _it->second, try_updating_state);
+        }
+    }
 }
 
 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")
 
@@ -264,8 +264,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)
@@ -295,7 +295,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"))
 
         bob.sendrawtransaction(tx1_conflict_conflict) # kick tx1_conflict out of the mempool
         bob.sendrawtransaction(raw_tx1) #re-broadcast tx1 because it is no longer conflicted