From 30c863d32cf1b221965d6e970929de4c3507f906 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 16 Oct 2023 17:10:49 +0000 Subject: [PATCH] lnaddr: don't call validate_features in parser - see comment in lnaddr.py - Previously we used feature bit 50/51 for trampoline. The spec subsequently defined fbit 50/51 as option_zeroconf, which requires fbit 46/47 (option_scid_alias) to also be set. We moved the non-standard trampoline fbit to a different int. However, old wallets might have old invoices saved that set fbit 50/51 for trampoline, and those would not have the dependent bit set. Invoices are parsed at wallet-open, so if the parser ran these checks, those wallets could not be opened. - note: we could potentially also run lnaddr.validate_and_compare_features when saving new invoices into the wallet but this is not done here --- electrum/lnaddr.py | 8 ++++--- electrum/lnworker.py | 6 +++++ electrum/tests/test_bolt11.py | 3 --- electrum/tests/test_lnpeer.py | 43 ++++++++++++++++++++++++++++++++--- 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/electrum/lnaddr.py b/electrum/lnaddr.py index 0476bf0c1..65ee3e9dc 100644 --- a/electrum/lnaddr.py +++ b/electrum/lnaddr.py @@ -523,9 +523,11 @@ def lndecode(invoice: str, *, verbose=False, net=None) -> LnAddr: elif tag == '9': features = tagdata.uint addr.tags.append(('9', features)) - from .lnutil import validate_features - validate_features(features) - + # note: The features are not validated here in the parser, + # instead, validation is done just before we try paying the invoice (in lnworker._check_invoice). + # Context: invoice parsing happens when opening a wallet. If there was a backwards-incompatible + # change to a feature, and we raised, some existing wallets could not be opened. Such a change + # can happen to features not-yet-merged-to-BOLTs (e.g. trampoline feature bit was moved and reused). else: addr.unknown_tags.append((tag, tagdata)) diff --git a/electrum/lnworker.py b/electrum/lnworker.py index 3f34eb99b..e89b7ec26 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -1714,9 +1714,13 @@ class LNWallet(LNWorker): return None def _check_invoice(self, invoice: str, *, amount_msat: int = None) -> LnAddr: + """Parses and validates a bolt11 invoice str into a LnAddr. + Includes pre-payment checks external to the parser. + """ addr = lndecode(invoice) if addr.is_expired(): raise InvoiceError(_("This invoice has expired")) + # check amount if amount_msat: # replace amt in invoice. main usecase is paying zero amt invoices existing_amt_msat = addr.get_amount_msat() if existing_amt_msat and amount_msat < existing_amt_msat: @@ -1724,10 +1728,12 @@ class LNWallet(LNWorker): addr.amount = Decimal(amount_msat) / COIN / 1000 if addr.amount is None: raise InvoiceError(_("Missing amount")) + # check cltv if addr.get_min_final_cltv_expiry() > lnutil.NBLOCK_CLTV_EXPIRY_TOO_FAR_INTO_FUTURE: raise InvoiceError("{}\n{}".format( _("Invoice wants us to risk locking funds for unreasonably long."), f"min_final_cltv_expiry: {addr.get_min_final_cltv_expiry()}")) + # check features addr.validate_and_compare_features(self.features) return addr diff --git a/electrum/tests/test_bolt11.py b/electrum/tests/test_bolt11.py index 148cbb9b5..7586d1066 100644 --- a/electrum/tests/test_bolt11.py +++ b/electrum/tests/test_bolt11.py @@ -161,9 +161,6 @@ class TestBolt11(ElectrumTestCase): self.assertEqual(33282, lnaddr.get_tag('9')) self.assertEqual(LnFeatures(33282), lnaddr.get_features()) - with self.assertRaises(UnknownEvenFeatureBits): - lndecode("lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdees9q4pqqqqqqqqqqqqqqqqqqszk3ed62snp73037h4py4gry05eltlp0uezm2w9ajnerhmxzhzhsu40g9mgyx5v3ad4aqwkmvyftzk4k9zenz90mhjcy9hcevc7r3lx2sphzfxz7") - def test_payment_secret(self): lnaddr = lndecode("lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqsp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygsdq5vdhkven9v5sxyetpdees9q5sqqqqqqqqqqqqqqqpqsqvvh7ut50r00p3pg34ea68k7zfw64f8yx9jcdk35lh5ft8qdr8g4r0xzsdcrmcy9hex8un8d8yraewvhqc9l0sh8l0e0yvmtxde2z0hgpzsje5l") self.assertEqual((1 << 9) + (1 << 15) + (1 << 99), lnaddr.get_tag('9')) diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index 08b3f51d6..72a9fd769 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -24,7 +24,7 @@ from electrum.ecc import ECPrivkey from electrum import simple_config, lnutil from electrum.lnaddr import lnencode, LnAddr, lndecode from electrum.bitcoin import COIN, sha256 -from electrum.util import NetworkRetryManager, bfh, OldTaskGroup, EventListener +from electrum.util import NetworkRetryManager, bfh, OldTaskGroup, EventListener, InvoiceError from electrum.lnpeer import Peer from electrum.lnutil import LNPeerAddr, Keypair, privkey_to_pubkey from electrum.lnutil import PaymentFailure, LnFeatures, HTLCOwner @@ -452,6 +452,8 @@ class TestPeer(ElectrumTestCase): include_routing_hints=False, payment_preimage: bytes = None, payment_hash: bytes = None, + invoice_features: LnFeatures = None, + min_final_cltv_expiry_delta: int = None, ) -> Tuple[LnAddr, str]: amount_btc = amount_msat/Decimal(COIN*1000) if payment_preimage is None and not payment_hash: @@ -467,16 +469,19 @@ class TestPeer(ElectrumTestCase): else: routing_hints = [] trampoline_hints = [] - invoice_features = w2.features.for_invoice() + if invoice_features is None: + invoice_features = w2.features.for_invoice() if invoice_features.supports(LnFeatures.PAYMENT_SECRET_OPT): payment_secret = w2.get_payment_secret(payment_hash) else: payment_secret = None + if min_final_cltv_expiry_delta is None: + min_final_cltv_expiry_delta = lnutil.MIN_FINAL_CLTV_EXPIRY_FOR_INVOICE lnaddr1 = LnAddr( paymenthash=payment_hash, amount=amount_btc, tags=[ - ('c', lnutil.MIN_FINAL_CLTV_EXPIRY_FOR_INVOICE), + ('c', min_final_cltv_expiry_delta), ('d', 'coffee'), ('9', invoice_features), ] + routing_hints, @@ -776,6 +781,38 @@ class TestPeerDirect(TestPeer): with self.assertRaises(PaymentFailure): await self._test_simple_payment(test_trampoline=test_trampoline, test_hold_invoice=True, test_failure=True) + async def test_check_invoice_before_payment(self): + alice_channel, bob_channel = create_test_channels() + p1, p2, w1, w2, _q1, _q2 = self.prepare_peers(alice_channel, bob_channel) + async def try_paying_some_invoices(): + # feature bits: unknown even fbit + invoice_features = w2.features.for_invoice() | (1 << 990) # add undefined even fbit + lnaddr, pay_req = self.prepare_invoice(w2, invoice_features=invoice_features) + with self.assertRaises(lnutil.UnknownEvenFeatureBits): + result, log = await w1.pay_invoice(pay_req) + # feature bits: not all transitive dependencies are set + invoice_features = LnFeatures((1 << 8) + (1 << 17)) + lnaddr, pay_req = self.prepare_invoice(w2, invoice_features=invoice_features) + with self.assertRaises(lnutil.IncompatibleOrInsaneFeatures): + result, log = await w1.pay_invoice(pay_req) + # too large CLTV + lnaddr, pay_req = self.prepare_invoice(w2, min_final_cltv_expiry_delta=10**6) + with self.assertRaises(InvoiceError): + result, log = await w1.pay_invoice(pay_req) + raise SuccessfulTest() + + async def f(): + async with OldTaskGroup() as group: + await group.spawn(p1._message_loop()) + await group.spawn(p1.htlc_switch()) + await group.spawn(p2._message_loop()) + await group.spawn(p2.htlc_switch()) + await asyncio.sleep(0.01) + await group.spawn(try_paying_some_invoices()) + + with self.assertRaises(SuccessfulTest): + await f() + async def test_payment_race(self): """Alice and Bob pay each other simultaneously. They both send 'update_add_htlc' and receive each other's update