Merge pull request #10230 from f321x/refactor_htlc_handling

lightning: refactor htlc switch

Author: SomberNight

electrum/commands.py

@@ -1389,7 +1389,9 @@ async def add_hold_invoice(
     ) -> dict:
         """
         Create a lightning hold invoice for the given payment hash. Hold invoices have to get settled manually later.
-        HTLCs will get failed automatically if block_height + 144 > htlc.cltv_abs.
+        HTLCs will get failed automatically if block_height + 144 > htlc.cltv_abs, if the intention is to
+        settle them as late as possible a safety margin of some blocks should be used to prevent them
+        from getting failed accidentally.
 
         arg:str:payment_hash:Hex encoded payment hash to be used for the invoice
         arg:decimal:amount:Optional requested amount (in btc)
@@ -1399,7 +1401,7 @@ async def add_hold_invoice(
         """
         assert len(payment_hash) == 64, f"Invalid payment hash length: {len(payment_hash)} != 64"
         assert payment_hash not in wallet.lnworker.payment_info, "Payment hash already used!"
-        assert payment_hash not in wallet.lnworker.dont_settle_htlcs, "Payment hash already used!"
+        assert payment_hash not in wallet.lnworker.dont_expire_htlcs, "Payment hash already used!"
         assert wallet.lnworker.get_preimage(bfh(payment_hash)) is None, "Already got a preimage for this payment hash!"
         assert MIN_FINAL_CLTV_DELTA_ACCEPTED < min_final_cltv_expiry_delta < 576, "Use a sane min_final_cltv_expiry_delta value"
         amount = amount if amount and satoshis(amount) > 0 else None  # make amount either >0 or None
@@ -1419,7 +1421,9 @@ async def add_hold_invoice(
             message=memo,
             fallback_address=None
         )
-        wallet.lnworker.dont_settle_htlcs[payment_hash] = None
+        # this prevents incoming htlcs from getting expired while the preimage isn't set.
+        # If their blocks to expiry fall below MIN_FINAL_CLTV_DELTA_ACCEPTED they will get failed.
+        wallet.lnworker.dont_expire_htlcs[payment_hash] = MIN_FINAL_CLTV_DELTA_ACCEPTED
         wallet.set_label(payment_hash, memo)
         result = {
             "invoice": invoice
@@ -1439,12 +1443,11 @@ async def settle_hold_invoice(self, preimage: str, wallet: Abstract_Wallet = Non
         assert payment_hash not in wallet.lnworker._preimages, f"Invoice {payment_hash=} already settled"
         assert payment_hash in wallet.lnworker.payment_info, \
             f"Couldn't find lightning invoice for {payment_hash=}"
-        assert payment_hash in wallet.lnworker.dont_settle_htlcs, f"Invoice {payment_hash=} not a hold invoice?"
+        assert payment_hash in wallet.lnworker.dont_expire_htlcs, f"Invoice {payment_hash=} not a hold invoice?"
         assert wallet.lnworker.is_complete_mpp(bfh(payment_hash)), \
             f"MPP incomplete, cannot settle hold invoice {payment_hash} yet"
         info: Optional['PaymentInfo'] = wallet.lnworker.get_payment_info(bfh(payment_hash))
         assert (wallet.lnworker.get_payment_mpp_amount_msat(bfh(payment_hash)) or 0) >= (info.amount_msat or 0)
-        del wallet.lnworker.dont_settle_htlcs[payment_hash]
         wallet.lnworker.save_preimage(bfh(payment_hash), bfh(preimage))
         util.trigger_callback('wallet_updated', wallet)
         result = {
@@ -1462,15 +1465,15 @@ async def cancel_hold_invoice(self, payment_hash: str, wallet: Abstract_Wallet =
         assert payment_hash in wallet.lnworker.payment_info, \
             f"Couldn't find lightning invoice for payment hash {payment_hash}"
         assert payment_hash not in wallet.lnworker._preimages, "Cannot cancel anymore, preimage already given."
-        assert payment_hash in wallet.lnworker.dont_settle_htlcs, f"{payment_hash=} not a hold invoice?"
+        assert payment_hash in wallet.lnworker.dont_expire_htlcs, f"{payment_hash=} not a hold invoice?"
         # set to PR_UNPAID so it can get deleted
         wallet.lnworker.set_payment_status(bfh(payment_hash), PR_UNPAID)
         wallet.lnworker.delete_payment_info(payment_hash)
         wallet.set_label(payment_hash, None)
+        del wallet.lnworker.dont_expire_htlcs[payment_hash]
         while wallet.lnworker.is_complete_mpp(bfh(payment_hash)):
-            # wait until the htlcs got failed so the payment won't get settled accidentally in a race
+            # block until the htlcs got failed
             await asyncio.sleep(0.1)
-        del wallet.lnworker.dont_settle_htlcs[payment_hash]
         result = {
             "cancelled": payment_hash
         }
@@ -1503,15 +1506,14 @@ async def check_hold_invoice(self, payment_hash: str, wallet: Abstract_Wallet =
         elif not is_complete_mpp and not wallet.lnworker.get_preimage_hex(payment_hash):
             # is_complete_mpp is False for settled payments
             result["status"] = "unpaid"
-        elif is_complete_mpp and payment_hash in wallet.lnworker.dont_settle_htlcs:
+        elif is_complete_mpp and payment_hash in wallet.lnworker.dont_expire_htlcs:
             result["status"] = "paid"
             payment_key: str = wallet.lnworker._get_payment_key(bfh(payment_hash)).hex()
             htlc_status = wallet.lnworker.received_mpp_htlcs[payment_key]
             result["closest_htlc_expiry_height"] = min(
-                htlc.cltv_abs for _, htlc in htlc_status.htlc_set
+                mpp_htlc.htlc.cltv_abs for mpp_htlc in htlc_status.htlcs
             )
-        elif wallet.lnworker.get_preimage_hex(payment_hash) is not None \
-                and payment_hash not in wallet.lnworker.dont_settle_htlcs:
+        elif wallet.lnworker.get_preimage_hex(payment_hash) is not None:
             result["status"] = "settled"
             plist = wallet.lnworker.get_payments(status='settled')[bfh(payment_hash)]
             _dir, amount_msat, _fee, _ts = wallet.lnworker.get_payment_value(info, plist)

electrum/lnchannel.py

@@ -783,8 +783,8 @@ def __init__(self, state: 'StoredDict', *, name=None, lnworker=None, initial_fee
         self.onion_keys = state['onion_keys']  # type: Dict[int, bytes]
         self.data_loss_protect_remote_pcp = state['data_loss_protect_remote_pcp']
         self.hm = HTLCManager(log=state['log'], initial_feerate=initial_feerate)
-        self.unfulfilled_htlcs = state["unfulfilled_htlcs"]  # type: Dict[int, Tuple[str, Optional[str]]]
-        # ^ htlc_id -> onion_packet_hex, forwarding_key
+        self.unfulfilled_htlcs = state["unfulfilled_htlcs"]  # type: Dict[int, Optional[str]]
+        # ^ htlc_id -> onion_packet_hex
         self._state = ChannelState[state['state']]
         self.peer_state = PeerState.DISCONNECTED
         self._outgoing_channel_update = None  # type: Optional[bytes]
@@ -1112,6 +1112,7 @@ def _assert_can_add_htlc(self, *, htlc_proposer: HTLCOwner, amount_msat: int,
             if amount_msat <= 0:
                 raise PaymentFailure("HTLC value must be positive")
             if amount_msat < chan_config.htlc_minimum_msat:
+                # todo: for incoming htlcs this could be handled more gracefully with `amount_below_minimum`
                 raise PaymentFailure(f'HTLC value too small: {amount_msat} msat')
 
         if self.htlc_slots_left(htlc_proposer) == 0:
@@ -1226,7 +1227,7 @@ def receive_htlc(self, htlc: UpdateAddHtlc, onion_packet:bytes = None) -> Update
         with self.db_lock:
             self.hm.recv_htlc(htlc)
             if onion_packet:
-                self.unfulfilled_htlcs[htlc.htlc_id] = onion_packet.hex(), None
+                self.unfulfilled_htlcs[htlc.htlc_id] = onion_packet.hex()
 
         self.logger.info("receive_htlc")
         return htlc

electrum/lnonion.py

@@ -26,7 +26,8 @@
 import io
 import hashlib
 from functools import cached_property
-from typing import Sequence, List, Tuple, NamedTuple, TYPE_CHECKING, Dict, Any, Optional, Union, Mapping
+from typing import (Sequence, List, Tuple, NamedTuple, TYPE_CHECKING, Dict, Any, Optional, Union,
+                    Mapping, Iterator)
 from enum import IntEnum
 from dataclasses import dataclass, field, replace
 from types import MappingProxyType
@@ -485,6 +486,55 @@ def process_onion_packet(
     return ProcessedOnionPacket(are_we_final, hop_data, next_onion_packet, trampoline_onion_packet)
 
 
+def compare_trampoline_onions(
+    trampoline_onions: Iterator[Optional[ProcessedOnionPacket]],
+    *,
+    exclude_amt_to_fwd: bool = False,
+) -> bool:
+    """
+    compare values of trampoline onions payloads and are_we_final.
+    If we are receiver of a multi trampoline payment amt_to_fwd can differ between the trampoline
+    parts of the payment, so it needs to be excluded from the comparison when comparing all trampoline
+    onions of the whole payment (however it can be compared between the onions in a single trampoline part).
+    """
+    try:
+        first_onion = next(trampoline_onions)
+    except StopIteration:
+        raise ValueError("nothing to compare")
+
+    if first_onion is None:
+        # we don't support mixed mpp sets of htlcs with trampoline onions and regular non-trampoline htlcs.
+        # In theory this could happen if a sender e.g. uses trampoline as fallback to deliver
+        # outstanding mpp parts if local pathfinding wasn't successful for the whole payment,
+        # resulting in a mixed payment. However, it's not even clear if the spec allows for such a constellation.
+        return all(onion is None for onion in trampoline_onions)
+    assert isinstance(first_onion, ProcessedOnionPacket), f"{first_onion=}"
+
+    are_we_final = first_onion.are_we_final
+    payload = first_onion.hop_data.payload
+    total_msat = first_onion.total_msat
+    outgoing_cltv = first_onion.outgoing_cltv_value
+    payment_secret = first_onion.payment_secret
+    for onion in trampoline_onions:
+        if onion is None:
+            return False
+        assert isinstance(onion, ProcessedOnionPacket), f"{onion=}"
+        assert onion.trampoline_onion_packet is None, f"{onion=} cannot have trampoline_onion_packet"
+        if onion.are_we_final != are_we_final:
+            return False
+        if not exclude_amt_to_fwd:
+            if onion.hop_data.payload != payload:
+                return False
+        else:
+            if onion.total_msat != total_msat:
+                return False
+            if onion.outgoing_cltv_value != outgoing_cltv:
+                return False
+            if onion.payment_secret != payment_secret:
+                return False
+    return True
+
+
 class FailedToDecodeOnionError(Exception): pass
 
 
@@ -521,6 +571,21 @@ def decode_data(self) -> Optional[Dict[str, Any]]:
             payload = None
         return payload
 
+    def to_wire_msg(self, onion_packet: OnionPacket, privkey: bytes, local_height: int) -> bytes:
+        onion_error = construct_onion_error(self, onion_packet.public_key, privkey, local_height)
+        error_bytes = obfuscate_onion_error(onion_error, onion_packet.public_key, privkey)
+        return error_bytes
+
+
+class OnionParsingError(OnionRoutingFailure):
+    """
+    Onion parsing error will cause a htlc to get failed with update_fail_malformed_htlc.
+    Using INVALID_ONION_VERSION as there is no unspecific BADONION failure code defined in the spec
+    for the case we just cannot parse the onion.
+    """
+    def __init__(self, data: bytes):
+        OnionRoutingFailure.__init__(self, code=OnionFailureCode.INVALID_ONION_VERSION, data=data)
+
 
 def construct_onion_error(
         error: OnionRoutingFailure,