From 53a8453e3b12937ce4e32e41e7348258c2fbf0de Mon Sep 17 00:00:00 2001 From: SomberNight Date: Fri, 27 Oct 2023 14:22:01 +0000 Subject: [PATCH] trampoline: fix off-by-one confusion of fees The convention is that edges (start_node -> edge_node) store the policy/fees for the *start_node*. This is what the non-trampoline edges were already using (for a long time), but the trampoline ones were off-by-one (policy was for end_node), which was then worked around in multiple places, to correct for... i.e. I think because of all the workarounds, there was no actual bug, but it was just very confusing. Also note that the prior usage of trampoline edges would not work if we (sender) were not directly connected to a TF (trampoline-forwarder) but had extra edges in the route to even get to the first TF. Having the policy corresponding to the start_node of the edge would work even in that case. --- electrum/lnonion.py | 9 ++----- electrum/lnrouter.py | 20 +++++++-------- electrum/lnworker.py | 2 +- electrum/trampoline.py | 56 ++++++++++++++++++++++++------------------ 4 files changed, 45 insertions(+), 42 deletions(-) diff --git a/electrum/lnonion.py b/electrum/lnonion.py index 71d1af179..d30e5ad14 100644 --- a/electrum/lnonion.py +++ b/electrum/lnonion.py @@ -241,10 +241,6 @@ def calc_hops_data_for_payment( # payloads, backwards from last hop (but excluding the first edge): for edge_index in range(len(route) - 1, 0, -1): route_edge = route[edge_index] - is_trampoline = route_edge.is_trampoline() - if is_trampoline: - amt += route_edge.fee_for_edge(amt) - cltv_abs += route_edge.cltv_delta hop_payload = { "amt_to_forward": {"amt_to_forward": amt}, "outgoing_cltv_value": {"outgoing_cltv_value": cltv_abs}, @@ -252,9 +248,8 @@ def calc_hops_data_for_payment( } hops_data.append( OnionHopsDataSingle(payload=hop_payload)) - if not is_trampoline: - amt += route_edge.fee_for_edge(amt) - cltv_abs += route_edge.cltv_delta + amt += route_edge.fee_for_edge(amt) + cltv_abs += route_edge.cltv_delta hops_data.reverse() return hops_data, amt, cltv_abs diff --git a/electrum/lnrouter.py b/electrum/lnrouter.py index 8b8ea91a0..8be752b1a 100644 --- a/electrum/lnrouter.py +++ b/electrum/lnrouter.py @@ -73,10 +73,10 @@ class PathEdge: @attr.s class RouteEdge(PathEdge): - fee_base_msat = attr.ib(type=int, kw_only=True) - fee_proportional_millionths = attr.ib(type=int, kw_only=True) - cltv_delta = attr.ib(type=int, kw_only=True) - node_features = attr.ib(type=int, kw_only=True, repr=lambda val: str(int(val))) # note: for end node! + fee_base_msat = attr.ib(type=int, kw_only=True) # for start_node + fee_proportional_millionths = attr.ib(type=int, kw_only=True) # for start_node + cltv_delta = attr.ib(type=int, kw_only=True) # for start_node + node_features = attr.ib(type=int, kw_only=True, repr=lambda val: str(int(val))) # note: for end_node! def fee_for_edge(self, amount_msat: int) -> int: return fee_for_edge_msat(forwarded_amount_msat=amount_msat, @@ -87,7 +87,7 @@ class RouteEdge(PathEdge): def from_channel_policy( cls, *, - channel_policy: 'Policy', + channel_policy: 'Policy', # for start_node short_channel_id: bytes, start_node: bytes, end_node: bytes, @@ -138,26 +138,26 @@ LNPaymentRoute = Sequence[RouteEdge] LNPaymentTRoute = Sequence[TrampolineEdge] -def is_route_sane_to_use(route: LNPaymentRoute, invoice_amount_msat: int, min_final_cltv_delta: int) -> bool: +def is_route_sane_to_use(route: LNPaymentRoute, *, amount_msat_for_dest: int, cltv_delta_for_dest: int) -> bool: """Run some sanity checks on the whole route, before attempting to use it. called when we are paying; so e.g. lower cltv is better """ if len(route) > NUM_MAX_EDGES_IN_PAYMENT_PATH: return False - amt = invoice_amount_msat - cltv_delta = min_final_cltv_delta + amt = amount_msat_for_dest + cltv_delta = cltv_delta_for_dest for route_edge in reversed(route[1:]): if not route_edge.is_sane_to_use(amt): return False amt += route_edge.fee_for_edge(amt) cltv_delta += route_edge.cltv_delta - total_fee = amt - invoice_amount_msat + total_fee = amt - amount_msat_for_dest # TODO revise ad-hoc heuristics if cltv_delta > NBLOCK_CLTV_DELTA_TOO_FAR_INTO_FUTURE: return False # FIXME in case of MPP, the fee checks are done independently for each part, # which is ok for the proportional checks but not for the absolute ones. # This is not that big of a deal though as we don't split into *too many* parts. - if not is_fee_sane(total_fee, payment_amount_msat=invoice_amount_msat): + if not is_fee_sane(total_fee, payment_amount_msat=amount_msat_for_dest): return False return True diff --git a/electrum/lnworker.py b/electrum/lnworker.py index 61773aae6..06844c9cf 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -2023,7 +2023,7 @@ class LNWallet(LNWorker): if not route: raise NoPathFound() # test sanity - if not is_route_sane_to_use(route, amount_msat, min_final_cltv_delta): + if not is_route_sane_to_use(route, amount_msat_for_dest=amount_msat, cltv_delta_for_dest=min_final_cltv_delta): self.logger.info(f"rejecting insane route {route}") raise NoPathFound() assert len(route) > 0 diff --git a/electrum/trampoline.py b/electrum/trampoline.py index c97ac4474..04fa45f55 100644 --- a/electrum/trampoline.py +++ b/electrum/trampoline.py @@ -5,7 +5,7 @@ import random from typing import Mapping, DefaultDict, Tuple, Optional, Dict, List, Iterable, Sequence, Set from .lnutil import LnFeatures -from .lnonion import calc_hops_data_for_payment, new_onion_packet +from .lnonion import calc_hops_data_for_payment, new_onion_packet, OnionPacket from .lnrouter import RouteEdge, TrampolineEdge, LNPaymentRoute, is_route_sane_to_use, LNPaymentTRoute from .lnutil import NoPathFound, LNPeerAddr from . import constants @@ -168,14 +168,19 @@ def trampoline_policy( def _extend_trampoline_route( - route: List, - start_node: bytes, + route: List[TrampolineEdge], + *, + start_node: bytes = None, end_node: bytes, trampoline_fee_level: int, - pay_fees=True + pay_fees: bool = True, ): """Extends the route and modifies it in place.""" + if start_node is None: + assert route + start_node = route[-1].end_node trampoline_features = LnFeatures.VAR_ONION_OPT + # get policy for *start_node* policy = trampoline_policy(trampoline_fee_level) route.append( TrampolineEdge( @@ -223,17 +228,19 @@ def create_trampoline_route( # we build a route of trampoline hops and extend the route list in place route = [] - second_trampoline = None # our first trampoline hop is decided by the channel we use - _extend_trampoline_route(route, my_pubkey, my_trampoline, trampoline_fee_level) + _extend_trampoline_route( + route, start_node=my_pubkey, end_node=my_trampoline, + trampoline_fee_level=trampoline_fee_level, pay_fees=False, + ) if is_legacy: # we add another different trampoline hop for privacy if use_two_trampolines: trampolines = trampolines_by_id() second_trampoline = _choose_second_trampoline(my_trampoline, list(trampolines.keys()), failed_routes) - _extend_trampoline_route(route, my_trampoline, second_trampoline, trampoline_fee_level) + _extend_trampoline_route(route, end_node=second_trampoline, trampoline_fee_level=trampoline_fee_level) # the last trampoline onion must contain routing hints for the last trampoline # node to find the recipient invoice_routing_info = encode_routing_info(r_tags) @@ -255,14 +262,17 @@ def create_trampoline_route( add_trampoline = True if add_trampoline: second_trampoline = _choose_second_trampoline(my_trampoline, invoice_trampolines, failed_routes) - _extend_trampoline_route(route, my_trampoline, second_trampoline, trampoline_fee_level) + _extend_trampoline_route(route, end_node=second_trampoline, trampoline_fee_level=trampoline_fee_level) - # final edge (not part of the route if payment is legacy, but eclair requires an encrypted blob) - _extend_trampoline_route(route, route[-1].end_node, invoice_pubkey, trampoline_fee_level, pay_fees=False) + # Add final edge. note: eclair requires an encrypted t-onion blob even in legacy case. + # Also needed for fees for last TF! + _extend_trampoline_route(route, end_node=invoice_pubkey, trampoline_fee_level=trampoline_fee_level) # check that we can pay amount and fees - for edge in route[::-1]: - amount_msat += edge.fee_for_edge(amount_msat) - if not is_route_sane_to_use(route, amount_msat, min_final_cltv_delta): + if not is_route_sane_to_use( + route=route, + amount_msat_for_dest=amount_msat, + cltv_delta_for_dest=min_final_cltv_delta, + ): raise NoPathFound("We cannot afford to pay the fees.") return route @@ -275,7 +285,7 @@ def create_trampoline_onion( total_msat: int, payment_hash: bytes, payment_secret: bytes, -): +) -> Tuple[OnionPacket, int, int]: # all edges are trampoline hops_data, amount_msat, cltv_abs = calc_hops_data_for_payment( route, @@ -318,20 +328,21 @@ def create_trampoline_onion( def create_trampoline_route_and_onion( *, - amount_msat, - total_msat, + amount_msat: int, # that final receiver gets + total_msat: int, min_final_cltv_delta: int, - invoice_pubkey, + invoice_pubkey: bytes, invoice_features, my_pubkey: bytes, - node_id, + node_id: bytes, r_tags, payment_hash: bytes, payment_secret: bytes, local_height: int, trampoline_fee_level: int, use_two_trampolines: bool, - failed_routes: Iterable[Sequence[str]]): + failed_routes: Iterable[Sequence[str]], +) -> Tuple[LNPaymentTRoute, OnionPacket, int, int]: # create route for the trampoline_onion trampoline_route = create_trampoline_route( amount_msat=amount_msat, @@ -343,7 +354,8 @@ def create_trampoline_route_and_onion( r_tags=r_tags, trampoline_fee_level=trampoline_fee_level, use_two_trampolines=use_two_trampolines, - failed_routes=failed_routes) + failed_routes=failed_routes, + ) # compute onion and fees final_cltv_abs = local_height + min_final_cltv_delta trampoline_onion, amount_with_fees, bucket_cltv_abs = create_trampoline_onion( @@ -354,8 +366,4 @@ def create_trampoline_route_and_onion( payment_hash=payment_hash, payment_secret=payment_secret) bucket_cltv_delta = bucket_cltv_abs - local_height - bucket_cltv_delta += trampoline_route[0].cltv_delta - # trampoline fee for this very trampoline - trampoline_fee = trampoline_route[0].fee_for_edge(amount_with_fees) - amount_with_fees += trampoline_fee return trampoline_route, trampoline_onion, amount_with_fees, bucket_cltv_delta