diff --git a/electrum/gui/qt/confirm_tx_dialog.py b/electrum/gui/qt/confirm_tx_dialog.py index 990720a3692f..f58e03d316b6 100644 --- a/electrum/gui/qt/confirm_tx_dialog.py +++ b/electrum/gui/qt/confirm_tx_dialog.py @@ -26,7 +26,7 @@ import asyncio from decimal import Decimal from functools import partial -from typing import TYPE_CHECKING, Optional, Union +from typing import TYPE_CHECKING, Optional, Union, Sequence from concurrent.futures import Future from enum import Enum, auto @@ -39,7 +39,7 @@ from electrum.util import (UserCancelled, quantize_feerate, profiler, NotEnoughFunds, NoDynamicFeeEstimates, get_asyncio_loop, wait_for2, UserFacingException) from electrum.plugin import run_hook -from electrum.transaction import PartialTransaction, PartialTxOutput +from electrum.transaction import PartialTransaction, PartialTxOutput, Transaction from electrum.wallet import InternalAddressCorruption from electrum.bitcoin import DummyAddress from electrum.fee_policy import FeePolicy, FixedFeePolicy, FeeMethod @@ -82,7 +82,7 @@ def __init__( output_value: Union[int, str], payee_outputs: Optional[list[PartialTxOutput]] = None, context: TxEditorContext = TxEditorContext.PAYMENT, - batching_candidates=None, + batching_candidates: Sequence[Transaction] = None, ): WindowModalDialog.__init__(self, window, title=title) @@ -106,7 +106,7 @@ def __init__( self.needs_update = False self.context = context self.is_preview = False - self._base_tx = None # for batching + self._base_tx = None # type: Optional[Transaction] # for batching self.batching_candidates = batching_candidates self.swap_manager = self.wallet.lnworker.swap_manager if self.wallet.has_lightning() else None @@ -1123,7 +1123,7 @@ def __init__( output_value: Union[int, str], payee_outputs: Optional[list[PartialTxOutput]] = None, context: TxEditorContext = TxEditorContext.PAYMENT, - batching_candidates=None, + batching_candidates: Sequence[Transaction] = None, ): TxEditor.__init__( diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py index 257add816a37..d6bb67b3d79a 100644 --- a/electrum/gui/qt/main_window.py +++ b/electrum/gui/qt/main_window.py @@ -1507,7 +1507,7 @@ def confirm_tx_dialog( output_value, *, payee_outputs: Optional[list[TxOutput]] = None, context: TxEditorContext = TxEditorContext.PAYMENT, - batching_candidates=None, + batching_candidates: Sequence[Transaction] = None, ) -> tuple[Optional[PartialTransaction], bool, bool]: d = ConfirmTxDialog( window=self, diff --git a/electrum/wallet.py b/electrum/wallet.py index 29e2e52e2c97..cb052dda61bb 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -1811,6 +1811,7 @@ def get_candidates_for_batching( domain = self.get_addresses() for hist_item in self.adb.get_history(domain): # tx should not be mined yet + if hist_item.tx_mined_status.conf is None: continue if hist_item.tx_mined_status.conf > 0: continue # conservative future proofing of code: only allow known unconfirmed types if hist_item.tx_mined_status.height() not in ( @@ -2016,20 +2017,21 @@ def make_unsigned_transaction( coin_chooser = coinchooser.get_coin_chooser(self.config) # If there is an unconfirmed RBF tx, merge with it if base_tx: + assert base_tx.txid() is not None # pre-segwit and incomplete? # make sure we don't try to spend change from the tx-to-be-replaced: coins = [c for c in coins if c.prevout.txid.hex() != base_tx.txid()] is_local = self.adb.get_tx_height(base_tx.txid()).height() == TX_HEIGHT_LOCAL - # estimate base tx fee before stripping tx for more accurate estimate - base_tx_fee = base_tx.get_fee() - base_feerate = Decimal(base_tx_fee)/base_tx.estimated_size() - relayfeerate = Decimal(self.relayfee()) / 1000 - original_fee_estimator = fee_estimator + base_tx_size = base_tx.estimated_size() # estimate before stripping tx for more accurate estimate if not isinstance(base_tx, PartialTransaction): base_tx = PartialTransaction.from_tx(base_tx) base_tx.add_info_from_wallet(self) else: # don't cast PartialTransaction, because it removes make_witness base_tx.remove_signatures() + base_tx_fee = base_tx.get_fee() # FIXME could be None if some inputs are non-ismine + base_feerate = Decimal(base_tx_fee) / base_tx_size + relayfeerate = Decimal(self.relayfee()) / 1000 + original_fee_estimator = fee_estimator def fee_estimator(size: Union[int, float, Decimal]) -> int: size = Decimal(size) lower_bound_relayfee = int(base_tx_fee + round(size * relayfeerate)) if not is_local else 0 diff --git a/tests/test_wallet_vertical.py b/tests/test_wallet_vertical.py index ffe3490d6bca..c0e26e0ddabd 100644 --- a/tests/test_wallet_vertical.py +++ b/tests/test_wallet_vertical.py @@ -2159,7 +2159,7 @@ async def _rbf_sufficient_fee_increase_adding_outputs_to_base_tx(self, *, simula tx2_feerate_sat_vb = tx2.get_fee() / tx2.estimated_size() self.assertGreater(tx2_feerate_sat_vb, tx1_feerate_sat_vb, msg="tx2 feerate lower than tx1") - def _create_cause_carbon_wallet(self): + def _create_cause_carbon_wallet(self) -> tuple[Standard_Wallet, str, str]: data = read_test_vector('cause_carbon_wallet.json') ks = keystore.from_seed(data['seed'], passphrase='', for_multisig=False) wallet = WalletIntegrityHelper.create_standard_wallet(ks, gap_limit=2, gap_limit_for_change=2, config=self.config) @@ -2356,6 +2356,66 @@ async def test_rbf_batching__merge_duplicate_outputs(self): wallet.adb.receive_tx_callback(tx3, tx_height=TX_HEIGHT_UNCONFIRMED) self.assertEqual((0, 197_000, 0), wallet.get_balance()) + async def test_rbf_batching__happypath_using_get_candidates_for_batching(self): + """Test GUI simple-send RBF 'batch with' functionality: + - while there is already an unconfirmed outgoing tx1 in the wallet history, + - try making another payment, and batch it with tx1. + """ + wallet_faucet = self._create_cause_carbon_wallet()[0] + mywallet = self.create_standard_wallet_from_seed( + 'response era cable net spike again observe dumb wage wonder sail tortoise', + config=self.config) + + # fund mywallet with 2 UTXOs + def fund_mywallet_from_faucet(): + addr = mywallet.get_receiving_address() + assert not mywallet.adb.is_used(addr) + funding_tx1 = wallet_faucet.make_unsigned_transaction( + outputs=[PartialTxOutput.from_address_and_value(addr, 40_000)], + fee_policy=FixedFeePolicy(200), locktime=4_000_000, rbf=True, + ) + wallet_faucet.sign_transaction(funding_tx1, password=None) + wallet_faucet.adb.receive_tx_callback(funding_tx1, tx_height=TX_HEIGHT_UNCONFIRMED) + mywallet.adb.receive_tx_callback(funding_tx1, tx_height=TX_HEIGHT_UNCONFIRMED) + assert mywallet.adb.is_used(addr) + fund_mywallet_from_faucet() + fund_mywallet_from_faucet() + + external_addr1 = wallet_faucet.get_receiving_addresses()[0] + external_addr2 = wallet_faucet.get_receiving_addresses()[1] + assert external_addr1 and external_addr2 and (external_addr1 != external_addr2) + + # create outgoing tx1 + output1 = PartialTxOutput.from_address_and_value(external_addr1, 30_000) + candidates1 = mywallet.get_candidates_for_batching([output1], coins=mywallet.get_spendable_coins(domain=None)) + self.assertEqual(candidates1, []) + outgoing_tx1 = mywallet.make_unsigned_transaction( + outputs=[output1], + fee_policy=FixedFeePolicy(200), locktime=4_000_001, rbf=True, + ) + mywallet.sign_transaction(outgoing_tx1, password=None) + mywallet.adb.receive_tx_callback(outgoing_tx1, tx_height=TX_HEIGHT_UNCONFIRMED) + + # create outgoing tx2, and try to batch it with tx1 + output2 = PartialTxOutput.from_address_and_value(external_addr2, 30_000) + candidates2 = mywallet.get_candidates_for_batching([output2], coins=mywallet.get_spendable_coins(domain=None)) + self.assertEqual(1, len(candidates2)) + base_tx = candidates2[0] + self.assertEqual(outgoing_tx1.txid(), base_tx.txid()) + outgoing_tx2 = mywallet.make_unsigned_transaction( + outputs=[output2], + fee_policy=FixedFeePolicy(200), locktime=4_000_001, rbf=True, + base_tx=base_tx, + ) + mywallet.sign_transaction(outgoing_tx2, password=None) + mywallet.adb.receive_tx_callback(outgoing_tx2, tx_height=TX_HEIGHT_UNCONFIRMED) + + self.assertEqual(3, len(outgoing_tx2.outputs())) + self.assertIn(output1, outgoing_tx2.outputs()) + self.assertIn(output2, outgoing_tx2.outputs()) + self.assertEqual('02000000000102ab99cf51f3e219735c49491a9ecf354517a215a2b462227df7e7624188a7ff830000000000fdffffff417bbd802768510a0c641fc79af6e691a01fd10f37b84162f253b594e5cb87c50100000000fdffffff032c4c0000000000001600144e1b662f616fe134430054e29295ea6e5c18f17330750000000000001600142266c890fad71396f106319368107d5b2a1146fe307500000000000016001476efaaa243327bf3a2c0f5380cb3914099448cec02473044022033f8ddcb07ad7e06cef417208a5244507157cccdd6817029e968ec60a2395add02200720eb6a7c8cea86b470398ce75d69abcc66624a2253b18ea81cd1566eb5132c0121029b1a61d66896486ab893741b38dbafb9673b91a82237d6e4ca0da3cda7cbeb7c0247304402205e00156d74bcd85ed26ee9d0bdbd72890656881e25c04b2ac94f1c6b91f1176f02205f94d055e6385b48fbd3ac1a2dc2f57a640f4b33740cd9fb9e0fc20eb0cf5dcb012102c1ed648e71f15643950b444b864ab784b9d0e31e6ca6ec7d849d3dda4d98da0501093d00', + str(outgoing_tx2)) + async def test_join_psbts__merge_duplicate_outputs(self): """txos paying to the same address might be merged into a single output with a larger value""" rawtx1 = "70736274ff01007102000000019264597cffcce8f0c17b16a02adca7a95ae90f2ea51bd4b4df60c76dfe86686e0000000000fdffffff02400d03000000000016001458aee0f1201d1ae12dbd241209bbe92ed45e39b6108c0400000000001600144e1b662f616fe134430054e29295ea6e5c18f173c2ad26000001011f20a1070000000000160014542266519a44eb9b903761d40c6fe1055d33fa050100de02000000000101013548c9019890e27ce9e58766de05f18ea40ede70751fb6cd7a3a1715ece0a30100000000fdffffff0220a1070000000000160014542266519a44eb9b903761d40c6fe1055d33fa05485a080000000000160014bc69f7d82c403a9f35dfb6d1a4531d6b19cab0e3024730440220346b200f21c3024e1d51fb4ecddbdbd68bd24ae7b9dfd501519f6dcbeb7c052402200617e3ce7b0eb308e30caf23894fb0388b68fb1c15dd0681dd13ae5e735f148101210360d0c9ef15b8b6a16912d341ad218a4e4e4e07e9347f4a2dbc7ca8d974f8bc9ec1ad26002206029b1a61d66896486ab893741b38dbafb9673b91a82237d6e4ca0da3cda7cbeb7c101f1b48320000008000000000000000000000220203db4846ec1841f48484590e67fcd7d1039f124a04410c5794f38ec8625329ea23101f1b483200000080010000000000000000"