← Watch feed
wallet: make_unsigned_tx: fix base_tx for GUI simple-send batching
Public commit record
91/100 · Strong
What the developer wrote
Authored by SomberNight
wallet: make_unsigned_tx: fix base_tx for GUI simple-send batching
fixes regression from https://github.com/spesmilo/electrum/commit/a9f20e4d3de9848247a9079b5b164d8006cf80db :
For the GUI / manual new tx constructions usecase, the flow relies on `base_tx.add_info_from_wallet(self)` being called before `base_tx_fee = base_tx.get_fee()`.
fixes https://github.com/spesmilo/electrum/issues/10587
maybe fixes https://github.com/spesmilo/electrum/issues/8876
probably not a full fix: base_tx could have inputs for which add_info_from_wallet is not sufficient
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference
What changed, and why it matters
This commit fixes a bug in Electrum's Bitcoin wallet where combining a new payment with an existing unconfirmed transaction (a feature called RBF batching) could fail or compute fees incorrectly. The root cause was that the code tried to read the existing transaction's fee before it had loaded all the necessary wallet information about that transaction's inputs. The fix reorders the code so the wallet enriches the transaction first, then reads its fee. It also adds a safety check to skip transactions whose confirmation status is unknown when looking for batching candidates.
Recommended action
Review the fee-estimation path for other places where `get_fee()` may be called before `add_info_from_wallet()`. Consider whether `base_tx_fee = None` for non-ismine inputs needs explicit handling, as noted by the added FIXME. Users relying on RBF batching in the Qt GUI should upgrade.
Security signals we found
01
Incorrect fee estimation when batching with an unconfirmed base transaction
02
Use of transaction data before wallet enrichment (add_info_from_wallet)
03
Potential crash or misbehavior in GUI simple-send RBF batching
04
Missing handling of unknown confirmation status in batching candidate selection
Evidence from the diff
The patch addresses a regression introduced by commit a9f20e4d in wallet.py:make_unsigned_transaction. Previously, base_tx.get_fee() was called before base_tx.add_info_from_wallet(self) for non-PartialTransaction base transactions, meaning fee calculation could use incomplete input data. The fix moves base_tx.add_info_from_wallet(self) (and base_tx.remove_signatures() for PartialTransaction) ahead of base_tx.get_fee(), and records base_tx_size before stripping signatures. It also adds assert base_tx.txid() is not None and a guard in get_candidates_for_batching() to skip history items where tx_mined_status.conf is None. Type annotations in the Qt GUI are tightened to Sequence[Transaction]. A regression test test_rbf_batching__happypath_using_get_candidates_for_batching is added.
Changed components
electrum/wallet.pyelectrum/gui/qt/confirm_tx_dialog.pyelectrum/gui/qt/main_window.pytests/test_wallet_vertical.pyInspect captured patch +74 / −12
diff --git a/electrum/gui/qt/confirm_tx_dialog.py b/electrum/gui/qt/confirm_tx_dialog.py
index 990720a..f58e03d 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.i18n import _
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 @@ class TxEditor(WindowModalDialog, QtEventListener, Logger):
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 @@ class TxEditor(WindowModalDialog, QtEventListener, Logger):
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 @@ class ConfirmTxDialog(TxEditor):
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 257add8..d6bb67b 100644
--- a/electrum/gui/qt/main_window.py
+++ b/electrum/gui/qt/main_window.py
@@ -1507,7 +1507,7 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener):
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 29e2e52..cb052dd 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -1811,6 +1811,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
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 @@ class Abstract_Wallet(ABC, Logger, EventListener):
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 ffe3490..c0e26e0 100644
--- a/tests/test_wallet_vertical.py
+++ b/tests/test_wallet_vertical.py
@@ -2159,7 +2159,7 @@ class TestWalletSending(ElectrumTestCase):
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 @@ class TestWalletSending(ElectrumTestCase):
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"
Risk score
Our methodology →Why this scored 34/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.