From aa518c0ea52d7dba996210ce52c7ab3a55242de5 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Thu, 7 Nov 2019 02:40:10 +0100 Subject: [PATCH] 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 | 22 +++++++++---- electrum/gui/qt/transaction_dialog.py | 33 ++++++++++++++++--- electrum/tests/test_psbt.py | 2 ++ electrum/transaction.py | 37 +++++++++++++++++++++- electrum/wallet.py | 2 +- 5 files changed, 84 insertions(+), 12 deletions(-) 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],