From f2fe9670f06234ba1376b2301cee983e6234e989 Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Mon, 2 Nov 2026 10:36:21 +0100 Subject: [PATCH 1/5] askrene: clamp the accumulation of bias Node and channel bias add up but they never should exceed the [-100, 100] range Signed-off-by: Lagrang3 --- plugins/askrene/layer.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/plugins/askrene/layer.c b/plugins/askrene/layer.c index 0406c98eead2..8a7f842caddc 100644 --- a/plugins/askrene/layer.c +++ b/plugins/askrene/layer.c @@ -1086,6 +1086,17 @@ const struct node_bias *layer_set_node_bias(struct layer *layer, return bias; } +static void bias_add_clamped(s8 *bias, s8 add) +{ + s32 x = *bias; + x += add; + if (x > 100) + x = 100; + if (x < -100) + x = -100; + *bias = x; +} + void layer_apply_biases(const struct layer *layer, const struct gossmap *gossmap, s8 *biases) @@ -1112,8 +1123,8 @@ void layer_apply_biases(const struct layer *layer, c = gossmap_nth_chan(gossmap, n, i, &dir); idx = (gossmap_chan_idx(gossmap, c) << 1) | dir; - biases[idx] += node_bias->out_bias; - biases[idx ^ 1] += node_bias->in_bias; + bias_add_clamped(&biases[idx], node_bias->out_bias); + bias_add_clamped(&biases[idx ^ 1], node_bias->in_bias); } } @@ -1121,12 +1132,13 @@ void layer_apply_biases(const struct layer *layer, bias; bias = bias_hash_next(layer->biases, &it)) { struct gossmap_chan *c; + u32 idx; c = gossmap_find_chan(gossmap, &bias->scidd.scid); if (!c) continue; - biases[(gossmap_chan_idx(gossmap, c) << 1) | bias->scidd.dir] - += bias->bias; + idx = (gossmap_chan_idx(gossmap, c) << 1) | bias->scidd.dir; + bias_add_clamped(&biases[idx], bias->bias); } } From 1fb16876b80a8d73a881a6f4de8c57db851e14ab Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Thu, 29 Oct 2026 08:50:38 +0100 Subject: [PATCH 2/5] xpay: avoid bad nodes We have had reports of payment failures by xpay timeout after multiple routes have been tried over the same node with weird errors like unknown_next_peer and fee_insufficient. These nodes are usually well connected hubs with hundreds of channels and they tend to be preferred by getroutes even after the first erring channels are disabled. Naturally, we only disable channels for the current payment session, so next time a second payment prefers that route it will fail as well. To fix this we add a negative bias on the failing channels that persist across payments and if we detect a pattern in the failing node we disable it completely on the current session and add a persistent bias to it. Changelog-Fixed: xpay: avoid channel and nodes that fail too often with weird errors Reported-by: @blockdyor and @ErikDeSmedt Signed-off-by: Lagrang3 --- plugins/xpay/xpay.c | 231 ++++++++++++++++++++++++++++++++++---------- tests/test_pay.py | 2 +- tests/test_xpay.py | 75 +++++++++++++- 3 files changed, 254 insertions(+), 54 deletions(-) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index a7ba821ad407..a92a953c3e43 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -70,6 +70,18 @@ static struct gossmap *get_gossmap(struct xpay *xpay) return xpay->global_gossmap; } +/* A record of a weird error returned by a forwarding node. */ +struct weird_error { + struct short_channel_id scid; + enum onion_wire failcode; +}; + +/* A per node record of weird errors. */ +struct noderr { + struct node_id nodeid; + struct weird_error *errs; +}; + /* The unifies bolt11 and bolt12 handling */ struct payment { /* Inside xpay->payments */ @@ -147,10 +159,8 @@ 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; + /* Keep a record of nodes that returned weird errors. */ + struct noderr **noderrs; /* Requests currently outstanding */ struct out_req **requests; @@ -915,39 +925,166 @@ 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) +static void append_node_error(struct noderr *noderr, + struct short_channel_id scid, + enum onion_wire failcode) +{ + struct weird_error err; + err.scid = scid; + err.failcode = failcode; + tal_arr_expand(&noderr->errs, err); +} + +static const char *fmt_onion_error(const tal_t *ctx, enum onion_wire failcode) +{ + const char *name = onion_wire_name(failcode), *str; + if (strstarts(name, "WIRE_")) + str = str_lowering(ctx, name + strlen("WIRE_")); + else + str = tal_strdup(ctx, name); + return str; +} + +/* eg. "unknown_next_peer on 1x2x3, fee_insufficient on 4x5x6" */ +static const char *fmt_noderr(const tal_t *ctx, const struct noderr *noderr) +{ + char *s = tal_strdup(ctx, ""); + + for (size_t i = 0; i < tal_count(noderr->errs); i++) { + const char *name = + fmt_onion_error(tmpctx, noderr->errs[i].failcode); + tal_append_fmt( + &s, "%s%s on %s", i == 0 ? "" : ", ", name, + fmt_short_channel_id(tmpctx, noderr->errs[i].scid)); + } + return s; +} + +/* A node failing too often with weird errors is probably going to fail every + * other request. */ +static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, + size_t index, enum onion_wire failcode) { struct payment *payment = attempt->payment; - struct node_id erring; struct out_req *req; - bool seen = false; + struct node_id nodeid; + struct short_channel_id scid; + struct noderr *current_noderr = NULL; + bool same_channel = false; + /* we returned the error ourselves */ 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; + + scid = attempt->hops[index].scidd.scid; + node_id_from_pubkey(&nodeid, &attempt->hops[index - 1].next_node); + + for (size_t i = 0; i < tal_count(payment->noderrs); i++) { + if (node_id_eq(&payment->noderrs[i]->nodeid, &nodeid)) { + current_noderr = payment->noderrs[i]; + + for (size_t j = 0; j < tal_count(current_noderr->errs); + j++) { + if (short_channel_id_eq( + scid, current_noderr->errs[j].scid)) { + same_channel = true; + break; + } + } + + if (!same_channel) { + /* We have not seen this scid failing for this + * node before. */ + append_node_error(current_noderr, scid, + failcode); + } + break; } } - if (!seen) { - tal_arr_expand(&payment->unknown_next_peers, erring); + + if (!current_noderr) { + current_noderr = tal(payment, struct noderr); + current_noderr->nodeid = nodeid; + current_noderr->errs = + tal_arr(current_noderr, struct weird_error, 0); + tal_arr_expand(&payment->noderrs, current_noderr); + + append_node_error(current_noderr, scid, failcode); + } + + if (same_channel) { + /* Our fault. We already saw some weird error on this channel + * and we still try it a second time. Probably two parallel + * attempts shared the same channel. But we don't want to + * penalize this node twice for the same observation. */ 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); + /* Also add a bias to this channel, so that future payments avoid it. + * It doesn't apply to fake channels because of name clash, these are + * kept private between payments. */ + if (!attempt->hops[index].fake_channel) { + add_result_summary(attempt, LOG_DBG, + "adding a global bias to channel %s", + describe_scidd(attempt, index)); + req = payment_ignored_req(aux_cmd, attempt, + "askrene-bias-channel"); + json_add_string(req->js, "layer", "xpay"); + json_add_short_channel_id_dir(req->js, "short_channel_id_dir", + attempt->hops[index].scidd); + json_add_s32(req->js, "bias", -5); /* cost x 1.18 */ + json_add_bool(req->js, "relative", true); + json_add_string( + req->js, "description", + tal_fmt(tmpctx, "xpay: weird error %s during payment %s", + fmt_onion_error(tmpctx, failcode), + fmt_sha256(tmpctx, &payment->payment_hash))); + send_payment_req(aux_cmd, attempt->payment, req); + } + + if (tal_count(current_noderr->errs) >= 2) { + /* Also add a bias to this node, so that future payments avoid + * it. */ + add_result_summary( + attempt, LOG_DBG, "adding a global bias to node %s", + fmt_node_id(tmpctx, ¤t_noderr->nodeid)); + req = + payment_ignored_req(aux_cmd, attempt, "askrene-bias-node"); + json_add_string(req->js, "layer", "xpay"); + json_add_node_id(req->js, "node", ¤t_noderr->nodeid); + json_add_string(req->js, "direction", "out"); + json_add_s32(req->js, "bias", -10); /* cost x 1.41 */ + json_add_bool(req->js, "relative", true); + json_add_string( + req->js, "description", + tal_fmt( + tmpctx, + "xpay: too many weird errors (%s) during payment %s", + fmt_noderr(tmpctx, current_noderr), + fmt_sha256(tmpctx, &payment->payment_hash))); + send_payment_req(aux_cmd, attempt->payment, req); + } + + /* Our tolerance could be any number, but the higher it is + * we risk failing the payment retrying too much on a bad node. */ + if (tal_count(current_noderr->errs) >= 2) { + /* FIXME: we should check if this node is a compulsory bridge + * between us and the destination (eg. an LSP), and in that case + * we should not disable it. A high tolerance value reduces + * this risk. */ + add_result_summary( + attempt, LOG_DBG, + "Repeated weird errors (%s) from %s: " + "disabling node for this payment", + fmt_noderr(tmpctx, current_noderr), + fmt_node_id(tmpctx, ¤t_noderr->nodeid)); + 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", ¤t_noderr->nodeid); + send_payment_req(aux_cmd, attempt->payment, req); + } } static void update_knowledge_from_error(struct command *aux_cmd, @@ -1044,11 +1181,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, } else from_final = false; failcode = fromwire_peektype(replymsg); - failcode_name = onion_wire_name(failcode); - if (strstarts(failcode_name, "WIRE_")) - failcode_name = str_lowering(tmpctx, - failcode_name - + strlen("WIRE_")); + failcode_name = fmt_onion_error(tmpctx, failcode); /* For local errors, error message is informative. */ if (index == 0) { @@ -1114,7 +1247,12 @@ static void update_knowledge_from_error(struct command *aux_cmd, case WIRE_INVALID_ONION_BLINDING: /* Blame hop *leading to* final node */ index--; - goto strange_error; + add_result_summary( + attempt, LOG_UNUSUAL, + "Unexpected error (%s) from final node: disabling " + "%s for this payment", + errmsg, describe_scidd(attempt, index)); + goto disable_channel; case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: { struct xpay *xpay = xpay_of(attempt->payment->plugin); @@ -1165,7 +1303,15 @@ static void update_knowledge_from_error(struct command *aux_cmd, case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: case WIRE_MPP_TIMEOUT: - goto strange_error; + /* We disable the erroneous channel for this */ + add_result_summary( + attempt, LOG_UNUSUAL, + "Unexpected error (%s) from intermediate node: " + "disabling %s for this payment", + errmsg, describe_scidd(attempt, index)); + + maybe_exclude_node(aux_cmd, attempt, index, failcode); + goto disable_channel; case WIRE_TEMPORARY_CHANNEL_FAILURE: add_result_summary(attempt, LOG_DBG, @@ -1176,16 +1322,6 @@ static void update_knowledge_from_error(struct command *aux_cmd, 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_AMOUNT_BELOW_MINIMUM: @@ -1201,6 +1337,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, "We got a weird error (%s) for %s: disabling it for this payment", errmsg, describe_scidd(attempt, index)); + maybe_exclude_node(aux_cmd, attempt, index, failcode); goto disable_channel; case WIRE_INVALID_ONION_BLINDING: @@ -1214,14 +1351,6 @@ static void update_knowledge_from_error(struct command *aux_cmd, } } -strange_error: - /* We disable the erroneous channel for this */ - add_result_summary(attempt, LOG_UNUSUAL, - "Unexpected error (%s) from %s node: disabling %s for this payment", - errmsg, - from_final ? "final" : "intermediate", - describe_scidd(attempt, index)); - disable_channel: /* We only do this for the current payment */ req = payment_ignored_req(aux_cmd, attempt, "askrene-update-channel"); @@ -2750,7 +2879,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->noderrs = tal_arr(payment, struct noderr*, 0); payment->requests = tal_arr(payment, struct out_req *, 0); payment->start_time = clock_time(); payment->pay_compat = as_pay; diff --git a/tests/test_pay.py b/tests/test_pay.py index ee62142e685e..0512857bd781 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -168,7 +168,7 @@ def test_pay_exclude_node(node_factory, bitcoind): inv = l3.rpc.invoice(amount, "test1", 'description')['bolt11'] # It should have eliminated route hint. - with pytest.raises(RpcError, match=r"Failed after 1 attempts. We got a weird error \(temporary_node_failure\) for the invoice's route hint \([0-9x]*/0\): disabling it for this payment. Then routing failed: We could not find a usable set of paths. All 1 channels to the destination are disabled."): + with pytest.raises(RpcError, match=r"Failed after 1 attempts. We got a weird error \(temporary_node_failure\) for the invoice's route hint \([0-9x]*/0\): disabling it for this payment.* Then routing failed: We could not find a usable set of paths. All 1 channels to the destination are disabled."): l1.rpc.pay(inv) # Get a fresh invoice, but do it before other routes exist, so routehint diff --git a/tests/test_xpay.py b/tests/test_xpay.py index e8584a93b0df..ba023fc89315 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1873,7 +1873,78 @@ def test_xpay_unknown_next_peer_excludes_node(node_factory, bitcoind): 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') + unknown = msg.count('We got a weird error (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 + assert 'disabling node' in msg or 'Repeated weird errors' in msg + + +def setup_failing_hub(node_factory, num_spokes, alt_ppm): + """payer -> hub -> {spokes} -> dest, plus payer -> alt -> altspoke -> dest. + + hub is the cheap, well-connected path but fails every forward with + fee_insufficient (with no channel_update, so there is nothing to learn + from it). The alternative path through alt charges alt_ppm. + """ + + def setup(plugin): + @plugin.hook("htlc_accepted") + def on_htlc_accepted(onion, plugin, **kwargs): + # Forwards carry forward_msat. Local invoices do not. + if onion.get("forward_msat") is None: + return {"result": "continue"} + # WIRE_FEE_INSUFFICIENT = UPDATE|12, htlc_msat, empty channel_update + return {"result": "fail", "failure_message": "100c" + "00" * 8 + "0000"} + + payer = node_factory.get_node() + hub = node_factory.get_node(inline_plugin=setup) + alt, altspoke, dest, *spokes = node_factory.get_nodes(3 + num_spokes) + node_factory.join_nodes([payer, hub], fundamount=10**6, wait_for_announce=True) + for spoke in spokes: + node_factory.join_nodes( + [hub, spoke, dest], fundamount=10**6, wait_for_announce=True + ) + node_factory.join_nodes( + [payer, alt, altspoke, dest], fundamount=10**6, wait_for_announce=True + ) + + alt.rpc.setchannel(altspoke.info["id"], feebase=1, feeppm=alt_ppm) + num_chans = 1 + 2 * num_spokes + 3 + wait_for(lambda: len(payer.rpc.listchannels()["channels"]) == num_chans * 2) + wait_for( + lambda: ( + [ + c["fee_per_millionth"] + for c in payer.rpc.listchannels(source=alt.info["id"])["channels"] + if c["destination"] == altspoke.info["id"] + ] + == [alt_ppm] + ) + ) + return payer, hub, alt, dest + + +def test_xpay_fee_insufficient_bias(node_factory, bitcoind): + """A hub repeatedly returning fee_insufficient gets a node bias. + + The alternative route is only slightly more expensive, so the node bias + added on the second failure is enough to route around the hub: we should + not need to disable the hub. + """ + payer, hub, alt, dest = setup_failing_hub(node_factory, num_spokes=5, alt_ppm=300) + + inv = dest.rpc.invoice(100000, "fee-bias", "fee-bias")["bolt11"] + ret = payer.rpc.xpay(invstring=inv, retry_for=30) + assert 0 < ret["failed_parts"] <= 2, ret + assert payer.daemon.is_in_log("disabling node for this payment") + assert payer.daemon.is_in_log( + "adding a global bias to node {}".format(hub.info["id"]) + ) + + # The bias persists in the xpay layer for future payments. + layer = only_one(payer.rpc.askrene_listlayers("xpay")["layers"]) + node_bias = only_one( + [b for b in layer["node_biases"] if b["node"] == hub.info["id"]] + ) + assert node_bias["out_bias"] < 0 + assert "fee_insufficient" in node_bias["description"] From 34c5af740ed09269acf774c60e9a2929b5a1a0bb Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Wed, 11 Nov 2026 14:58:36 +0100 Subject: [PATCH 3/5] xpay: add helpers to bias nodes and channels Changelog-None Signed-off-by: Lagrang3 --- plugins/xpay/xpay.c | 114 ++++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 47 deletions(-) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index a92a953c3e43..1c57d06b0d37 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -578,13 +578,10 @@ static void add_result_summary(struct attempt *attempt, attempt_log(attempt, level, "%s", msg); } -static const char *describe_scidd(struct attempt *attempt, size_t index) +static const char *describe_scidd(struct attempt *attempt, struct short_channel_id_dir scidd) { - struct short_channel_id_dir scidd = attempt->hops[index].scidd; struct payment *payment = attempt->payment; - assert(index < tal_count(attempt->hops)); - /* Blinded paths? */ if (scidd.scid.u64 < tal_count(payment->paths)) { if (tal_count(payment->paths) == 1) @@ -619,6 +616,12 @@ static const char *describe_scidd(struct attempt *attempt, size_t index) return fmt_short_channel_id_dir(tmpctx, &scidd); } +static const char *describe_scidd_from_index(struct attempt *attempt, size_t index) +{ + assert(index < tal_count(attempt->hops)); + return describe_scidd(attempt, attempt->hops[index].scidd); +} + /* How much did previous successes deliver? */ static struct amount_msat total_delivered(const struct payment *payment) { @@ -720,6 +723,48 @@ static u32 error_blockheight(const u8 *errmsg) return height; } +static void add_node_bias(struct command *aux_cmd, + struct attempt *attempt, + const struct node_id *nodeid, + const char *layer_name, + int bias, + const char *why) +{ + struct out_req *req; + add_result_summary(attempt, LOG_DBG, "adding a global bias to node %s", + fmt_node_id(tmpctx, nodeid)); + req = payment_ignored_req(aux_cmd, attempt, "askrene-bias-node"); + json_add_string(req->js, "layer", layer_name); + json_add_node_id(req->js, "node", nodeid); + json_add_string(req->js, "direction", "out"); + json_add_s32(req->js, "bias", bias); + json_add_bool(req->js, "relative", true); + if (why) + json_add_string(req->js, "description", why); + send_payment_req(aux_cmd, attempt->payment, req); +} + +static void add_channel_bias(struct command *aux_cmd, + struct attempt *attempt, + const struct short_channel_id_dir scidd, + const char *layer_name, + int bias, + const char *why) +{ + struct out_req *req; + add_result_summary(attempt, LOG_DBG, + "adding a global bias to channel %s", + describe_scidd(attempt, scidd)); + req = payment_ignored_req(aux_cmd, attempt, "askrene-bias-channel"); + json_add_string(req->js, "layer", layer_name); + json_add_short_channel_id_dir(req->js, "short_channel_id_dir", scidd); + json_add_s32(req->js, "bias", bias); + json_add_bool(req->js, "relative", true); + if (why) + json_add_string(req->js, "description", why); + send_payment_req(aux_cmd, attempt->payment, req); +} + /* Return true if this contained a channel_update which (potentially) changed something. */ static bool process_channel_update_from_onion_error(struct command *aux_cmd, struct attempt *attempt, @@ -831,17 +876,10 @@ static bool process_channel_update_from_onion_error(struct command *aux_cmd, /* We also bias *against* the channel. This should help if the node is * stuck somehow, or trying to track us. */ - req = payment_ignored_req(aux_cmd, attempt, "askrene-bias-channel"); - json_add_string(req->js, "layer", attempt->payment->private_layer); - json_add_short_channel_id_dir(req->js, - "short_channel_id_dir", - scidd); - json_add_s32(req->js, "bias", -1); - json_add_string(req->js, "description", - tal_fmt(tmpctx, "negative bias due to channel_update in error %s", - errname)); - json_add_bool(req->js, "relative", true); - send_payment_req(aux_cmd, attempt->payment, req); + add_channel_bias( + aux_cmd, attempt, scidd, attempt->payment->private_layer, -1, + tal_fmt(tmpctx, "negative bias due to channel_update in error %s", + errname)); return true; } @@ -1025,45 +1063,27 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, * It doesn't apply to fake channels because of name clash, these are * kept private between payments. */ if (!attempt->hops[index].fake_channel) { - add_result_summary(attempt, LOG_DBG, - "adding a global bias to channel %s", - describe_scidd(attempt, index)); - req = payment_ignored_req(aux_cmd, attempt, - "askrene-bias-channel"); - json_add_string(req->js, "layer", "xpay"); - json_add_short_channel_id_dir(req->js, "short_channel_id_dir", - attempt->hops[index].scidd); - json_add_s32(req->js, "bias", -5); /* cost x 1.18 */ - json_add_bool(req->js, "relative", true); - json_add_string( - req->js, "description", + add_channel_bias( + aux_cmd, attempt, attempt->hops[index].scidd, + /* layer = */ "xpay", + /* bias = */ -5 /* cost x 1.18 */, tal_fmt(tmpctx, "xpay: weird error %s during payment %s", fmt_onion_error(tmpctx, failcode), fmt_sha256(tmpctx, &payment->payment_hash))); - send_payment_req(aux_cmd, attempt->payment, req); } if (tal_count(current_noderr->errs) >= 2) { /* Also add a bias to this node, so that future payments avoid * it. */ - add_result_summary( - attempt, LOG_DBG, "adding a global bias to node %s", - fmt_node_id(tmpctx, ¤t_noderr->nodeid)); - req = - payment_ignored_req(aux_cmd, attempt, "askrene-bias-node"); - json_add_string(req->js, "layer", "xpay"); - json_add_node_id(req->js, "node", ¤t_noderr->nodeid); - json_add_string(req->js, "direction", "out"); - json_add_s32(req->js, "bias", -10); /* cost x 1.41 */ - json_add_bool(req->js, "relative", true); - json_add_string( - req->js, "description", + add_node_bias( + aux_cmd, attempt, ¤t_noderr->nodeid, + /* layer = */ "xpay", + /* bias = */ -10 /* cost x 1.41 */, tal_fmt( tmpctx, "xpay: too many weird errors (%s) during payment %s", fmt_noderr(tmpctx, current_noderr), fmt_sha256(tmpctx, &payment->payment_hash))); - send_payment_req(aux_cmd, attempt->payment, req); } /* Our tolerance could be any number, but the higher it is @@ -1151,7 +1171,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, description = "Garbled error message"; add_result_summary(attempt, LOG_UNUSUAL, "We got a garbled error message, and chose to (randomly) to disable %s for this payment", - describe_scidd(attempt, index)); + describe_scidd_from_index(attempt, index)); goto disable_channel; } @@ -1251,7 +1271,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, attempt, LOG_UNUSUAL, "Unexpected error (%s) from final node: disabling " "%s for this payment", - errmsg, describe_scidd(attempt, index)); + errmsg, describe_scidd_from_index(attempt, index)); goto disable_channel; case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: { @@ -1288,7 +1308,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, add_result_summary(attempt, LOG_DBG, "We got %s for %s, containing a channel_update:" " updating our map", - errmsg, describe_scidd(attempt, index)); + errmsg, describe_scidd_from_index(attempt, index)); goto check_previous_success; } @@ -1308,7 +1328,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, attempt, LOG_UNUSUAL, "Unexpected error (%s) from intermediate node: " "disabling %s for this payment", - errmsg, describe_scidd(attempt, index)); + errmsg, describe_scidd_from_index(attempt, index)); maybe_exclude_node(aux_cmd, attempt, index, failcode); goto disable_channel; @@ -1317,7 +1337,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, add_result_summary(attempt, LOG_DBG, "We got %s for %s, assuming it can't carry %s", errmsg, - describe_scidd(attempt, index), + describe_scidd_from_index(attempt, index), fmt_amount_msat(tmpctx, attempt->hops[index].amount_out)); goto channel_capacity; @@ -1336,7 +1356,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, add_result_summary(attempt, LOG_DBG, "We got a weird error (%s) for %s: disabling it for this payment", errmsg, - describe_scidd(attempt, index)); + describe_scidd_from_index(attempt, index)); maybe_exclude_node(aux_cmd, attempt, index, failcode); goto disable_channel; From f83da15209118e99bb8bc0cb1966439c758acf74 Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Thu, 29 Oct 2026 09:51:36 +0100 Subject: [PATCH 4/5] xpay: add xpay-failing-node-tolerance We disable a node for the current payment session if the number of weird failures is equal to the xpay-failing-node-tolerance. For repeated failing nodes we also apply a global negative bias to avoid that node on other payments. The bias is by default applied after the first 2 failures and it accumulates. If xpay-failing-node-tolerance is less than 2 we apply the bias the moment we disable the node. Changelog-Added: xpay: add xpay-failing-node-tolerance option to set the number of weird failures that trigger a node to be disabled during the current payment session. Signed-off-by: Lagrang3 --- contrib/msggen/msggen/schema.json | 6 +++ doc/lightningd-config.5.md | 4 ++ doc/schemas/listconfigs.json | 6 +++ plugins/xpay/xpay.c | 20 ++++++++- tests/test_xpay.py | 68 ++++++++++++++++++++++++++++++- 5 files changed, 100 insertions(+), 4 deletions(-) diff --git a/contrib/msggen/msggen/schema.json b/contrib/msggen/msggen/schema.json index 78d8975e2985..79fd9cd801a0 100644 --- a/contrib/msggen/msggen/schema.json +++ b/contrib/msggen/msggen/schema.json @@ -23087,6 +23087,12 @@ "source": "default", "plugin": "/usr/local/libexec/plugins/cln-xpay" }, + "xpay-failing-node-tolerance": { + "value_int": 4, + "source": "default", + "plugin": "/usr/local/libexec/plugins/cln-xpay", + "dynamic": true + }, "xpay-handle-pay": { "value_bool": true, "source": "default", diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index de10ab6d8b29..c77de1ceb61e 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -572,6 +572,10 @@ command, so they invoices can also be paid onchain. Specify the name of a layer `xpay` shall use always for every payment. This is specially useful when combined with `xpay-handle-pay` since the `layers` parameter is not available in the `pay` interface. This can be specified multiple times to add more layers. +* **xpay-failing-node-tolerance**=*NUMBER* [plugin `xpay`, *dynamic*] + + How many times a forwarding node can give us a weird error before we disable it for the current payment session. If a node gives us weird errors on its channels repeatedly it could either be that we were unlucky and all the channels we tried have some kind of issue or it is the node that has an issue. In the first case it would just be a matter of trying again through a different channel, but in the latter case it is better to avoid routing through it otherwise it'll make us waste our time and eventually fail the payment on timeout. The tolerance must balance the odds of those two cases (the default is 4). + * **askrene-timeout**=*SECONDS* [plugin `askrene`, *dynamic*] This option makes the `getroutes` call fail if it takes more than this many seconds. Setting it to zero is a fun way to ensure your node never makes payments. diff --git a/doc/schemas/listconfigs.json b/doc/schemas/listconfigs.json index 2556d834c59a..23ed64ccdd67 100644 --- a/doc/schemas/listconfigs.json +++ b/doc/schemas/listconfigs.json @@ -2469,6 +2469,12 @@ "source": "default", "plugin": "/usr/local/libexec/plugins/cln-xpay" }, + "xpay-failing-node-tolerance": { + "value_int": 4, + "source": "default", + "plugin": "/usr/local/libexec/plugins/cln-xpay", + "dynamic": true + }, "xpay-handle-pay": { "value_bool": true, "source": "default", diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 1c57d06b0d37..609310c9c821 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -56,6 +56,10 @@ struct xpay { /* Suppress calls to askrene-age */ bool dev_no_age; const char **user_layers; + + /* How many times a forwarding node can give us a weird error before we + * disable it for the current payment session. */ + u32 fail_node_tolerance; }; static struct xpay *xpay_of(struct plugin *plugin) @@ -1004,11 +1008,19 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, size_t index, enum onion_wire failcode) { struct payment *payment = attempt->payment; + struct xpay *xpay = xpay_of(attempt->payment->plugin); struct out_req *req; struct node_id nodeid; struct short_channel_id scid; struct noderr *current_noderr = NULL; bool same_channel = false; + u32 apply_bias_tolerance = 2; + + /* By default we apply the first bias after 2 hits, but if the fail + * tolerance option is lower than that we apply the bias together with + * the node disable. */ + if (xpay->fail_node_tolerance < apply_bias_tolerance) + apply_bias_tolerance = xpay->fail_node_tolerance; /* we returned the error ourselves */ if (index == 0) @@ -1072,7 +1084,7 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, fmt_sha256(tmpctx, &payment->payment_hash))); } - if (tal_count(current_noderr->errs) >= 2) { + if (tal_count(current_noderr->errs) >= apply_bias_tolerance) { /* Also add a bias to this node, so that future payments avoid * it. */ add_node_bias( @@ -1088,7 +1100,7 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, /* Our tolerance could be any number, but the higher it is * we risk failing the payment retrying too much on a bad node. */ - if (tal_count(current_noderr->errs) >= 2) { + if (tal_count(current_noderr->errs) >= xpay->fail_node_tolerance) { /* FIXME: we should check if this node is a compulsory bridge * between us and the destination (eg. an LSP), and in that case * we should not disable it. A high tolerance value reduces @@ -3950,6 +3962,7 @@ int main(int argc, char *argv[]) xpay->slow_mode = false; xpay->dev_no_age = false; xpay->user_layers = tal_arr(xpay, const char *, 0); + xpay->fail_node_tolerance = 4; list_head_init(&xpay->payments); plugin_main(argv, init, take(xpay), PLUGIN_RESTARTABLE, true, NULL, @@ -3963,6 +3976,9 @@ int main(int argc, char *argv[]) plugin_option_dynamic("xpay-slow-mode", "bool", "Wait until all parts have completed before returning success or failure", bool_option, bool_jsonfmt, &xpay->slow_mode), + plugin_option_dynamic("xpay-failing-node-tolerance", "int", + "How many times a node can give us a weird error before we disable it.", + u32_option, u32_jsonfmt, &xpay->fail_node_tolerance), plugin_option_multi("xpay-user-layer", "string", "Add a layer that will be used for every payment", multi_string_option, string_array_jsonfmt, &xpay->user_layers), diff --git a/tests/test_xpay.py b/tests/test_xpay.py index ba023fc89315..cb5866da09a0 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1851,7 +1851,7 @@ def test_xpay_unknown_next_peer_excludes_node(node_factory, bitcoind): """ plugin = os.path.join(os.path.dirname(__file__), 'plugins/fail_unknown_next_peer.py') payer, hub, a, b, c, d, dest = node_factory.get_nodes( - 7, opts=[{}, + 7, opts=[{'xpay-failing-node-tolerance': 2}, {'plugin': plugin}, {}, {}, @@ -1936,7 +1936,7 @@ def test_xpay_fee_insufficient_bias(node_factory, bitcoind): inv = dest.rpc.invoice(100000, "fee-bias", "fee-bias")["bolt11"] ret = payer.rpc.xpay(invstring=inv, retry_for=30) assert 0 < ret["failed_parts"] <= 2, ret - assert payer.daemon.is_in_log("disabling node for this payment") + assert not payer.daemon.is_in_log("disabling node for this payment") assert payer.daemon.is_in_log( "adding a global bias to node {}".format(hub.info["id"]) ) @@ -1948,3 +1948,67 @@ def test_xpay_fee_insufficient_bias(node_factory, bitcoind): ) assert node_bias["out_bias"] < 0 assert "fee_insufficient" in node_bias["description"] + + +def test_xpay_fee_insufficient_disable(node_factory, bitcoind): + """A hub repeatedly returning fee_insufficient gets disabled. + + The alternative route is so expensive that the node bias is not + enough, so we keep trying the hub until it reaches the default + xpay-failing-node-tolerance (4) and is disabled for this payment. It has + more channels than that, so it's the disabling which makes us succeed. + """ + payer, hub, alt, dest = setup_failing_hub(node_factory, num_spokes=5, alt_ppm=5000) + assert ( + payer.rpc.listconfigs("xpay-failing-node-tolerance")["configs"][ + "xpay-failing-node-tolerance" + ]["value_int"] + == 4 + ) + + inv = dest.rpc.invoice(100000, "fee-disable", "fee-disable")["bolt11"] + ret = payer.rpc.xpay(invstring=inv, retry_for=30) + assert 2 < ret["failed_parts"] <= 4, ret + line = payer.daemon.is_in_log( + r"Repeated weird errors \(.*\) from {}: disabling node for this payment".format( + hub.info["id"] + ) + ) + assert line.count("fee_insufficient on") == 4, line + + +def test_xpay_failing_node_tolerance(node_factory, bitcoind): + """xpay-failing-node-tolerance controls when a failing node is disabled.""" + payer, hub, alt, dest = setup_failing_hub(node_factory, num_spokes=5, alt_ppm=5000) + disabled = ( + r"Repeated weird errors \(.*\) from {}: disabling node for this payment".format( + hub.info["id"] + ) + ) + + with pytest.raises(RpcError): + payer.rpc.setconfig("xpay-failing-node-tolerance", -1) + + payer.rpc.setconfig("xpay-failing-node-tolerance", 1) + conf = payer.rpc.listconfigs("xpay-failing-node-tolerance")["configs"][ + "xpay-failing-node-tolerance" + ] + assert conf["value_int"] == 1 + + # A single failure is now enough to disable the hub. + start = len(payer.daemon.logs) + inv = dest.rpc.invoice(100000, "tolerance-1", "tolerance-1")["bolt11"] + ret = payer.rpc.xpay(invstring=inv, retry_for=30) + assert ret["failed_parts"] == 1, ret + assert payer.daemon.is_in_log(disabled, start=start) + + # With a tolerance above the hub's channel count, we never disable it: + # we walk all its channels before using the expensive route. + payer.rpc.setconfig("xpay-failing-node-tolerance", 100) + start = len(payer.daemon.logs) + # The alternative route not only costs more fees but it is also likely to + # fail for a payment this big, askrene will like to split this payment. + inv = dest.rpc.invoice(90000000, "tolerance-100", "tolerance-100")["bolt11"] + ret = payer.rpc.xpay(invstring=inv, retry_for=60) + assert ret["failed_parts"] == 5, ret + assert not payer.daemon.is_in_log(disabled, start=start) From 5b3cfb9c9b4663fafaa2cf27bbb1b8c5d74c310e Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Wed, 11 Nov 2026 16:05:55 +0100 Subject: [PATCH 5/5] xpay: disable intermediate nodes that give final errors Changelog-Fixed: xpay: instantly disable nodes that return plain wrong or impossible to recover error codes, eg. incorrect_or_unknown_payment_details from an intermediate node or permanent_node_failure, the disable operation applies to the current payment session and in addition a negative bias is applied to this node for all subsequent payments. Signed-off-by: Lagrang3 --- doc/lightningd-config.5.md | 2 +- plugins/xpay/xpay.c | 26 +++++++++++++++++-------- tests/test_xpay.py | 40 +++++++++++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 14 deletions(-) diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index c77de1ceb61e..599e30682cac 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -574,7 +574,7 @@ command, so they invoices can also be paid onchain. * **xpay-failing-node-tolerance**=*NUMBER* [plugin `xpay`, *dynamic*] - How many times a forwarding node can give us a weird error before we disable it for the current payment session. If a node gives us weird errors on its channels repeatedly it could either be that we were unlucky and all the channels we tried have some kind of issue or it is the node that has an issue. In the first case it would just be a matter of trying again through a different channel, but in the latter case it is better to avoid routing through it otherwise it'll make us waste our time and eventually fail the payment on timeout. The tolerance must balance the odds of those two cases (the default is 4). + How many times a forwarding node can give us a weird error before we disable it for the current payment session. If a node gives us weird errors on its channels repeatedly it could either be that we were unlucky and all the channels we tried have some kind of issue or it is the node that has an issue. In the first case it would just be a matter of trying again through a different channel, but in the latter case it is better to avoid routing through it otherwise it'll make us waste our time and eventually fail the payment on timeout. The tolerance must balance the odds of those two cases (the default is 4). Some critical error types will trigger the node disable at the first hit regardless of the value of the tolerance. * **askrene-timeout**=*SECONDS* [plugin `askrene`, *dynamic*] diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 609310c9c821..31de8d29c1c2 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -1005,10 +1005,10 @@ static const char *fmt_noderr(const tal_t *ctx, const struct noderr *noderr) /* A node failing too often with weird errors is probably going to fail every * other request. */ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, - size_t index, enum onion_wire failcode) + size_t index, enum onion_wire failcode, + u32 exclude_tolerance) { struct payment *payment = attempt->payment; - struct xpay *xpay = xpay_of(attempt->payment->plugin); struct out_req *req; struct node_id nodeid; struct short_channel_id scid; @@ -1019,8 +1019,8 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, /* By default we apply the first bias after 2 hits, but if the fail * tolerance option is lower than that we apply the bias together with * the node disable. */ - if (xpay->fail_node_tolerance < apply_bias_tolerance) - apply_bias_tolerance = xpay->fail_node_tolerance; + if (exclude_tolerance < apply_bias_tolerance) + apply_bias_tolerance = exclude_tolerance; /* we returned the error ourselves */ if (index == 0) @@ -1100,7 +1100,7 @@ static void maybe_exclude_node(struct command *aux_cmd, struct attempt *attempt, /* Our tolerance could be any number, but the higher it is * we risk failing the payment retrying too much on a bad node. */ - if (tal_count(current_noderr->errs) >= xpay->fail_node_tolerance) { + if (tal_count(current_noderr->errs) >= exclude_tolerance) { /* FIXME: we should check if this node is a compulsory bridge * between us and the destination (eg. an LSP), and in that case * we should not disable it. A high tolerance value reduces @@ -1124,6 +1124,7 @@ static void update_knowledge_from_error(struct command *aux_cmd, const jsmntok_t *error, struct attempt *attempt) { + struct xpay *xpay = xpay_of(attempt->payment->plugin); const jsmntok_t *tok; struct onionreply *reply; struct out_req *req; @@ -1287,7 +1288,6 @@ static void update_knowledge_from_error(struct command *aux_cmd, goto disable_channel; case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: { - struct xpay *xpay = xpay_of(attempt->payment->plugin); u32 blockheight = error_blockheight(replymsg); if (blockheight > attempt->payment->start_blockheight) { attempt_log(attempt, LOG_INFORM, @@ -1342,7 +1342,8 @@ static void update_knowledge_from_error(struct command *aux_cmd, "disabling %s for this payment", errmsg, describe_scidd_from_index(attempt, index)); - maybe_exclude_node(aux_cmd, attempt, index, failcode); + maybe_exclude_node(aux_cmd, attempt, index, failcode, + /* tolerance = */ 0); goto disable_channel; case WIRE_TEMPORARY_CHANNEL_FAILURE: @@ -1362,6 +1363,14 @@ static void update_knowledge_from_error(struct command *aux_cmd, case WIRE_EXPIRY_TOO_FAR: case WIRE_EXPIRY_TOO_SOON: case WIRE_CHANNEL_DISABLED: + add_result_summary(attempt, LOG_DBG, + "We got a weird error (%s) for %s: disabling it for this payment", + errmsg, + describe_scidd_from_index(attempt, index)); + maybe_exclude_node(aux_cmd, attempt, index, failcode, + xpay->fail_node_tolerance); + goto disable_channel; + case WIRE_PERMANENT_NODE_FAILURE: case WIRE_TEMPORARY_NODE_FAILURE: case WIRE_REQUIRED_NODE_FEATURE_MISSING: @@ -1369,7 +1378,8 @@ static void update_knowledge_from_error(struct command *aux_cmd, "We got a weird error (%s) for %s: disabling it for this payment", errmsg, describe_scidd_from_index(attempt, index)); - maybe_exclude_node(aux_cmd, attempt, index, failcode); + maybe_exclude_node(aux_cmd, attempt, index, failcode, + /* tolerance = */ 0); goto disable_channel; case WIRE_INVALID_ONION_BLINDING: diff --git a/tests/test_xpay.py b/tests/test_xpay.py index cb5866da09a0..d22e244e4c8a 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1879,12 +1879,14 @@ def test_xpay_unknown_next_peer_excludes_node(node_factory, bitcoind): assert 'disabling node' in msg or 'Repeated weird errors' in msg -def setup_failing_hub(node_factory, num_spokes, alt_ppm): +def setup_failing_hub(node_factory, num_spokes, alt_ppm, + failure_message="100c" + "00" * 8 + "0000"): """payer -> hub -> {spokes} -> dest, plus payer -> alt -> altspoke -> dest. hub is the cheap, well-connected path but fails every forward with - fee_insufficient (with no channel_update, so there is nothing to learn - from it). The alternative path through alt charges alt_ppm. + failure_message, by default fee_insufficient (with no channel_update, so + there is nothing to learn from it). The alternative path through alt + charges alt_ppm. """ def setup(plugin): @@ -1893,8 +1895,7 @@ def on_htlc_accepted(onion, plugin, **kwargs): # Forwards carry forward_msat. Local invoices do not. if onion.get("forward_msat") is None: return {"result": "continue"} - # WIRE_FEE_INSUFFICIENT = UPDATE|12, htlc_msat, empty channel_update - return {"result": "fail", "failure_message": "100c" + "00" * 8 + "0000"} + return {"result": "fail", "failure_message": failure_message} payer = node_factory.get_node() hub = node_factory.get_node(inline_plugin=setup) @@ -2012,3 +2013,32 @@ def test_xpay_failing_node_tolerance(node_factory, bitcoind): ret = payer.rpc.xpay(invstring=inv, retry_for=60) assert ret["failed_parts"] == 5, ret assert not payer.daemon.is_in_log(disabled, start=start) + + +def test_xpay_failing_node_nonfinal(node_factory, bitcoind): + """If an intermediate node returns an error that is meant for the final, + then we disable that node right away regardless of the value of + xpay-failing-node-tolerance.""" + # WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS = PERM|15, htlc_msat, height + payer, hub, alt, dest = setup_failing_hub( + node_factory, + num_spokes=5, + alt_ppm=5000, + failure_message="400f" + "00" * 8 + "00" * 4, + ) + disabled = r"Repeated weird errors \(incorrect_or_unknown_payment_details on [0-9x]+\) from {}: disabling node for this payment".format( + hub.info["id"] + ) + + payer.rpc.setconfig("xpay-failing-node-tolerance", 100) + conf = payer.rpc.listconfigs("xpay-failing-node-tolerance")["configs"][ + "xpay-failing-node-tolerance" + ] + assert conf["value_int"] == 100 + + # Despite the high tolerance, a single failure disables the hub. + start = len(payer.daemon.logs) + inv = dest.rpc.invoice(100000, "nonfinal", "nonfinal")["bolt11"] + ret = payer.rpc.xpay(invstring=inv, retry_for=30) + assert ret["failed_parts"] == 1, ret + assert payer.daemon.is_in_log(disabled, start=start)