diff --git a/electrum/gui/kivy/uix/dialogs/tx_dialog.py b/electrum/gui/kivy/uix/dialogs/tx_dialog.py index 814185229..0487aa0c3 100644 --- a/electrum/gui/kivy/uix/dialogs/tx_dialog.py +++ b/electrum/gui/kivy/uix/dialogs/tx_dialog.py @@ -1,3 +1,4 @@ +import copy from datetime import datetime from typing import NamedTuple, Callable, TYPE_CHECKING @@ -16,10 +17,10 @@ from electrum.gui.kivy.i18n import _ from electrum.util import InvalidPassword from electrum.address_synchronizer import TX_HEIGHT_LOCAL from electrum.wallet import CannotBumpFee +from electrum.transaction import Transaction, PartialTransaction if TYPE_CHECKING: from ...main_window import ElectrumWindow - from electrum.transaction import Transaction Builder.load_string(''' @@ -159,6 +160,7 @@ class TxDialog(Factory.Popup): self.date_label = '' self.date_str = '' + self.can_sign = self.wallet.can_sign(self.tx) if amount is None: self.amount_str = _("Transaction unrelated to your wallet") elif amount > 0: @@ -167,14 +169,17 @@ class TxDialog(Factory.Popup): else: self.is_mine = True self.amount_str = format_amount(-amount) - if fee is not None: + risk_of_burning_coins = (isinstance(self.tx, PartialTransaction) + and self.can_sign + and fee is not None + and self.tx.is_there_risk_of_burning_coins_as_fees()) + if fee is not None and not risk_of_burning_coins: self.fee_str = format_amount(fee) fee_per_kb = fee / self.tx.estimated_size() * 1000 self.feerate_str = self.app.format_fee_rate(fee_per_kb) else: self.fee_str = _('unknown') self.feerate_str = _('unknown') - self.can_sign = self.wallet.can_sign(self.tx) self.ids.output_list.update(self.tx.outputs()) self.is_local_tx = tx_mined_status.height == TX_HEIGHT_LOCAL self.update_action_button() @@ -261,10 +266,15 @@ class TxDialog(Factory.Popup): def show_qr(self): from electrum.bitcoin import base_encode, bfh - raw_tx = self.tx.serialize() - text = bfh(raw_tx) + original_raw_tx = str(self.tx) + tx = copy.deepcopy(self.tx) # make copy as we mutate tx + if isinstance(tx, PartialTransaction): + # this makes QR codes a lot smaller (or just possible in the first place!) + tx.convert_all_utxos_to_witness_utxos() + + text = tx.serialize_as_bytes() text = base_encode(text, base=43) - self.app.qr_dialog(_("Raw Transaction"), text, text_for_clipboard=raw_tx) + self.app.qr_dialog(_("Raw Transaction"), text, text_for_clipboard=original_raw_tx) def remove_local_tx(self): txid = self.tx.txid() diff --git a/electrum/gui/qt/transaction_dialog.py b/electrum/gui/qt/transaction_dialog.py index 2c59c4a96..951069875 100644 --- a/electrum/gui/qt/transaction_dialog.py +++ b/electrum/gui/qt/transaction_dialog.py @@ -31,7 +31,7 @@ import time from typing import TYPE_CHECKING, Callable from PyQt5.QtCore import QSize, Qt -from PyQt5.QtGui import QTextCharFormat, QBrush, QFont +from PyQt5.QtGui import QTextCharFormat, QBrush, QFont, QPixmap from PyQt5.QtWidgets import (QDialog, QLabel, QPushButton, QHBoxLayout, QVBoxLayout, QTextEdit, QFrame, QAction, QToolButton, QMenu) import qrcode @@ -45,8 +45,9 @@ from electrum.util import bfh from electrum.transaction import SerializationError, Transaction, PartialTransaction, PartialTxInput from electrum.logging import get_logger -from .util import (MessageBoxMixin, read_QIcon, Buttons, CopyButton, - MONOSPACE_FONT, ColorScheme, ButtonsLineEdit, text_dialog) +from .util import (MessageBoxMixin, read_QIcon, Buttons, CopyButton, icon_path, + MONOSPACE_FONT, ColorScheme, ButtonsLineEdit, text_dialog, + char_width_in_lineedit) if TYPE_CHECKING: from .main_window import ElectrumWindow @@ -241,6 +242,10 @@ class TxDialog(QDialog, MessageBoxMixin): def show_qr(self, *, tx: Transaction = None): if tx is None: tx = self.tx + tx = copy.deepcopy(tx) # make copy as we mutate tx + if isinstance(tx, PartialTransaction): + # this makes QR codes a lot smaller (or just possible in the first place!) + tx.convert_all_utxos_to_witness_utxos() text = tx.serialize_as_bytes() text = base_encode(text, base=43) try: @@ -389,6 +394,10 @@ class TxDialog(QDialog, MessageBoxMixin): feerate_warning = simple_config.FEERATE_WARNING_HIGH_FEE if fee_rate > feerate_warning: fee_str += ' - ' + _('Warning') + ': ' + _("high fee") + '!' + if isinstance(self.tx, PartialTransaction): + risk_of_burning_coins = (can_sign and fee is not None + and self.tx.is_there_risk_of_burning_coins_as_fees()) + self.fee_warning_icon.setVisible(risk_of_burning_coins) self.amount_label.setText(amount_str) self.fee_label.setText(fee_str) self.size_label.setText(size_str) @@ -464,8 +473,24 @@ class TxDialog(QDialog, MessageBoxMixin): vbox_left.addWidget(self.date_label) self.amount_label = TxDetailLabel() vbox_left.addWidget(self.amount_label) + + fee_hbox = QHBoxLayout() self.fee_label = TxDetailLabel() - vbox_left.addWidget(self.fee_label) + fee_hbox.addWidget(self.fee_label) + self.fee_warning_icon = QLabel() + pixmap = QPixmap(icon_path("warning")) + pixmap_size = round(2 * char_width_in_lineedit()) + pixmap = pixmap.scaled(pixmap_size, pixmap_size, Qt.KeepAspectRatio, Qt.SmoothTransformation) + self.fee_warning_icon.setPixmap(pixmap) + self.fee_warning_icon.setToolTip(_("Warning") + ": " + + _("The fee could not be verified. Signing non-segwit inputs is risky:\n" + "if this transaction was maliciously modified before you sign,\n" + "you might end up paying a higher mining fee than displayed.")) + self.fee_warning_icon.setVisible(False) + fee_hbox.addWidget(self.fee_warning_icon) + fee_hbox.addStretch(1) + vbox_left.addLayout(fee_hbox) + vbox_left.addStretch(1) hbox_stats.addLayout(vbox_left, 50) diff --git a/electrum/tests/test_psbt.py b/electrum/tests/test_psbt.py index f06afb4a9..77b3ea570 100644 --- a/electrum/tests/test_psbt.py +++ b/electrum/tests/test_psbt.py @@ -1,4 +1,5 @@ from pprint import pprint +import unittest from electrum import constants from electrum.transaction import (tx_from_any, PartialTransaction, BadHeaderMagic, UnexpectedEndOfStream, @@ -223,6 +224,7 @@ class TestInvalidPSBT(TestCaseForTestnet): class TestPSBTSignerChecks(TestCaseForTestnet): # test cases from BIP-0174 + @unittest.skip("the check this test is testing is intentionally disabled in transaction.py") def test_psbt_fails_signer_checks_001(self): # Case: A Witness UTXO is provided for a non-witness input with self.assertRaises(PSBTInputConsistencyFailure): diff --git a/electrum/transaction.py b/electrum/transaction.py index 7fe98c798..684dbb4f7 100644 --- a/electrum/transaction.py +++ b/electrum/transaction.py @@ -1057,7 +1057,10 @@ class PartialTxInput(TxInput, PSBTSection): if self.prevout.txid.hex() != self.utxo.txid(): raise PSBTInputConsistencyFailure(f"PSBT input validation: " f"If a non-witness UTXO is provided, its hash must match the hash specified in the prevout") - if for_signing: + # The following test is disabled, so we are willing to sign non-segwit inputs + # without verifying the input amount. This means, given a maliciously modified PSBT, + # for non-segwit inputs, we might end up burning coins as miner fees. + if for_signing and False: if not Transaction.is_segwit_input(self) and self.witness_utxo: raise PSBTInputConsistencyFailure(f"PSBT input validation: " f"If a witness UTXO is provided, no non-witness signature may be created") @@ -1265,6 +1268,11 @@ class PartialTxInput(TxInput, PSBTSection): else: self.witness_utxo = None + def convert_utxo_to_witness_utxo(self) -> None: + if self.utxo: + self.witness_utxo = self.utxo.outputs()[self.prevout.out_idx] + self.utxo = None # type: Optional[Transaction] + def is_native_segwit(self) -> Optional[bool]: """Whether this input is native segwit. None means inconclusive.""" if self._is_native_segwit is None: @@ -1807,6 +1815,33 @@ class PartialTransaction(Transaction): txout.bip32_paths.clear() txout._unknown.clear() + def convert_all_utxos_to_witness_utxos(self) -> None: + """Replaces all NON-WITNESS-UTXOs with WITNESS-UTXOs. + This will likely make an exported PSBT invalid spec-wise, + but it makes e.g. QR codes significantly smaller. + """ + for txin in self.inputs(): + txin.convert_utxo_to_witness_utxo() + + def is_there_risk_of_burning_coins_as_fees(self) -> bool: + """Returns whether there is risk of burning coins as fees if we sign. + + Note: + - legacy sighash does not commit to any input amounts + - BIP-0143 sighash only commits to the *corresponding* input amount + - BIP-taproot sighash commits to *all* input amounts + """ + for txin in self.inputs(): + # if we have full previous tx, we *know* the input amount + if txin.utxo: + continue + # if we have just the previous output, we only have guarantees if + # the sighash commits to this data + if txin.witness_utxo and Transaction.is_segwit_input(txin): + continue + return True + return False + def remove_signatures(self): for txin in self.inputs(): txin.part_sigs = {} diff --git a/electrum/wallet.py b/electrum/wallet.py index 8e258dc1e..85cca75cd 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -1252,7 +1252,7 @@ class Abstract_Wallet(AddressSynchronizer): # This could have happened if previously another wallet had put a NON-WITNESS UTXO for txin, # as they did not know if it was segwit. This switch is needed to interop with bitcoin core. if txin.utxo and Transaction.is_segwit_input(txin): - txin.witness_utxo = txin.utxo.outputs()[txin.prevout.out_idx] + txin.convert_utxo_to_witness_utxo() txin.ensure_there_is_only_one_utxo() def _learn_derivation_path_for_address_from_txinout(self, txinout: Union[PartialTxInput, PartialTxOutput],