From 4e6e6f76cab2e7094615bf7cd767f59849959187 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Tue, 22 Aug 2023 18:10:21 +0000 Subject: [PATCH 1/2] invoices: also run amount-validator on setter - @amount_msat.validator prevents the creation of invoices with e.g. too large amounts - however the qml gui is mutating invoices by directly setting the `amount_msat` field, and it looks like attrs validators only run during init. We can use `on_setattr` (introduced in attrs==20.1.0). - a wallet db upgrade is added to rm existing insane invoices - btw the qml gui was already doing its own input validation on the textedit (see qeconfig.btcAmountRegex). however that only limits the input to not have more chars than what is needed to represent 21M BTC (e.g. you can still enter 99M BTC, which the invoice logic does not tolerate later on - but is normally caught). fixes https://github.com/spesmilo/electrum/issues/8582 --- contrib/requirements/requirements.txt | 2 +- electrum/invoices.py | 14 +++++++--- electrum/tests/test_invoices.py | 40 +++++++++++++++++++++++++-- electrum/wallet_db.py | 20 +++++++++++++- 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/contrib/requirements/requirements.txt b/contrib/requirements/requirements.txt index 820e9cca0..47753c4b2 100644 --- a/contrib/requirements/requirements.txt +++ b/contrib/requirements/requirements.txt @@ -6,7 +6,7 @@ aiohttp>=3.3.0,<4.0.0 aiohttp_socks>=0.3 certifi bitstring -attrs>=19.2.0 +attrs>=20.1.0 # Note that we also need the dnspython[DNSSEC] extra which pulls in cryptography, # but as that is not pure-python it cannot be listed in this file! diff --git a/electrum/invoices.py b/electrum/invoices.py index d5bf7bfdc..9f790ea77 100644 --- a/electrum/invoices.py +++ b/electrum/invoices.py @@ -94,13 +94,18 @@ class BaseInvoice(StoredObject): """ Base class for Invoice and Request In the code, we use 'invoice' for outgoing payments, and 'request' for incoming payments. + + TODO this class is getting too complicated for "attrs"... maybe we should rewrite it without. """ # mandatory fields - amount_msat = attr.ib(kw_only=True) # type: Optional[Union[int, str]] # can be '!' or None + amount_msat = attr.ib( # can be '!' or None + kw_only=True, on_setattr=attr.setters.validate) # type: Optional[Union[int, str]] message = attr.ib(type=str, kw_only=True) - time = attr.ib(type=int, kw_only=True, validator=attr.validators.instance_of(int)) # timestamp of the invoice - exp = attr.ib(type=int, kw_only=True, validator=attr.validators.instance_of(int)) # expiration delay (relative). 0 means never + time = attr.ib( # timestamp of the invoice + type=int, kw_only=True, validator=attr.validators.instance_of(int), on_setattr=attr.setters.validate) + exp = attr.ib( # expiration delay (relative). 0 means never + type=int, kw_only=True, validator=attr.validators.instance_of(int), on_setattr=attr.setters.validate) # optional fields. # an request (incoming) can be satisfied onchain, using lightning or using a swap @@ -108,7 +113,8 @@ class BaseInvoice(StoredObject): # onchain only outputs = attr.ib(kw_only=True, converter=_decode_outputs) # type: Optional[List[PartialTxOutput]] - height = attr.ib(type=int, kw_only=True, validator=attr.validators.instance_of(int)) # only for receiving + height = attr.ib( # only for receiving + type=int, kw_only=True, validator=attr.validators.instance_of(int), on_setattr=attr.setters.validate) bip70 = attr.ib(type=str, kw_only=True) # type: Optional[str] #bip70_requestor = attr.ib(type=str, kw_only=True) # type: Optional[str] diff --git a/electrum/tests/test_invoices.py b/electrum/tests/test_invoices.py index 06dafd494..a5ea83fca 100644 --- a/electrum/tests/test_invoices.py +++ b/electrum/tests/test_invoices.py @@ -5,10 +5,10 @@ from . import ElectrumTestCase from electrum.simple_config import SimpleConfig from electrum.wallet import restore_wallet_from_text, Standard_Wallet, Abstract_Wallet -from electrum.invoices import PR_UNPAID, PR_PAID, PR_UNCONFIRMED, BaseInvoice +from electrum.invoices import PR_UNPAID, PR_PAID, PR_UNCONFIRMED, BaseInvoice, Invoice, LN_EXPIRY_NEVER from electrum.address_synchronizer import TX_HEIGHT_UNCONFIRMED from electrum.transaction import Transaction, PartialTxOutput -from electrum.util import TxMinedInfo +from electrum.util import TxMinedInfo, InvoiceError class TestWalletPaymentRequests(ElectrumTestCase): @@ -266,3 +266,39 @@ class TestWalletPaymentRequests(ElectrumTestCase): BaseInvoice._get_cur_time = lambda *args: time.time() + 200_000 self.assertEqual(PR_UNCONFIRMED, wallet1.get_invoice_status(pr2)) self.assertEqual(pr2, wallet1.get_request_by_addr(addr1)) + + +class TestBaseInvoice(ElectrumTestCase): + TESTNET = True + + async def test_arg_validation(self): + amount_sat = 10_000 + outputs = [PartialTxOutput.from_address_and_value("tb1qmjzmg8nd4z56ar4fpngzsr6euktrhnjg9td385", amount_sat)] + invoice = Invoice( + amount_msat=amount_sat * 1000, + message="mymsg", + time=1692716965, + exp=LN_EXPIRY_NEVER, + outputs=outputs, + bip70=None, + height=0, + lightning_invoice=None, + ) + with self.assertRaises(InvoiceError): + invoice.amount_msat = 10**20 + with self.assertRaises(InvoiceError): + invoice2 = Invoice( + amount_msat=10**20, + message="mymsg", + time=1692716965, + exp=LN_EXPIRY_NEVER, + outputs=outputs, + bip70=None, + height=0, + lightning_invoice=None, + ) + with self.assertRaises(TypeError): + invoice.time = "asd" + with self.assertRaises(TypeError): + invoice.exp = "asd" + diff --git a/electrum/wallet_db.py b/electrum/wallet_db.py index d148a0a9c..01a4c6f49 100644 --- a/electrum/wallet_db.py +++ b/electrum/wallet_db.py @@ -54,7 +54,7 @@ from .version import ELECTRUM_VERSION OLD_SEED_VERSION = 4 # electrum versions < 2.0 NEW_SEED_VERSION = 11 # electrum versions >= 2.0 -FINAL_SEED_VERSION = 53 # electrum >= 2.7 will set this to prevent +FINAL_SEED_VERSION = 54 # electrum >= 2.7 will set this to prevent # old versions from overwriting new format @@ -239,6 +239,7 @@ class WalletDB(JsonDB): self._convert_version_51() self._convert_version_52() self._convert_version_53() + self._convert_version_54() self.put('seed_version', FINAL_SEED_VERSION) # just to be sure self._after_upgrade_tasks() @@ -1076,6 +1077,23 @@ class WalletDB(JsonDB): cb['local_payment_pubkey'] = None self.data['seed_version'] = 53 + def _convert_version_54(self): + # note: similar to convert_version_38 + if not self._is_upgrade_method_needed(53, 53): + return + from .bitcoin import TOTAL_COIN_SUPPLY_LIMIT_IN_BTC, COIN + max_sats = TOTAL_COIN_SUPPLY_LIMIT_IN_BTC * COIN + requests = self.data.get('payment_requests', {}) + invoices = self.data.get('invoices', {}) + for d in [invoices, requests]: + for key, item in list(d.items()): + amount_msat = item['amount_msat'] + if amount_msat == '!': + continue + if not (isinstance(amount_msat, int) and 0 <= amount_msat <= max_sats * 1000): + del d[key] + self.data['seed_version'] = 54 + def _convert_imported(self): if not self._is_upgrade_method_needed(0, 13): return From ffa3acc0135a1def7034fe882ebccd1f5208fa06 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Tue, 22 Aug 2023 18:12:15 +0000 Subject: [PATCH 2/2] invoices: don't modify .amount_msat directly --- electrum/gui/kivy/uix/screens.py | 2 +- electrum/gui/qml/qeinvoice.py | 7 +++---- electrum/gui/text.py | 2 +- electrum/invoices.py | 13 +++++++++++++ electrum/payment_identifier.py | 2 +- electrum/tests/test_invoices.py | 2 ++ 6 files changed, 21 insertions(+), 7 deletions(-) diff --git a/electrum/gui/kivy/uix/screens.py b/electrum/gui/kivy/uix/screens.py index d5191b97a..37b080126 100644 --- a/electrum/gui/kivy/uix/screens.py +++ b/electrum/gui/kivy/uix/screens.py @@ -351,7 +351,7 @@ class SendScreen(CScreen, Logger): assert type(amount_sat) is int invoice = Invoice.from_bech32(address) if invoice.amount_msat is None: - invoice.amount_msat = int(amount_sat * 1000) + invoice.set_amount_msat(int(amount_sat * 1000)) return invoice else: # on-chain diff --git a/electrum/gui/qml/qeinvoice.py b/electrum/gui/qml/qeinvoice.py index 7a73ece05..3dde33612 100644 --- a/electrum/gui/qml/qeinvoice.py +++ b/electrum/gui/qml/qeinvoice.py @@ -379,8 +379,7 @@ class QEInvoice(QObject, QtEventListener): if self.amount.isEmpty: if self.amountOverride.isEmpty: raise Exception('can not pay 0 amount') - # TODO: is update amount_msat for overrideAmount sufficient? - self._effectiveInvoice.amount_msat = self.amountOverride.satsInt * 1000 + self._effectiveInvoice.set_amount_msat(self.amountOverride.satsInt * 1000) self._wallet.pay_lightning_invoice(self._effectiveInvoice) @@ -627,9 +626,9 @@ class QEInvoiceParser(QEInvoice): if not self._effectiveInvoice.amount_msat and not self.amountOverride.isEmpty: if self.invoiceType == QEInvoice.Type.OnchainInvoice and self.amountOverride.isMax: - self._effectiveInvoice.amount_msat = '!' + self._effectiveInvoice.set_amount_msat('!') else: - self._effectiveInvoice.amount_msat = self.amountOverride.satsInt * 1000 + self._effectiveInvoice.set_amount_msat(self.amountOverride.satsInt * 1000) self.canSave = False diff --git a/electrum/gui/text.py b/electrum/gui/text.py index 1996dd1fe..f18e604be 100644 --- a/electrum/gui/text.py +++ b/electrum/gui/text.py @@ -606,7 +606,7 @@ class ElectrumGui(BaseElectrumGui, EventListener): if invoice.amount_msat is None: amount_sat = self.parse_amount(self.str_amount) if amount_sat: - invoice.amount_msat = int(amount_sat * 1000) + invoice.set_amount_msat(int(amount_sat * 1000)) else: self.show_error(_('No amount')) return diff --git a/electrum/invoices.py b/electrum/invoices.py index 9f790ea77..39e2dad43 100644 --- a/electrum/invoices.py +++ b/electrum/invoices.py @@ -178,6 +178,19 @@ class BaseInvoice(StoredObject): return amount_msat return int(amount_msat // 1000) + def set_amount_msat(self, amount_msat: Union[int, str]) -> None: + """The GUI uses this to fill the amount for a zero-amount invoice.""" + if amount_msat == "!": + amount_sat = amount_msat + else: + assert isinstance(amount_msat, int), f"{amount_msat=!r}" + assert amount_msat >= 0, amount_msat + amount_sat = (amount_msat // 1000) + int(amount_msat % 1000 > 0) # round up + if outputs := self.outputs: + assert len(self.outputs) == 1, len(self.outputs) + self.outputs = [PartialTxOutput(scriptpubkey=outputs[0].scriptpubkey, value=amount_sat)] + self.amount_msat = amount_msat + @amount_msat.validator def _validate_amount(self, attribute, value): if value is None: diff --git a/electrum/payment_identifier.py b/electrum/payment_identifier.py index 6aad287ae..c95e47467 100644 --- a/electrum/payment_identifier.py +++ b/electrum/payment_identifier.py @@ -674,7 +674,7 @@ def invoice_from_payment_identifier( if not invoice: return if invoice.amount_msat is None: - invoice.amount_msat = int(amount_sat * 1000) + invoice.set_amount_msat(int(amount_sat * 1000)) return invoice else: outputs = pi.get_onchain_outputs(amount_sat) diff --git a/electrum/tests/test_invoices.py b/electrum/tests/test_invoices.py index a5ea83fca..9f7e017c2 100644 --- a/electrum/tests/test_invoices.py +++ b/electrum/tests/test_invoices.py @@ -286,6 +286,8 @@ class TestBaseInvoice(ElectrumTestCase): ) with self.assertRaises(InvoiceError): invoice.amount_msat = 10**20 + with self.assertRaises(InvoiceError): + invoice.set_amount_msat(10**20) with self.assertRaises(InvoiceError): invoice2 = Invoice( amount_msat=10**20,