From 816e5dd838589156f67acc1b9c36fdd7bbe5348d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 17 Nov 2025 12:09:38 +1030 Subject: [PATCH 1/2] pytest: Test that we don't try to pay too many htlcs at once through an unknown channel. Signed-off-by: Rusty Russell --- tests/test_xpay.py | 47 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 696e2cbe161b..98310f6b2b06 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1020,6 +1020,53 @@ def test_xpay_bip353(node_factory): l2.rpc.xpay('fake@fake.com', 100) +@pytest.mark.xfail(strict=True) +def test_xpay_limited_max_accepted_htlcs(node_factory): + """xpay should try to reduce flows to 6 if there is an unannounced channel, and only try more if that fails""" + CHANNEL_SIZE_SATS = 10**6 + l1, l2 = node_factory.line_graph(2, + fundamount=CHANNEL_SIZE_SATS * 20, + opts=[{}, {'max-concurrent-htlcs': 6}], + announce_channels=False) + + # We want 10 paths between l3 and l1. + l3 = node_factory.get_node() + nodes = node_factory.get_nodes(10) + for n in nodes: + node_factory.join_nodes([l3, n, l1], fundamount=CHANNEL_SIZE_SATS) + + # We don't want to use up capacity, so we make payment fail. + inv1 = l1.rpc.invoice(f"{CHANNEL_SIZE_SATS * 5}sat", + 'test_xpay_limited_max_accepted_htlcs', + 'test_xpay_limited_max_accepted_htlcs')['bolt11'] + l1.rpc.delinvoice('test_xpay_limited_max_accepted_htlcs', 'unpaid') + + with pytest.raises(RpcError, match="Destination said it doesn't know invoice"): + l3.rpc.xpay(inv1) + + # 7 flows. + l3.daemon.wait_for_log('Final answer has 7 flows') + + # If we have a routehint, it will squeeze into 6. + inv2 = l2.rpc.invoice(f"{CHANNEL_SIZE_SATS * 5}sat", + 'test_xpay_limited_max_accepted_htlcs', + 'test_xpay_limited_max_accepted_htlcs')['bolt11'] + l2.rpc.delinvoice('test_xpay_limited_max_accepted_htlcs', 'unpaid') + with pytest.raises(RpcError, match="Destination said it doesn't know invoice"): + l3.rpc.xpay(inv2) + + # 6 flows. + l3.daemon.wait_for_log('Final answer has 6 flows') + + # If we force it, it will use more flows. + inv2 = l2.rpc.invoice(f"{CHANNEL_SIZE_SATS * 6}sat", + 'test_xpay_limited_max_accepted_htlcs2', + 'test_xpay_limited_max_accepted_htlcs2')['bolt11'] + l2.rpc.delinvoice('test_xpay_limited_max_accepted_htlcs2', 'unpaid') + with pytest.raises(RpcError, match="We got temporary_channel_failure"): + l3.rpc.xpay(inv2) + + def test_xpay_blockheight_mismatch(node_factory, bitcoind, executor): """We should wait a (reasonable) amount if the final node gives us a blockheight that would explain our failure.""" l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True) From 95de3f8f200aeeb86b2a89b34a62bcf36eddb71e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 17 Nov 2025 16:14:28 +1030 Subject: [PATCH 2/2] xpay: restrict maxparts to 6 for non-public nodes, but remove it if we can't route. This attempts to solve a problem we have with Phoenix clients: This payment has been split in two many parts by the sender: 31 parts vs max 6 parts allowed for on-the-fly funding. The problem is that we don't have any way in bolt11 or bolt12 to specify the maximum number of HTLCs. As a workaround, we start by restricting askrene to 6 parts if the node is not openly reachable, and if it struggles, we remove the restriction. This would work much better if askrene handled maxparts more completely! See-Also: https://github.com/ElementsProject/lightning/issues/8331 Changelog-Fixed: `xpay` will not try to send too many HTLCs through unknown channels (6, as that is Phoenix's limit) unless it has no choice Signed-off-by: Rusty Russell --- plugins/xpay/xpay.c | 51 +++++++++++++++++++++++++++------------------ tests/test_xpay.py | 11 +++++++--- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index aa59feae702d..0198a1f900f7 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -85,7 +85,7 @@ struct payment { struct amount_msat maxfee; /* Maximum delay on the route we're ok with */ u32 maxdelay; - /* Maximum number of payment routes that can be pending. */ + /* If non-zero: maximum number of payment routes that can be pending. */ u32 maxparts; /* Do we have to do it all in a single part? */ bool disable_mpp; @@ -179,7 +179,6 @@ static struct command_result *xpay_core(struct command *cmd, u32 retryfor, const struct amount_msat *partial, u32 maxdelay, - u32 dev_maxparts, bool as_pay); /* Wrapper for pending commands (ignores return) */ @@ -1317,6 +1316,16 @@ static struct command_result *getroutes_done_err(struct command *aux_cmd, msg = json_strdup(tmpctx, buf, json_get_member(buf, error, "message")); json_to_int(buf, json_get_member(buf, error, "code"), &code); + /* If we were restricting the number of parts, we remove that + * restriction and try again. */ + if (payment->maxparts) { + payment_log(payment, LOG_INFORM, + "getroute failed with maxparts=%u, so retrying without that restriction", + payment->maxparts); + payment->maxparts = 0; + return getroutes_for(aux_cmd, payment, payment->amount_being_routed); + } + /* Simple case: failed immediately. */ if (payment->total_num_attempts == 0) { payment_give_up(aux_cmd, payment, code, "Failed: %s", msg); @@ -1378,7 +1387,6 @@ static struct command_result *getroutes_for(struct command *aux_cmd, struct out_req *req; const struct pubkey *dst; struct amount_msat maxfee; - size_t count_pending; /* I would normally assert here, but we have reports of this happening... */ if (amount_msat_is_zero(deliver)) { @@ -1461,9 +1469,11 @@ static struct command_result *getroutes_for(struct command *aux_cmd, json_add_amount_msat(req->js, "maxfee_msat", maxfee); json_add_u32(req->js, "final_cltv", payment->final_cltv); json_add_u32(req->js, "maxdelay", payment->maxdelay); - count_pending = count_current_attempts(payment); - assert(payment->maxparts > count_pending); - json_add_u32(req->js, "maxparts", payment->maxparts - count_pending); + if (payment->maxparts) { + size_t count_pending = count_current_attempts(payment); + assert(payment->maxparts > count_pending); + json_add_u32(req->js, "maxparts", payment->maxparts - count_pending); + } return send_payment_req(aux_cmd, payment, req); } @@ -1774,7 +1784,7 @@ struct xpay_params { struct amount_msat *msat, *maxfee, *partial; const char **layers; unsigned int retryfor; - u32 maxdelay, dev_maxparts; + u32 maxdelay; const char *bip353; }; @@ -1791,7 +1801,7 @@ invoice_fetched(struct command *cmd, return xpay_core(cmd, take(to_canonical_invstr(NULL, take(inv))), NULL, params->maxfee, params->layers, params->retryfor, params->partial, params->maxdelay, - params->dev_maxparts, false); + false); } static struct command_result * @@ -1852,7 +1862,7 @@ static struct command_result *json_xpay_params(struct command *cmd, struct amount_msat *msat, *maxfee, *partial; const char *invstring; const char **layers; - u32 *maxdelay, *maxparts; + u32 *maxdelay; unsigned int *retryfor; struct out_req *req; struct xpay_params *xparams; @@ -1865,14 +1875,9 @@ static struct command_result *json_xpay_params(struct command *cmd, p_opt_def("retry_for", param_number, &retryfor, 60), p_opt("partial_msat", param_msat, &partial), p_opt_def("maxdelay", param_u32, &maxdelay, 2016), - p_opt_dev("dev_maxparts", param_u32, &maxparts, 100), NULL)) return command_param_failed(); - if (*maxparts == 0) - return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "maxparts cannot be zero"); - /* Is this a one-shot vibe payment? Kids these days! */ if (!as_pay && bolt12_has_offer_prefix(invstring)) { struct command_result *ret; @@ -1891,7 +1896,6 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->layers = layers; xparams->retryfor = *retryfor; xparams->maxdelay = *maxdelay; - xparams->dev_maxparts = *maxparts; xparams->bip353 = NULL; return do_fetchinvoice(cmd, invstring, xparams); @@ -1906,7 +1910,6 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->layers = layers; xparams->retryfor = *retryfor; xparams->maxdelay = *maxdelay; - xparams->dev_maxparts = *maxparts; xparams->bip353 = invstring; req = jsonrpc_request_start(cmd, "fetchbip353", @@ -1917,7 +1920,7 @@ static struct command_result *json_xpay_params(struct command *cmd, } return xpay_core(cmd, invstring, - msat, maxfee, layers, *retryfor, partial, *maxdelay, *maxparts, + msat, maxfee, layers, *retryfor, partial, *maxdelay, as_pay); } @@ -1929,11 +1932,12 @@ static struct command_result *xpay_core(struct command *cmd, u32 retryfor, const struct amount_msat *partial, u32 maxdelay, - u32 dev_maxparts, bool as_pay) { struct payment *payment = tal(cmd, struct payment); struct xpay *xpay = xpay_of(cmd->plugin); + struct gossmap *gossmap = get_gossmap(xpay); + struct node_id dstid; u64 now, invexpiry; struct out_req *req; char *err; @@ -1957,10 +1961,8 @@ static struct command_result *xpay_core(struct command *cmd, else payment->layers = NULL; payment->maxdelay = maxdelay; - payment->maxparts = dev_maxparts; if (bolt12_has_prefix(payment->invstring)) { - struct gossmap *gossmap = get_gossmap(xpay); struct tlv_invoice *b12inv = invoice_decode(tmpctx, payment->invstring, strlen(payment->invstring), @@ -2086,6 +2088,15 @@ static struct command_result *xpay_core(struct command *cmd, } else payment->maxfee = *maxfee; + /* If we are using an unannounced channel, we assume we can + * only do 6 HTLCs at a time. This is currently true for + * Phoenix, which is a large and significant node. */ + node_id_from_pubkey(&dstid, &payment->destination); + if (!gossmap_find_node(gossmap, &dstid)) + payment->maxparts = 6; + else + payment->maxparts = 0; + /* Now preapprove, then start payment. */ if (command_check_only(cmd)) { req = jsonrpc_request_start(cmd, "check", diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 98310f6b2b06..087ade6bbe40 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1020,7 +1020,6 @@ def test_xpay_bip353(node_factory): l2.rpc.xpay('fake@fake.com', 100) -@pytest.mark.xfail(strict=True) def test_xpay_limited_max_accepted_htlcs(node_factory): """xpay should try to reduce flows to 6 if there is an unannounced channel, and only try more if that fails""" CHANNEL_SIZE_SATS = 10**6 @@ -1047,6 +1046,9 @@ def test_xpay_limited_max_accepted_htlcs(node_factory): # 7 flows. l3.daemon.wait_for_log('Final answer has 7 flows') + # Make sure xpay has completely finished! + wait_for(lambda: l3.rpc.askrene_listreservations() == {'reservations': []}) + # If we have a routehint, it will squeeze into 6. inv2 = l2.rpc.invoice(f"{CHANNEL_SIZE_SATS * 5}sat", 'test_xpay_limited_max_accepted_htlcs', @@ -1058,13 +1060,16 @@ def test_xpay_limited_max_accepted_htlcs(node_factory): # 6 flows. l3.daemon.wait_for_log('Final answer has 6 flows') - # If we force it, it will use more flows. + # Make sure xpay has completely finished! + wait_for(lambda: l3.rpc.askrene_listreservations() == {'reservations': []}) + + # If we force it, it will use more flows. And fail on 7th part! inv2 = l2.rpc.invoice(f"{CHANNEL_SIZE_SATS * 6}sat", 'test_xpay_limited_max_accepted_htlcs2', 'test_xpay_limited_max_accepted_htlcs2')['bolt11'] - l2.rpc.delinvoice('test_xpay_limited_max_accepted_htlcs2', 'unpaid') with pytest.raises(RpcError, match="We got temporary_channel_failure"): l3.rpc.xpay(inv2) + l3.daemon.wait_for_log('Final answer has 7 flows') def test_xpay_blockheight_mismatch(node_factory, bitcoind, executor):