From 68bf714ae60c9a30755fa2105be04352580ad8b1 Mon Sep 17 00:00:00 2001 From: ThomasV Date: Fri, 9 Sep 2022 19:52:36 +0200 Subject: [PATCH] Trampoline: Remember failed routes (fixes #7967). Upon receiving UNKNOWN_NEXT_PEER, TEMPORARY_NODE_FAILURE or TEMPORARY_CHANNEL_FAILURE, remember the trampoline route that was used, and try other routes before raising the trampoline fee. Before this commit, we used to raise the trampoline fee upon receiving any error message, in order to ensure termination of the payment loop. Note that we will still retry failing routes after we have raised the trampoline fee. This choice is questionable, it is unclear if doing so significantly increases the probability of success. Tests: add a test for trampoline handling of UNKNOWN_NEXT_PEER --- electrum/lnworker.py | 38 +++++++++++++------- electrum/tests/test_lnchannel.py | 4 +-- electrum/tests/test_lnpeer.py | 60 +++++++++++++++++++++++++------- electrum/trampoline.py | 51 +++++++++++++++++++-------- 4 files changed, 110 insertions(+), 43 deletions(-) diff --git a/electrum/lnworker.py b/electrum/lnworker.py index ade2adfcc..14cda39ba 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -1225,6 +1225,7 @@ class LNWallet(LNWorker): # of privacy use_two_trampolines = True self.trampoline_fee_level = self.INITIAL_TRAMPOLINE_FEE_LEVEL + self.failed_trampoline_routes = [] start_time = time.time() amount_inflight = 0 # what we sent in htlcs (that receiver gets, without fees) while True: @@ -1249,7 +1250,7 @@ class LNWallet(LNWorker): channels=channels, ) # 2. send htlcs - async for route, amount_msat, total_msat, amount_receiver_msat, cltv_delta, bucket_payment_secret, trampoline_onion in routes: + async for route, amount_msat, total_msat, amount_receiver_msat, cltv_delta, bucket_payment_secret, trampoline_onion, trampoline_route in routes: amount_inflight += amount_receiver_msat if amount_inflight > amount_to_pay: # safety belts raise Exception(f"amount_inflight={amount_inflight} > amount_to_pay={amount_to_pay}") @@ -1262,7 +1263,8 @@ class LNWallet(LNWorker): payment_secret=bucket_payment_secret, min_cltv_expiry=cltv_delta, trampoline_onion=trampoline_onion, - trampoline_fee_level=self.trampoline_fee_level) + trampoline_fee_level=self.trampoline_fee_level, + trampoline_route=trampoline_route) util.trigger_callback('invoice_status', self.wallet, payment_hash.hex()) # 3. await a queue self.logger.info(f"amount inflight {amount_inflight}") @@ -1301,6 +1303,7 @@ class LNWallet(LNWorker): def maybe_raise_trampoline_fee(htlc_log): if htlc_log.trampoline_fee_level == self.trampoline_fee_level: self.trampoline_fee_level += 1 + self.failed_trampoline_routes = [] self.logger.info(f'raising trampoline fee level {self.trampoline_fee_level}') else: self.logger.info(f'NOT raising trampoline fee level, already at {self.trampoline_fee_level}') @@ -1322,7 +1325,11 @@ class LNWallet(LNWorker): OnionFailureCode.UNKNOWN_NEXT_PEER, OnionFailureCode.TEMPORARY_NODE_FAILURE, OnionFailureCode.TEMPORARY_CHANNEL_FAILURE): - maybe_raise_trampoline_fee(htlc_log) + trampoline_route = htlc_log.route + r = [hop.end_node.hex() for hop in trampoline_route] + self.logger.info(f'failed trampoline route: {r}') + assert r not in self.failed_trampoline_routes + self.failed_trampoline_routes.append(r) continue else: raise PaymentFailure(failure_msg.code_name()) @@ -1340,7 +1347,8 @@ class LNWallet(LNWorker): payment_secret: Optional[bytes], min_cltv_expiry: int, trampoline_onion: bytes = None, - trampoline_fee_level: int) -> None: + trampoline_fee_level: int, + trampoline_route: Optional[List]) -> None: # send a single htlc short_channel_id = route[0].short_channel_id @@ -1360,7 +1368,7 @@ class LNWallet(LNWorker): trampoline_onion=trampoline_onion) key = (payment_hash, short_channel_id, htlc.htlc_id) - self.sent_htlcs_info[key] = route, payment_secret, amount_msat, total_msat, amount_receiver_msat, trampoline_fee_level + self.sent_htlcs_info[key] = route, payment_secret, amount_msat, total_msat, amount_receiver_msat, trampoline_fee_level, trampoline_route # if we sent MPP to a trampoline, add item to sent_buckets if not self.channel_db and amount_msat != total_msat: if payment_secret not in self.sent_buckets: @@ -1588,7 +1596,8 @@ class LNWallet(LNWorker): payment_secret=payment_secret, local_height=local_height, trampoline_fee_level=trampoline_fee_level, - use_two_trampolines=use_two_trampolines) + use_two_trampolines=use_two_trampolines, + failed_routes=self.failed_trampoline_routes) trampoline_payment_secret = os.urandom(32) trampoline_total_msat = amount_with_fees if chan.available_to_spend(LOCAL, strict=True) < amount_with_fees: @@ -1605,7 +1614,7 @@ class LNWallet(LNWorker): cltv_expiry_delta=0, node_features=trampoline_features) ] - yield route, amount_with_fees, trampoline_total_msat, amount_msat, cltv_delta, trampoline_payment_secret, trampoline_onion + yield route, amount_with_fees, trampoline_total_msat, amount_msat, cltv_delta, trampoline_payment_secret, trampoline_onion, trampoline_route break else: raise NoPathFound() @@ -1622,7 +1631,7 @@ class LNWallet(LNWorker): full_path=full_path ) ) - yield route, amount_msat, final_total_msat, amount_msat, min_cltv_expiry, payment_secret, fwd_trampoline_onion + yield route, amount_msat, final_total_msat, amount_msat, min_cltv_expiry, payment_secret, fwd_trampoline_onion, None except NoPathFound: # fall back to payment splitting self.logger.info("no path found, trying multi-part payment") if not invoice_features.supports(LnFeatures.BASIC_MPP_OPT): @@ -1672,7 +1681,8 @@ class LNWallet(LNWorker): payment_secret=payment_secret, local_height=local_height, trampoline_fee_level=trampoline_fee_level, - use_two_trampolines=use_two_trampolines) + use_two_trampolines=use_two_trampolines, + failed_routes=self.failed_trampoline_routes) # node_features is only used to determine is_tlv per_trampoline_secret = os.urandom(32) per_trampoline_fees = per_trampoline_amount_with_fees - per_trampoline_amount @@ -1697,7 +1707,7 @@ class LNWallet(LNWorker): node_features=trampoline_features) ] self.logger.info(f'adding route {part_amount_msat} {delta_fee} {margin}') - routes.append((route, part_amount_msat_with_fees, per_trampoline_amount_with_fees, part_amount_msat, per_trampoline_cltv_delta, per_trampoline_secret, trampoline_onion)) + routes.append((route, part_amount_msat_with_fees, per_trampoline_amount_with_fees, part_amount_msat, per_trampoline_cltv_delta, per_trampoline_secret, trampoline_onion, trampoline_route)) if per_trampoline_fees != 0: self.logger.info('not enough margin to pay trampoline fee') raise NoPathFound() @@ -1735,7 +1745,7 @@ class LNWallet(LNWorker): full_path=None ) ) - yield route, part_amount_msat, final_total_msat, part_amount_msat, min_cltv_expiry, payment_secret, fwd_trampoline_onion + yield route, part_amount_msat, final_total_msat, part_amount_msat, min_cltv_expiry, payment_secret, fwd_trampoline_onion, None yielded_from_split_configuration = True except NoPathFound: continue @@ -1991,7 +2001,7 @@ class LNWallet(LNWorker): self._on_maybe_forwarded_htlc_resolved(chan=chan, htlc_id=htlc_id) q = self.sent_htlcs.get(payment_hash) if q: - route, payment_secret, amount_msat, bucket_msat, amount_receiver_msat, trampoline_fee_level = self.sent_htlcs_info[(payment_hash, chan.short_channel_id, htlc_id)] + route, payment_secret, amount_msat, bucket_msat, amount_receiver_msat, trampoline_fee_level, trampoline_route = self.sent_htlcs_info[(payment_hash, chan.short_channel_id, htlc_id)] htlc_log = HtlcLog( success=True, route=route, @@ -2018,7 +2028,7 @@ class LNWallet(LNWorker): # detect if it is part of a bucket # if yes, wait until the bucket completely failed key = (payment_hash, chan.short_channel_id, htlc_id) - route, payment_secret, amount_msat, bucket_msat, amount_receiver_msat, trampoline_fee_level = self.sent_htlcs_info[key] + route, payment_secret, amount_msat, bucket_msat, amount_receiver_msat, trampoline_fee_level, trampoline_route = self.sent_htlcs_info[key] if error_bytes: # TODO "decode_onion_error" might raise, catch and maybe blacklist/penalise someone? try: @@ -2043,6 +2053,8 @@ class LNWallet(LNWorker): self.logger.info('bucket failed') amount_receiver_msat = amount_sent + if trampoline_route: + route = trampoline_route htlc_log = HtlcLog( success=False, route=route, diff --git a/electrum/tests/test_lnchannel.py b/electrum/tests/test_lnchannel.py index b42229166..db77278d8 100644 --- a/electrum/tests/test_lnchannel.py +++ b/electrum/tests/test_lnchannel.py @@ -49,8 +49,8 @@ def create_channel_state(funding_txid, funding_index, funding_sat, is_initiator, local_amount, remote_amount, privkeys, other_pubkeys, seed, cur, nex, other_node_id, l_dust, r_dust, l_csv, r_csv): - assert local_amount > 0 - assert remote_amount > 0 + #assert local_amount > 0 + #assert remote_amount > 0 channel_id, _ = lnpeer.channel_id_from_funding_tx(funding_txid, funding_index) state = { "channel_id":channel_id.hex(), diff --git a/electrum/tests/test_lnpeer.py b/electrum/tests/test_lnpeer.py index 0c7e499ac..dbcfb6a38 100644 --- a/electrum/tests/test_lnpeer.py +++ b/electrum/tests/test_lnpeer.py @@ -331,6 +331,15 @@ low_fee_channel = { 'remote_fee_rate_millionths': 1, } +depleted_channel = { + 'local_balance_msat': 0, + 'remote_balance_msat': 10 * bitcoin.COIN * 1000, + 'local_base_fee_msat': 1_000, + 'local_fee_rate_millionths': 1, + 'remote_base_fee_msat': 1_000, + 'remote_fee_rate_millionths': 1, +} + GRAPH_DEFINITIONS = { 'square_graph': { 'alice': { @@ -761,6 +770,7 @@ class TestPeer(TestCaseForTestnet): min_cltv_expiry=lnaddr2.get_min_final_cltv_expiry(), payment_secret=lnaddr2.payment_secret, trampoline_fee_level=0, + trampoline_route=None, ) p1.maybe_send_commitment = _maybe_send_commitment1 # bob sends htlc BUT NOT COMMITMENT_SIGNED @@ -776,6 +786,7 @@ class TestPeer(TestCaseForTestnet): min_cltv_expiry=lnaddr1.get_min_final_cltv_expiry(), payment_secret=lnaddr1.payment_secret, trampoline_fee_level=0, + trampoline_route=None, ) p2.maybe_send_commitment = _maybe_send_commitment2 # sleep a bit so that they both receive msgs sent so far @@ -1081,7 +1092,7 @@ class TestPeer(TestCaseForTestnet): graph = self.prepare_chans_and_peers_in_graph(GRAPH_DEFINITIONS['square_graph']) self._run_mpp(graph, {'mpp_invoice': False}, {'mpp_invoice': True}) - def _run_trampoline_payment(self, is_legacy): + def _run_trampoline_payment(self, is_legacy, direct, drop_dave= []): async def turn_on_trampoline_alice(): if graph.workers['alice'].network.channel_db: graph.workers['alice'].network.channel_db.stop() @@ -1091,9 +1102,16 @@ class TestPeer(TestCaseForTestnet): async def pay(lnaddr, pay_req): self.assertEqual(PR_UNPAID, graph.workers['dave'].get_payment_status(lnaddr.paymenthash)) result, log = await graph.workers['alice'].pay_invoice(pay_req, attempts=10) - self.assertTrue(result) - self.assertEqual(PR_PAID, graph.workers['dave'].get_payment_status(lnaddr.paymenthash)) - raise PaymentDone() + if result: + self.assertEqual(PR_PAID, graph.workers['dave'].get_payment_status(lnaddr.paymenthash)) + raise PaymentDone() + else: + raise NoPathFound() + + def do_drop_dave(t): + # this will trigger UNKNOWN_NEXT_PEER + dave_node_id = graph.workers['dave'].node_keypair.pubkey + graph.workers[t].peers.pop(dave_node_id) async def f(): await turn_on_trampoline_alice() @@ -1103,16 +1121,21 @@ class TestPeer(TestCaseForTestnet): await group.spawn(peer.htlc_switch()) await asyncio.sleep(0.2) lnaddr, pay_req = self.prepare_invoice(graph.workers['dave'], include_routing_hints=True) + for p in drop_dave: + do_drop_dave(p) await group.spawn(pay(lnaddr, pay_req)) graph_definition = GRAPH_DEFINITIONS['square_graph'].copy() - # insert a channel from bob to carol for faster tests, - # otherwise will fail randomly - graph_definition['bob']['channels']['carol'] = high_fee_channel + if not direct: + # deplete channel from alice to carol + graph_definition['alice']['channels']['carol'] = depleted_channel + # insert a channel from bob to carol + graph_definition['bob']['channels']['carol'] = high_fee_channel + graph = self.prepare_chans_and_peers_in_graph(graph_definition) peers = graph.peers.values() if is_legacy: - # turn off trampoline features + # turn off trampoline features in invoice graph.workers['dave'].features = graph.workers['dave'].features ^ LnFeatures.OPTION_TRAMPOLINE_ROUTING_OPT # declare routing nodes as trampoline nodes @@ -1121,16 +1144,26 @@ class TestPeer(TestCaseForTestnet): graph.workers['carol'].name: LNPeerAddr(host="127.0.0.1", port=9735, pubkey=graph.workers['carol'].node_keypair.pubkey), } + run(f()) + + @needs_test_with_all_chacha20_implementations + def test_payment_trampoline_legacy(self): with self.assertRaises(PaymentDone): - run(f()) + self._run_trampoline_payment(is_legacy=True, direct=False) @needs_test_with_all_chacha20_implementations - def test_trampoline_payment_legacy(self): - self._run_trampoline_payment(True) + def test_payment_trampoline_e2e_direct(self): + with self.assertRaises(PaymentDone): + self._run_trampoline_payment(is_legacy=False, direct=True) @needs_test_with_all_chacha20_implementations - def test_trampoline_payment_e2e(self): - self._run_trampoline_payment(False) + def test_payment_trampoline_e2e_indirect(self): + # must use two trampolines + with self.assertRaises(PaymentDone): + self._run_trampoline_payment(is_legacy=False, direct=False, drop_dave=['bob']) + # both trampolines drop dave + with self.assertRaises(NoPathFound): + self._run_trampoline_payment(is_legacy=False, direct=False, drop_dave=['bob', 'carol']) @needs_test_with_all_chacha20_implementations def test_payment_multipart_trampoline_e2e(self): @@ -1412,6 +1445,7 @@ class TestPeer(TestCaseForTestnet): payment_secret=payment_secret, min_cltv_expiry=min_cltv_expiry, trampoline_fee_level=0, + trampoline_route=None, ) await asyncio.gather(pay, p1._message_loop(), p2._message_loop(), p1.htlc_switch(), p2.htlc_switch()) with self.assertRaises(PaymentFailure): diff --git a/electrum/trampoline.py b/electrum/trampoline.py index 3ce7e2ea4..0e3c98743 100644 --- a/electrum/trampoline.py +++ b/electrum/trampoline.py @@ -10,7 +10,6 @@ from .lnrouter import RouteEdge, TrampolineEdge, LNPaymentRoute, is_route_sane_t from .lnutil import NoPathFound, LNPeerAddr from . import constants - # trampoline nodes are supposed to advertise their fee and cltv in node_update message TRAMPOLINE_FEES = [ { @@ -165,6 +164,18 @@ def extend_trampoline_route( node_features=trampoline_features)) +def choose_second_trampoline(my_trampoline, trampolines, failed_routes): + if my_trampoline in trampolines: + trampolines.remove(my_trampoline) + for r in failed_routes: + if len(r) > 2: + t2 = bytes.fromhex(r[1]) + if t2 in trampolines: + trampolines.remove(t2) + if not trampolines: + raise NoPathFound('all routes have failed') + return random.choice(trampolines) + def create_trampoline_route( *, amount_msat: int, @@ -172,28 +183,28 @@ def create_trampoline_route( invoice_pubkey: bytes, invoice_features: int, my_pubkey: bytes, - trampoline_node_id: bytes, # the first trampoline in the path; which we are directly connected to + my_trampoline: bytes, # the first trampoline in the path; which we are directly connected to r_tags, trampoline_fee_level: int, - use_two_trampolines: bool + use_two_trampolines: bool, + failed_routes: list, ) -> LNPaymentRoute: # we decide whether to convert to a legacy payment is_legacy, invoice_trampolines = is_legacy_relay(invoice_features, r_tags) # 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, trampoline_node_id, trampoline_fee_level) + extend_trampoline_route(route, my_pubkey, my_trampoline, trampoline_fee_level) if is_legacy: # we add another different trampoline hop for privacy if use_two_trampolines: trampolines = trampolines_by_id() - del trampolines[trampoline_node_id] - second_trampoline_pubkey = random.choice(list(trampolines.keys())) - extend_trampoline_route(route, trampoline_node_id, second_trampoline_pubkey, trampoline_fee_level) - + second_trampoline = choose_second_trampoline(my_trampoline, list(trampolines.keys()), failed_routes) + extend_trampoline_route(route, my_trampoline, second_trampoline, 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) @@ -201,13 +212,21 @@ def create_trampoline_route( route[-1].invoice_features = invoice_features route[-1].outgoing_node_id = invoice_pubkey else: - if invoice_trampolines and trampoline_node_id not in invoice_trampolines: - second_trampoline_pubkey = random.choice(invoice_trampolines) - extend_trampoline_route(route, trampoline_node_id, second_trampoline_pubkey, trampoline_fee_level) + if invoice_trampolines: + if my_trampoline in invoice_trampolines: + short_route = [my_trampoline.hex(), invoice_pubkey.hex()] + if short_route in failed_routes: + add_trampoline = True + else: + add_trampoline = False + else: + 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) # 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) - # check that we can pay amount and fees for edge in route[::-1]: amount_msat += edge.fee_for_edge(amount_msat) @@ -269,7 +288,8 @@ def create_trampoline_route_and_onion( payment_secret, local_height: int, trampoline_fee_level: int, - use_two_trampolines: bool): + use_two_trampolines: bool, + failed_routes: list): # create route for the trampoline_onion trampoline_route = create_trampoline_route( amount_msat=amount_msat, @@ -277,10 +297,11 @@ def create_trampoline_route_and_onion( my_pubkey=my_pubkey, invoice_pubkey=invoice_pubkey, invoice_features=invoice_features, - trampoline_node_id=node_id, + my_trampoline=node_id, r_tags=r_tags, trampoline_fee_level=trampoline_fee_level, - use_two_trampolines=use_two_trampolines) + use_two_trampolines=use_two_trampolines, + failed_routes=failed_routes) # compute onion and fees final_cltv = local_height + min_cltv_expiry trampoline_onion, amount_with_fees, bucket_cltv = create_trampoline_onion(