RBF batching: some fixes

electrum/address_synchronizer.py

@@ -25,7 +25,7 @@ import threading
 import asyncio
 import itertools
 from collections import defaultdict
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Dict
 
 from . import bitcoin
 from .bitcoin import COINBASE_MATURITY, TYPE_ADDRESS, TYPE_PUBKEY
@@ -457,7 +457,7 @@ class AddressSynchronizer(PrintError):
                 self.spent_outpoints = defaultdict(dict)
                 self.history = {}
                 self.verified_tx = {}
-                self.transactions = {}
+                self.transactions = {}  # type: Dict[str, Transaction]
                 self.save_transactions()
 
     def get_txpos(self, tx_hash):
@@ -484,12 +484,6 @@ class AddressSynchronizer(PrintError):
                 self.threadlocal_cache.local_height = orig_val
         return f
 
-    def get_unconfirmed_tx(self):
-        for tx_hash, tx_mined_status, delta, balance in self.get_history():
-            if tx_mined_status.conf <= 0 and delta < 0:
-                tx = self.transactions.get(tx_hash)
-                return tx
-
     @with_local_height_cached
     def get_history(self, domain=None):
         # get domain

electrum/coinchooser.py

@@ -84,8 +84,8 @@ def strip_unneeded(bkts, sufficient_funds):
     for i in range(len(bkts)):
         if not sufficient_funds(bkts[i + 1:]):
             return bkts[i:]
-    # Shouldn't get here
-    return bkts
+    # none of the buckets are needed
+    return []
 
 class CoinChooserBase(PrintError):
 
@@ -203,12 +203,13 @@ class CoinChooserBase(PrintError):
 
         # Copy the outputs so when adding change we don't modify "outputs"
         tx = Transaction.from_io(inputs[:], outputs[:])
-        v = tx.input_value()
+        input_value = tx.input_value()
 
         # Weight of the transaction with no inputs and no change
         # Note: this will use legacy tx serialization as the need for "segwit"
         # would be detected from inputs. The only side effect should be that the
         # marker and flag are excluded, which is compensated in get_tx_weight()
+        # FIXME calculation will be off by this (2 wu) in case of RBF batching
         base_weight = tx.estimated_weight()
         spent_amount = tx.output_value()
 
@@ -232,7 +233,7 @@ class CoinChooserBase(PrintError):
         def sufficient_funds(buckets):
             '''Given a list of buckets, return True if it has enough
             value to pay for the transaction'''
-            total_input = v + sum(bucket.value for bucket in buckets)
+            total_input = input_value + sum(bucket.value for bucket in buckets)
             total_weight = get_tx_weight(buckets)
             return total_input >= spent_amount + fee_estimator_w(total_weight)
 

electrum/gui/qt/main_window.py

@@ -2763,7 +2763,7 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, PrintError):
         batch_rbf_cb.setChecked(self.config.get('batch_rbf', False))
         batch_rbf_cb.setEnabled(use_rbf)
         batch_rbf_cb.setToolTip(
-            _('If you check this box, your unconfirmed transactios will be consolidated in a single transaction') + '\n' + \
+            _('If you check this box, your unconfirmed transactions will be consolidated into a single transaction.') + '\n' + \
             _('This will save fees.'))
         def on_batch_rbf(x):
             self.config.set_key('batch_rbf', bool(x))

electrum/transaction.py

@@ -864,7 +864,7 @@ class Transaction:
     @classmethod
     def serialize_witness(self, txin, estimate_size=False):
         _type = txin['type']
-        if not self.is_segwit_input(txin) and not self.is_input_value_needed(txin):
+        if not self.is_segwit_input(txin) and not txin['type'] == 'address':
             return '00'
         if _type == 'coinbase':
             return txin['witness']
@@ -902,10 +902,6 @@ class Transaction:
     def is_segwit_inputtype(cls, txin_type):
         return txin_type in ('p2wpkh', 'p2wpkh-p2sh', 'p2wsh', 'p2wsh-p2sh')
 
-    @classmethod
-    def is_input_value_needed(cls, txin):
-        return cls.is_segwit_input(txin) or txin['type'] == 'address'
-
     @classmethod
     def guess_txintype_from_address(cls, addr):
         # It's not possible to tell the script type in general

electrum/wallet.py

@@ -536,6 +536,29 @@ class Abstract_Wallet(AddressSynchronizer):
     def dust_threshold(self):
         return dust_threshold(self.network)
 
+    def get_unconfirmed_base_tx_for_batching(self) -> Optional[Transaction]:
+        candidate = None
+        for tx_hash, tx_mined_status, delta, balance in self.get_history():
+            # tx should not be mined yet
+            if tx_mined_status.conf > 0: continue
+            # tx should be "outgoing" from wallet
+            if delta >= 0: continue
+            tx = self.transactions.get(tx_hash)
+            if not tx: continue
+            # is_mine outputs should not be spent yet
+            # to avoid cancelling our own dependent transactions
+            for output_idx, o in enumerate(tx.outputs()):
+                if self.is_mine(o.address) and self.spent_outpoints[tx.txid()].get(output_idx):
+                    continue
+            # prefer txns already in mempool (vs local)
+            if tx_mined_status.height == TX_HEIGHT_LOCAL:
+                candidate = tx
+                continue
+            # tx must have opted-in for RBF
+            if tx.is_final(): continue
+            return tx
+        return candidate
+
     def make_unsigned_transaction(self, coins, outputs, config, fixed_fee=None,
                                   change_addr=None, is_sweep=False):
         # check outputs
@@ -592,8 +615,8 @@ class Abstract_Wallet(AddressSynchronizer):
             max_change = self.max_change_outputs if self.multiple_change else 1
             coin_chooser = coinchooser.get_coin_chooser(config)
             # If there is an unconfirmed RBF tx, merge with it
-            base_tx = self.get_unconfirmed_tx()
-            if config.get('batch_rbf', False) and base_tx and not base_tx.is_final():
+            base_tx = self.get_unconfirmed_base_tx_for_batching()
+            if config.get('batch_rbf', False) and base_tx:
                 base_tx = Transaction(base_tx.serialize())
                 base_tx.deserialize(force_full_parse=True)
                 base_tx.remove_signatures()
@@ -602,7 +625,7 @@ class Abstract_Wallet(AddressSynchronizer):
                 fee_per_byte = Decimal(base_fee) / base_tx.estimated_size()
                 fee_estimator = lambda size: base_fee + round(fee_per_byte * size)
                 txi = base_tx.inputs()
-                txo = list(filter(lambda x: not self.is_change(x[1]), base_tx.outputs()))
+                txo = list(filter(lambda o: not self.is_change(o.address), base_tx.outputs()))
             else:
                 txi = []
                 txo = []