psbt: allow insecure signing of legacy UTXOs without full previous tx

When "importing" a psbt, we accept witness utxos even for legacy inputs
(warning shown to user in gui).
When "exporting" a psbt, we follow the spec; except when exporting as a QR code,
in which case we include witness utxos for all inputs.
This makes QR codes for psbts with legacy inputs feasible, just like they
were before, with our custom tx serialization format (with the same risk,
of burning coins as miner fees).

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()

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)
 

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):

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 = {}

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],