Simplify RBF user experience:

- replace complex strategies with a simpler choice,
   between preserving or decreasing the payment.
 - Always expose that choice to the user.
 - Show the resulting fees to the user before they click OK

electrum/commands.py

@@ -51,7 +51,7 @@ from .transaction import (Transaction, multisig_script, TxOutput, PartialTransac
                           tx_from_any, PartialTxInput, TxOutpoint)
 from .invoices import PR_PAID, PR_UNPAID, PR_UNKNOWN, PR_EXPIRED
 from .synchronizer import Notifier
-from .wallet import Abstract_Wallet, create_new_wallet, restore_wallet_from_text, Deterministic_Wallet, BumpFeeStrategy
+from .wallet import Abstract_Wallet, create_new_wallet, restore_wallet_from_text, Deterministic_Wallet
 from .address_synchronizer import TX_HEIGHT_LOCAL
 from .mnemonic import Mnemonic
 from .lnutil import SENT, RECEIVED
@@ -755,31 +755,18 @@ class Commands:
         return json_normalize(wallet.get_detailed_history(**kwargs))
 
     @command('wp')
-    async def bumpfee(self, tx, new_fee_rate, from_coins=None, strategies=None, password=None, unsigned=False, wallet: Abstract_Wallet = None):
+    async def bumpfee(self, tx, new_fee_rate, from_coins=None, decrease_payment=False, password=None, unsigned=False, wallet: Abstract_Wallet = None):
         """ Bump the Fee for an unconfirmed Transaction """
         tx = Transaction(tx)
         domain_coins = from_coins.split(',') if from_coins else None
         coins = wallet.get_spendable_coins(None)
         if domain_coins is not None:
             coins = [coin for coin in coins if (coin.prevout.to_str() in domain_coins)]
-        strategies = strategies.split(',') if strategies else None
-        bumpfee_strategies = None
-        if strategies is not None:
-            bumpfee_strategies = []
-            for strategy in strategies:
-                if strategy == 'CoinChooser':
-                    bumpfee_strategies.append(BumpFeeStrategy.COINCHOOSER)
-                elif strategy == 'DecreaseChange':
-                    bumpfee_strategies.append(BumpFeeStrategy.DECREASE_CHANGE)
-                elif strategy == 'DecreasePayment':
-                    bumpfee_strategies.append(BumpFeeStrategy.DECREASE_PAYMENT)
-                else:
-                    raise Exception("Invalid Choice of Strategies")
         new_tx = wallet.bump_fee(
             tx=tx,
             txid=tx.txid(),
             coins=coins,
-            strategies=bumpfee_strategies,
+            decrease_payment=decrease_payment,
             new_fee_rate=new_fee_rate)
         if not unsigned:
             wallet.sign_transaction(new_tx, password)
@@ -1413,6 +1400,7 @@ command_options = {
     'privkey':     (None, "Private key. Set to '?' to get a prompt."),
     'unsigned':    ("-u", "Do not sign transaction"),
     'rbf':         (None, "Whether to signal opt-in Replace-By-Fee in the transaction (true/false)"),
+    'decrease_payment': (None, "Whether payment amount will be decreased (true/false)"),
     'locktime':    (None, "Set locktime block number"),
     'addtransaction': (None,'Whether transaction is to be used for broadcasting afterwards. Adds transaction to the wallet'),
     'domain':      ("-D", "List of addresses"),
@@ -1436,7 +1424,6 @@ command_options = {
     'gossip':      (None, "Apply command to gossip node instead of wallet"),
     'connection_string':      (None, "Lightning network node ID or network address"),
     'new_fee_rate': (None, "The Updated/Increased Transaction fee rate (in sat/byte)"),
-    'strategies': (None, "Select RBF any one or multiple RBF strategies in any order, separated by ','; Options : 'CoinChooser','DecreaseChange','DecreasePayment' "),
     'from_amount': (None, "Amount to convert (default: 1)"),
     'from_ccy':    (None, "Currency to convert from"),
     'to_ccy':      (None, "Currency to convert to"),

electrum/gui/qt/rbf_dialog.py

@@ -14,7 +14,7 @@ from .util import (ColorScheme, WindowModalDialog, Buttons,
 
 from electrum.i18n import _
 from electrum.transaction import PartialTransaction
-from electrum.wallet import BumpFeeStrategy
+from electrum.wallet import CannotBumpFee
 
 if TYPE_CHECKING:
     from .main_window import ElectrumWindow
@@ -28,47 +28,30 @@ class _BaseRBFDialog(WindowModalDialog):
             main_window: 'ElectrumWindow',
             tx: PartialTransaction,
             txid: str,
-            title: str,
-            help_text: str,
-    ):
+            title: str):
+
         WindowModalDialog.__init__(self, main_window, title=title)
         self.window = main_window
         self.wallet = main_window.wallet
         self.tx = tx
+        self.new_tx = None
         assert txid
         self.txid = txid
+        self.message = ''
 
         fee = tx.get_fee()
         assert fee is not None
         tx_size = tx.estimated_size()
-        old_fee_rate = fee / tx_size  # sat/vbyte
+        self.old_fee_rate = old_fee_rate = fee / tx_size  # sat/vbyte
         vbox = QVBoxLayout(self)
-        vbox.addWidget(WWLabel(help_text))
-
-        ok_button = OkButton(self)
-        self.adv_button = QPushButton(_("Show advanced settings"))
-        self.adv_button.setEnabled(False)
-        self.adv_button.setVisible(False)
-        warning_label = WWLabel('\n')
-        warning_label.setStyleSheet(ColorScheme.RED.as_stylesheet())
+        vbox.addWidget(WWLabel(self.help_text))
+        vbox.addStretch(1)
+
+        self.ok_button = OkButton(self)
+        self.message_label = QLabel('')
         self.feerate_e = FeerateEdit(lambda: 0)
         self.feerate_e.setAmount(max(old_fee_rate * 1.5, old_fee_rate + 1))
-
-        def on_feerate():
-            fee_rate = self.feerate_e.get_amount()
-            warning_text = '\n'
-            if fee_rate is not None:
-                try:
-                    new_tx = self.rbf_func(fee_rate)
-                except Exception as e:
-                    new_tx = None
-                    warning_text = str(e).replace('\n', ' ')
-            else:
-                new_tx = None
-            ok_button.setEnabled(new_tx is not None)
-            warning_label.setText(warning_text)
-
-        self.feerate_e.textChanged.connect(on_feerate)
+        self.feerate_e.textChanged.connect(self.update)
 
         def on_slider(dyn, pos, fee_rate):
             fee_slider.activate()
@@ -81,77 +64,116 @@ class _BaseRBFDialog(WindowModalDialog):
         self.feerate_e.textEdited.connect(fee_slider.deactivate)
 
         grid = QGridLayout()
-        grid.addWidget(QLabel(_('Current Fee') + ':'), 0, 0)
-        grid.addWidget(QLabel(self.window.format_amount(fee) + ' ' + self.window.base_unit()), 0, 1)
-        grid.addWidget(QLabel(_('Current Fee rate') + ':'), 1, 0)
-        grid.addWidget(QLabel(self.window.format_fee_rate(1000 * old_fee_rate)), 1, 1)
-        grid.addWidget(QLabel(_('New Fee rate') + ':'), 2, 0)
-        grid.addWidget(self.feerate_e, 2, 1)
-        grid.addWidget(fee_slider, 3, 1)
-        grid.addWidget(fee_combo, 3, 2)
-        vbox.addLayout(grid)
-        self._add_advanced_options_cont(vbox)
-        vbox.addWidget(warning_label)
 
+        self.method_label = QLabel(_('Method') + ':')
+        self.method_combo = QComboBox()
+        self.method_combo.addItems([_('Preserve payment'), _('Decrease payment')])
+        self.method_combo.currentIndexChanged.connect(self.update)
+        grid.addWidget(self.method_label, 0, 0)
+        grid.addWidget(self.method_combo, 0, 1)
+
+        grid.addWidget(QLabel(_('Current fee') + ':'), 1, 0)
+        grid.addWidget(QLabel(self.window.format_amount_and_units(fee)), 1, 1)
+        grid.addWidget(QLabel(_('Current fee rate') + ':'), 2, 0)
+        grid.addWidget(QLabel(self.window.format_fee_rate(1000 * old_fee_rate)), 2, 1)
+
+        grid.addWidget(QLabel(_('New fee rate') + ':'), 3, 0)
+        grid.addWidget(self.feerate_e, 3, 1)
+        grid.addWidget(fee_slider, 3, 2)
+        grid.addWidget(fee_combo, 3, 3)
+        grid.addWidget(self.message_label, 5, 0, 1, 3)
+
+        vbox.addLayout(grid)
+        vbox.addStretch(1)
         btns_hbox = QHBoxLayout()
-        btns_hbox.addWidget(self.adv_button)
         btns_hbox.addStretch(1)
         btns_hbox.addWidget(CancelButton(self))
-        btns_hbox.addWidget(ok_button)
+        btns_hbox.addWidget(self.ok_button)
         vbox.addLayout(btns_hbox)
 
+        new_fee_rate = old_fee_rate + max(1, old_fee_rate // 20)
+        self.feerate_e.setAmount(new_fee_rate)
+        self._update_tx(new_fee_rate)
+        self._update_message()
+        # give focus to fee slider
+        fee_slider.activate()
+        fee_slider.setFocus()
+        # are we paying max?
+        invoices = self.wallet.get_relevant_invoices_for_tx(txid)
+        if len(invoices) == 1 and len(invoices[0].outputs) == 1:
+            if invoices[0].outputs[0].value == '!':
+                self.set_decrease_payment()
+
+    def is_decrease_payment(self):
+        return self.method_combo.currentIndex() == 1
+
+    def set_decrease_payment(self):
+        self.method_combo.setCurrentIndex(1)
+
     def rbf_func(self, fee_rate) -> PartialTransaction:
         raise NotImplementedError()  # implemented by subclasses
 
-    def _add_advanced_options_cont(self, vbox: QVBoxLayout) -> None:
-        adv_vbox = QVBoxLayout()
-        adv_vbox.setContentsMargins(0, 0, 0, 0)
-        adv_widget = QWidget()
-        adv_widget.setLayout(adv_vbox)
-        adv_widget.setVisible(False)
-        def show_adv_settings():
-            self.adv_button.setEnabled(False)
-            adv_widget.setVisible(True)
-        self.adv_button.clicked.connect(show_adv_settings)
-        self._add_advanced_options(adv_vbox)
-        vbox.addWidget(adv_widget)
-
-    def _add_advanced_options(self, adv_vbox: QVBoxLayout) -> None:
-        pass
-
     def run(self) -> None:
         if not self.exec_():
             return
-        new_fee_rate = self.feerate_e.get_amount()
-        try:
-            new_tx = self.rbf_func(new_fee_rate)
-        except Exception as e:
-            self.window.show_error(str(e))
-            return
-        new_tx.set_rbf(True)
+        self.new_tx.set_rbf(True)
         tx_label = self.wallet.get_label_for_txid(self.txid)
-        self.window.show_transaction(new_tx, tx_desc=tx_label)
+        self.window.show_transaction(self.new_tx, tx_desc=tx_label)
         # TODO maybe save tx_label as label for new tx??
 
+    def update(self):
+        fee_rate = self.feerate_e.get_amount()
+        self._update_tx(fee_rate)
+        self._update_message()
+
+    def _update_tx(self, fee_rate):
+        if fee_rate is None:
+            self.new_tx = None
+            self.message = ''
+        elif fee_rate <= self.old_fee_rate:
+            self.new_tx = None
+            self.message = _("The new fee rate needs to be higher than the old fee rate.")
+        else:
+            try:
+                self.new_tx = self.rbf_func(fee_rate)
+            except CannotBumpFee as e:
+                self.new_tx = None
+                self.message = str(e)
+        if not self.new_tx:
+            return
+        delta = self.new_tx.get_fee() - self.tx.get_fee()
+        if not self.is_decrease_payment():
+            self.message = _("You will pay {} more.").format(self.window.format_amount_and_units(delta))
+        else:
+            self.message = _("The recipient will receive {} less.").format(self.window.format_amount_and_units(delta))
+
+    def _update_message(self):
+        enabled = bool(self.new_tx)
+        self.ok_button.setEnabled(enabled)
+        if enabled:
+            style = ColorScheme.BLUE.as_stylesheet()
+        else:
+            style = ColorScheme.RED.as_stylesheet()
+        self.message_label.setStyleSheet(style)
+        self.message_label.setText(self.message)
+
 
 class BumpFeeDialog(_BaseRBFDialog):
 
+    help_text = _("Increase your transaction's fee to improve its position in mempool.")
+
     def __init__(
             self,
             *,
             main_window: 'ElectrumWindow',
             tx: PartialTransaction,
-            txid: str,
-    ):
-        help_text = _("Increase your transaction's fee to improve its position in mempool.")
+            txid: str):
         _BaseRBFDialog.__init__(
             self,
             main_window=main_window,
             tx=tx,
             txid=txid,
-            title=_('Bump Fee'),
-            help_text=help_text,
-        )
+            title=_('Bump Fee'))
 
     def rbf_func(self, fee_rate):
         return self.wallet.bump_fee(
@@ -159,52 +181,29 @@ class BumpFeeDialog(_BaseRBFDialog):
             txid=self.txid,
             new_fee_rate=fee_rate,
             coins=self.window.get_coins(),
-            strategies=self.option_index_to_strats[self.strat_combo.currentIndex()],
-        )
-
-    def _add_advanced_options(self, adv_vbox: QVBoxLayout) -> None:
-        self.adv_button.setVisible(True)
-        self.adv_button.setEnabled(True)
-        self.strat_combo = QComboBox()
-        options = [
-            _("decrease change, or add new inputs, or decrease any outputs"),
-            _("decrease change, or decrease any outputs"),
-            _("decrease payment"),
-        ]
-        self.option_index_to_strats = {
-            0: [BumpFeeStrategy.COINCHOOSER, BumpFeeStrategy.DECREASE_CHANGE],
-            1: [BumpFeeStrategy.DECREASE_CHANGE],
-            2: [BumpFeeStrategy.DECREASE_PAYMENT],
-        }
-        self.strat_combo.addItems(options)
-        self.strat_combo.setCurrentIndex(0)
-        strat_hbox = QHBoxLayout()
-        strat_hbox.addWidget(QLabel(_("Strategy") + ":"))
-        strat_hbox.addWidget(self.strat_combo)
-        strat_hbox.addStretch(1)
-        adv_vbox.addLayout(strat_hbox)
+            decrease_payment=self.is_decrease_payment())
 
 
 class DSCancelDialog(_BaseRBFDialog):
 
+    help_text = _(
+        "Cancel an unconfirmed transaction by replacing it with "
+        "a higher-fee transaction that spends back to your wallet.")
+
     def __init__(
             self,
             *,
             main_window: 'ElectrumWindow',
             tx: PartialTransaction,
-            txid: str,
-    ):
-        help_text = _(
-            "Cancel an unconfirmed RBF transaction by double-spending "
-            "its inputs back to your wallet with a higher fee.")
+            txid: str):
         _BaseRBFDialog.__init__(
             self,
             main_window=main_window,
             tx=tx,
             txid=txid,
-            title=_('Cancel transaction'),
-            help_text=help_text,
-        )
+            title=_('Cancel transaction'))
+        self.method_label.setVisible(False)
+        self.method_combo.setVisible(False)
 
     def rbf_func(self, fee_rate):
         return self.wallet.dscancel(tx=self.tx, new_fee_rate=fee_rate)

electrum/tests/test_wallet_vertical.py

@@ -12,7 +12,7 @@ from electrum import SimpleConfig
 from electrum import util
 from electrum.address_synchronizer import TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT
 from electrum.wallet import (sweep, Multisig_Wallet, Standard_Wallet, Imported_Wallet,
-                             restore_wallet_from_text, Abstract_Wallet, BumpFeeStrategy)
+                             restore_wallet_from_text, Abstract_Wallet)
 from electrum.util import (
     bfh, bh2u, NotEnoughFunds, UnrelatedTransactionException,
     UserFacingException)
@@ -1199,7 +1199,7 @@ class TestWalletSending(TestCaseForTestnet):
         tx = wallet.bump_fee(
             tx=tx_from_any(orig_rbf_tx.serialize()),
             new_fee_rate=60,
-            strategies=[BumpFeeStrategy.DECREASE_PAYMENT],
+            decrease_payment=True,
         )
         tx.locktime = 1936085
         tx.version = 2
@@ -1241,7 +1241,7 @@ class TestWalletSending(TestCaseForTestnet):
         tx = wallet.bump_fee(
             tx=tx_from_any(orig_rbf_tx.serialize()),
             new_fee_rate=60,
-            strategies=[BumpFeeStrategy.DECREASE_PAYMENT],
+            decrease_payment=True,
         )
         tx.locktime = 1936095
         tx.version = 2
@@ -1574,7 +1574,7 @@ class TestWalletSending(TestCaseForTestnet):
         self.assertEqual((0, 0, 0), wallet.get_balance())
 
         # bump tx
-        tx = wallet.bump_fee(tx=tx_from_any(tx.serialize()), new_fee_rate=70.0)
+        tx = wallet.bump_fee(tx=tx_from_any(tx.serialize()), new_fee_rate=70.0, decrease_payment=True)
         tx.locktime = 1325500
         tx.version = 1
         if simulate_moving_txs:

electrum/wallet.py

@@ -100,12 +100,6 @@ TX_STATUS = [
 ]
 
 
-class BumpFeeStrategy(enum.Enum):
-    COINCHOOSER = enum.auto()
-    DECREASE_CHANGE = enum.auto()
-    DECREASE_PAYMENT = enum.auto()
-
-
 async def _append_utxos_to_inputs(*, inputs: List[PartialTxInput], network: 'Network',
                                   pubkey: str, txin_type: str, imax: int) -> None:
     if txin_type in ('p2pkh', 'p2wpkh', 'p2wpkh-p2sh'):
@@ -1780,7 +1774,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             txid: str = None,
             new_fee_rate: Union[int, float, Decimal],
             coins: Sequence[PartialTxInput] = None,
-            strategies: Sequence[BumpFeeStrategy] = None,
+            decrease_payment=False,
     ) -> PartialTransaction:
         """Increase the miner fee of 'tx'.
         'new_fee_rate' is the target min rate in sat/vbyte
@@ -1808,41 +1802,28 @@ class Abstract_Wallet(ABC, Logger, EventListener):
         if new_fee_rate <= old_fee_rate:
             raise CannotBumpFee(_("The new fee rate needs to be higher than the old fee rate."))
 
-        if not strategies:
-            strategies = [BumpFeeStrategy.COINCHOOSER, BumpFeeStrategy.DECREASE_CHANGE]
-        tx_new = None
-        exc = None
-        for strat in strategies:
+        if not decrease_payment:
+            # FIXME: we should try decreasing change first,
+            # but it requires updating a bunch of unit tests
             try:
-                if strat == BumpFeeStrategy.COINCHOOSER:
-                    tx_new = self._bump_fee_through_coinchooser(
-                        tx=tx,
-                        txid=txid,
-                        new_fee_rate=new_fee_rate,
-                        coins=coins,
-                    )
-                elif strat == BumpFeeStrategy.DECREASE_CHANGE:
-                    tx_new = self._bump_fee_through_decreasing_change(
-                        tx=tx, new_fee_rate=new_fee_rate)
-                elif strat == BumpFeeStrategy.DECREASE_PAYMENT:
-                    tx_new = self._bump_fee_through_decreasing_payment(
-                        tx=tx, new_fee_rate=new_fee_rate)
-                else:
-                    raise NotImplementedError(f"unexpected strategy: {strat}")
+                tx_new = self._bump_fee_through_coinchooser(
+                    tx=tx,
+                    txid=txid,
+                    new_fee_rate=new_fee_rate,
+                    coins=coins,
+                )
             except CannotBumpFee as e:
-                exc = e
-            else:
-                strat_used = strat
-                break
-        if tx_new is None:
-            assert exc
-            raise exc  # all strategies failed, re-raise last exception
+                tx_new = self._bump_fee_through_decreasing_change(
+                    tx=tx, new_fee_rate=new_fee_rate)
+        else:
+            tx_new = self._bump_fee_through_decreasing_payment(
+                tx=tx, new_fee_rate=new_fee_rate)
 
         target_min_fee = new_fee_rate * tx_new.estimated_size()
         actual_fee = tx_new.get_fee()
         if actual_fee + 1 < target_min_fee:
             raise CannotBumpFee(
-                f"bump_fee fee target was not met (strategy: {strat_used}). "
+                f"bump_fee fee target was not met. "
                 f"got {actual_fee}, expected >={target_min_fee}. "
                 f"target rate was {new_fee_rate}")
         tx_new.locktime = get_locktime_for_new_transaction(self.network)
@@ -1920,10 +1901,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
 
         - keeps all inputs
         - no new inputs are added
-        - allows decreasing and removing outputs (change is decreased first)
-        This is less "safe" than "coinchooser" method as it might end up decreasing
-        e.g. a payment to a merchant; but e.g. if the user has sent "Max" previously,
-        this is the only way to RBF.
+        - change outputs are decreased or removed
         """
         tx = copy.deepcopy(tx)
         tx.add_info_from_wallet(self)
@@ -1933,11 +1911,8 @@ class Abstract_Wallet(ABC, Logger, EventListener):
 
         # use own outputs
         s = list(filter(lambda o: self.is_mine(o.address), outputs))
-        # ... unless there is none
-        if not s:
-            s = [out for out in outputs if self._is_rbf_allowed_to_touch_tx_output(out)]
         if not s:
-            raise CannotBumpFee('No outputs at all??')
+            raise CannotBumpFee('No suitable output')
 
         # prioritize low value outputs, to get rid of dust
         s = sorted(s, key=lambda o: o.value)
@@ -1973,12 +1948,13 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             tx: PartialTransaction,
             new_fee_rate: Union[int, Decimal],
     ) -> PartialTransaction:
-        """Increase the miner fee of 'tx'.
+        """
+        Increase the miner fee of 'tx' by decreasing amount paid.
+        This should be used for transactions that pay "Max".
 
         - keeps all inputs
         - no new inputs are added
-        - decreases payment outputs (not change!). Each non-ismine output is decreased
-          proportionally to their byte-size.
+        - Each non-ismine output is decreased proportionally to their byte-size.
         """
         tx = copy.deepcopy(tx)
         tx.add_info_from_wallet(self)