From a383f56909f1090ffc155a6dca7db0246cb564f0 Mon Sep 17 00:00:00 2001 From: ThomasV Date: Mon, 26 Sep 2022 14:40:28 +0200 Subject: [PATCH] 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 | 21 +-- electrum/gui/qt/rbf_dialog.py | 207 ++++++++++++------------- electrum/tests/test_wallet_vertical.py | 8 +- electrum/wallet.py | 68 +++----- 4 files changed, 133 insertions(+), 171 deletions(-) diff --git a/electrum/commands.py b/electrum/commands.py index 79fb051e2..4cf99b205 100644 --- a/electrum/commands.py +++ b/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"), diff --git a/electrum/gui/qt/rbf_dialog.py b/electrum/gui/qt/rbf_dialog.py index bf4df3b27..f803d2910 100644 --- a/electrum/gui/qt/rbf_dialog.py +++ b/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) diff --git a/electrum/tests/test_wallet_vertical.py b/electrum/tests/test_wallet_vertical.py index 62c58a598..a7f3beace 100644 --- a/electrum/tests/test_wallet_vertical.py +++ b/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: diff --git a/electrum/wallet.py b/electrum/wallet.py index 0d5bc6332..6795e5949 100644 --- a/electrum/wallet.py +++ b/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)