AaronFiore/electrum
1
0
Fork 0
Code Activity

wallet: add sighash check to class Abstract_Wallet

Browse Source 
qml: use backend sighash check and add user confirmation path
qt: use backend sighash check and add user confirmation path
qml: include get_warning_for_risk_of_burning_coins_as_fees test in txdetails
qt: add warning icon to sighash warning
add sighash and fee checks to wallet.sign_transaction, making all warnings fatal unless ignore_warnings is set to True
tests: test sign_transaction on both code paths with ignore_warnings True and False,
raise specific exceptions TransactionPotentiallyDangerousException and TransactionDangerousException
This commit is contained in:
Sander van Grieken
2023-11-20 15:23:49 +01:00
committed by accumulator
parent 6467db0b7d
commit 7b96a83350
9 changed files with 167 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
electrum/commands.py
View File

@@ -469,10 +469,10 @@ class Commands:
return tx.serialize()
@command('wp')
async def signtransaction(self, tx, password=None, wallet: Abstract_Wallet = None):
async def signtransaction(self, tx, password=None, wallet: Abstract_Wallet = None, iknowwhatimdoing: bool=False):
"""Sign a transaction. The wallet keys will be used to sign the transaction."""
tx = tx_from_any(tx)
wallet.sign_transaction(tx, password)
wallet.sign_transaction(tx, password, ignore_warnings=iknowwhatimdoing)
return tx.serialize()
@command('')

25
electrum/gui/qml/components/TxDetails.qml
View File

@@ -49,6 +49,16 @@ Pane {
text: qsTr('On-chain Transaction')
}
InfoTextArea {
id: warn
Layout.columnSpan: 2
Layout.fillWidth: true
Layout.bottomMargin: constants.paddingLarge
visible: txdetails.warning
text: txdetails.warning
iconStyle: InfoTextArea.IconStyle.Warn
}
InfoTextArea {
id: bumpfeeinfo
Layout.columnSpan: 2
@@ -336,7 +346,20 @@ Pane {
icon.source: '../../icons/key.png'
text: qsTr('Sign')
visible: txdetails.canSign
onClicked: txdetails.sign()
onClicked: {
if (txdetails.shouldConfirm) {
var dialog = app.messageDialog.createObject(app, {
text: qsTr('Confirm signing non-standard transaction?'),
yesno: true
})
dialog.accepted.connect(function() {
txdetails.sign()
})
dialog.open()
} else {
txdetails.sign()
}
}
}
FlatButton {

27
electrum/gui/qml/qetxdetails.py
View File

@@ -5,7 +5,7 @@ from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject
from electrum.i18n import _
from electrum.logging import get_logger
from electrum.util import format_time, TxMinedInfo
from electrum.transaction import tx_from_any, Transaction
from electrum.transaction import tx_from_any, Transaction, PartialTxInput, Sighash, PartialTransaction
from electrum.network import Network
from electrum.address_synchronizer import TX_HEIGHT_UNCONF_PARENT, TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_FUTURE
@@ -32,6 +32,7 @@ class QETxDetails(QObject, QtEventListener):
self._txid = ''
self._rawtx = ''
self._label = ''
self._warning = ''
self._tx = None # type: Optional[Transaction]
@@ -56,6 +57,7 @@ class QETxDetails(QObject, QtEventListener):
self._is_mined = False
self._is_final = False
self._lock_delay = 0
self._should_confirm = False
self._mempool_depth = ''
@@ -140,6 +142,10 @@ class QETxDetails(QObject, QtEventListener):
def status(self):
return self._status
@pyqtProperty(str, notify=detailsChanged)
def warning(self):
return self._warning
@pyqtProperty(QEAmount, notify=detailsChanged)
def amount(self):
return self._amount
@@ -240,6 +246,10 @@ class QETxDetails(QObject, QtEventListener):
def lockDelay(self):
return self._lock_delay
@pyqtProperty(bool, notify=detailsChanged)
def shouldConfirm(self):
return self._should_confirm
def update(self, from_txid: bool = False):
assert self._wallet
@@ -284,6 +294,9 @@ class QETxDetails(QObject, QtEventListener):
fee_per_kb = txinfo.fee / size * 1000
self._feerate_str = self._wallet.wallet.config.format_fee_rate(fee_per_kb)
self._should_confirm = False
should_reject = False
self._lock_delay = 0
self._is_mined = False if not txinfo.tx_mined_status else txinfo.tx_mined_status.height > 0
if self._is_mined:
@@ -293,6 +306,10 @@ class QETxDetails(QObject, QtEventListener):
self._mempool_depth = self._wallet.wallet.config.depth_tooltip(txinfo.mempool_depth_bytes)
elif txinfo.tx_mined_status.height == TX_HEIGHT_FUTURE:
self._lock_delay = txinfo.tx_mined_status.wanted_height - self._wallet.wallet.adb.get_local_height()
if isinstance(self._tx, PartialTransaction):
self._should_confirm, should_reject, message = self._wallet.wallet.check_sighash(self._tx)
if message:
self._warning = '\n'.join([_('Danger! This transaction is non-standard!'), message])
if self._wallet.wallet.lnworker:
# Calling lnworker.get_onchain_history and wallet.get_full_history here
@@ -320,7 +337,13 @@ class QETxDetails(QObject, QtEventListener):
self._can_cpfp = txinfo.can_cpfp and not txinfo.can_remove
self._can_save_as_local = txinfo.can_save_as_local and not txinfo.can_remove
self._can_remove = txinfo.can_remove
self._can_sign = not self._is_complete and self._wallet.wallet.can_sign(self._tx)
self._can_sign = not self._is_complete and self._wallet.wallet.can_sign(self._tx) and not should_reject
if isinstance(self._tx, PartialTransaction):
risk_of_burning_coins = (self._can_sign and txinfo.fee is not None
and self._wallet.wallet.get_warning_for_risk_of_burning_coins_as_fees(self._tx))
if risk_of_burning_coins:
self._warning = '\n'.join([self._warning, risk_of_burning_coins])
self.detailsChanged.emit()

3
electrum/gui/qml/qewallet.py
View File

@@ -512,7 +512,8 @@ class QEWallet(AuthMixin, QObject, QtEventListener):
def do_sign(self, tx, broadcast):
try:
tx = self.wallet.sign_transaction(tx, self.password)
# ignore_warnings=True, because UI checks and asks user confirmation itself
tx = self.wallet.sign_transaction(tx, self.password, ignore_warnings=True)
except BaseException as e:
self._logger.error(f'{e!r}')
self.signFailed.emit(str(e))

3
electrum/gui/qt/main_window.py
View File

@@ -1270,7 +1270,8 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener):
# can sign directly
task = partial(tx.sign, external_keypairs)
else:
task = partial(self.wallet.sign_transaction, tx, password)
# ignore_warnings=True, because UI checks and asks user confirmation itself
task = partial(self.wallet.sign_transaction, tx, password, ignore_warnings=True)
msg = _('Signing transaction...')
WaitingDialog(self, msg, task, on_success, on_failure)

41
electrum/gui/qt/transaction_dialog.py
View File

@@ -50,8 +50,7 @@ from electrum.bitcoin import base_encode, NLOCKTIME_BLOCKHEIGHT_MAX, DummyAddres
from electrum.i18n import _
from electrum.plugin import run_hook
from electrum import simple_config
from electrum.transaction import SerializationError, Transaction, PartialTransaction, PartialTxInput, TxOutpoint
from electrum.transaction import TxinDataFetchProgress
from electrum.transaction import SerializationError, Transaction, PartialTransaction, TxOutpoint, TxinDataFetchProgress
from electrum.logging import get_logger
from electrum.util import ShortID, get_asyncio_loop
from electrum.network import Network
@@ -77,15 +76,16 @@ _logger = get_logger(__name__)
dialogs = [] # Otherwise python randomly garbage collects the dialogs...
class TxSizeLabel(QLabel):
def setAmount(self, byte_size):
self.setText(('x %s bytes =' % byte_size) if byte_size else '')
class TxFiatLabel(QLabel):
def setAmount(self, fiat_fee):
self.setText(('≈ %s' % fiat_fee) if fiat_fee else '')
class QTextBrowserWithDefaultSize(QTextBrowser):
def __init__(self, width: int = 0, height: int = 0):
self._width = width
@@ -96,8 +96,8 @@ class QTextBrowserWithDefaultSize(QTextBrowser):
def sizeHint(self):
return QSize(self._width, self._height)
class TxInOutWidget(QWidget):
class TxInOutWidget(QWidget):
def __init__(self, main_window: 'ElectrumWindow', wallet: 'Abstract_Wallet'):
QWidget.__init__(self)
@@ -113,9 +113,24 @@ class TxInOutWidget(QWidget):
self.inputs_textedit.setContextMenuPolicy(Qt.CustomContextMenu)
self.inputs_textedit.customContextMenuRequested.connect(self.on_context_menu_for_inputs)
self.sighash_label = QLabel()
self.sighash_label.setStyleSheet('font-weight: bold')
self.sighash_confirm = False
self.sighash_reject = False
self.sighash_message = ''
self.inputs_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.inputs_warning_icon.setPixmap(pixmap)
self.inputs_warning_icon.setVisible(False)
self.inheader_hbox = QHBoxLayout()
self.inheader_hbox.setContentsMargins(0, 0, 0, 0)
self.inheader_hbox.addWidget(self.inputs_header)
self.inheader_hbox.addStretch(2)
self.inheader_hbox.addWidget(self.sighash_label)
self.inheader_hbox.addWidget(self.inputs_warning_icon)
self.txo_color_recv = TxOutputColoring(
legend=_("Wallet Address"), color=ColorScheme.GREEN, tooltip=_("Wallet receiving address"))
@@ -246,6 +261,13 @@ class TxInOutWidget(QWidget):
short_id=str(txin.short_id), addr=addr, value=txin_value,
)
if isinstance(self.tx, PartialTransaction):
self.sighash_confirm, self.sighash_reject, self.sighash_message = self.wallet.check_sighash(self.tx)
if self.sighash_message:
self.sighash_label.setText(_('Danger! This transaction is non-standard!'))
self.inputs_warning_icon.setVisible(True)
self.inputs_warning_icon.setToolTip(self.sighash_message)
self.outputs_header.setText(_("Outputs") + ' (%d)'%len(self.tx.outputs()))
o_text = self.outputs_textedit
o_text.clear()
@@ -645,6 +667,14 @@ class TxDialog(QDialog, MessageBoxMixin):
self.update()
self.main_window.pop_top_level_window(self)
if self.io_widget.sighash_confirm:
if not self.question('\n'.join([
_('Danger! This transaction is non-standard!'),
self.io_widget.sighash_message,
'',
_('Are you sure you want to sign this transaction?')
])):
return
self.sign_button.setDisabled(True)
self.main_window.push_top_level_window(self)
self.main_window.sign_tx(self.tx, callback=sign_done, external_keypairs=self.external_keypairs)
@@ -771,7 +801,8 @@ class TxDialog(QDialog, MessageBoxMixin):
self.broadcast_button.setEnabled(tx_details.can_broadcast)
can_sign = not self.tx.is_complete() and \
(self.wallet.can_sign(self.tx) or bool(self.external_keypairs))
self.sign_button.setEnabled(can_sign)
self.sign_button.setEnabled(can_sign and not self.io_widget.sighash_reject)
self.sign_button.setToolTip(self.io_widget.sighash_message)
if tx_details.txid:
self.tx_hash_e.setText(tx_details.txid)
else:

24
electrum/tests/test_wallet_vertical.py
View File

@@ -12,12 +12,10 @@ 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, CannotBumpFee, BumpFeeStrategy)
from electrum.util import (
bfh, NotEnoughFunds, UnrelatedTransactionException,
UserFacingException)
from electrum.transaction import (TxOutput, Transaction, PartialTransaction, PartialTxOutput,
PartialTxInput, tx_from_any, TxOutpoint)
restore_wallet_from_text, Abstract_Wallet, CannotBumpFee, BumpFeeStrategy,
TransactionPotentiallyDangerousException, TransactionDangerousException)
from electrum.util import bfh, NotEnoughFunds, UnrelatedTransactionException, UserFacingException
from electrum.transaction import Transaction, PartialTxOutput, tx_from_any
from electrum.mnemonic import seed_type
from electrum.network import Network
@@ -2963,13 +2961,23 @@ class TestWalletSending(ElectrumTestCase):
partial_tx)
# load tx into cosignerB's offline wallet
tx = tx_from_any(partial_tx)
wallet1b_offline.sign_transaction(tx, password=None)
wallet1b_offline.sign_transaction(tx, password=None, ignore_warnings=True)
self.assertEqual('02000000014e375f685f3205e0c7841036525b10f01654632c5ae91e7e04513b815e46a5e100000000d9004730440220414287f36a02b004d2e9a3892e1862edaf49c35d50b65ae10b601879b8c793ef0220073234c56d5a8ae9f4fcfeaecaa757e2724bf830d45aabfab8ffe37329ebf4590147304402203ba7cc21e407ce31c1eecd11c367df716a5d47f06e0bf7109f08063ede25a364022039f6bef0dd401aa2c3103b8cbab57cc4fed3905ccb0a726dc6594bf5930ae0b401475221026addf5fd752c92e8a53955e430ca5964feb1b900ce569f968290f65ae7fecbfd2103a8b896e5216fe7239516a494407c0cc90c6dc33918c7df04d1cda8d57a3bb98152aefdffffff02400d0300000000001600144770c0bc4c42ed1cad089749cc887856ec0f9d99588004000000000017a914493900cdec652a41c633436b53d574647e329b18871c112500',
str(tx))
self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.txid())
self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.wtxid())
# again, but raise on warnings (here: signing non-segwit inputs is risky)
tx = tx_from_any(partial_tx)
try:
wallet1b_offline.sign_transaction(tx, password=None)
self.assertFalse(uses_qr_code2)
except TransactionDangerousException:
raise
except TransactionPotentiallyDangerousException:
self.assertTrue(uses_qr_code2)
@mock.patch.object(wallet.Abstract_Wallet, 'save_db')
async def test_we_dont_sign_tx_including_dummy_address(self, mock_save_db):
wallet1 = self.create_standard_wallet_from_seed('bitter grass shiver impose acquire brush forget axis eager alone wine silver')
@@ -3095,7 +3103,7 @@ class TestWalletOfflineSigning(ElectrumTestCase):
self.assertEqual(tx.txid(), tx_copy.txid())
# sign tx
tx = wallet_offline.sign_transaction(tx_copy, password=None)
tx = wallet_offline.sign_transaction(tx_copy, password=None, ignore_warnings=True)
self.assertTrue(tx.is_complete())
self.assertFalse(tx.is_segwit())
self.assertEqual('d9c21696eca80321933e7444ca928aaf25eeda81aaa2f4e5c085d4d0a9cf7aa7', tx.txid())

1
electrum/transaction.py
View File

@@ -42,6 +42,7 @@ import copy
from . import ecc, bitcoin, constants, segwit_addr, bip32
from .bip32 import BIP32Node
from .i18n import _
from .util import profiler, to_bytes, bfh, chunks, is_hex_str, parse_max_spend
from .bitcoin import (TYPE_ADDRESS, TYPE_SCRIPT, hash_160,
hash160_to_p2sh, hash160_to_p2pkh, hash_to_segwit_addr,

61
electrum/wallet.py
View File

@@ -71,7 +71,7 @@ from .storage import StorageEncryptionVersion, WalletStorage
from .wallet_db import WalletDB
from . import transaction, bitcoin, coinchooser, paymentrequest, ecc, bip32
from .transaction import (Transaction, TxInput, UnknownTxinType, TxOutput,
PartialTransaction, PartialTxInput, PartialTxOutput, TxOutpoint)
PartialTransaction, PartialTxInput, PartialTxOutput, TxOutpoint, Sighash)
from .plugin import run_hook
from .address_synchronizer import (AddressSynchronizer, TX_HEIGHT_LOCAL,
TX_HEIGHT_UNCONF_PARENT, TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_FUTURE, TX_TIMESTAMP_INF)
@@ -233,20 +233,29 @@ class CannotBumpFee(CannotRBFTx):
def __str__(self):
return _('Cannot bump fee') + ':\n\n' + Exception.__str__(self)
class CannotDoubleSpendTx(CannotRBFTx):
def __str__(self):
return _('Cannot cancel transaction') + ':\n\n' + Exception.__str__(self)
class CannotCPFP(Exception):
def __str__(self):
return _('Cannot create child transaction') + ':\n\n' + Exception.__str__(self)
class InternalAddressCorruption(Exception):
def __str__(self):
return _("Wallet file corruption detected. "
"Please restore your wallet from seed, and compare the addresses in both files")
class TransactionPotentiallyDangerousException(Exception): pass
class TransactionDangerousException(TransactionPotentiallyDangerousException): pass
class BumpFeeStrategy(enum.Enum):
PRESERVE_PAYMENT = enum.auto()
DECREASE_PAYMENT = enum.auto()
@@ -2454,7 +2463,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
txout.is_change = self.is_change(address)
self._add_txinout_derivation_info(txout, address, only_der_suffix=only_der_suffix)
def sign_transaction(self, tx: Transaction, password) -> Optional[PartialTransaction]:
def sign_transaction(self, tx: Transaction, password, *, ignore_warnings: bool = False) -> Optional[PartialTransaction]:
""" returns tx if successful else None """
if self.is_watching_only():
return
@@ -2467,6 +2476,24 @@ class Abstract_Wallet(ABC, Logger, EventListener):
if swap:
self.lnworker.swap_manager.sign_tx(tx, swap)
return tx
# check if signing is dangerous
should_confirm, should_reject, message = self.check_sighash(tx)
if should_reject:
raise TransactionDangerousException('Not signing transaction:\n' + message)
if not ignore_warnings:
if should_confirm:
message = '\n'.join([_('Danger! This transaction is non-standard!'), message])
fee = self.get_wallet_delta(tx).fee
risk_of_burning_coins = (fee is not None
and self.get_warning_for_risk_of_burning_coins_as_fees(tx))
if risk_of_burning_coins:
should_confirm = True
message = '\n'.join([message, risk_of_burning_coins])
if should_confirm:
raise TransactionPotentiallyDangerousException('Not signing transaction:\n' + message)
# add info to a temporary tx copy; including xpubs
# and full derivation paths as hw keystores might want them
tmp_tx = copy.deepcopy(tx)
@@ -3041,6 +3068,36 @@ class Abstract_Wallet(ABC, Logger, EventListener):
"do not accept to sign it more than once,\n"
"otherwise you could end up paying a different fee."))
def check_sighash(self, tx: 'PartialTransaction') -> (bool, bool, str):
"""Checks the Sighash for my inputs and return hints in the form
(confirm, reject, message), where confirm indicates the user should explicitly confirm,
reject indicates the transaction should be rejected (unless overruled in e.g. a config property),
and message containing a user-facing message describing the context"""
hintmap = dict([
(0, (False, False, None)),
(Sighash.NONE, (True, True, _('Input {} is marked SIGHASH_NONE.'))),
(Sighash.SINGLE, (True, False, _('Input {} is marked SIGHASH_SINGLE.'))),
(Sighash.ALL, (False, False, None)),
(Sighash.ANYONECANPAY, (True, False, _('Input {} is marked SIGHASH_ANYONECANPAY.')))
])
confirm = False
reject = False
messages = []
for txin_idx, txin in enumerate(tx.inputs()):
if txin.sighash and txin.sighash != Sighash.ALL: # non-standard
addr = self.adb.get_txin_address(txin)
if self.adb.is_mine(addr):
sh_out = txin.sighash & (Sighash.ANYONECANPAY ^ 0xff)
sh_in = txin.sighash & Sighash.ANYONECANPAY
confirm |= hintmap[sh_out][0] | hintmap[sh_in][0]
reject |= hintmap[sh_out][1] | hintmap[sh_in][1]
for sh in [sh_out, sh_in]:
msg = hintmap[sh][2]
if msg:
messages.append('%s: %s' % (_('Fatal') if hintmap[sh][1] else _('Warning'),
msg.format(txin_idx)))
return confirm, reject, '\n'.join(messages)
def get_tx_fee_warning(
self, *,
invoice_amt: int,