From bb8c73cabdcf2865a804cf97d946d6a46cce62e1 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 10 Jul 2023 18:16:56 +0000 Subject: [PATCH] qt: kind of fix bip70 notify_merchant logic by passing around PI ``` 229.18 | E | gui.qt.main_window.[test_segwit_2] | on_error Traceback (most recent call last): File "...\electrum\gui\qt\util.py", line 917, in run result = task.task() File "...\electrum\gui\qt\send_tab.py", line 681, in broadcast_thread if self.payto_e.payment_identifier.has_expired(): AttributeError: 'NoneType' object has no attribute 'has_expired' ``` In SendTab.broadcast_transaction.broadcast_thread, self.payto_e.payment_identifier was referenced - but do_clear() has already cleared it by then. E.g. consider SendTab.pay_onchain_dialog: it calls save_pending_invoice(), which calls do_clear(), and later (in sign_done), it calls window.broadcast_or_show, which will call SendTab.broadcast_transaction(). As there might be multiple independent transaction dialogs open simultaneously, the single shared state send_tab.payto_e.payment_identifier approach was problematic -- I think it is conceptually nicer to pass around the payment_identifiers as needed, as done with this change. However, this change is not a full proper fix, as it still somewhat relies on send_tab.payto_e.payment_identifier (e.g. in pay_onchain_dialog). Hence, e.g. when using the invoice_list context menu "Pay..." item, as payto_e.payment_identifier is not set, payment_identifier will be None in broadcast_transaction. but at least we handle PI being None gracefully -- before this change, broadcast_transaction expected PI to be set, and it was never set to the correct thing (as do_clear() already ran by then): depending on timing it was either None or a new empty PI. In the former case, producing the above traceback and hard failing (not only for bip70 stuff!), and in the latter, silently ignoring the logic bug. --- electrum/gui/qt/main_window.py | 22 ++++++++++++++-------- electrum/gui/qt/paytoedit.py | 4 ++-- electrum/gui/qt/send_tab.py | 26 ++++++++++++++++---------- electrum/gui/qt/transaction_dialog.py | 23 ++++++++++++++++++++--- 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py index d79973bd4..71f652417 100644 --- a/electrum/gui/qt/main_window.py +++ b/electrum/gui/qt/main_window.py @@ -1062,8 +1062,14 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener): from .channel_details import ChannelDetailsDialog ChannelDetailsDialog(self, chan).show() - def show_transaction(self, tx: Transaction, *, external_keypairs=None): - show_transaction(tx, parent=self, external_keypairs=external_keypairs) + def show_transaction( + self, + tx: Transaction, + *, + external_keypairs=None, + payment_identifier: PaymentIdentifier = None, + ): + show_transaction(tx, parent=self, external_keypairs=external_keypairs, payment_identifier=payment_identifier) def show_lightning_transaction(self, tx_item): from .lightning_tx_dialog import LightningTxDialog @@ -1208,18 +1214,18 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener): """ return self.utxo_list.get_spend_list() - def broadcast_or_show(self, tx: Transaction): + def broadcast_or_show(self, tx: Transaction, *, payment_identifier: PaymentIdentifier = None): if not tx.is_complete(): - self.show_transaction(tx) + self.show_transaction(tx, payment_identifier=payment_identifier) return if not self.network: self.show_error(_("You can't broadcast a transaction without a live network connection.")) - self.show_transaction(tx) + self.show_transaction(tx, payment_identifier=payment_identifier) return - self.broadcast_transaction(tx) + self.broadcast_transaction(tx, payment_identifier=payment_identifier) - def broadcast_transaction(self, tx: Transaction): - self.send_tab.broadcast_transaction(tx) + def broadcast_transaction(self, tx: Transaction, *, payment_identifier: PaymentIdentifier = None): + self.send_tab.broadcast_transaction(tx, payment_identifier=payment_identifier) @protected def sign_tx(self, tx, *, callback, external_keypairs, password): diff --git a/electrum/gui/qt/paytoedit.py b/electrum/gui/qt/paytoedit.py index 3fb8f659f..0ffe02314 100644 --- a/electrum/gui/qt/paytoedit.py +++ b/electrum/gui/qt/paytoedit.py @@ -173,7 +173,7 @@ class PayToEdit(QWidget, Logger, GenericInputHandler): self.edit_timer.setInterval(1000) self.edit_timer.timeout.connect(self._on_edit_timer) - self.payment_identifier = None + self.payment_identifier = None # type: Optional[PaymentIdentifier] @property def multiline(self): @@ -206,8 +206,8 @@ class PayToEdit(QWidget, Logger, GenericInputHandler): self.line_edit.setToolTip(tt) self.text_edit.setToolTip(tt) - '''set payment identifier only if valid, else exception''' def try_payment_identifier(self, text) -> None: + '''set payment identifier only if valid, else exception''' text = text.strip() pi = PaymentIdentifier(self.send_tab.wallet, text) if not pi.is_valid(): diff --git a/electrum/gui/qt/send_tab.py b/electrum/gui/qt/send_tab.py index 2dfe8be09..b0d1f1f8f 100644 --- a/electrum/gui/qt/send_tab.py +++ b/electrum/gui/qt/send_tab.py @@ -17,7 +17,7 @@ from electrum.util import NotEnoughFunds, NoDynamicFeeEstimates, parse_max_spend from electrum.invoices import PR_PAID, Invoice, PR_BROADCASTING, PR_BROADCAST from electrum.transaction import Transaction, PartialTxInput, PartialTxOutput from electrum.network import TxBroadcastError, BestEffortRequestFailed -from electrum.payment_identifier import PaymentIdentifierState, PaymentIdentifierType +from electrum.payment_identifier import PaymentIdentifierState, PaymentIdentifierType, PaymentIdentifier from .amountedit import AmountEdit, BTCAmountEdit, SizedFreezableLineEdit from .paytoedit import InvalidPaymentIdentifier @@ -268,7 +268,8 @@ class SendTab(QWidget, MessageBoxMixin, Logger): def pay_onchain_dialog( self, - outputs: List[PartialTxOutput], *, + outputs: List[PartialTxOutput], + *, nonlocal_only=False, external_keypairs=None, get_coins: Callable[..., Sequence[PartialTxInput]] = None, @@ -276,6 +277,9 @@ class SendTab(QWidget, MessageBoxMixin, Logger): # trustedcoin requires this if run_hook('abort_send', self): return + # save current PI as local now. this is best-effort only... + # does not work e.g. when using InvoiceList context menu "pay" + payment_identifier = self.payto_e.payment_identifier is_sweep = bool(external_keypairs) # we call get_coins inside make_tx, so that inputs can be changed dynamically if get_coins is None: @@ -302,12 +306,12 @@ class SendTab(QWidget, MessageBoxMixin, Logger): return is_preview = conf_dlg.is_preview if is_preview: - self.window.show_transaction(tx, external_keypairs=external_keypairs) + self.window.show_transaction(tx, external_keypairs=external_keypairs, payment_identifier=payment_identifier) return self.save_pending_invoice() def sign_done(success): if success: - self.window.broadcast_or_show(tx) + self.window.broadcast_or_show(tx, payment_identifier=payment_identifier) self.window.sign_tx( tx, callback=sign_done, @@ -527,7 +531,7 @@ class SendTab(QWidget, MessageBoxMixin, Logger): outputs += invoice.outputs self.pay_onchain_dialog(outputs) - def do_edit_invoice(self, invoice: 'Invoice'): + def do_edit_invoice(self, invoice: 'Invoice'): # FIXME broken assert not bool(invoice.get_amount_sat()) text = invoice.lightning_invoice if invoice.is_lightning() else invoice.get_address() self.payto_e._on_input_btn(text) @@ -674,11 +678,13 @@ class SendTab(QWidget, MessageBoxMixin, Logger): coro = lnworker.pay_invoice(invoice.lightning_invoice, amount_msat=amount_msat) self.window.run_coroutine_from_thread(coro, _('Sending payment')) - def broadcast_transaction(self, tx: Transaction): + def broadcast_transaction(self, tx: Transaction, *, payment_identifier: PaymentIdentifier = None): + # note: payment_identifier is explicitly passed as self.payto_e.payment_identifier might + # already be cleared or otherwise have changed. def broadcast_thread(): # non-GUI thread - if self.payto_e.payment_identifier.has_expired(): + if payment_identifier and payment_identifier.has_expired(): return False, _("Invoice has expired") try: self.network.run_from_another_thread(self.network.broadcast_transaction(tx)) @@ -688,9 +694,9 @@ class SendTab(QWidget, MessageBoxMixin, Logger): return False, repr(e) # success txid = tx.txid() - if self.payto_e.payment_identifier.need_merchant_notify(): + if payment_identifier and payment_identifier.need_merchant_notify(): refund_address = self.wallet.get_receiving_address() - self.payto_e.payment_identifier.notify_merchant( + payment_identifier.notify_merchant( tx=tx, refund_address=refund_address, on_finished=self.notify_merchant_done_signal.emit @@ -718,7 +724,7 @@ class SendTab(QWidget, MessageBoxMixin, Logger): WaitingDialog(self, _('Broadcasting transaction...'), broadcast_thread, broadcast_done, self.window.on_error) - def on_notify_merchant_done(self, pi): + def on_notify_merchant_done(self, pi: PaymentIdentifier): if pi.is_error(): self.logger.debug(f'merchant notify error: {pi.get_error()}') else: diff --git a/electrum/gui/qt/transaction_dialog.py b/electrum/gui/qt/transaction_dialog.py index 579a08e25..b6a483d24 100644 --- a/electrum/gui/qt/transaction_dialog.py +++ b/electrum/gui/qt/transaction_dialog.py @@ -70,6 +70,7 @@ from .my_treeview import create_toolbar_with_menu if TYPE_CHECKING: from .main_window import ElectrumWindow from electrum.wallet import Abstract_Wallet + from electrum.payment_identifier import PaymentIdentifier _logger = get_logger(__name__) @@ -378,9 +379,16 @@ def show_transaction( parent: 'ElectrumWindow', prompt_if_unsaved: bool = False, external_keypairs=None, + payment_identifier: 'PaymentIdentifier' = None, ): try: - d = TxDialog(tx, parent=parent, prompt_if_unsaved=prompt_if_unsaved, external_keypairs=external_keypairs) + d = TxDialog( + tx, + parent=parent, + prompt_if_unsaved=prompt_if_unsaved, + external_keypairs=external_keypairs, + payment_identifier=payment_identifier, + ) except SerializationError as e: _logger.exception('unable to deserialize the transaction') parent.show_critical(_("Electrum was unable to deserialize the transaction:") + "\n" + str(e)) @@ -392,7 +400,15 @@ class TxDialog(QDialog, MessageBoxMixin): throttled_update_sig = pyqtSignal() # emit from thread to do update in main thread - def __init__(self, tx: Transaction, *, parent: 'ElectrumWindow', prompt_if_unsaved: bool, external_keypairs=None): + def __init__( + self, + tx: Transaction, + *, + parent: 'ElectrumWindow', + prompt_if_unsaved: bool, + external_keypairs=None, + payment_identifier: 'PaymentIdentifier' = None, + ): '''Transactions in the wallet will show their description. Pass desc to give a description for txs not yet in the wallet. ''' @@ -403,6 +419,7 @@ class TxDialog(QDialog, MessageBoxMixin): self.main_window = parent self.config = parent.config self.wallet = parent.wallet + self.payment_identifier = payment_identifier self.prompt_if_unsaved = prompt_if_unsaved self.saved = False self.desc = None @@ -537,7 +554,7 @@ class TxDialog(QDialog, MessageBoxMixin): self.main_window.push_top_level_window(self) self.main_window.send_tab.save_pending_invoice() try: - self.main_window.broadcast_transaction(self.tx) + self.main_window.broadcast_transaction(self.tx, payment_identifier=self.payment_identifier) finally: self.main_window.pop_top_level_window(self) self.saved = True