wallet.bump_fee: the glorious return of BumpFeeStrategy :D

gui/qt/rbf_dialog.py (old) lines 57-64 were implementing logic that should not be part of GUI code.
Case in point, gui/qml/qetxfinalizer.py (old) lines 511-513 duplicated half of that logic but not the other half.
That logic is now moved to wallet.get_bumpfee_strategies_for_tx().

More context: a user on irc got confused when using the qml gui. They were sending "max" and wanted to bump_fee.
The qml gui selected the "preserve_payment" strategy by default, using which there was no solution, and the user
did not notice that the strategy can be altered (via the "method" dropdown). The qt gui had logic to select
"decrease_payment" by default in such a case (which does find a solution to bump) but this logic was not
duplicated in the qml gui.
Instead of duplicating the logic, this commit moves it to shared lib code.

electrum/commands.py

@@ -52,7 +52,7 @@ from .transaction import (Transaction, multisig_script, TxOutput, PartialTransac
 from . import transaction
 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
+from .wallet import Abstract_Wallet, create_new_wallet, restore_wallet_from_text, Deterministic_Wallet, BumpFeeStrategy
 from .address_synchronizer import TX_HEIGHT_LOCAL
 from .mnemonic import Mnemonic
 from .lnutil import SENT, RECEIVED
@@ -819,7 +819,7 @@ class Commands:
             tx=tx,
             txid=tx.txid(),
             coins=coins,
-            decrease_payment=decrease_payment,
+            strategy=BumpFeeStrategy.DECREASE_PAYMENT if decrease_payment else BumpFeeStrategy.PRESERVE_PAYMENT,
             new_fee_rate=new_fee_rate)
         if not unsigned:
             wallet.sign_transaction(new_tx, password)

electrum/gui/qml/components/RbfBumpFeeDialog.qml

@@ -62,15 +62,11 @@ ElDialog {
 
                     ElComboBox {
                         id: bumpMethodComboBox
-                        enabled: rbffeebumper.canChangeBumpMethod
 
                         textRole: 'text'
                         valueRole: 'value'
 
-                        model: [
-                            { text: qsTr('Preserve payment'), value: 'preserve_payment' },
-                            { text: qsTr('Decrease payment'), value: 'decrease_payment' }
-                        ]
+                        model: rbffeebumper.bumpMethodsAvailable
                         onCurrentValueChanged: {
                             if (activeFocus)
                                 rbffeebumper.bumpMethod = currentValue

electrum/gui/qml/qetxfinalizer.py

@@ -8,7 +8,7 @@ from electrum.logging import get_logger
 from electrum.i18n import _
 from electrum.transaction import PartialTxOutput, PartialTransaction, Transaction
 from electrum.util import NotEnoughFunds, profiler
-from electrum.wallet import CannotBumpFee, CannotDoubleSpendTx, CannotCPFP
+from electrum.wallet import CannotBumpFee, CannotDoubleSpendTx, CannotCPFP, BumpFeeStrategy
 from electrum.plugin import run_hook
 
 from .qewallet import QEWallet
@@ -460,13 +460,8 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin):
         self._oldfee_rate = 0
         self._orig_tx = None
         self._rbf = True
-        self._bump_method = 'preserve_payment'
-        self._can_change_bump_method = True
-
-    canChangeBumpMethodChanged = pyqtSignal()
-    @pyqtProperty(bool, notify=canChangeBumpMethodChanged)
-    def canChangeBumpMethod(self):
-        return self._can_change_bump_method
+        self._bump_method = BumpFeeStrategy.PRESERVE_PAYMENT.name
+        self._bump_methods_available = []
 
     oldfeeChanged = pyqtSignal()
     @pyqtProperty(QEAmount, notify=oldfeeChanged)
@@ -496,21 +491,26 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin):
         return self._bump_method
 
     @bumpMethod.setter
-    def bumpMethod(self, bumpmethod):
-        assert self._can_change_bump_method
+    def bumpMethod(self, bumpmethod: str) -> None:
         if self._bump_method != bumpmethod:
             self._bump_method = bumpmethod
             self.bumpMethodChanged.emit()
             self.update()
 
+    bumpMethodsAvailableChanged = pyqtSignal()
+    @pyqtProperty('QVariantList', notify=bumpMethodsAvailableChanged)
+    def bumpMethodsAvailable(self):
+        return self._bump_methods_available
+
     def get_tx(self):
         assert self._txid
         self._orig_tx = self._wallet.wallet.db.get_transaction(self._txid)
         assert self._orig_tx
 
-        if self._wallet.wallet.get_swap_by_funding_tx(self._orig_tx):
-            self._can_change_bump_method = False
-            self.canChangeBumpMethodChanged.emit()
+        strategies, def_strat_idx = self._wallet.wallet.get_bumpfee_strategies_for_tx(tx=self._orig_tx, txid=self._txid)
+        self._bump_methods_available = [{'value': strat.name, 'text': strat.text()} for strat in strategies]
+        self.bumpMethodsAvailableChanged.emit()
+        self.bumpMethod = strategies[def_strat_idx].name
 
         if not isinstance(self._orig_tx, PartialTransaction):
             self._orig_tx = PartialTransaction.from_tx(self._orig_tx)
@@ -547,7 +547,7 @@ class QETxRbfFeeBumper(TxFeeSlider, TxMonMixin):
                 tx=self._orig_tx,
                 txid=self._txid,
                 new_fee_rate=new_fee_rate,
-                decrease_payment=self._bump_method=='decrease_payment'
+                strategy=BumpFeeStrategy[self._bump_method],
             )
         except CannotBumpFee as e:
             self._valid = False

electrum/gui/qt/rbf_dialog.py

@@ -15,7 +15,7 @@ from .util import (ColorScheme, WindowModalDialog, Buttons,
 
 from electrum.i18n import _
 from electrum.transaction import PartialTransaction
-from electrum.wallet import CannotRBFTx
+from electrum.wallet import CannotRBFTx, BumpFeeStrategy
 
 if TYPE_CHECKING:
     from .main_window import ElectrumWindow
@@ -54,19 +54,13 @@ class _BaseRBFDialog(TxEditor):
         self.feerate_e.setAmount(new_fee_rate)
         self.update()
         self.fee_slider.deactivate()
-        # 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()
-        # do not decrease payment if it is a swap
-        if self.wallet.get_swap_by_funding_tx(self.old_tx):
-            self.method_combo.setEnabled(False)
 
     def create_grid(self):
         self.method_label = QLabel(_('Method') + ':')
         self.method_combo = QComboBox()
-        self.method_combo.addItems([_('Preserve payment'), _('Decrease payment')])
+        self._strategies, def_strat_idx = self.wallet.get_bumpfee_strategies_for_tx(tx=self.old_tx, txid=self.old_txid)
+        self.method_combo.addItems([strat.text() for strat in self._strategies])
+        self.method_combo.setCurrentIndex(def_strat_idx)
         self.method_combo.currentIndexChanged.connect(self.trigger_update)
         self.method_combo.setFocusPolicy(Qt.NoFocus)
         old_size_label = TxSizeLabel()
@@ -93,12 +87,6 @@ class _BaseRBFDialog(TxEditor):
         grid.addWidget(self.locktime_e, 5, 1, 1, 2)
         return grid
 
-    def is_decrease_payment(self):
-        return self.method_combo.currentIndex() == 1
-
-    def set_decrease_payment(self):
-        self.method_combo.setCurrentIndex(1)
-
     def run(self) -> None:
         if not self.exec_():
             return
@@ -133,10 +121,12 @@ class _BaseRBFDialog(TxEditor):
         if not self.tx:
             return
         delta = self.tx.get_fee() - self.old_tx.get_fee()
-        if not self.is_decrease_payment():
+        if self._strategies[self.method_combo.currentIndex()] == BumpFeeStrategy.PRESERVE_PAYMENT:
             msg = _("You will pay {} more.").format(self.main_window.format_amount_and_units(delta))
-        else:
+        elif self._strategies[self.method_combo.currentIndex()] == BumpFeeStrategy.DECREASE_PAYMENT:
             msg = _("The recipient will receive {} less.").format(self.main_window.format_amount_and_units(delta))
+        else:
+            raise Exception(f"unknown strategy: {self=}")
         messages.insert(0, msg)
         return messages
 
@@ -164,7 +154,8 @@ class BumpFeeDialog(_BaseRBFDialog):
             txid=self.old_txid,
             new_fee_rate=fee_rate,
             coins=self.main_window.get_coins(nonlocal_only=True, confirmed_only=confirmed_only),
-            decrease_payment=self.is_decrease_payment())
+            strategy=self._strategies[self.method_combo.currentIndex()],
+        )
 
 
 class DSCancelDialog(_BaseRBFDialog):

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, CannotBumpFee)
+                             restore_wallet_from_text, Abstract_Wallet, CannotBumpFee, BumpFeeStrategy)
 from electrum.util import (
     bfh, NotEnoughFunds, UnrelatedTransactionException,
     UserFacingException)
@@ -1229,7 +1229,7 @@ class TestWalletSending(ElectrumTestCase):
         tx = wallet.bump_fee(
             tx=tx_from_any(orig_rbf_tx.serialize()),
             new_fee_rate=60,
-            decrease_payment=True,
+            strategy=BumpFeeStrategy.DECREASE_PAYMENT,
         )
         tx.locktime = 1936085
         tx.version = 2
@@ -1271,7 +1271,7 @@ class TestWalletSending(ElectrumTestCase):
         tx = wallet.bump_fee(
             tx=tx_from_any(orig_rbf_tx.serialize()),
             new_fee_rate=60,
-            decrease_payment=True,
+            strategy=BumpFeeStrategy.DECREASE_PAYMENT,
         )
         tx.locktime = 1936095
         tx.version = 2
@@ -1313,19 +1313,19 @@ class TestWalletSending(ElectrumTestCase):
             tx = wallet.bump_fee(
                 tx=tx_from_any(orig_rbf_tx.serialize()),
                 new_fee_rate=99999,
-                decrease_payment=True,
+                strategy=BumpFeeStrategy.DECREASE_PAYMENT,
             )
         with self.assertRaises(CannotBumpFee):
             tx = wallet.bump_fee(
                 tx=tx_from_any(orig_rbf_tx.serialize()),
                 new_fee_rate=99999,
-                decrease_payment=False,
+                strategy=BumpFeeStrategy.PRESERVE_PAYMENT,
             )
 
         tx = wallet.bump_fee(
             tx=tx_from_any(orig_rbf_tx.serialize()),
             new_fee_rate=60,
-            decrease_payment=True,
+            strategy=BumpFeeStrategy.DECREASE_PAYMENT,
         )
         tx.locktime = 1936085
         tx.version = 2
@@ -1647,7 +1647,7 @@ class TestWalletSending(ElectrumTestCase):
         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, decrease_payment=True)
+        tx = wallet.bump_fee(tx=tx_from_any(tx.serialize()), new_fee_rate=70.0, strategy=BumpFeeStrategy.DECREASE_PAYMENT)
         tx.locktime = 1325500
         tx.version = 1
         if simulate_moving_txs:

electrum/wallet.py

@@ -247,6 +247,23 @@ class InternalAddressCorruption(Exception):
                  "Please restore your wallet from seed, and compare the addresses in both files")
 
 
+class BumpFeeStrategy(enum.Enum):
+    PRESERVE_PAYMENT = enum.auto()
+    DECREASE_PAYMENT = enum.auto()
+
+    @classmethod
+    def all(cls) -> Sequence['BumpFeeStrategy']:
+        return list(BumpFeeStrategy.__members__.values())
+
+    def text(self) -> str:
+        if self == self.PRESERVE_PAYMENT:
+            return _('Preserve payment')
+        elif self == self.DECREASE_PAYMENT:
+            return _('Decrease payment')
+        else:
+            raise Exception(f"unknown strategy: {self=}")
+
+
 class ReceiveRequestHelp(NamedTuple):
     # help texts (warnings/errors):
     address_help: str
@@ -1955,6 +1972,28 @@ class Abstract_Wallet(ABC, Logger, EventListener):
     def can_export(self):
         return not self.is_watching_only() and hasattr(self.keystore, 'get_private_key')
 
+    def get_bumpfee_strategies_for_tx(
+        self,
+        *,
+        tx: Transaction,
+        txid: str = None,
+    ) -> Tuple[Sequence[BumpFeeStrategy], int]:
+        """Returns tuple(list of available strategies, idx of recommended option among those)."""
+        txid = txid or tx.txid()
+        assert txid
+        assert tx.txid() in (None, txid)
+        all_strats = BumpFeeStrategy.all()
+        # are we paying max?
+        invoices = self.get_relevant_invoices_for_tx(txid)
+        if len(invoices) == 1 and len(invoices[0].outputs) == 1:
+            if invoices[0].outputs[0].value == '!':
+                return all_strats, all_strats.index(BumpFeeStrategy.DECREASE_PAYMENT)
+        # do not decrease payment if it is a swap
+        if self.get_swap_by_funding_tx(tx):
+            return [BumpFeeStrategy.PRESERVE_PAYMENT], 0
+        # default
+        return all_strats, all_strats.index(BumpFeeStrategy.PRESERVE_PAYMENT)
+
     def bump_fee(
             self,
             *,
@@ -1962,7 +2001,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             txid: str = None,
             new_fee_rate: Union[int, float, Decimal],
             coins: Sequence[PartialTxInput] = None,
-            decrease_payment=False,
+            strategy: BumpFeeStrategy = BumpFeeStrategy.PRESERVE_PAYMENT,
     ) -> PartialTransaction:
         """Increase the miner fee of 'tx'.
         'new_fee_rate' is the target min rate in sat/vbyte
@@ -1991,7 +2030,7 @@ 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 decrease_payment:
+        if strategy == BumpFeeStrategy.PRESERVE_PAYMENT:
             # FIXME: we should try decreasing change first,
             # but it requires updating a bunch of unit tests
             try:
@@ -2004,9 +2043,11 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             except CannotBumpFee as e:
                 tx_new = self._bump_fee_through_decreasing_change(
                     tx=tx, new_fee_rate=new_fee_rate)
-        else:
+        elif strategy == BumpFeeStrategy.DECREASE_PAYMENT:
             tx_new = self._bump_fee_through_decreasing_payment(
                 tx=tx, new_fee_rate=new_fee_rate)
+        else:
+            raise Exception(f"unknown strategy: {strategy=}")
 
         target_min_fee = new_fee_rate * tx_new.estimated_size()
         actual_fee = tx_new.get_fee()