From 54e07ee22ff16fc68583ade0d2b8ffffc81d444a Mon Sep 17 00:00:00 2001 From: ishaanam Date: Wed, 17 May 2023 20:56:25 -0400 Subject: [PATCH] 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 | 13 ++++++-- src/wallet/wallet.cpp | 36 +++++++++++++++++++++-- src/wallet/wallet.h | 1 + test/functional/wallet_abandonconflict.py | 6 +++- test/functional/wallet_conflicts.py | 12 ++++---- 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 2ffd85afa9c..9c27574103b 100644 --- a/src/wallet/transaction.h +++ b/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 @@ public: 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 mempool_conflicts; + template void Serialize(Stream& s) const { @@ -335,9 +343,10 @@ public: void updateState(interfaces::Chain& chain); bool isAbandoned() const { return state() && state()->abandoned; } + bool isMempoolConflicted() const { return !mempool_conflicts.empty(); } bool isBlockConflicted() const { return state(); } bool isInactive() const { return state(); } - bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !isConfirmed(); } + bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !isMempoolConflicted() && !isConfirmed(); } bool isConfirmed() const { return state(); } const Txid& GetHash() const LIFETIMEBOUND { return tx->GetHash(); } const Wtxid& GetWitnessHash() const LIFETIMEBOUND { return tx->GetWitnessHash(); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1053b69f32c..2adc502642a 100644 --- a/src/wallet/wallet.cpp +++ b/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 todo; std::set 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 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) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d55b683f1cd..0bc460942c9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -364,6 +364,7 @@ private: /** 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); diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py index 26915077736..bc489524315 100755 --- a/test/functional/wallet_abandonconflict.py +++ b/test/functional/wallet_abandonconflict.py @@ -232,7 +232,11 @@ class AbandonConflictTest(BitcoinTestFramework): 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")) diff --git a/test/functional/wallet_conflicts.py b/test/functional/wallet_conflicts.py index 3ca7eb246c9..cb6b1c7eaaf 100755 --- a/test/functional/wallet_conflicts.py +++ b/test/functional/wallet_conflicts.py @@ -174,9 +174,9 @@ class TxConflicts(BitcoinTestFramework): # 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 @@ class TxConflicts(BitcoinTestFramework): 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 @@ class TxConflicts(BitcoinTestFramework): 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}])