diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 8e1b2f5d14f0..91491c5438c0 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -145,6 +145,11 @@ struct payment { /* Useful information from prior attempts if any. */ char *prior_results; + /* Nodes that have already returned unknown_next_peer. The first + * failure only disables that channel (it can be a stale scid). A + * second failure from the same node excludes it. */ + struct node_id *unknown_next_peers; + /* Requests currently outstanding */ struct out_req **requests; @@ -905,6 +910,41 @@ static void payment_already_paid(struct payment *payment) send_outreq(req); } +/* unknown_next_peer is returned by the node that cannot forward. One hit + * can be a stale scid, so we only disable that channel. The same node + * returning it again means every other channel through it will fail too. */ +static void maybe_exclude_unknown_next_peer(struct command *aux_cmd, + struct attempt *attempt, + size_t index) +{ + struct payment *payment = attempt->payment; + struct node_id erring; + struct out_req *req; + bool seen = false; + + if (index == 0) + return; + node_id_from_pubkey(&erring, &attempt->hops[index - 1].next_node); + for (size_t i = 0; i < tal_count(payment->unknown_next_peers); i++) { + if (node_id_eq(&payment->unknown_next_peers[i], &erring)) { + seen = true; + break; + } + } + if (!seen) { + tal_arr_expand(&payment->unknown_next_peers, erring); + return; + } + + add_result_summary(attempt, LOG_DBG, + "Repeated unknown_next_peer from %s: disabling node for this payment", + fmt_node_id(tmpctx, &erring)); + req = payment_ignored_req(aux_cmd, attempt, "askrene-disable-node"); + json_add_string(req->js, "layer", payment->private_layer); + json_add_node_id(req->js, "node", &erring); + send_payment_req(aux_cmd, attempt->payment, req); +} + static void update_knowledge_from_error(struct command *aux_cmd, const char *buf, const jsmntok_t *error, @@ -1057,6 +1097,12 @@ static void update_knowledge_from_error(struct command *aux_cmd, case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: case WIRE_UNKNOWN_NEXT_PEER: + /* A final node must not send this. The erring node is + * the one that cannot forward; exclude it on repeat. */ + maybe_exclude_unknown_next_peer(aux_cmd, attempt, index); + index--; + goto strange_error; + case WIRE_AMOUNT_BELOW_MINIMUM: case WIRE_FEE_INSUFFICIENT: case WIRE_INCORRECT_CLTV_EXPIRY: @@ -1130,9 +1176,19 @@ static void update_knowledge_from_error(struct command *aux_cmd, fmt_amount_msat(tmpctx, attempt->hops[index].amount_out)); goto channel_capacity; + case WIRE_UNKNOWN_NEXT_PEER: + /* One hit can be a stale scid, so disable that channel. + * The same node returning it again is excluded, or we + * walk every other channel through it. */ + add_result_summary(attempt, LOG_DBG, + "We got %s for %s: disabling it for this payment", + errmsg, + describe_scidd(attempt, index)); + maybe_exclude_unknown_next_peer(aux_cmd, attempt, index); + goto disable_channel; + case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: - case WIRE_UNKNOWN_NEXT_PEER: case WIRE_AMOUNT_BELOW_MINIMUM: case WIRE_FEE_INSUFFICIENT: case WIRE_INCORRECT_CLTV_EXPIRY: @@ -2648,6 +2704,7 @@ static struct payment *new_payment(const tal_t *ctx, list_head_init(&payment->past_attempts); payment->amount_being_routed = AMOUNT_MSAT(0); payment->prior_results = tal_strdup(payment, ""); + payment->unknown_next_peers = tal_arr(payment, struct node_id, 0); payment->requests = tal_arr(payment, struct out_req *, 0); payment->start_time = clock_time(); payment->pay_compat = as_pay; diff --git a/tests/plugins/fail_unknown_next_peer.py b/tests/plugins/fail_unknown_next_peer.py new file mode 100755 index 000000000000..a8587fb5a26b --- /dev/null +++ b/tests/plugins/fail_unknown_next_peer.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python3 +"""Fail payment forwards with unknown_next_peer. Leave local invoices alone. + +Used by test_xpay_unknown_next_peer_excludes_node (issue 9590). +""" +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.hook("htlc_accepted") +def on_htlc_accepted(onion, plugin, **kwargs): + # Forwards carry forward_msat. Local invoices and funding HTLCs do not. + if not isinstance(onion, dict) or onion.get("forward_msat") is None: + return {"result": "continue"} + plugin.log("failing forward with unknown_next_peer") + # WIRE_UNKNOWN_NEXT_PEER = PERM | 10 = 0x400a + return {"result": "fail", "failure_message": "400a"} + + +plugin.run() diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 301f89fd4708..692ef3185ac8 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1559,3 +1559,46 @@ def test_sendamount_bip353(node_factory): ret = l2.rpc.sendamount("fake@fake.com", "100sat") assert ret["successful_parts"] == 1 assert ret["amount_sent_msat"] == 100000 + + +def test_xpay_unknown_next_peer_excludes_node(node_factory, bitcoind): + """Issue 9590: repeated unknown_next_peer must exclude the next node. + + payer -> hub -> {a,b,c,d} -> dest + + hub fails every forward with unknown_next_peer (the code lnd returns when + the next peer is offline). A single failure may be a stale scid, so the + first channel is disabled and another path is allowed. The second failure + to the same next node must exclude that node, so xpay must not walk every + remaining channel into it. + """ + plugin = os.path.join(os.path.dirname(__file__), 'plugins/fail_unknown_next_peer.py') + # This tree has no cln-grpc plugin. The harness passes --grpc-port unless it is disabled. + payer, hub, a, b, c, d, dest = node_factory.get_nodes(7, opts={'disable-plugin': 'cln-grpc'}) + node_factory.join_nodes([payer, hub], fundamount=10**6, wait_for_announce=True) + for spoke in (a, b, c, d): + node_factory.join_nodes([hub, spoke, dest], fundamount=10**6, wait_for_announce=True) + + wait_for(lambda: len(payer.rpc.listchannels()['channels']) >= 9 * 2) + # lightningd execs the shebang. Point it at this interpreter so pyln imports. + launched = plugin + '.launch' + with open(plugin) as f: + src = f.read() + with open(launched, 'w') as f: + f.write('#!' + sys.executable + '\n' + src.split('\n', 1)[1]) + os.chmod(launched, 0o755) + hub.rpc.plugin_start(launched) + + inv = dest.rpc.invoice(10000, 'issue-9590', 'issue 9590')['bolt11'] + started = time.time() + with pytest.raises(RpcError) as err: + payer.rpc.xpay(invstring=inv, retry_for=15) + elapsed = time.time() - started + + msg = err.value.error['message'] + # One channel disable, then the node is excluded. Walking all four exits + # is the bug. The exclusion line also contains the failcode name. + unknown = msg.count('We got unknown_next_peer') + assert unknown <= 2, msg + assert elapsed < 10, 'spent the retry window walking the same node: {}'.format(msg) + assert 'disabling node' in msg or 'Repeated unknown_next_peer' in msg