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: #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 <[email protected]>

Author: rustyrussell

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;
@@ -181,7 +181,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) */
@@ -1319,6 +1318,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);
@@ -1380,7 +1389,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)) {
@@ -1463,9 +1471,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);
 }
@@ -1776,7 +1786,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;
 };
 
@@ -1793,7 +1803,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 *
@@ -1854,7 +1864,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;
@@ -1867,14 +1877,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;
@@ -1893,7 +1898,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);
@@ -1908,7 +1912,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",
@@ -1919,7 +1922,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);
 }
 
@@ -1931,11 +1934,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;
@@ -1959,10 +1963,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),
@@ -2088,6 +2090,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",

tests/test_xpay.py

@@ -1020,7 +1020,6 @@ def test_xpay_bip353(node_factory):
     l2.rpc.xpay('[email protected]', 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):