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

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

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
 

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

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