MPP: limit the number of HTLCs based on destination connectivity

plugins/keysend.c

@@ -52,18 +52,9 @@ static struct keysend_data *keysend_init(struct payment *p)
 }
 
 static void keysend_cb(struct keysend_data *d, struct payment *p) {
-	struct route_hop *last_hop;
 	struct createonion_hop *last_payload;
 	size_t hopcount;
 
-	if (p->step == PAYMENT_STEP_GOT_ROUTE) {
-		/* Force the last step to be a TLV, we might not have an
-		 * announcement and it still supports it. Required later when
-		 * we adjust the payload. */
-		last_hop = &p->route[tal_count(p->route) - 1];
-		last_hop->style = ROUTE_HOP_TLV;
-	}
-
 	if (p->step != PAYMENT_STEP_ONION_PAYLOAD)
 		return payment_continue(p);
 
@@ -143,6 +134,7 @@ static struct command_result *json_keysend(struct command *cmd, const char *buf,
 	p->json_buffer = tal_steal(p, buf);
 	p->json_toks = params;
 	p->destination = tal_steal(p, destination);
+	p->destination_has_tlv = true;
 	p->payment_secret = NULL;
 	p->amount = *msat;
 	p->invoice = NULL;

plugins/libplugin-pay.c

@@ -1,6 +1,7 @@
 #include <bitcoin/preimage.h>
 #include <ccan/array_size/array_size.h>
 #include <ccan/tal/str/str.h>
+#include <common/json_helpers.h>
 #include <common/json_stream.h>
 #include <common/pseudorand.h>
 #include <common/random_select.h>
@@ -39,12 +40,14 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd,
 	p->failroute_retry = false;
 	p->bolt11 = NULL;
 	p->routetxt = NULL;
+	p->max_htlcs = UINT32_MAX;
 
 	/* Copy over the relevant pieces of information. */
 	if (parent != NULL) {
 		assert(cmd == NULL);
 		tal_arr_expand(&parent->children, p);
 		p->destination = parent->destination;
+		p->destination_has_tlv = parent->destination_has_tlv;
 		p->amount = parent->amount;
 		p->label = parent->label;
 		p->payment_hash = parent->payment_hash;
@@ -1266,6 +1269,7 @@ static void payment_add_hop_onion_payload(struct payment *p,
 					  struct route_hop *node,
 					  struct route_hop *next,
 					  bool final,
+					  bool force_tlv,
 					  struct secret *payment_secret)
 {
 	struct createonion_request *cr = p->createonion_request;
@@ -1279,9 +1283,11 @@ static void payment_add_hop_onion_payload(struct payment *p,
 	 * `next` are the instructions to include in the payload, which is
 	 * basically the channel going to the next node. */
 	dst->style = node->style;
+	if (force_tlv)
+		dst->style = ROUTE_HOP_TLV;
 	dst->pubkey = node->nodeid;
 
-	switch (node->style) {
+	switch (dst->style) {
 	case ROUTE_HOP_LEGACY:
 		dst->legacy_payload = tal(cr->hops, struct legacy_payload);
 		dst->legacy_payload->forward_amt = next->amount;
@@ -1338,7 +1344,8 @@ static void payment_compute_onion_payloads(struct payment *p)
 		/* The message is destined for hop i, but contains fields for
 		 * i+1 */
 		payment_add_hop_onion_payload(p, &cr->hops[i], &p->route[i],
-					      &p->route[i + 1], false, NULL);
+					      &p->route[i + 1], false, false,
+					      NULL);
 		tal_append_fmt(&routetxt, "%s -> ",
 			       type_to_string(tmpctx, struct short_channel_id,
 					      &p->route[i].channel_id));
@@ -1347,7 +1354,8 @@ static void payment_compute_onion_payloads(struct payment *p)
 	/* Final hop */
 	payment_add_hop_onion_payload(
 	    p, &cr->hops[hopcount - 1], &p->route[hopcount - 1],
-	    &p->route[hopcount - 1], true, root->payment_secret);
+	    &p->route[hopcount - 1], true, root->destination_has_tlv,
+	    root->payment_secret);
 	tal_append_fmt(&routetxt, "%s",
 		       type_to_string(tmpctx, struct short_channel_id,
 				      &p->route[hopcount - 1].channel_id));
@@ -2200,8 +2208,21 @@ static struct command_result *routehint_getroute_result(struct command *cmd,
 	 * routehints. */
 	d->destination_reachable = (rtok != NULL);
 
-	if (d->destination_reachable)
+	if (d->destination_reachable) {
 		tal_arr_expand(&d->routehints, NULL);
+		/* The above could trigger a realloc.
+		 * However, p->invoice->routes and d->routehints are
+		 * actually the same array, so we need to update the
+		 * p->invoice->routes pointer, since the realloc
+		 * might have changed pointer addresses, in order to
+		 * ensure that the pointers are not stale.
+		 */
+		p->invoice->routes = d->routehints;
+
+		/* FIXME: ***DO*** we need to add this extra routehint?
+		 * Once we run out of routehints the default system will
+		 * just attempt directly routing to the destination anyway.  */
+	}
 
 	routehint_pre_getroute(d, p);
 
@@ -2254,6 +2275,27 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p)
 		if (p->parent == NULL) {
 			d->routehints = filter_routehints(d, p->local_id,
 							  p->invoice->routes);
+			/* filter_routehints modifies the array, but
+			 * this could trigger a resize and the resize
+			 * could trigger a realloc.
+			 * Keep the invoice pointer up-to-date.
+			 * FIXME: We should really consider that if we are
+			 * mutating p->invoices->routes, maybe we should
+			 * drop d->routehints and just use p->invoice->routes
+			 * directly.
+			 * It is probably not a good idea to *copy* the
+			 * routehints: other paymods are interested in
+			 * p->invoice->routes, and if the routehints system
+			 * itself adds or removes routehints from its
+			 * copy, the *actual* number of routehints that we
+			 * end up using is the one that the routehints paymod
+			 * is maintaining and traversing, and it is *that*
+			 * set of routehints that is the important one.
+			 * So rather than copying the array of routehints
+			 * in paymod, paymod should use (and mutate) the
+			 * p->invoice->routes array, and
+			 */
+			p->invoice->routes = d->routehints;
 
 			paymod_log(p, LOG_DBG,
 				   "After filtering routehints we're left with "
@@ -2281,7 +2323,7 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p)
 			}
 
 			hop.nodeid = *route_pubkey(p, routehint, i + 1);
-			hop.style = ROUTE_HOP_TLV;
+			hop.style = ROUTE_HOP_LEGACY;
 			hop.channel_id = routehint[i].short_channel_id;
 			hop.amount = dest_amount;
 			hop.delay = route_cltv(d->final_cltv, routehint + i + 1,
@@ -2665,7 +2707,7 @@ static void direct_pay_override(struct payment *p) {
 		p->route[0].channel_id = hint->scid.scid;
 		p->route[0].direction = hint->scid.dir;
 		p->route[0].nodeid = *p->destination;
-		p->route[0].style = ROUTE_HOP_TLV;
+		p->route[0].style = p->destination_has_tlv ? ROUTE_HOP_TLV : ROUTE_HOP_LEGACY;
 		paymod_log(p, LOG_DBG,
 			   "Found a direct channel (%s) with sufficient "
 			   "capacity, skipping route computation.",
@@ -2868,16 +2910,53 @@ static struct presplit_mod_data *presplit_mod_data_init(struct payment *p)
 
 static u32 payment_max_htlcs(const struct payment *p)
 {
+	const struct payment *root;
 	struct channel_hint *h;
 	u32 res = 0;
 	for (size_t i = 0; i < tal_count(p->channel_hints); i++) {
 		h = &p->channel_hints[i];
 		if (h->local && h->enabled)
 			res += h->htlc_budget;
 	}
+	root = p;
+	while (root->parent)
+		root = root->parent;
+	if (res > root->max_htlcs)
+		res = root->max_htlcs;
 	return res;
 }
 
+/** payment_lower_max_htlcs
+ *
+ * @brief indicates that we have a good reason to believe that
+ * we should limit our number of max HTLCs.
+ *
+ * @desc Causes future payment_max_htlcs to have a maximum value
+ * they return.
+ * Can be called by multiple paymods: the lowest one any paymod
+ * has given will be used.
+ * If this is called with a limit higher than the existing limit,
+ * it just successfully returns without doing anything.
+ *
+ * @param p - a payment on the payment tree we should limit.
+ * @param limit - the number of max HTLCs.
+ * @param why - the reason we think the given max HTLCs is
+ * reasonable.
+ */
+static void payment_lower_max_htlcs(struct payment *p, u32 limit,
+				    const char *why)
+{
+	struct payment *root = payment_root(p);
+	if (root->max_htlcs > limit) {
+		paymod_log(p, LOG_INFORM,
+			   "%s limit on max HTLCs: %"PRIu32", %s",
+			   root->max_htlcs == UINT32_MAX ?
+				"Initial" : "Lowering",
+			   limit, why);
+		root->max_htlcs = limit;
+	}
+}
+
 static bool payment_supports_mpp(struct payment *p)
 {
 	if (p->invoice == NULL || p->invoice->features == NULL)
@@ -3176,3 +3255,106 @@ static void adaptive_splitter_cb(struct adaptive_split_mod_data *d, struct payme
 
 REGISTER_PAYMENT_MODIFIER(adaptive_splitter, struct adaptive_split_mod_data *,
 			  adaptive_splitter_data_init, adaptive_splitter_cb);
+
+
+/*****************************************************************************
+ * payee_incoming_limit
+ *
+ * @desc every channel has a limit on the number of HTLCs it is willing to
+ * transport.
+ * This is particularly crucial for the payers and payees, as they represent
+ * the bottleneck to and from the network.
+ * The `payment_max_htlcs` function will, by itself, be able to count the
+ * payer-side channels, but assessing the payee requires us to probe the
+ * area around it.
+ *
+ * This paymod must be *after* `routehints` but *before* `presplit` paymods:
+ *
+ * - If we cannot find the destination on the public network, we can only
+ *   use channels it put in the routehints.
+ *   In this case, that is the number of channels we assess the payee as
+ *   having.
+ *   However, the `routehints` paymod may filter out some routehints, thus
+ *   we should assess based on the post-filtered routehints.
+ * - The `presplit` is the first splitter that executes, so we have to have
+ *   performed the payee-channels assessment by then.
+ */
+
+/* The default `max-concurrent-htlcs` is 30, but node operators might want
+ * to push it even lower to reduce their liabilities in case they have to
+ * unilaterally close.
+ * This will not necessarily improve even in a post-anchor-commitments world,
+ * since one of the reasons to unilaterally close is if some HTLC is about to
+ * expire, which of course requires the HTLCs to be published anyway, meaning
+ * it will still be potentially costly.
+ * So our initial assumption is 15 HTLCs per channel.
+ *
+ * The presplitter will divide this by `PRESPLIT_MAX_HTLC_SHARE` as well.
+ */
+#define ASSUMED_MAX_HTLCS_PER_CHANNEL 15
+
+static struct command_result *
+payee_incoming_limit_count(struct command *cmd,
+			   const char *buf,
+			   const jsmntok_t *result,
+			   struct payment *p)
+{
+	const jsmntok_t *channelstok;
+	size_t num_channels = 0;
+
+	channelstok = json_get_member(buf, result, "channels");
+	assert(channelstok);
+
+	/* Count channels.
+	 * `listchannels` returns half-channels, i.e. it normally
+	 * gives two objects per channel, one for each direction.
+	 * However, `listchannels <source>` returns only half-channel
+	 * objects whose `source` is the given channel.
+	 * Thus, the length of `channels` is accurately the number
+	 * of channels.
+	 */
+	num_channels = channelstok->size;
+
+	/* If num_channels is 0, check if there is an invoice.  */
+	if (num_channels == 0 && p->invoice)
+		num_channels = tal_count(p->invoice->routes);
+
+	/* If we got a decent number of channels, limit!  */
+	if (num_channels != 0) {
+		const char *why;
+		u32 lim;
+		why = tal_fmt(tmpctx,
+			      "Destination %s has %zd channels, "
+			      "assuming %d HTLCs per channel",
+			      type_to_string(tmpctx, struct node_id,
+					     p->destination),
+			      num_channels,
+			      ASSUMED_MAX_HTLCS_PER_CHANNEL);
+		lim = num_channels * ASSUMED_MAX_HTLCS_PER_CHANNEL;
+		payment_lower_max_htlcs(p, lim, why);
+	}
+
+	payment_continue(p);
+	return command_still_pending(cmd);
+}
+
+static void payee_incoming_limit_step_cb(void *d UNUSED, struct payment *p)
+{
+	/* Only operate at the initialization of te root payment.
+	 * Also, no point operating if payment does not support MPP anyway.
+	 */
+	if (p->parent || p->step != PAYMENT_STEP_INITIALIZED
+	 || !payment_supports_mpp(p))
+		return payment_continue(p);
+
+	/* Get information on the destination.  */
+	struct out_req *req;
+	req = jsonrpc_request_start(p->plugin, NULL, "listchannels",
+				    &payee_incoming_limit_count,
+				    &payment_rpc_failure, p);
+	json_add_node_id(req->js, "source", p->destination);
+	(void) send_outreq(p->plugin, req);
+}
+
+REGISTER_PAYMENT_MODIFIER(payee_incoming_limit, void *, NULL,
+			  payee_incoming_limit_step_cb);

plugins/libplugin-pay.h

@@ -169,6 +169,8 @@ struct payment {
 
 	/* Real destination we want to route to */
 	struct node_id *destination;
+	/* Do we know for sure that this supports OPT_VAR_ONION? */
+	bool destination_has_tlv;
 
 	/* Payment hash extracted from the invoice if any. */
 	struct sha256 *payment_hash;
@@ -268,6 +270,16 @@ struct payment {
 
 	/* A short description of the route of this payment.  */
 	char *routetxt;
+
+	/* The maximum number of parallel outgoing HTLCs we will allow.
+	 * If unset, the maximum is based on the number of outgoing HTLCs.
+	 * This only applies for the root payment, and is ignored on non-root
+	 * payments.
+	 * Clients of the paymod system MUST NOT modify it, and individual
+	 * paymods MUST interact with it only via the payment_max_htlcs
+	 * and payment_lower_max_htlcs functions.
+	 */
+	u32 max_htlcs;
 };
 
 struct payment_modifier {
@@ -386,6 +398,11 @@ REGISTER_PAYMENT_MODIFIER_HEADER(adaptive_splitter, struct adaptive_split_mod_da
  * or are disabled. We do this only for the root payment, to minimize the
  * overhead. */
 REGISTER_PAYMENT_MODIFIER_HEADER(local_channel_hints, void);
+/* The payee might be less well-connected than ourselves.
+ * This paymod limits the number of HTLCs based on the number of channels
+ * we detect the payee to have, in order to not exhaust the number of HTLCs
+ * each of those channels can bear.  */
+REGISTER_PAYMENT_MODIFIER_HEADER(payee_incoming_limit, void);
 
 struct payment *payment_new(tal_t *ctx, struct command *cmd,
 			    struct payment *parent,

plugins/pay.c

@@ -1915,9 +1915,10 @@ struct payment_modifier *paymod_mods[] = {
 	&exemptfee_pay_mod,
 	&directpay_pay_mod,
 	&shadowroute_pay_mod,
-	/* NOTE: The order in which these two paymods are executed is
+	/* NOTE: The order in which these three paymods are executed is
 	 * significant!
-	 * routehints *must* execute first before presplit.
+	 * routehints *must* execute first before payee_incoming_limit
+	 * which *must* execute bfore presplit.
 	 *
 	 * FIXME: Giving an ordered list of paymods to the paymod
 	 * system is the wrong interface, given that the order in
@@ -1932,6 +1933,7 @@ struct payment_modifier *paymod_mods[] = {
 	 * use.
 	 */
 	&routehints_pay_mod,
+	&payee_incoming_limit_pay_mod,
 	&presplit_pay_mod,
 	&waitblockheight_pay_mod,
 	&retry_pay_mod,
@@ -2021,6 +2023,7 @@ static struct command_result *json_paymod(struct command *cmd,
 	p->json_buffer = tal_steal(p, buf);
 	p->json_toks = params;
 	p->destination = &b11->receiver_id;
+	p->destination_has_tlv = feature_offered(b11->features, OPT_VAR_ONION);
 	p->payment_hash = tal_dup(p, struct sha256, &b11->payment_hash);
 	p->payment_secret = b11->payment_secret
 				? tal_dup(p, struct secret, b11->payment_secret)