xpay: stop retrying a node that repeats unknown_next_peer

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,9 @@ 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:
+			index--;
+			goto strange_error;
+
 		case WIRE_AMOUNT_BELOW_MINIMUM:
 		case WIRE_FEE_INSUFFICIENT:
 		case WIRE_INCORRECT_CLTV_EXPIRY:
@@ -1130,9 +1173,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 +2701,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;

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()

tests/test_xpay.py

@@ -1559,3 +1559,45 @@ 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'},
+                 {'disable-plugin': 'cln-grpc', 'plugin': plugin},
+                 {'disable-plugin': 'cln-grpc'},
+                 {'disable-plugin': 'cln-grpc'},
+                 {'disable-plugin': 'cln-grpc'},
+                 {'disable-plugin': 'cln-grpc'},
+                 {'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)
+
+    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 0 < 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