qt confirm_tx_dialog: fix wallet.get_candidates_for_batching
What the developer wrote
Authored by SomberNight
qt confirm_tx_dialog: fix wallet.get_candidates_for_batching
I don't understand what the "coins not used" comment meant here.
It was added in the change away from the old config.WALLET_BATCH_RBF option
(https://github.com/spesmilo/electrum/commit/ab14c3e1382c1af48baff73b790aecfbd069eb8a).
The `coins` param *is used* in wallet.get_candidates_for_batching.
Without setting that, the returned set of candidates was restricted to
only base txs that had a large enough change output to cover *all* the newly added outputs.
Instead, it is desirable to allow adding new inputs.
What changed, and why it matters
This commit fixes a bug in Electrum's transaction batching feature. Previously, when suggesting which existing transactions could be combined (batched) with a new payment, the code ignored the wallet's available unspent coins. This made the batching suggestions too restrictive and could lead to suboptimal or unexpected transaction construction. The fix passes the available coins into the candidate-selection logic and documents the behavior. There is no direct evidence in the commit of a security vulnerability or exploit.
Review the batching logic for correctness and consider whether the conservative coin selection (`confirmed_only=True`) adequately mitigates race conditions with `make_tx()`. No immediate security patch appears required based solely on this diff.
Security signals we found
Functional bug in transaction batching candidate selection
Potential for unexpected transaction construction or fee/change behavior due to missing UTXO context
No explicit security claim, exploit primitive, or vulnerability disclosure present in commit
Evidence from the diff
The patch changes wallet.get_candidates_for_batching to require coins as a keyword argument and updates the Qt send tab to supply a conservative set of spendable UTXOs (nonlocal_only=True, confirmed_only=True). Previously the caller passed an empty list, which caused get_candidates_for_batching to only consider base transactions whose change output could cover all new outputs, instead of also allowing new inputs to be added. The change also adds a docstring clarifying the role of coins. A test is updated to use the new keyword-only signature.
Changed components
electrum/gui/qt/send_tab.pyelectrum/wallet.pytests/test_wallet_vertical.pyInspect captured patch +19 / −3
diff --git a/electrum/gui/qt/send_tab.py b/electrum/gui/qt/send_tab.py
index 1550e85..3c1adcc 100644
--- a/electrum/gui/qt/send_tab.py
+++ b/electrum/gui/qt/send_tab.py
@@ -327,7 +327,14 @@ class SendTab(QWidget, MessageBoxMixin, Logger):
is_max = any(parse_max_spend(outval) for outval in output_values)
output_value = '!' if is_max else sum(output_values)
- candidates = self.wallet.get_candidates_for_batching(outputs, []) # coins not used
+ # To find batching candidates, we need to know our available UTXOs.
+ # Ideally should use same set of coins make_tx() will use.
+ # note: - prone to races: coins set might change due to new txs between now and make_tx() call
+ # - make_tx() might pass different params to get_coins()
+ # - to mitigate, we prefer to be more restrictive. hence confirmed_only=True
+ coins_conservative = get_coins(nonlocal_only=True, confirmed_only=True)
+ candidates = self.wallet.get_candidates_for_batching(outputs, coins=coins_conservative)
+
tx, is_preview = self.window.confirm_tx_dialog(make_tx, output_value, batching_candidates=candidates)
if tx is None:
# user cancelled
diff --git a/electrum/wallet.py b/electrum/wallet.py
index 843d521..e0ef006 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -1788,7 +1788,16 @@ class Abstract_Wallet(ABC, Logger, EventListener):
def dust_threshold(self):
return dust_threshold(self.network)
- def get_candidates_for_batching(self, outputs, coins) -> Sequence[Transaction]:
+ def get_candidates_for_batching(
+ self,
+ outputs: Sequence[PartialTxOutput],
+ *,
+ coins: Sequence[PartialTxInput],
+ ) -> Sequence[Transaction]:
+ """
+ coins: utxos available to add as inputs into the final tx. If empty, the set of candidates is restricted to
+ base txs with large enough change outputs to cover paying for all the `outputs`.
+ """
# do not batch if we spend max (not supported by make_unsigned_transaction)
if any([parse_max_spend(o.value) is not None for o in outputs]):
return []
diff --git a/tests/test_wallet_vertical.py b/tests/test_wallet_vertical.py
index 1a41562..ffe737a 100644
--- a/tests/test_wallet_vertical.py
+++ b/tests/test_wallet_vertical.py
@@ -2162,7 +2162,7 @@ class TestWalletSending(ElectrumTestCase):
coins = wallet.get_spendable_coins(domain=None)
self.assertEqual(2, len(coins))
- candidates = wallet.get_candidates_for_batching(outputs, coins)
+ candidates = wallet.get_candidates_for_batching(outputs, coins=coins)
self.assertEqual(candidates, [])
with self.assertRaises(NotEnoughFunds):
wallet.make_unsigned_transaction(coins=coins, outputs=outputs, fee_policy=FixedFeePolicy(1000), base_tx=toself_tx)
Why this scored 34/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.