lnaddr: make payment_secret field mandatory, in both directions

we now require payment_secret both for sending and for receiving
(previously was optional for both)

see
lightning/bolts#898
ACINQ/eclair#1810
ElementsProject/lightning#4646

note: payment_secret depends on var_onion_optin, so that becomes mandatory as well,
however this commit does not yet remove the ability of creating legacy onions

electrum/lnaddr.py

@@ -184,6 +184,7 @@ def lnencode(addr: 'LnAddr', privkey) -> str:
     tags_set = set()
 
     # Payment hash
+    assert addr.paymenthash is not None
     data += tagged_bytes('p', addr.paymenthash)
     tags_set.add('p')
 
@@ -196,7 +197,7 @@ def lnencode(addr: 'LnAddr', privkey) -> str:
         # BOLT #11:
         #
         # A writer MUST NOT include more than one `d`, `h`, `n` or `x` fields,
-        if k in ('d', 'h', 'n', 'x', 'p', 's'):
+        if k in ('d', 'h', 'n', 'x', 'p', 's', '9'):
             if k in tags_set:
                 raise LnEncodeException("Duplicate '{}' tag".format(k))
 
@@ -317,6 +318,17 @@ def get_features(self) -> 'LnFeatures':
         from .lnutil import LnFeatures
         return LnFeatures(self.get_tag('9') or 0)
 
+    def validate_and_compare_features(self, myfeatures: 'LnFeatures') -> None:
+        """Raises IncompatibleOrInsaneFeatures.
+
+        note: these checks are not done by the parser (in lndecode), as then when we started requiring a new feature,
+              old saved already paid invoices could no longer be parsed.
+        """
+        from .lnutil import validate_features, ln_compare_features
+        invoice_features = self.get_features()
+        validate_features(invoice_features)
+        ln_compare_features(myfeatures.for_invoice(), invoice_features)
+
     def __str__(self):
         return "LnAddr[{}, amount={}{} tags=[{}]]".format(
             hexlify(self.pubkey.serialize()).decode('utf-8') if self.pubkey else None,
@@ -381,6 +393,9 @@ def serialize(self):
         return self.pubkey.get_public_key_bytes(True)
 
 def lndecode(invoice: str, *, verbose=False, net=None) -> LnAddr:
+    """Parses a string into an LnAddr object.
+    Can raise LnDecodeException or IncompatibleOrInsaneFeatures.
+    """
     if net is None:
         net = constants.net
     decoded_bech32 = bech32_decode(invoice, ignore_long_length=True)

electrum/lnonion.py

@@ -150,7 +150,7 @@ def from_fd(cls, fd: io.BytesIO) -> 'OnionHopsDataSingle':
         elif first_byte == b'\x01':
             # reserved for future use
             raise Exception("unsupported hop payload: length==1")
-        else:
+        else:  # tlv format
             hop_payload_length = read_bigsize_int(fd)
             hop_payload = fd.read(hop_payload_length)
             if hop_payload_length != len(hop_payload):
@@ -266,8 +266,9 @@ def calc_hops_data_for_payment(
         route: 'LNPaymentRoute',
         amount_msat: int,
         final_cltv: int, *,
-        total_msat=None,
-        payment_secret: bytes = None) -> Tuple[List[OnionHopsDataSingle], int, int]:
+        total_msat: int,
+        payment_secret: bytes,
+) -> Tuple[List[OnionHopsDataSingle], int, int]:
 
     """Returns the hops_data to be used for constructing an onion packet,
     and the amount_msat and cltv to be used on our immediate channel.
@@ -283,12 +284,11 @@ def calc_hops_data_for_payment(
     }
     # for multipart payments we need to tell the receiver about the total and
     # partial amounts
-    if payment_secret is not None:
-        hop_payload["payment_data"] = {
-            "payment_secret": payment_secret,
-            "total_msat": total_msat,
-            "amount_msat": amt
-        }
+    hop_payload["payment_data"] = {
+        "payment_secret": payment_secret,
+        "total_msat": total_msat,
+        "amount_msat": amt
+    }
     hops_data = [OnionHopsDataSingle(
         is_tlv_payload=route[-1].has_feature_varonion(), payload=hop_payload)]
     # payloads, backwards from last hop (but excluding the first edge):

electrum/lnpeer.py

@@ -1441,7 +1441,7 @@ def pay(self, *,
             total_msat: int,
             payment_hash: bytes,
             min_final_cltv_expiry: int,
-            payment_secret: bytes = None,
+            payment_secret: bytes,
             trampoline_onion=None) -> UpdateAddHtlc:
 
         assert amount_msat > 0, "amount_msat is not greater zero"
@@ -1786,7 +1786,8 @@ def log_fail_reason(reason: str):
         try:
             total_msat = processed_onion.hop_data.payload["payment_data"]["total_msat"]
         except Exception:
-            total_msat = amt_to_forward # fall back to "amt_to_forward"
+            log_fail_reason(f"'total_msat' missing from onion")
+            raise exc_incorrect_or_unknown_pd
 
         if not is_trampoline and amt_to_forward != htlc.amount_msat:
             log_fail_reason(f"amt_to_forward != htlc.amount_msat")
@@ -1795,14 +1796,10 @@ def log_fail_reason(reason: str):
                 data=htlc.amount_msat.to_bytes(8, byteorder="big"))
 
         try:
-            payment_secret_from_onion = processed_onion.hop_data.payload["payment_data"]["payment_secret"]
+            payment_secret_from_onion = processed_onion.hop_data.payload["payment_data"]["payment_secret"]  # type: bytes
         except Exception:
-            if total_msat > amt_to_forward:
-                # payment_secret is required for MPP
-                log_fail_reason(f"'payment_secret' missing from onion")
-                raise exc_incorrect_or_unknown_pd
-            # TODO fail here if invoice has set PAYMENT_SECRET_REQ
-            payment_secret_from_onion = None
+            log_fail_reason(f"'payment_secret' missing from onion")
+            raise exc_incorrect_or_unknown_pd
 
         payment_status = self.lnworker.check_received_htlc(payment_secret_from_onion, chan.short_channel_id, htlc, total_msat)
         if payment_status is None:
@@ -1825,14 +1822,13 @@ def log_fail_reason(reason: str):
             log_fail_reason(f"no payment_info found for RHASH {htlc.payment_hash.hex()}")
             raise exc_incorrect_or_unknown_pd
         preimage = self.lnworker.get_preimage(htlc.payment_hash)
-        if payment_secret_from_onion:
-            expected_payment_secrets = [self.lnworker.get_payment_secret(htlc.payment_hash)]
-            if preimage:
-                # legacy secret for old invoices
-                expected_payment_secrets.append(derive_payment_secret_from_payment_preimage(preimage))
-            if payment_secret_from_onion not in expected_payment_secrets:
-                log_fail_reason(f'incorrect payment secret {payment_secret_from_onion.hex()} != {expected_payment_secrets[0].hex()}')
-                raise exc_incorrect_or_unknown_pd
+        expected_payment_secrets = [self.lnworker.get_payment_secret(htlc.payment_hash)]
+        if preimage:
+            # legacy secret for old invoices
+            expected_payment_secrets.append(derive_payment_secret_from_payment_preimage(preimage))
+        if payment_secret_from_onion not in expected_payment_secrets:
+            log_fail_reason(f'incorrect payment secret {payment_secret_from_onion.hex()} != {expected_payment_secrets[0].hex()}')
+            raise exc_incorrect_or_unknown_pd
         invoice_msat = info.amount_msat
         if not (invoice_msat is None or invoice_msat <= total_msat <= 2 * invoice_msat):
             log_fail_reason(f"total_msat={total_msat} too different from invoice_msat={invoice_msat}")

electrum/lnworker.py

@@ -194,6 +194,8 @@ class ErrorAddingPeer(Exception): pass
     | LnFeatures.OPTION_DATA_LOSS_PROTECT_REQ
     | LnFeatures.OPTION_STATIC_REMOTEKEY_REQ
     | LnFeatures.GOSSIP_QUERIES_REQ
+    | LnFeatures.VAR_ONION_REQ
+    | LnFeatures.PAYMENT_SECRET_REQ
     | LnFeatures.BASIC_MPP_OPT
     | LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT_ELECTRUM
     | LnFeatures.OPTION_SHUTDOWN_ANYSEGWIT_OPT
@@ -1238,7 +1240,7 @@ async def pay_to_node(
             self, *,
             node_pubkey: bytes,
             payment_hash: bytes,
-            payment_secret: Optional[bytes],
+            payment_secret: bytes,
             amount_to_pay: int,  # in msat
             min_cltv_expiry: int,
             r_tags,
@@ -1389,7 +1391,7 @@ async def pay_to_route(
             total_msat: int,
             amount_receiver_msat:int,
             payment_hash: bytes,
-            payment_secret: Optional[bytes],
+            payment_secret: bytes,
             min_cltv_expiry: int,
             trampoline_onion: bytes = None,
             trampoline_fee_level: int,
@@ -1546,8 +1548,7 @@ def _decode_channel_update_msg(cls, chan_upd_msg: bytes) -> Optional[Dict[str, A
             except Exception:
                 return None
 
-    @staticmethod
-    def _check_invoice(invoice: str, *, amount_msat: int = None) -> LnAddr:
+    def _check_invoice(self, invoice: str, *, amount_msat: int = None) -> LnAddr:
         addr = lndecode(invoice)
         if addr.is_expired():
             raise InvoiceError(_("This invoice has expired"))
@@ -1562,6 +1563,7 @@ def _check_invoice(invoice: str, *, amount_msat: int = None) -> LnAddr:
             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()}"))
+        addr.validate_and_compare_features(self.features)
         return addr
 
     def is_trampoline_peer(self, node_id: bytes) -> bool:
@@ -1615,8 +1617,8 @@ async def create_routes_for_payment(
             min_cltv_expiry,
             r_tags,
             invoice_features: int,
-            payment_hash,
-            payment_secret,
+            payment_hash: bytes,
+            payment_secret: bytes,
             trampoline_fee_level: int,
             use_two_trampolines: bool,
             fwd_trampoline_onion=None,