Fix 10937

[accumulator]

add BOLT11DecodeException and InvalidBitcoinURI wraps for missing uncaught exceptions.

fixes #10937

Note: this makes bolt11 decoding more strict, therefore a wallet_db migration has been added to remove invoices not passing the strict parse.

[f321x]

This new strict parameter is only used for the wallet upgrades, what do you think about catching the exception in the wallet upgrades instead of introducing an additional parameter?
This would keep the API simpler to understand.

Something like:

diff --git a/electrum/bolt11.py b/electrum/bolt11.py
index 840d805f09..f84b3e290b 100644
--- a/electrum/bolt11.py
+++ b/electrum/bolt11.py
@@ -415,14 +415,9 @@ class SerializableKey:
         return self.pubkey.get_public_key_bytes(True)
 
 
-def decode_bolt11_invoice(invoice: str, *, verbose=False, net=None, strict=True) -> BOLT11Addr:
+def decode_bolt11_invoice(invoice: str, *, verbose=False, net=None) -> BOLT11Addr:
     """Parses a string into a BOLT11Addr object.
     Can raise BOLT11DecodeException.
-
-    :param strict:
-        if True, fail decode on malformed 'r' and 't' tags. False restores old behavior.
-        False is used when migrating wallet_db: an invoice stored by an older version can be
-        malformed.
     """
 
     def _convertbits_tag(tag, *args, **kwargs):
@@ -497,9 +492,7 @@ def decode_bolt11_invoice(invoice: str, *, verbose=False, net=None, strict=True)
             #    * `feebase` (32 bits, big-endian)
             #    * `feerate` (32 bits, big-endian)
             #    * `cltv_expiry_delta` (16 bits, big-endian)
-            tagdata = convertbits(tagdata, 5, 8, False)
-            if strict and tagdata is None:
-                raise BOLT11DecodeException(f"Failed to decode tag '{tag}'")
+            tagdata = _convertbits_tag(tag, tagdata, 5, 8, False)
             if not tagdata:
                 continue
             route = []
@@ -519,9 +512,7 @@ def decode_bolt11_invoice(invoice: str, *, verbose=False, net=None, strict=True)
             if route:
                 addr.tags.append(('r',route))
         elif tag == 't':
-            tagdata = convertbits(tagdata, 5, 8, False)
-            if strict and tagdata is None:
-                raise BOLT11DecodeException(f"Failed to decode tag '{tag}'")
+            tagdata = _convertbits_tag(tag, tagdata, 5, 8, False)
             if not tagdata:
                 continue
             route = []
diff --git a/electrum/wallet_db.py b/electrum/wallet_db.py
index aba35bf046..7aa8e73c74 100644
--- a/electrum/wallet_db.py
+++ b/electrum/wallet_db.py
@@ -932,13 +932,18 @@ class WalletDBUpgrader(Logger):
         # the new key for all requests is a wallet address, not done here
         for name in ['invoices', 'payment_requests']:
             invoices = self.data.get(name, {})
-            for key, item in invoices.items():
+            for key, item in list(invoices.items()):
                 is_lightning = item['type'] == 2
                 lightning_invoice = item['invoice'] if is_lightning else None
                 outputs = item['outputs'] if not is_lightning else None
                 bip70 = item['bip70'] if not is_lightning else None
                 if is_lightning:
-                    lnaddr = decode_bolt11_invoice(item['invoice'], strict=False)
+                    try:
+                        lnaddr = decode_bolt11_invoice(item['invoice'])
+                    except BOLT11InvoiceException as e:
+                        self.logger.warning(f"removing {name} item {key} that fails bolt11 decode: {e}")
+                        del invoices[key]
+                        continue
                     amount_msat = lnaddr.get_amount_msat()
                     timestamp = lnaddr.date
                     exp_delay = lnaddr.get_expiry()
@@ -999,7 +1004,12 @@ class WalletDBUpgrader(Logger):
         for key, item in list(requests.items()):
             lnaddr = item.get('lightning_invoice')
             if lnaddr:
-                lnaddr = decode_bolt11_invoice(lnaddr, strict=False)
+                try:
+                    lnaddr = decode_bolt11_invoice(lnaddr)
+                except BOLT11InvoiceException as e:
+                    self.logger.warning(f"removing request {key} that fails bolt11 decode: {e}")
+                    del requests[key]
+                    continue
                 rhash = lnaddr.paymenthash.hex()
                 if key != rhash:
                     requests[rhash] = item
@@ -1049,7 +1059,12 @@ class WalletDBUpgrader(Logger):
             if lightning_invoice is None:
                 payment_hash = None
             else:
-                lnaddr = decode_bolt11_invoice(lightning_invoice, strict=False)
+                try:
+                    lnaddr = decode_bolt11_invoice(lightning_invoice)
+                except BOLT11InvoiceException as e:
+                    self.logger.warning(f"removing request {key} that fails bolt11 decode: {e}")
+                    del requests[key]
+                    continue
                 payment_hash = lnaddr.paymenthash.hex()
             item['payment_hash'] = payment_hash
         self.data['seed_version'] = 51
@@ -1486,15 +1501,15 @@ class WalletDBUpgrader(Logger):
         from .bolt11 import decode_bolt11_invoice
         if not self._is_upgrade_method_needed(72, 72):
             return
-        # remove invoices not passing strict bolt11 invoice check
+        # remove invoices not passing stricter bolt11 invoice parsing (https://github.com/spesmilo/electrum/pull/10940)
         invoices = self.data.get('invoices', {})
         for key, item in list(invoices.items()):
             lnaddr = item.get('lightning_invoice')
             if lnaddr:
                 try:
-                    decode_bolt11_invoice(lnaddr, strict=True)
+                    decode_bolt11_invoice(lnaddr)
                 except BOLT11InvoiceException as e:
-                    self.logger.warning(f"removing invoice {key} that fails strict bolt11 check: {e}")
+                    self.logger.warning(f"removing invoice {key} that fails bolt11 decode: {e}")
                     del invoices[key]
         self.data['seed_version'] = 73
 
diff --git a/tests/test_bolt11.py b/tests/test_bolt11.py
index a4b2e9e395..7530f14250 100644
--- a/tests/test_bolt11.py
+++ b/tests/test_bolt11.py
@@ -211,7 +211,7 @@ class TestBolt11(ElectrumTestCase):
                          [17] + [0] * 4,    # p2pkh with a too-short hash160
                          [0] + [0] * 4,     # p2wpkh with a too-short witness program
                          [19, 0, 0]):       # unknown witness version
-            lnaddr = decode_bolt11_invoice(self._encode_invoice_with_raw_tag('f', tagdata5), strict=True)
+            lnaddr = decode_bolt11_invoice(self._encode_invoice_with_raw_tag('f', tagdata5))
             self.assertIsNone(lnaddr.get_tag('f'))
             self.assertEqual('', lnaddr.get_fallback_address())
             self.assertEqual(['f'], [tag for tag, _ in lnaddr.unknown_tags])
@@ -236,7 +236,7 @@ class TestBolt11(ElectrumTestCase):
                               ('t', [0, 1])):
             with self.subTest(tag=tag, tagdata5=tagdata5):
                 with self.assertRaises(BOLT11DecodeException):
-                    decode_bolt11_invoice(self._encode_invoice_with_raw_tag(tag, tagdata5), strict=True)
+                    decode_bolt11_invoice(self._encode_invoice_with_raw_tag(tag, tagdata5))
 
         # control: the same lengths with zero padding bits decode fine
         for tag, tagdata5 in (('d', [0, 0]),
@@ -255,46 +255,20 @@ class TestBolt11(ElectrumTestCase):
                 lnaddr = decode_bolt11_invoice(self._encode_invoice_with_raw_tag(tag, [0] * 51))
                 self.assertEqual([tag], [t for t, _ in lnaddr.unknown_tags])
 
-    def test_malformed_route_tags_are_skipped_when_not_strict(self):
-        # strict=False restores the pre-fix behaviour for 'r' and 't': a payload that cannot be
-        # converted back to bytes is silently dropped instead of aborting the decode. wallet_db
-        # passes strict=False, as an invoice already stored by an older version must stay loadable.
+        # 'r' and 't': an empty payload converts to b'' instead of failing, so it is skipped
         for tag in ('r', 't'):
-            for tagdata5 in ([1],        # 5 bits left over: no valid conversion
-                             [0, 1]):    # non-zero padding bits
-                invoice = self._encode_invoice_with_raw_tag(tag, tagdata5)
-                with self.subTest(tag=tag, tagdata5=tagdata5, strict=False):
-                    lnaddr = decode_bolt11_invoice(invoice, strict=False)
-                    # the malformed field is dropped: it lands in neither tags nor unknown_tags
-                    self.assertIsNone(lnaddr.get_tag(tag))
-                    self.assertEqual([], lnaddr.get_routing_info(tag))
-                    self.assertEqual([], lnaddr.unknown_tags)
-                    # ...and the rest of the invoice still decodes
-                    self.assertEqual(RHASH, lnaddr.paymenthash)
-                    self.assertEqual('test', lnaddr.get_description())
-                # strict is the default, and rejects the same invoice
-                for kwargs in ({}, {'strict': True}):
-                    with self.subTest(tag=tag, tagdata5=tagdata5, kwargs=kwargs):
-                        with self.assertRaises(BOLT11DecodeException):
-                            decode_bolt11_invoice(invoice, **kwargs)
-
-        # an empty payload converts to b'' instead of failing, so it is skipped in both modes
-        for tag in ('r', 't'):
-            for strict in (False, True):
-                with self.subTest(tag=tag, tagdata5=[], strict=strict):
-                    lnaddr = decode_bolt11_invoice(self._encode_invoice_with_raw_tag(tag, []), strict=strict)
-                    self.assertIsNone(lnaddr.get_tag(tag))
-                    self.assertEqual([], lnaddr.unknown_tags)
+            with self.subTest(tag=tag, tagdata5=[]):
+                lnaddr = decode_bolt11_invoice(self._encode_invoice_with_raw_tag(tag, []))
+                self.assertIsNone(lnaddr.get_tag(tag))
+                self.assertEqual([], lnaddr.unknown_tags)
 
-        # control: a well-formed hop is parsed identically in both modes
+        # control: a well-formed hop is parsed
         r_hop = bytes(33) + bytes(8) + (1).to_bytes(4, 'big') + (2).to_bytes(4, 'big') + (3).to_bytes(2, 'big')
         t_hop = bytes(33) + (1).to_bytes(4, 'big') + (2).to_bytes(4, 'big') + (3).to_bytes(2, 'big')
         for tag, hop in (('r', r_hop), ('t', t_hop)):
             with self.subTest(tag=tag):
                 invoice = self._encode_invoice_with_raw_tag(tag, list(convertbits(hop, 8, 5)))
                 self.assertEqual(1, len(decode_bolt11_invoice(invoice).get_routing_info(tag)))
-                self.assertEqual(decode_bolt11_invoice(invoice, strict=False).get_routing_info(tag),
-                                 decode_bolt11_invoice(invoice, strict=True).get_routing_info(tag))
 
     def test_invalid_signature(self):
         # The trailing 65 bytes of an invoice are attacker-controlled: every way the ecc lib
diff --git a/tests/test_storage_upgrade.py b/tests/test_storage_upgrade.py
index e691add066..07f610aeab 100644
--- a/tests/test_storage_upgrade.py
+++ b/tests/test_storage_upgrade.py
@@ -343,10 +343,12 @@ class TestStorageUpgrade(WalletTestCase):
 
     @as_testnet
     async def test_upgrade_removes_invoice_with_malformed_route_tag(self):
-        # Db conversion 72->73 drops stored invoices that do not pass the strict bolt11 check.
+        # Db conversion 72->73 drops stored invoices that fail bolt11 decoding.
         # Older versions decoded a malformed 'r'/'t' tag by silently skipping it, so such an
         # invoice can be sitting in a wallet file; without this conversion it would now abort
         # the load in Invoice._validate_invoice_str, leaving the file unopenable.
+        # The older conversions that decode invoices themselves (45, 47, 51) drop such items
+        # the same way, so a file from before those versions upgrades too.
         # The malformed invoices below are correctly signed, but their 'r'/'t' payload has
         # non-zero padding bits; see TestBolt11._encode_invoice_with_raw_tag.
         bad_r = ('lntb1ps9zprzpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq8w3jhxaqrqzq'
@@ -383,6 +385,14 @@ class TestStorageUpgrade(WalletTestCase):
         with self.assertRaises(BOLT11DecodeException):
             self._load_db_from_json_string(wallet_json=json.dumps(data), upgrade=True)
 
+        # a pre-45 file: conversion 45 decodes the invoices itself and drops the bad ones
+        data['seed_version'] = 44
+        data['invoices'] = {key: {'type': 2, 'invoice': invoice_str}
+                            for key, invoice_str in (('bad_r', bad_r), ('bad_t', bad_t), ('good', good))}
+        db = self._load_db_from_json_string(wallet_json=json.dumps(data), upgrade=True)
+        self.assertEqual(73, db.get('seed_version'))
+        self.assertEqual(['good'], list(db.get_dict('invoices').keys()))
+
 
 ##########
 

[f321x]

Instead of adding these tags with incorrect data_length to unknown_tags we should probably just raise BOLT11DecodeException too:
https://github.com/lightning/bolts/blob/152897261850d93c4f4597f39cf22d7d22d6ede6/11-payment-encoding.md?plain=1#L213

[f321x]

The addr.amount setter below can also raise BOLT11InvoiceException, this could be converted to BOLT11DecodeException?

[f321x]

Maybe we should also limit expiry timestamps to 'sane' values (like < 2**37):

>>> util.format_time(2 ** 37)
'6325-04-08 17:04'
>>> util.format_time(2 ** 38)
Traceback (most recent call last):
  File "<python-input-5>", line 1, in <module>
    util.format_time(2 ** 38)
    ~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/var/home/user/code/code_vm/electrum/electrum/util.py", line 915, in format_time
    date = timestamp_to_datetime(timestamp)
  File "/var/home/user/code/code_vm/electrum/electrum/util.py", line 911, in timestamp_to_datetime
    return datetime.fromtimestamp(timestamp, tz=tz)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: year must be in 1..9999, not 10680