txbatcher: be careful when removing local transactions
What the developer wrote
Authored by ThomasV
txbatcher: be careful when removing local transactions
1. Do not remove local transaction in find_base_tx.
This logic was intended to cleanup claim transactions that are
never broadcast (for example, if the counterparty gets a refund)
(see 1bf1de36cbbab3ef9bd06bf5009074f11f0d456d)
However, this code is too unspecific and may result in fund loss,
because the transaction being removed may contain outgoing payments.
For example, if the electrum server is not responsive, the tx will
be seen as local and deleted. In that case, another payment will
be attempted, thus paying twice.
2. Do not remove tx after try_broadcasting returns False.
The server might be lying to us. We can only remove the local tx
if there is a base_tx, because the next tx we create will try to
spend the same output.
What changed, and why it matters
This commit fixes a bug in Electrum's transaction batching code that could accidentally delete a transaction that had already been sent to the Bitcoin network. If the Electrum server was slow, unresponsive, or dishonest, the wallet could think a payment was never broadcast, remove it from local records, and then create and send a second payment for the same thing—effectively paying twice and losing funds. The fix stops the wallet from automatically removing 'local' transactions in two risky situations and instead tries to rebroadcast them safely.
Users running Electrum versions with the txbatcher module (especially those using submarine swaps or batch payments) should upgrade to a version containing this commit. Operators should monitor for any unexpected duplicate outgoing payments and verify transaction history against the blockchain if they experienced unresponsive servers.
Security signals we found
Potential double-spend/double-payment due to premature local transaction removal
Server trust assumption: wallet relied on Electrum server height status to delete local transactions
Fund-loss condition when server is unresponsive or dishonest
Fix removes unsafe cleanup paths and replaces with rebroadcast logic
Test case validating unsafe behavior removed
Evidence from the diff
In electrum/txbatcher.py, the patch changes find_base_tx() to be async and removes the logic that deleted transactions marked TX_HEIGHT_LOCAL when the address database was considered up-to-date. It now rebroadcasts such transactions instead, because an unresponsive or malicious server could report a transaction as local when it is actually in the mempool. In run_iteration(), after a failed broadcast, the code now only removes the newly created transaction if a base_tx exists, ensuring the next created transaction will spend the same prevout and cannot double-pay. Without a base_tx, the local transaction is retained for later rebroadcast. A test that verified the old unsafe removal behavior is deleted.
Changed components
electrum/txbatcher.pytests/test_txbatcher.pyTxBatch.find_base_txTxBatch.run_iterationwallet transaction removal logicInspect captured patch +30 / −41
diff --git a/electrum/txbatcher.py b/electrum/txbatcher.py
index 419621c..ccf6143 100644
--- a/electrum/txbatcher.py
+++ b/electrum/txbatcher.py
@@ -352,7 +352,7 @@ class TxBatch(Logger):
# todo: require more than one confirmation
return len(self.batch_inputs) == 0 and len(self.batch_payments) == 0 and len(self._batch_txids) == 0
- def find_base_tx(self) -> Optional[PartialTransaction]:
+ async def find_base_tx(self) -> Optional[PartialTransaction]:
if not self._prevout:
return None
prev_txid, index = self._prevout.split(':')
@@ -376,17 +376,16 @@ class TxBatch(Logger):
self.logger.info(f'base tx confirmed {txid}')
self._clear_unconfirmed_sweeps(tx)
self._start_new_batch(tx)
- elif tx_mined_status.height in [TX_HEIGHT_LOCAL, TX_HEIGHT_FUTURE]:
- # fixme: adb may return TX_HEIGHT_LOCAL when not up to date
- if self.wallet.adb.is_up_to_date():
- self.logger.info(f'removing local base_tx {txid}')
- self.wallet.adb.remove_transaction(txid)
- self._start_new_batch(None)
+ if tx_mined_status.height in [TX_HEIGHT_LOCAL]:
+ # this may happen if our Electrum server is unresponsive
+ # server could also be lying to us. Rebroadcasting might
+ # help, if we have switched to another server.
+ await self.wallet.network.try_broadcasting(tx, 'batch')
return self._base_tx
async def run_iteration(self) -> None:
- base_tx = self.find_base_tx()
+ base_tx = await self.find_base_tx()
try:
tx = self.create_next_transaction(base_tx)
except NoDynamicFeeEstimates:
@@ -419,15 +418,33 @@ class TxBatch(Logger):
self._new_base_tx(tx)
if not await self.wallet.network.try_broadcasting(tx, 'batch'):
- # most likely reason is that base_tx is not replaceable
- # this may be the case if it has children (because we don't pay enough fees to replace them)
- # or if we are trying to sweep unconfirmed inputs (replacement-adds-unconfirmed error)
self.logger.info(f'cannot broadcast tx {tx.txid()}')
- self.wallet.adb.remove_transaction(tx.txid())
if base_tx:
+ # The most likely cause is that base_tx is not
+ # replaceable. This may be the case if it has children
+ # (because we don't pay enough fees to replace them)
+ # or if we are trying to sweep unconfirmed inputs
+ # (replacement-adds-unconfirmed error)
+
+ # it is OK to remove the transaction, because
+ # create_next_transaction will create a new tx that is
+ # incompatible with the one we remove here, so we
+ # cannot double pay.
+ self.wallet.adb.remove_transaction(tx.txid())
self.logger.info(f'starting new batch because could not broadcast')
self._start_new_batch(base_tx)
-
+ else:
+ # it is dangerous to remove the transaction if there
+ # is no base_tx. Indeed, the transaction might have
+ # been broadcast. So, we just keep the transaction as
+ # local, and we will try to rebroadcast it later (see
+ # above).
+ #
+ # FIXME: it should be possible to ensure that
+ # create_next_transaction creates transactions that
+ # spend the same coins, using self._prevout. This
+ # would make them incompatible, and safe to broadcast.
+ pass
async def sign_transaction(self, tx: PartialTransaction) -> Optional[PartialTransaction]:
tx.add_info_from_wallet(self.wallet) # this adds input amounts
diff --git a/tests/test_txbatcher.py b/tests/test_txbatcher.py
index d257e57..c7a1b07 100644
--- a/tests/test_txbatcher.py
+++ b/tests/test_txbatcher.py
@@ -222,34 +222,6 @@ class TestTxBatcher(ElectrumTestCase):
assert new_tx.inputs()[0].prevout == tx.inputs()[0].prevout == txin.prevout
assert output1 in new_tx.outputs()
- @mock.patch.object(wallet.Abstract_Wallet, 'save_db')
- async def test_remove_local_base_tx(self, mock_save_db):
- """
- The swap claim tx does not get broadcast
- we test that txbatcher.find_base_tx() removes the local tx
- """
- self.maxDiff = None
- # create wallet
- wallet = self._create_wallet()
- # mock is_up_to_date
- wallet.adb.is_up_to_date = lambda: True
- # do not broadcast, wait forever
- async def do_wait(x, y):
- await asyncio.sleep(100000000)
- self.network.try_broadcasting = do_wait
- # add swap data
- wallet.adb.db.transactions[SWAPDATA.funding_txid] = tx = Transaction(SWAP_FUNDING_TX)
- wallet.adb.receive_tx_callback(tx, tx_height=1)
- wallet.txbatcher.add_sweep_input('default', SWAP_SWEEP_INFO)
- txbatch = wallet.txbatcher.tx_batches.get('default')
- base_tx = await self._wait_for_base_tx(txbatch)
- self.assertEqual(base_tx.txid(), '80a8cbc42de74cb48a09644c1e438c8b39144bd3b55c574f21d89d05c85fed34')
- await wallet.stop()
- txbatch.batch_inputs.clear()
- wallet.start_network(self.network)
- base_tx = await self._wait_for_base_tx(txbatch, should_be_none=True)
- self.assertEqual(base_tx, None)
-
async def _wait_for_base_tx(self, txbatch, should_be_none=False):
async with timeout_after(10):
while True:
Why this scored 69/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.