diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index f821d0cf8634..c6a868c8d03c 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -2919,9 +2920,6 @@ static u32 payment_max_htlcs(const struct payment *p) return res; } -/* Temporary comment out else GCC will complain that it is unused; - * will be used in a later commit. */ -#if 0 /** payment_lower_max_htlcs * * @brief indicates that we have a good reason to believe that @@ -2952,7 +2950,6 @@ static void payment_lower_max_htlcs(struct payment *p, u32 limit, root->max_htlcs = limit; } } -#endif static bool payment_supports_mpp(struct payment *p) { @@ -3252,3 +3249,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 ` 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); diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index d80fef5aa32c..9b640be121e6 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -396,6 +396,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, diff --git a/plugins/pay.c b/plugins/pay.c index 6f5d7c249472..84f09096bd2e 100644 --- a/plugins/pay.c +++ b/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, diff --git a/tests/test_pay.py b/tests/test_pay.py index ac439f9096fb..f110e48159d8 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3521,10 +3521,9 @@ def test_large_mpp_presplit(node_factory): @unittest.skipIf(VALGRIND, "6 nodes") @unittest.skipIf(not DEVELOPER, "builds large network, which is slow if not DEVELOPER") @pytest.mark.slow_test -@pytest.mark.xfail(strict=True) def test_mpp_overload_payee(node_factory, bitcoind): """ - We have a bug where if the payer is unusually well-connected compared + We had a bug where if the payer is unusually well-connected compared to the payee, the payee is unable to accept a large payment since the payer will split it into lots of tiny payments, which would choke the max-concurrent-htlcs limit of the payee.