From 3a7f5373ac859adffe4ab0ddae07b3d96fb33217 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Fri, 2 Jul 2021 18:44:39 +0200 Subject: [PATCH] trampoline: improve payment success - better error handling: previously we stopped all attempts on any of TRAMPOLINE_EXPIRY_TOO_SOON, UNKNOWN_NEXT_PEER, TEMPORARY_NODE_FAILURE. Instead we should retry (but see code comments). - previously payments failed if ALL of the following criteria applied: - sender is paying via trampoline, but not via the ACINQ node (which is special cased) - receiver only has private channels and has put r_tags into invoice, along with setting the trampoline feature bit in the invoice, however the receiver is not connected to any trampoline forwarders directly The sender would then assume that the private routing hints in the invoice correspond to trampoline forwarders. - also, previously if both the sender and the recipient used trampoline and they shared a trampoline forwarder (that they were both connected to), the private channels the recipient had (with nodes other than the shared TF) would never be attempted. --- electrum/lnchannel.py | 3 +- electrum/lnpeer.py | 7 ++++ electrum/lnworker.py | 10 ++++- electrum/tests/test_lnchannel.py | 4 +- electrum/tests/test_lnpeer.py | 24 +++++++++--- electrum/trampoline.py | 65 +++++++++++++++++++------------- 6 files changed, 77 insertions(+), 36 deletions(-) diff --git a/electrum/lnchannel.py b/electrum/lnchannel.py index 1d710a5ec..908ce6e7a 100644 --- a/electrum/lnchannel.py +++ b/electrum/lnchannel.py @@ -1053,8 +1053,7 @@ class Channel(AbstractChannel): # if we are forwarding, save error message to disk if self.lnworker.get_payment_info(htlc.payment_hash) is None: self.save_fail_htlc_reason(htlc.htlc_id, error_bytes, failure_message) - else: - self.lnworker.htlc_failed(self, htlc.payment_hash, htlc.htlc_id, error_bytes, failure_message) + self.lnworker.htlc_failed(self, htlc.payment_hash, htlc.htlc_id, error_bytes, failure_message) def save_fail_htlc_reason( self, diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 9553281e6..c2a48fff4 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -1455,6 +1455,12 @@ class Peer(Logger): htlc: UpdateAddHtlc, trampoline_onion: ProcessedOnionPacket): + forwarding_enabled = self.network.config.get('lightning_forward_payments', False) + forwarding_trampoline_enabled = self.network.config.get('lightning_forward_trampoline_payments', False) + if not (forwarding_enabled and forwarding_trampoline_enabled): + self.logger.info(f"trampoline forwarding is disabled. failing htlc.") + raise OnionRoutingFailure(code=OnionFailureCode.PERMANENT_CHANNEL_FAILURE, data=b'') + payload = trampoline_onion.hop_data.payload payment_hash = htlc.payment_hash payment_secret = os.urandom(32) @@ -1467,6 +1473,7 @@ class Peer(Logger): next_trampoline_onion = None invoice_features = payload["invoice_features"]["invoice_features"] invoice_routing_info = payload["invoice_routing_info"]["invoice_routing_info"] + # TODO use invoice_routing_info else: self.logger.info('forward_trampoline: end-to-end') invoice_features = LnFeatures.BASIC_MPP_OPT diff --git a/electrum/lnworker.py b/electrum/lnworker.py index a926ad618..b2730702c 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -1220,12 +1220,20 @@ class LNWallet(LNWorker): raise PaymentFailure(failure_msg.code_name()) # trampoline if not self.channel_db: - if code == OnionFailureCode.TRAMPOLINE_FEE_INSUFFICIENT: + # FIXME The trampoline nodes in the path are chosen randomly. + # Some of the errors might depend on how we have chosen them. + # Having more attempts is currently useful in part because of the randomness, + # instead we should give feedback to create_routes_for_payment. + if code in (OnionFailureCode.TRAMPOLINE_FEE_INSUFFICIENT, + OnionFailureCode.TRAMPOLINE_EXPIRY_TOO_SOON): # todo: parse the node parameters here (not returned by eclair yet) trampoline_fee_level += 1 continue elif use_two_trampolines: use_two_trampolines = False + elif code in (OnionFailureCode.UNKNOWN_NEXT_PEER, + OnionFailureCode.TEMPORARY_NODE_FAILURE): + continue else: raise PaymentFailure(failure_msg.code_name()) else: diff --git a/electrum/tests/test_lnchannel.py b/electrum/tests/test_lnchannel.py index e4d0e2bf8..d8009734e 100644 --- a/electrum/tests/test_lnchannel.py +++ b/electrum/tests/test_lnchannel.py @@ -151,7 +151,7 @@ def create_test_channels(*, feerate=6000, local_msat=None, remote_msat=None, bob_first, other_node_id=bob_pubkey, l_dust=200, r_dust=1300, l_csv=5, r_csv=4 ), - name=bob_name, + name=f"{alice_name}->{bob_name}", initial_feerate=feerate), lnchannel.Channel( create_channel_state( @@ -160,7 +160,7 @@ def create_test_channels(*, feerate=6000, local_msat=None, remote_msat=None, alice_first, other_node_id=alice_pubkey, l_dust=1300, r_dust=200, l_csv=4, r_csv=5 ), - name=alice_name, + name=f"{bob_name}->{alice_name}", initial_feerate=feerate) ) diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index 81a17b7cc..cda942b9d 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -12,6 +12,8 @@ from typing import Iterable, NamedTuple, Tuple, List, Dict from aiorpcx import TaskGroup, timeout_after, TaskTimeout +import electrum +import electrum.trampoline from electrum import bitcoin from electrum import constants from electrum.network import Network @@ -152,6 +154,8 @@ class MockLNWallet(Logger, NetworkRetryManager[LNPeerAddr]): self.preimages = {} self.stopping_soon = False + self.logger.info(f"created LNWallet[{name}] with nodeID={local_keypair.pubkey.hex()}") + def get_invoice_status(self, key): pass @@ -272,8 +276,8 @@ class PutIntoOthersQueueTransport(MockTransport): self.other_mock_transport.queue.put_nowait(data) def transport_pair(k1, k2, name1, name2): - t1 = PutIntoOthersQueueTransport(k1, name2) - t2 = PutIntoOthersQueueTransport(k2, name1) + t1 = PutIntoOthersQueueTransport(k1, name1) + t2 = PutIntoOthersQueueTransport(k2, name2) t1.other_mock_transport = t2 t2.other_mock_transport = t1 return t1, t2 @@ -341,7 +345,7 @@ class TestPeer(TestCaseForTestnet): self._loop_thread.join(timeout=1) super().tearDown() - def prepare_peers(self, alice_channel, bob_channel): + def prepare_peers(self, alice_channel: Channel, bob_channel: Channel): k1, k2 = keypair(), keypair() alice_channel.node_id = k2.pubkey bob_channel.node_id = k1.pubkey @@ -424,6 +428,8 @@ class TestPeer(TestCaseForTestnet): w_b.network.config.set_key('lightning_forward_payments', True) w_c.network.config.set_key('lightning_forward_payments', True) + w_b.network.config.set_key('lightning_forward_trampoline_payments', True) + w_c.network.config.set_key('lightning_forward_trampoline_payments', True) # forwarding fees, etc chan_ab.forwarding_fee_proportional_millionths *= 500 @@ -448,7 +454,7 @@ class TestPeer(TestCaseForTestnet): peer_cd.mark_open(chan_cd) peer_db.mark_open(chan_db) peer_dc.mark_open(chan_dc) - return SquareGraph( + graph = SquareGraph( w_a=w_a, w_b=w_b, w_c=w_c, @@ -470,6 +476,7 @@ class TestPeer(TestCaseForTestnet): chan_db=chan_db, chan_dc=chan_dc, ) + return graph @staticmethod async def prepare_invoice( @@ -935,7 +942,14 @@ class TestPeer(TestCaseForTestnet): def test_multipart_payment_with_trampoline(self): # single attempt will fail with insufficient trampoline fee graph = self.prepare_chans_and_peers_in_square() - self._run_mpp(graph, {'alice_uses_trampoline':True, 'attempts':1}, {'alice_uses_trampoline':True, 'attempts':3}) + electrum.trampoline._TRAMPOLINE_NODES_UNITTESTS = { + graph.w_b.name: LNPeerAddr(host="127.0.0.1", port=9735, pubkey=graph.w_b.node_keypair.pubkey), + graph.w_c.name: LNPeerAddr(host="127.0.0.1", port=9735, pubkey=graph.w_c.node_keypair.pubkey), + } + try: + self._run_mpp(graph, {'alice_uses_trampoline':True, 'attempts':1}, {'alice_uses_trampoline':True, 'attempts':30}) + finally: + electrum.trampoline._TRAMPOLINE_NODES_UNITTESTS = {} @needs_test_with_all_chacha20_implementations def test_fail_pending_htlcs_on_shutdown(self): diff --git a/electrum/trampoline.py b/electrum/trampoline.py index f49f6fbda..d5c92e11a 100644 --- a/electrum/trampoline.py +++ b/electrum/trampoline.py @@ -2,6 +2,8 @@ import os import bitstring import random +from typing import Mapping + from .logging import get_logger, Logger from .lnutil import LnFeatures from .lnonion import calc_hops_data_for_payment, new_onion_packet @@ -67,19 +69,24 @@ TRAMPOLINE_NODES_SIGNET = { 'wakiyamap.dev': LNPeerAddr(host='signet-electrumx.wakiyamap.dev', port=9735, pubkey=bytes.fromhex('02dadf6c28f3284d591cd2a4189d1530c1ff82c07059ebea150a33ab76e7364b4a')), } -def hardcoded_trampoline_nodes(): +_TRAMPOLINE_NODES_UNITTESTS = {} # used in unit tests + +def hardcoded_trampoline_nodes() -> Mapping[str, LNPeerAddr]: + nodes = {} if constants.net.NET_NAME == "mainnet": - return TRAMPOLINE_NODES_MAINNET - if constants.net.NET_NAME == "testnet": - return TRAMPOLINE_NODES_TESTNET - if constants.net.NET_NAME == "signet": - return TRAMPOLINE_NODES_SIGNET - return {} + nodes.update(TRAMPOLINE_NODES_MAINNET) + elif constants.net.NET_NAME == "testnet": + nodes.update(TRAMPOLINE_NODES_TESTNET) + elif constants.net.NET_NAME == "signet": + nodes.update(TRAMPOLINE_NODES_SIGNET) + nodes.update(_TRAMPOLINE_NODES_UNITTESTS) + return nodes def trampolines_by_id(): return dict([(x.pubkey, x) for x in hardcoded_trampoline_nodes().values()]) -is_hardcoded_trampoline = lambda node_id: node_id in trampolines_by_id().keys() +def is_hardcoded_trampoline(node_id: bytes) -> bool: + return node_id in trampolines_by_id() def encode_routing_info(r_tags): result = bitstring.BitArray() @@ -103,13 +110,26 @@ def create_trampoline_route( trampoline_fee_level: int, use_two_trampolines: bool) -> LNPaymentRoute: + # figure out whether we can use end-to-end trampoline, or fallback to pay-to-legacy + is_legacy = True + r_tag_chosen_for_e2e_trampoline = None invoice_features = LnFeatures(invoice_features) - if invoice_features.supports(LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT)\ - or invoice_features.supports(LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT_ECLAIR): - is_legacy = False - else: - is_legacy = True - + if (invoice_features.supports(LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT) + or invoice_features.supports(LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT_ECLAIR)): + if not r_tags: # presumably the recipient has public channels + is_legacy = False + else: + # - We choose one routing hint at random, and + # use end-to-end trampoline if that node is a trampoline-forwarder (TF). + # - In case of e2e, the route will have either one or two TFs (one neighbour of sender, + # and one neighbour of recipient; and these might coincide). Note that there are some + # channel layouts where two TFs are needed for a payment to succeed, e.g. both + # endpoints connected to T1 and T2, and sender only has send-capacity with T1, while + # recipient only has recv-capacity with T2. + singlehop_r_tags = [x for x in r_tags if len(x) == 1] + r_tag_chosen_for_e2e_trampoline = random.choice(singlehop_r_tags)[0] + pubkey, scid, feebase, feerate, cltv = r_tag_chosen_for_e2e_trampoline + is_legacy = not is_hardcoded_trampoline(pubkey) # fee level. the same fee is used for all trampolines if trampoline_fee_level < len(TRAMPOLINE_FEES): params = TRAMPOLINE_FEES[trampoline_fee_level] @@ -157,20 +177,13 @@ def create_trampoline_route( route[-1].invoice_routing_info = invoice_routing_info route[-1].invoice_features = invoice_features route[-1].outgoing_node_id = invoice_pubkey - else: - last_trampoline = route[-1].end_node - r_tags = [x for x in r_tags if len(x) == 1] - random.shuffle(r_tags) - for r_tag in r_tags: - pubkey, scid, feebase, feerate, cltv = r_tag[0] - if pubkey == trampoline_node_id: - break - else: - pubkey, scid, feebase, feerate, cltv = r_tag[0] - if route[-1].node_id != pubkey: + else: # end-to-end trampoline + if r_tag_chosen_for_e2e_trampoline: + pubkey, scid, feebase, feerate, cltv = r_tag_chosen_for_e2e_trampoline + if route[-1].end_node != pubkey: route.append( TrampolineEdge( - start_node=route[-1].node_id, + start_node=route[-1].end_node, end_node=pubkey, fee_base_msat=feebase, fee_proportional_millionths=feerate,