Merge pull request #10591 from SomberNight/202604_fix_wallet_mktx_base_tx

wallet: make_unsigned_tx: fix base_tx for GUI simple-send batching

Author: SomberNight

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__(

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,

electrum/wallet.py

@@ -1803,6 +1803,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 (
@@ -2008,20 +2009,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

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"