From 94babc2ba110ec8beca21eaba27a4132daa52a1a Mon Sep 17 00:00:00 2001 From: Jesse de Wit Date: Tue, 3 Sep 2024 12:45:07 +0200 Subject: [PATCH 1/2] libplugin-pay: use map for channel hints This commit fixes a "FIXME: This is slow" in the pay plugin. For nodes with many channels this is a tremendous improvement in pay performance. PR #7611 improves payment performance from 15 seconds to 13.5 seconds on one of our nodes. This commit improves payment performance from 13.5 seconds to 5.7 seconds. Changelog-Fixed: Improved pathfinding speed for large nodes. --- plugins/libplugin-pay.c | 201 ++++++++++++++++-------------- plugins/libplugin-pay.h | 16 ++- plugins/test/run-route-calc.c | 6 + plugins/test/run-route-overlong.c | 6 + 4 files changed, 133 insertions(+), 96 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 9d1f7eda8bb8..68a6fa7dd55f 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -68,6 +68,33 @@ int libplugin_pay_poll(struct pollfd *fds, nfds_t nfds, int timeout) return daemon_poll(fds, nfds, timeout); } +size_t channel_hint_hash(const struct short_channel_id_dir *out) +{ + struct siphash24_ctx ctx; + siphash24_init(&ctx, siphash_seed()); + siphash24_update(&ctx, &out->scid.u64, sizeof(u64)); + siphash24_update(&ctx, &out->dir, sizeof(int)); + return siphash24_done(&ctx); +} + +const struct short_channel_id_dir *channel_hint_keyof(const struct channel_hint *out) +{ + return &out->scid; +} + +bool channel_hint_eq(const struct channel_hint *a, + const struct short_channel_id_dir *b) +{ + return short_channel_id_eq(a->scid.scid, b->scid) && + a->scid.dir == b->dir; +} + +static void memleak_help_channel_hint_map(struct htable *memtable, + struct channel_hint_map *channel_hints) +{ + memleak_scan_htable(memtable, &channel_hints->raw); +} + struct payment *payment_new(tal_t *ctx, struct command *cmd, struct payment *parent, struct payment_modifier **mods) @@ -132,7 +159,9 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, p->partid = 0; p->next_partid = 1; p->plugin = cmd->plugin; - p->channel_hints = tal_arr(p, struct channel_hint, 0); + p->channel_hints = tal(p, struct channel_hint_map); + channel_hint_map_init(p->channel_hints); + memleak_add_helper(p->channel_hints, memleak_help_channel_hint_map); p->excluded_nodes = tal_arr(p, struct node_id, 0); p->id = next_id++; p->description = NULL; @@ -445,78 +474,79 @@ static void channel_hints_update(struct payment *p, const struct amount_msat *estimated_capacity, u16 *htlc_budget) { + struct channel_hint *hint; struct payment *root = payment_root(p); - struct channel_hint newhint; + struct channel_hint *newhint; + struct short_channel_id_dir scidd; + scidd.scid = scid; + scidd.dir = direction; u32 timestamp = time_now().ts.tv_sec; /* If the channel is marked as enabled it must have an estimate. */ assert(!enabled || estimated_capacity != NULL); - /* Try and look for an existing hint: */ - for (size_t i=0; ichannel_hints); i++) { - struct channel_hint *hint = &root->channel_hints[i]; - if (short_channel_id_eq(hint->scid.scid, scid) && - hint->scid.dir == direction) { - bool modified = false; - /* Prefer to disable a channel. */ - if (!enabled && hint->enabled) { - hint->enabled = false; - modified = true; - } + hint = channel_hint_map_get(root->channel_hints, &scidd); - /* Prefer the more conservative estimate. */ - if (estimated_capacity != NULL && - amount_msat_greater(hint->estimated_capacity, - *estimated_capacity)) { - hint->estimated_capacity = *estimated_capacity; - modified = true; - } - if (htlc_budget != NULL) { - assert(hint->local); - hint->local->htlc_budget = *htlc_budget; - modified = true; - } + if (hint) { + bool modified = false; + /* Prefer to disable a channel. */ + if (!enabled && hint->enabled) { + hint->enabled = false; + modified = true; + } - if (modified) { - hint->timestamp = timestamp; - paymod_log(p, LOG_DBG, - "Updated a channel hint for %s: " - "enabled %s, " - "estimated capacity %s", - fmt_short_channel_id_dir(tmpctx, - &hint->scid), - hint->enabled ? "true" : "false", - fmt_amount_msat(tmpctx, - hint->estimated_capacity)); - channel_hint_notify(p->plugin, hint); - } - return; + /* Prefer the more conservative estimate. */ + if (estimated_capacity != NULL && + amount_msat_greater(hint->estimated_capacity, + *estimated_capacity)) { + hint->estimated_capacity = *estimated_capacity; + modified = true; } + if (htlc_budget != NULL) { + assert(hint->local); + hint->local->htlc_budget = *htlc_budget; + modified = true; + } + + if (modified) { + hint->timestamp = timestamp; + paymod_log(p, LOG_DBG, + "Updated a channel hint for %s: " + "enabled %s, " + "estimated capacity %s", + fmt_short_channel_id_dir(tmpctx, + &hint->scid), + hint->enabled ? "true" : "false", + fmt_amount_msat(tmpctx, + hint->estimated_capacity)); + channel_hint_notify(p->plugin, hint); + } + return; } /* No hint found, create one. */ - newhint.enabled = enabled; - newhint.timestamp = timestamp; - newhint.scid.scid = scid; - newhint.scid.dir = direction; + newhint = tal(root->channel_hints, struct channel_hint); + newhint->enabled = enabled; + newhint->timestamp = timestamp; + newhint->scid = scidd; if (local) { - newhint.local = tal(root->channel_hints, struct local_hint); + newhint->local = tal(newhint, struct local_hint); assert(htlc_budget); - newhint.local->htlc_budget = *htlc_budget; + newhint->local->htlc_budget = *htlc_budget; } else - newhint.local = NULL; + newhint->local = NULL; if (estimated_capacity != NULL) - newhint.estimated_capacity = *estimated_capacity; + newhint->estimated_capacity = *estimated_capacity; - tal_arr_expand(&root->channel_hints, newhint); + channel_hint_map_add(root->channel_hints, newhint); paymod_log( p, LOG_DBG, "Added a channel hint for %s: enabled %s, estimated capacity %s", - fmt_short_channel_id_dir(tmpctx, &newhint.scid), - newhint.enabled ? "true" : "false", - fmt_amount_msat(tmpctx, newhint.estimated_capacity)); - channel_hint_notify(p->plugin, &newhint); + fmt_short_channel_id_dir(tmpctx, &newhint->scid), + newhint->enabled ? "true" : "false", + fmt_amount_msat(tmpctx, newhint->estimated_capacity)); + channel_hint_notify(p->plugin, newhint); } static void payment_exclude_most_expensive(struct payment *p) @@ -591,15 +621,11 @@ static struct channel_hint *payment_chanhints_get(struct payment *p, struct route_hop *h) { struct payment *root = payment_root(p); - struct channel_hint *curhint; - for (size_t j = 0; j < tal_count(root->channel_hints); j++) { - curhint = &root->channel_hints[j]; - if (short_channel_id_eq(curhint->scid.scid, h->scid) && - curhint->scid.dir == h->direction) { - return curhint; - } - } - return NULL; + struct short_channel_id_dir scid; + scid.scid = h->scid; + scid.dir = h->direction; + + return channel_hint_map_get(root->channel_hints, &scid); } /* Given a route and a couple of channel hints, apply the route to the channel @@ -726,8 +752,10 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p) struct channel_hint *hint; struct short_channel_id_dir *res = tal_arr(ctx, struct short_channel_id_dir, 0); - for (size_t i = 0; i < tal_count(root->channel_hints); i++) { - hint = &root->channel_hints[i]; + struct channel_hint_map_iter iter; + for (hint = channel_hint_map_first(root->channel_hints, &iter); + hint; + hint = channel_hint_map_next(root->channel_hints, &iter)) { if (!hint->enabled) tal_arr_expand(&res, hint->scid); @@ -751,19 +779,6 @@ static const struct node_id *payment_get_excluded_nodes(const tal_t *ctx, return root->excluded_nodes; } -/* FIXME: This is slow! */ -static const struct channel_hint *find_hint(const struct channel_hint *hints, - struct short_channel_id scid, - int dir) -{ - for (size_t i = 0; i < tal_count(hints); i++) { - if (short_channel_id_eq(scid, hints[i].scid.scid) - && dir == hints[i].scid.dir) - return &hints[i]; - } - return NULL; -} - /* FIXME: This is slow! */ static bool dst_is_excluded(const struct gossmap *gossmap, const struct gossmap_chan *c, @@ -791,7 +806,7 @@ static bool payment_route_check(const struct gossmap *gossmap, struct amount_msat amount, struct payment *p) { - struct short_channel_id scid; + struct short_channel_id_dir scidd; const struct channel_hint *hint; if (dst_is_excluded(gossmap, c, dir, payment_root(p)->excluded_nodes)) @@ -800,8 +815,9 @@ static bool payment_route_check(const struct gossmap *gossmap, if (dst_is_excluded(gossmap, c, dir, p->temp_exclusion)) return false; - scid = gossmap_chan_scid(gossmap, c); - hint = find_hint(payment_root(p)->channel_hints, scid, dir); + scidd.scid = gossmap_chan_scid(gossmap, c); + scidd.dir = dir; + hint = channel_hint_map_get(payment_root(p)->channel_hints, &scidd); if (!hint) return true; @@ -2852,7 +2868,7 @@ static bool routehint_excluded(struct payment *p, const struct node_id *nodes = payment_get_excluded_nodes(tmpctx, p); const struct short_channel_id_dir *chans = payment_get_excluded_channels(tmpctx, p); - const struct channel_hint *hints = payment_root(p)->channel_hints; + const struct channel_hint_map *hints = payment_root(p)->channel_hints; /* Note that we ignore direction here: in theory, we could have * found that one direction of a channel is unavailable, but they @@ -2892,13 +2908,17 @@ static bool routehint_excluded(struct payment *p, * know the exact capacity we need to send via this * channel, which is greater than the destination. */ - for (size_t j = 0; j < tal_count(hints); j++) { - if (!short_channel_id_eq(hints[j].scid.scid, r->short_channel_id)) + struct channel_hint *hint; + struct channel_hint_map_iter iter; + for (hint = channel_hint_map_first(hints, &iter); + hint; + hint = channel_hint_map_next(hints, &iter)) { + if (!short_channel_id_eq(hint->scid.scid, r->short_channel_id)) continue; /* We exclude on equality because we set the estimate * to the smallest failed attempt. */ if (amount_msat_greater_eq(needed_capacity, - hints[j].estimated_capacity)) + hint->estimated_capacity)) return true; } } @@ -3532,14 +3552,7 @@ static void direct_pay_override(struct payment *p) { /* If we have a channel we need to make sure that it still has * sufficient capacity. Look it up in the channel_hints. */ - for (size_t i=0; ichannel_hints); i++) { - struct short_channel_id_dir *cur = &root->channel_hints[i].scid; - if (short_channel_id_eq(cur->scid, d->chan->scid) && - cur->dir == d->chan->dir) { - hint = &root->channel_hints[i]; - break; - } - } + hint = channel_hint_map_get(root->channel_hints, d->chan); if (hint && hint->enabled && amount_msat_greater(hint->estimated_capacity, p->our_amount)) { @@ -3643,9 +3656,11 @@ static u32 payment_max_htlcs(const struct payment *p) { const struct payment *root; struct channel_hint *h; + struct channel_hint_map_iter iter; u32 res = 0; - for (size_t i = 0; i < tal_count(p->channel_hints); i++) { - h = &p->channel_hints[i]; + for (h = channel_hint_map_first(p->channel_hints, &iter); + h; + h = channel_hint_map_next(p->channel_hints, &iter)) { if (h->local && h->enabled) res += h->local->htlc_budget; } diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index 283c312fa5fe..067e6ca27261 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -172,6 +172,16 @@ struct payment_constraints { u32 cltv_budget; }; +size_t channel_hint_hash(const struct short_channel_id_dir *out); + +const struct short_channel_id_dir *channel_hint_keyof(const struct channel_hint *out); + +bool channel_hint_eq(const struct channel_hint *a, + const struct short_channel_id_dir *b); + +HTABLE_DEFINE_TYPE(struct channel_hint, channel_hint_keyof, + channel_hint_hash, channel_hint_eq, channel_hint_map) + struct payment { /* Usually in global payments list */ struct list_node list; @@ -268,9 +278,9 @@ struct payment { struct route_info **routes; const u8 *features; - /* tal_arr of channel_hints we incrementally learn while performing - * payment attempts. */ - struct channel_hint *channel_hints; + /* htable of channel_hints we incrementally learn while performing + * payment attempts, indexed by scid and direction. */ + struct channel_hint_map *channel_hints; struct node_id *excluded_nodes; /* Optional temporarily excluded channels/nodes (i.e. this routehint) */ diff --git a/plugins/test/run-route-calc.c b/plugins/test/run-route-calc.c index da73c25a9a8b..1cc3fa573854 100644 --- a/plugins/test/run-route-calc.c +++ b/plugins/test/run-route-calc.c @@ -259,6 +259,12 @@ struct json_stream *jsonrpc_stream_fail(struct command *cmd UNNEEDED, /* Generated stub for jsonrpc_stream_success */ struct json_stream *jsonrpc_stream_success(struct command *cmd UNNEEDED) { fprintf(stderr, "jsonrpc_stream_success called!\n"); abort(); } +/* Generated stub for memleak_add_helper_ */ +void memleak_add_helper_(const tal_t *p UNNEEDED, void (*cb)(struct htable *memtable UNNEEDED, + const tal_t *)){ } +/* Generated stub for memleak_scan_htable */ +void memleak_scan_htable(struct htable *memtable UNNEEDED, const struct htable *ht UNNEEDED) +{ fprintf(stderr, "memleak_scan_htable called!\n"); abort(); } /* Generated stub for notleak_ */ void *notleak_(void *ptr UNNEEDED, bool plus_children UNNEEDED) { fprintf(stderr, "notleak_ called!\n"); abort(); } diff --git a/plugins/test/run-route-overlong.c b/plugins/test/run-route-overlong.c index 4a7d94b3f7d2..2f771befa29b 100644 --- a/plugins/test/run-route-overlong.c +++ b/plugins/test/run-route-overlong.c @@ -256,6 +256,12 @@ struct json_stream *jsonrpc_stream_fail(struct command *cmd UNNEEDED, /* Generated stub for jsonrpc_stream_success */ struct json_stream *jsonrpc_stream_success(struct command *cmd UNNEEDED) { fprintf(stderr, "jsonrpc_stream_success called!\n"); abort(); } +/* Generated stub for memleak_add_helper_ */ +void memleak_add_helper_(const tal_t *p UNNEEDED, void (*cb)(struct htable *memtable UNNEEDED, + const tal_t *)){ } +/* Generated stub for memleak_scan_htable */ +void memleak_scan_htable(struct htable *memtable UNNEEDED, const struct htable *ht UNNEEDED) +{ fprintf(stderr, "memleak_scan_htable called!\n"); abort(); } /* Generated stub for notleak_ */ void *notleak_(void *ptr UNNEEDED, bool plus_children UNNEEDED) { fprintf(stderr, "notleak_ called!\n"); abort(); } From 701913c1e32274e1fa5f4626e5f0ad6dd13dcb81 Mon Sep 17 00:00:00 2001 From: Jesse de Wit Date: Wed, 4 Sep 2024 11:38:17 +0200 Subject: [PATCH 2/2] libplugin-pay: channel map in routehint_excluded --- plugins/libplugin-pay.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 68a6fa7dd55f..35099e502e7d 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2909,18 +2909,17 @@ static bool routehint_excluded(struct payment *p, * channel, which is greater than the destination. */ struct channel_hint *hint; - struct channel_hint_map_iter iter; - for (hint = channel_hint_map_first(hints, &iter); - hint; - hint = channel_hint_map_next(hints, &iter)) { - if (!short_channel_id_eq(hint->scid.scid, r->short_channel_id)) - continue; - /* We exclude on equality because we set the estimate - * to the smallest failed attempt. */ - if (amount_msat_greater_eq(needed_capacity, + struct short_channel_id_dir scidd; + scidd.scid = r->short_channel_id; + scidd.dir = node_id_cmp(&routehint->pubkey, + p->pay_destination) > 0 ? 1 : 0; + + hint = channel_hint_map_get(hints, &scidd); + /* We exclude on equality because we set the estimate + * to the smallest failed attempt. */ + if (hint && amount_msat_greater_eq(needed_capacity, hint->estimated_capacity)) - return true; - } + return true; } return false; }