From 180973a94180f9849bf7cb0dab7c9177a942efb8 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Thu, 1 Feb 2024 17:59:43 -0500 Subject: test: Add tests for wallet mempool conflicts --- test/functional/wallet_conflicts.py | 282 ++++++++++++++++++++++++++++++++++++ 1 file changed, 282 insertions(+) diff --git a/test/functional/wallet_conflicts.py b/test/functional/wallet_conflicts.py index 802b718cd5..3ca7eb246c 100755 --- a/test/functional/wallet_conflicts.py +++ b/test/functional/wallet_conflicts.py @@ -9,6 +9,7 @@ Test that wallet correctly tracks transactions that have been conflicted by bloc from decimal import Decimal +from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -28,6 +29,20 @@ class TxConflicts(BitcoinTestFramework): return next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(from_tx_id)["details"] if tx_out["amount"] == Decimal(f"{search_value}")) def run_test(self): + """ + The following tests check the behavior of the wallet when + transaction conflicts are created. These conflicts are created + using raw transaction RPCs that double-spend UTXOs and have more + fees, replacing the original transaction. + """ + + self.test_block_conflicts() + self.generatetoaddress(self.nodes[0], COINBASE_MATURITY + 7, self.nodes[2].getnewaddress()) + self.test_mempool_conflict() + self.test_mempool_and_block_conflicts() + self.test_descendants_with_mempool_conflicts() + + def test_block_conflicts(self): self.log.info("Send tx from which to conflict outputs later") txid_conflict_from_1 = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) txid_conflict_from_2 = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) @@ -123,5 +138,272 @@ class TxConflicts(BitcoinTestFramework): assert_equal(former_conflicted["confirmations"], 1) assert_equal(former_conflicted["blockheight"], 217) + def test_mempool_conflict(self): + self.nodes[0].createwallet("alice") + alice = self.nodes[0].get_wallet_rpc("alice") + + bob = self.nodes[1] + + self.nodes[2].send(outputs=[{alice.getnewaddress() : 25} for _ in range(3)]) + self.generate(self.nodes[2], 1) + + self.log.info("Test a scenario where a transaction has a mempool conflict") + + unspents = alice.listunspent() + assert_equal(len(unspents), 3) + assert all([tx["amount"] == 25 for tx in unspents]) + + # tx1 spends unspent[0] and unspent[1] + raw_tx = alice.createrawtransaction(inputs=[unspents[0], unspents[1]], outputs=[{bob.getnewaddress() : 49.9999}]) + tx1 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + # tx2 spends unspent[1] and unspent[2], conflicts with tx1 + raw_tx = alice.createrawtransaction(inputs=[unspents[1], unspents[2]], outputs=[{bob.getnewaddress() : 49.99}]) + tx2 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + # tx3 spends unspent[2], conflicts with tx2 + raw_tx = alice.createrawtransaction(inputs=[unspents[2]], outputs=[{bob.getnewaddress() : 24.9899}]) + tx3 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + # broadcast tx1 + tx1_txid = alice.sendrawtransaction(tx1) + + assert_equal(alice.listunspent(), [unspents[2]]) + assert_equal(alice.getbalance(), 25) + + # 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) + + self.log.info("Test scenario where a mempool conflict is removed") + + # broadcast tx3, replaces tx2 in mempool + # Now that tx1's conflict has been removed, tx1 is now + # not conflicted, and instead is inactive until it is + # rebroadcasted. Now unspent[0] is not available, because + # tx1 is no longer conflicted. + alice.sendrawtransaction(tx3) + + assert tx1_txid not in self.nodes[0].getrawmempool() + + # now all of alice's outputs should be considered spent + # unspent[0]: spent by inactive tx1 + # unspent[1]: spent by inactive tx1 + # unspent[2]: spent by active tx3 + assert_equal(alice.listunspent(), []) + assert_equal(alice.getbalance(), 0) + + # Clean up for next test + bob.sendall([self.nodes[2].getnewaddress()]) + self.generate(self.nodes[2], 1) + + alice.unloadwallet() + + def test_mempool_and_block_conflicts(self): + self.nodes[0].createwallet("alice_2") + alice = self.nodes[0].get_wallet_rpc("alice_2") + bob = self.nodes[1] + + self.nodes[2].send(outputs=[{alice.getnewaddress() : 25} for _ in range(3)]) + self.generate(self.nodes[2], 1) + + self.log.info("Test a scenario where a transaction has both a block conflict and a mempool conflict") + unspents = [{"txid" : element["txid"], "vout" : element["vout"]} for element in alice.listunspent()] + + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + + # alice and bob nodes are disconnected so that transactions can be + # created by alice, but broadcasted from bob so that alice's wallet + # doesn't know about them + self.disconnect_nodes(0, 1) + + # Sends funds to bob + raw_tx = alice.createrawtransaction(inputs=[unspents[0]], outputs=[{bob.getnewaddress() : 24.99999}]) + raw_tx1 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + tx1_txid = bob.sendrawtransaction(raw_tx1) # broadcast original tx spending unspents[0] only to bob + + # create a conflict to previous tx (also spends unspents[0]), but don't broadcast, sends funds back to alice + raw_tx = alice.createrawtransaction(inputs=[unspents[0], unspents[2]], outputs=[{alice.getnewaddress() : 49.999}]) + tx1_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + # Sends funds to bob + raw_tx = alice.createrawtransaction(inputs=[unspents[1]], outputs=[{bob.getnewaddress() : 24.9999}]) + raw_tx2 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + tx2_txid = bob.sendrawtransaction(raw_tx2) # broadcast another original tx spending unspents[1] only to bob + + # create a conflict to previous tx (also spends unspents[1]), but don't broadcast, sends funds to alice + raw_tx = alice.createrawtransaction(inputs=[unspents[1]], outputs=[{alice.getnewaddress() : 24.9999}]) + tx2_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + bob_unspents = [{"txid" : element, "vout" : 0} for element in [tx1_txid, tx2_txid]] + + # tx1 and tx2 are now in bob's mempool, and they are unconflicted, so bob has these funds + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("49.99989000")) + + # spend both of bob's unspents, child tx of tx1 and tx2 + raw_tx = bob.createrawtransaction(inputs=[bob_unspents[0], bob_unspents[1]], outputs=[{bob.getnewaddress() : 49.999}]) + raw_tx3 = bob.signrawtransactionwithwallet(raw_tx)['hex'] + tx3_txid = bob.sendrawtransaction(raw_tx3) # broadcast tx only to bob + + # alice knows about 0 txs, bob knows about 3 + assert_equal(len(alice.getrawmempool()), 0) + assert_equal(len(bob.getrawmempool()), 3) + + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("49.99900000")) + + # bob broadcasts tx_1 conflict + tx1_conflict_txid = bob.sendrawtransaction(tx1_conflict) + assert_equal(len(alice.getrawmempool()), 0) + assert_equal(len(bob.getrawmempool()), 2) # tx1_conflict kicks out both tx1, and its child tx3 + + 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) + + # we will be disconnecting this block in the future + alice.sendrawtransaction(tx2_conflict) + assert_equal(len(alice.getrawmempool()), 1) # currently alice's mempool is only aware of tx2_conflict + # 11 blocks are mined so that when they are invalidated, tx_2 + # does not get put back into the mempool + blk = self.generate(self.nodes[0], 11, sync_fun=self.no_op)[0] + assert_equal(len(alice.getrawmempool()), 0) # tx2_conflict is now mined + + self.connect_nodes(0, 1) + self.sync_blocks() + assert_equal(alice.getbestblockhash(), bob.getbestblockhash()) + + # now that tx2 has a block conflict, tx1_conflict should be the only tx in bob's mempool + assert tx1_conflict_txid in bob.getrawmempool() + assert_equal(len(bob.getrawmempool()), 1) + + # tx3 should now also be block-conflicted by tx2_conflict + assert_equal(bob.gettransaction(tx3_txid)["confirmations"], -11) + # bob has no pending funds, since tx1, tx2, and tx3 are all conflicted + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + bob.invalidateblock(blk) # remove tx2_conflict + # bob should still have no pending funds because tx1 and tx3 are still conflicted, and tx2 has not been re-broadcast + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + assert_equal(len(bob.getrawmempool()), 1) + # check that tx3 is no longer block-conflicted + assert_equal(bob.gettransaction(tx3_txid)["confirmations"], 0) + + bob.sendrawtransaction(raw_tx2) + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + + # 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}]) + tx1_conflict_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] + + 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 + + # Now bob has no pending funds because tx1 and tx2 are spent by tx3, which hasn't been re-broadcast yet + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + + bob.sendrawtransaction(raw_tx3) + assert_equal(len(bob.getrawmempool()), 4) # The mempool contains: tx1, tx2, tx1_conflict_conflict, tx3 + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("49.99900000")) + + # Clean up for next test + bob.reconsiderblock(blk) + assert_equal(alice.getbestblockhash(), bob.getbestblockhash()) + self.sync_mempools() + self.generate(self.nodes[2], 1) + + alice.unloadwallet() + + def test_descendants_with_mempool_conflicts(self): + self.nodes[0].createwallet("alice_3") + alice = self.nodes[0].get_wallet_rpc("alice_3") + + self.nodes[2].send(outputs=[{alice.getnewaddress() : 25} for _ in range(2)]) + self.generate(self.nodes[2], 1) + + self.nodes[1].createwallet("bob_1") + bob = self.nodes[1].get_wallet_rpc("bob_1") + + self.nodes[2].createwallet("carol") + carol = self.nodes[2].get_wallet_rpc("carol") + + self.log.info("Test a scenario where a transaction's parent has a mempool conflict") + + unspents = alice.listunspent() + assert_equal(len(unspents), 2) + assert all([tx["amount"] == 25 for tx in unspents]) + + assert_equal(alice.getrawmempool(), []) + + # Alice spends first utxo to bob in tx1 + raw_tx = alice.createrawtransaction(inputs=[unspents[0]], outputs=[{bob.getnewaddress() : 24.9999}]) + tx1 = alice.signrawtransactionwithwallet(raw_tx)['hex'] + tx1_txid = alice.sendrawtransaction(tx1) + + self.sync_mempools() + + assert_equal(alice.getbalance(), 25) + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("24.99990000")) + + raw_tx = bob.createrawtransaction(inputs=[bob.listunspent(minconf=0)[0]], outputs=[{carol.getnewaddress() : 24.999}]) + # Bob creates a child to tx1 + tx1_child = bob.signrawtransactionwithwallet(raw_tx)['hex'] + tx1_child_txid = bob.sendrawtransaction(tx1_child) + + self.sync_mempools() + + # Currently neither tx1 nor tx1_child should have any conflicts + assert tx1_txid in bob.getrawmempool() + assert tx1_child_txid in bob.getrawmempool() + assert_equal(len(bob.getrawmempool()), 2) + + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + assert_equal(carol.getbalances()["mine"]["untrusted_pending"], Decimal("24.99900000")) + + # Alice spends first unspent again, conflicting with tx1 + raw_tx = alice.createrawtransaction(inputs=[unspents[0], unspents[1]], outputs=[{carol.getnewaddress() : 49.99}]) + tx1_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] + tx1_conflict_txid = alice.sendrawtransaction(tx1_conflict) + + self.sync_mempools() + + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + assert_equal(carol.getbalances()["mine"]["untrusted_pending"], Decimal("49.99000000")) + + assert tx1_txid not in bob.getrawmempool() + assert tx1_child_txid not in bob.getrawmempool() + assert tx1_conflict_txid in bob.getrawmempool() + assert_equal(len(bob.getrawmempool()), 1) + + # Now create a conflict to tx1_conflict, so that it gets kicked out of the mempool + raw_tx = alice.createrawtransaction(inputs=[unspents[1]], outputs=[{carol.getnewaddress() : 24.9895}]) + tx1_conflict_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] + tx1_conflict_conflict_txid = alice.sendrawtransaction(tx1_conflict_conflict) + + self.sync_mempools() + + # Both tx1 and tx1_child are still not in the mempool because they have not be re-broadcasted + assert tx1_txid not in bob.getrawmempool() + assert tx1_child_txid not in bob.getrawmempool() + assert tx1_conflict_txid not in bob.getrawmempool() + assert tx1_conflict_conflict_txid in bob.getrawmempool() + assert_equal(len(bob.getrawmempool()), 1) + + assert_equal(alice.getbalance(), 0) + assert_equal(bob.getbalances()["mine"]["untrusted_pending"], 0) + assert_equal(carol.getbalances()["mine"]["untrusted_pending"], Decimal("24.98950000")) + + # Both tx1 and tx1_child can now be re-broadcasted + bob.sendrawtransaction(tx1) + bob.sendrawtransaction(tx1_child) + assert_equal(len(bob.getrawmempool()), 3) + + alice.unloadwallet() + bob.unloadwallet() + carol.unloadwallet() + if __name__ == '__main__': TxConflicts().main() -- cgit v1.2.3 From ffe5ff1fb622a8da11b66289e1b778e45e449811 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Thu, 29 Jun 2023 15:41:26 -0400 Subject: scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted -BEGIN VERIFY SCRIPT- sed -i 's/TxStateConflicted/TxStateBlockConflicted/g' src/wallet/wallet.cpp src/wallet/interfaces.cpp src/wallet/transaction.h src/wallet/transaction.cpp sed -i 's/isConflicted/isBlockConflicted/g' src/wallet/transaction.h src/wallet/wallet.cpp -END VERIFY SCRIPT- --- src/wallet/interfaces.cpp | 2 +- src/wallet/transaction.cpp | 2 +- src/wallet/transaction.h | 16 ++++++++-------- src/wallet/wallet.cpp | 16 ++++++++-------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d15273dfc9..6405fb9bba 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -92,7 +92,7 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx) WalletTxStatus result; result.block_height = wtx.state() ? wtx.state()->confirmed_block_height : - wtx.state() ? wtx.state()->conflicting_block_height : + wtx.state() ? wtx.state()->conflicting_block_height : std::numeric_limits::max(); result.blocks_to_maturity = wallet.GetTxBlocksToMaturity(wtx); result.depth_in_main_chain = wallet.GetTxDepthInMainChain(wtx); diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp index 6777257e53..561880482f 100644 --- a/src/wallet/transaction.cpp +++ b/src/wallet/transaction.cpp @@ -45,7 +45,7 @@ void CWalletTx::updateState(interfaces::Chain& chain) }; if (auto* conf = state()) { lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, m_state); - } else if (auto* conf = state()) { + } else if (auto* conf = state()) { lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, m_state); } } diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index ddeb931112..2ffd85afa9 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -43,11 +43,11 @@ struct TxStateInMempool { }; //! State of rejected transaction that conflicts with a confirmed block. -struct TxStateConflicted { +struct TxStateBlockConflicted { uint256 conflicting_block_hash; int conflicting_block_height; - explicit TxStateConflicted(const uint256& block_hash, int height) : conflicting_block_hash(block_hash), conflicting_block_height(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); } }; @@ -75,7 +75,7 @@ struct TxStateUnrecognized { }; //! All possible CWalletTx states -using TxState = std::variant; +using TxState = std::variant; //! Subset of states transaction sync logic is implemented to handle. using SyncTxState = std::variant; @@ -90,7 +90,7 @@ static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data) } else if (data.index >= 0) { return TxStateConfirmed{data.block_hash, /*height=*/-1, data.index}; } else if (data.index == -1) { - return TxStateConflicted{data.block_hash, /*height=*/-1}; + return TxStateBlockConflicted{data.block_hash, /*height=*/-1}; } return data; } @@ -102,7 +102,7 @@ static inline uint256 TxStateSerializedBlockHash(const TxState& state) [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; }, [](const TxStateInMempool& in_mempool) { return uint256::ZERO; }, [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; }, - [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; }, + [](const TxStateBlockConflicted& conflicted) { return conflicted.conflicting_block_hash; }, [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; } }, state); } @@ -114,7 +114,7 @@ static inline int TxStateSerializedIndex(const TxState& state) [](const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; }, [](const TxStateInMempool& in_mempool) { return 0; }, [](const TxStateConfirmed& confirmed) { return confirmed.position_in_block; }, - [](const TxStateConflicted& conflicted) { return -1; }, + [](const TxStateBlockConflicted& conflicted) { return -1; }, [](const TxStateUnrecognized& unrecognized) { return unrecognized.index; } }, state); } @@ -335,9 +335,9 @@ public: void updateState(interfaces::Chain& chain); bool isAbandoned() const { return state() && state()->abandoned; } - bool isConflicted() const { return state(); } + bool isBlockConflicted() const { return state(); } bool isInactive() const { return state(); } - bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); } + bool isUnconfirmed() const { return !isAbandoned() && !isBlockConflicted() && !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 e93cd4b4b9..f2116a297d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1197,7 +1197,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx auto it = mapWallet.find(txin.prevout.hash); if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; - if (auto* prev = prevtx.state()) { + if (auto* prev = prevtx.state()) { MarkConflicted(prev->conflicting_block_hash, prev->conflicting_block_height, wtx.GetHash()); } } @@ -1309,7 +1309,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) assert(!wtx.isConfirmed()); assert(!wtx.InMempool()); // If already conflicted or abandoned, no need to set abandoned - if (!wtx.isConflicted() && !wtx.isAbandoned()) { + if (!wtx.isBlockConflicted() && !wtx.isAbandoned()) { wtx.m_state = TxStateInactive{/*abandoned=*/true}; return TxUpdate::NOTIFY_CHANGED; } @@ -1346,7 +1346,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c if (conflictconfirms < GetTxDepthInMainChain(wtx)) { // Block is 'more conflicted' than current confirm; update. // Mark transaction as conflicted with this block. - wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + wtx.m_state = TxStateBlockConflicted{hashBlock, conflicting_height}; return TxUpdate::CHANGED; } return TxUpdate::UNCHANGED; @@ -1506,11 +1506,11 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) for (TxSpends::const_iterator _it = range.first; _it != range.second; ++_it) { CWalletTx& wtx = mapWallet.find(_it->second)->second; - if (!wtx.isConflicted()) continue; + if (!wtx.isBlockConflicted()) continue; auto try_updating_state = [&](CWalletTx& tx) { - if (!tx.isConflicted()) return TxUpdate::UNCHANGED; - if (tx.state()->conflicting_block_height >= disconnect_height) { + if (!tx.isBlockConflicted()) return TxUpdate::UNCHANGED; + if (tx.state()->conflicting_block_height >= disconnect_height) { tx.m_state = TxStateInactive{}; return TxUpdate::CHANGED; } @@ -2725,7 +2725,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old std::optional block_hash; if (auto* conf = wtx.state()) { block_hash = conf->confirmed_block_hash; - } else if (auto* conf = wtx.state()) { + } else if (auto* conf = wtx.state()) { block_hash = conf->conflicting_block_hash; } @@ -3315,7 +3315,7 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const if (auto* conf = wtx.state()) { assert(conf->confirmed_block_height >= 0); return GetLastBlockHeight() - conf->confirmed_block_height + 1; - } else if (auto* conf = wtx.state()) { + } else if (auto* conf = wtx.state()) { assert(conf->conflicting_block_height >= 0); return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1); } else { -- cgit v1.2.3 From d64922b5903e5ffc8d2ce0e6761f99f173b60800 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Wed, 1 Mar 2023 13:46:23 -0500 Subject: wallet refactor: use CWalletTx member functions to determine tx state --- src/wallet/interfaces.cpp | 2 +- src/wallet/receive.cpp | 7 +++---- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 5 ----- 4 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 6405fb9bba..d33e6f3873 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -101,7 +101,7 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx) result.is_trusted = CachedTxIsTrusted(wallet, wtx); result.is_abandoned = wtx.isAbandoned(); result.is_coinbase = wtx.IsCoinBase(); - result.is_in_main_chain = wallet.IsTxInMainChain(wtx); + result.is_in_main_chain = wtx.isConfirmed(); return result; } diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index b9d8d9abc9..ea3ffff549 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -149,7 +149,7 @@ CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, c { AssertLockHeld(wallet.cs_wallet); - if (wallet.IsTxImmatureCoinBase(wtx) && wallet.IsTxInMainChain(wtx)) { + if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) { return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, filter); } @@ -256,9 +256,8 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set& trusted_parents) { AssertLockHeld(wallet.cs_wallet); - int nDepth = wallet.GetTxDepthInMainChain(wtx); - if (nDepth >= 1) return true; - if (nDepth < 0) return false; + if (wtx.isConfirmed()) return true; + if (wtx.isBlockConflicted()) return false; // using wtx's cached debit if (!wallet.m_spend_zero_conf_change || !CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)) return false; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f2116a297d..1053b69f32 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -752,8 +752,8 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const const uint256& wtxid = it->second; const auto mit = mapWallet.find(wtxid); if (mit != mapWallet.end()) { - int depth = GetTxDepthInMainChain(mit->second); - if (depth > 0 || (depth == 0 && !mit->second.isAbandoned())) + const auto& wtx = mit->second; + if (!wtx.isAbandoned() && !wtx.isBlockConflicted()) return true; // Spent } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cc961068a5..d55b683f1c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -515,11 +515,6 @@ public: * referenced in transaction, and might cause assert failures. */ int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) - { - AssertLockHeld(cs_wallet); - return GetTxDepthInMainChain(wtx) > 0; - } /** * @return number of blocks to maturity for this transaction: -- cgit v1.2.3 From 54e07ee22ff16fc68583ade0d2b8ffffc81d444a Mon Sep 17 00:00:00 2001 From: ishaanam Date: Wed, 17 May 2023 20:56:25 -0400 Subject: 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 2ffd85afa9..9c27574103 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 1053b69f32..2adc502642 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 d55b683f1c..0bc460942c 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 2691507773..bc48952431 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 3ca7eb246c..cb6b1c7eaa 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}]) -- cgit v1.2.3 From 5952292133d6cc889f51ae771f2e0557311e1efe Mon Sep 17 00:00:00 2001 From: ishaanam Date: Sun, 16 Jul 2023 21:42:33 -0400 Subject: wallet, rpc: show mempool conflicts in `gettransaction` result --- src/wallet/rpc/transactions.cpp | 8 ++++++++ test/functional/wallet_basic.py | 2 +- test/functional/wallet_conflicts.py | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index e6c021d426..05b340995d 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -40,6 +40,10 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue for (const uint256& conflict : wallet.GetTxConflicts(wtx)) conflicts.push_back(conflict.GetHex()); entry.pushKV("walletconflicts", conflicts); + UniValue mempool_conflicts(UniValue::VARR); + for (const Txid& mempool_conflict : wtx.mempool_conflicts) + mempool_conflicts.push_back(mempool_conflict.GetHex()); + entry.pushKV("mempoolconflicts", mempool_conflicts); entry.pushKV("time", wtx.GetTxTime()); entry.pushKV("timereceived", int64_t{wtx.nTimeReceived}); @@ -417,6 +421,10 @@ static std::vector TransactionDescriptionString() }}, {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx was replaced."}, {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."}, + {RPCResult::Type::ARR, "mempoolconflicts", "Transactions that directly conflict with either this transaction or an ancestor transaction", + { + {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, + }}, {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."}, {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."}, diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index f798eee365..0b52ed7914 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -679,7 +679,7 @@ class WalletTest(BitcoinTestFramework): "category": baz["category"], "vout": baz["vout"]} expected_fields = frozenset({'amount', 'bip125-replaceable', 'confirmations', 'details', 'fee', - 'hex', 'lastprocessedblock', 'time', 'timereceived', 'trusted', 'txid', 'wtxid', 'walletconflicts'}) + 'hex', 'lastprocessedblock', 'time', 'timereceived', 'trusted', 'txid', 'wtxid', 'walletconflicts', 'mempoolconflicts'}) verbose_field = "decoded" expected_verbose_fields = expected_fields | {verbose_field} diff --git a/test/functional/wallet_conflicts.py b/test/functional/wallet_conflicts.py index cb6b1c7eaa..e5739a6a59 100755 --- a/test/functional/wallet_conflicts.py +++ b/test/functional/wallet_conflicts.py @@ -178,6 +178,8 @@ class TxConflicts(BitcoinTestFramework): assert_equal(alice.listunspent(), [unspents[0]]) assert_equal(alice.getbalance(), 25) + assert_equal(alice.gettransaction(tx1_txid)["mempoolconflicts"], [tx2_txid]) + self.log.info("Test scenario where a mempool conflict is removed") # broadcast tx3, replaces tx2 in mempool @@ -187,6 +189,7 @@ class TxConflicts(BitcoinTestFramework): # tx1 is no longer conflicted. alice.sendrawtransaction(tx3) + assert_equal(alice.gettransaction(tx1_txid)["mempoolconflicts"], []) assert tx1_txid not in self.nodes[0].getrawmempool() # now all of alice's outputs should be considered spent @@ -262,6 +265,10 @@ class TxConflicts(BitcoinTestFramework): assert tx2_txid in bob.getrawmempool() assert tx1_conflict_txid in bob.getrawmempool() + assert_equal(bob.gettransaction(tx1_txid)["mempoolconflicts"], [tx1_conflict_txid]) + assert_equal(bob.gettransaction(tx2_txid)["mempoolconflicts"], []) + assert_equal(bob.gettransaction(tx3_txid)["mempoolconflicts"], [tx1_conflict_txid]) + # 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")) @@ -348,6 +355,8 @@ class TxConflicts(BitcoinTestFramework): assert_equal(alice.getbalance(), 25) assert_equal(bob.getbalances()["mine"]["untrusted_pending"], Decimal("24.99990000")) + assert_equal(bob.gettransaction(tx1_txid)["mempoolconflicts"], []) + raw_tx = bob.createrawtransaction(inputs=[bob.listunspent(minconf=0)[0]], outputs=[{carol.getnewaddress() : 24.999}]) # Bob creates a child to tx1 tx1_child = bob.signrawtransactionwithwallet(raw_tx)['hex'] @@ -356,6 +365,8 @@ class TxConflicts(BitcoinTestFramework): self.sync_mempools() # Currently neither tx1 nor tx1_child should have any conflicts + assert_equal(bob.gettransaction(tx1_txid)["mempoolconflicts"], []) + assert_equal(bob.gettransaction(tx1_child_txid)["mempoolconflicts"], []) assert tx1_txid in bob.getrawmempool() assert tx1_child_txid in bob.getrawmempool() assert_equal(len(bob.getrawmempool()), 2) @@ -378,6 +389,10 @@ class TxConflicts(BitcoinTestFramework): assert tx1_conflict_txid in bob.getrawmempool() assert_equal(len(bob.getrawmempool()), 1) + # Now both tx1 and tx1_child are conflicted by tx1_conflict + assert_equal(bob.gettransaction(tx1_txid)["mempoolconflicts"], [tx1_conflict_txid]) + assert_equal(bob.gettransaction(tx1_child_txid)["mempoolconflicts"], [tx1_conflict_txid]) + # Now create a conflict to tx1_conflict, so that it gets kicked out of the mempool raw_tx = alice.createrawtransaction(inputs=[unspents[1]], outputs=[{carol.getnewaddress() : 24.9895}]) tx1_conflict_conflict = alice.signrawtransactionwithwallet(raw_tx)['hex'] @@ -385,6 +400,10 @@ class TxConflicts(BitcoinTestFramework): self.sync_mempools() + # Now that tx1_conflict has been removed, both tx1 and tx1_child + assert_equal(bob.gettransaction(tx1_txid)["mempoolconflicts"], []) + assert_equal(bob.gettransaction(tx1_child_txid)["mempoolconflicts"], []) + # Both tx1 and tx1_child are still not in the mempool because they have not be re-broadcasted assert tx1_txid not in bob.getrawmempool() assert tx1_child_txid not in bob.getrawmempool() -- cgit v1.2.3