diff --git a/Makefile b/Makefile index f835842c99ee..d190761d0e9d 100644 --- a/Makefile +++ b/Makefile @@ -572,7 +572,7 @@ check-discouraged-functions: # since it risks overflow. check-amount-access: @! (git grep -nE "(->|\.)(milli)?satoshis" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*" | grep -v '/* Raw:') - @! git grep -nE "\\(struct amount_(m)?sat\\)" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*" + @! git grep -nE "\\(struct amount_(m)?sat\\)" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*" | grep -vE "sizeof.struct amount_(m)?sat." # For those without working cppcheck check-source-no-cppcheck: check-makefile check-source-bolt check-whitespace check-spelling check-python check-includes check-shellcheck check-setup_locale check-tmpctx check-discouraged-functions check-amount-access diff --git a/common/json_stream.c b/common/json_stream.c index 31a5367b15e4..60517b3fbc82 100644 --- a/common/json_stream.c +++ b/common/json_stream.c @@ -626,6 +626,14 @@ void json_add_amount_msat(struct json_stream *result, json_add_u64(result, msatfieldname, msat.millisatoshis); /* Raw: low-level helper */ } +void json_add_amount_sat(struct json_stream *result, + const char *satfieldname, + struct amount_sat sat) +{ + assert(strends(satfieldname, "_sat") || streq(satfieldname, "sat")); + json_add_u64(result, satfieldname, sat.satoshis); /* Raw: low-level helper */ +} + void json_add_amount_sat_msat(struct json_stream *result, const char *msatfieldname, struct amount_sat sat) diff --git a/common/json_stream.h b/common/json_stream.h index eeca272c3e09..9cc539e29a52 100644 --- a/common/json_stream.h +++ b/common/json_stream.h @@ -5,10 +5,10 @@ #define LIGHTNING_COMMON_JSON_STREAM_H #include "config.h" -#include #define JSMN_STRICT 1 # include +#include #include #include #include @@ -334,6 +334,11 @@ void json_add_amount_msat(struct json_stream *result, struct amount_msat msat) NO_NULL_ARGS; +void json_add_amount_sat(struct json_stream *result, + const char *satfieldname, + struct amount_sat sat) + NO_NULL_ARGS; + /* Adds an 'msat' field */ void json_add_amount_sat_msat(struct json_stream *result, const char *msatfieldname, diff --git a/common/route.c b/common/route.c index 19ee2215c3b2..b699742abe90 100644 --- a/common/route.c +++ b/common/route.c @@ -81,6 +81,7 @@ static bool dijkstra_to_hops(struct route_hop **hops, const struct gossmap_node *next; size_t num_hops = tal_count(*hops); const struct half_chan *h; + struct amount_msat total_msat; if (dist == 0) return true; @@ -92,12 +93,9 @@ static bool dijkstra_to_hops(struct route_hop **hops, /* OK, populate other fields. */ c = dijkstra_best_chan(dij, curidx); - if (c->half[0].nodeidx == curidx) { - (*hops)[num_hops].direction = 0; - } else { - assert(c->half[1].nodeidx == curidx); - (*hops)[num_hops].direction = 1; - } + + assert(c->half[0].nodeidx == curidx || c->half[1].nodeidx == curidx); + (*hops)[num_hops].direction = c->half[0].nodeidx == curidx ? 0 : 1; (*hops)[num_hops].scid = gossmap_chan_scid(gossmap, c); /* Find other end of channel. */ @@ -107,6 +105,8 @@ static bool dijkstra_to_hops(struct route_hop **hops, if (!dijkstra_to_hops(hops, gossmap, dij, next, amount, cltv)) return false; + total_msat = gossmap_chan_get_capacity(gossmap, c); + (*hops)[num_hops].capacity = total_msat; (*hops)[num_hops].amount = *amount; (*hops)[num_hops].delay = *cltv; diff --git a/common/route.h b/common/route.h index d6ea2b06634e..716065d557d5 100644 --- a/common/route.h +++ b/common/route.h @@ -18,6 +18,7 @@ struct gossmap_node; * @direction: 0 (dest node_id < src node_id), 1 (dest node_id > src). * @node_id: the node_id of the destination of this hop. * @amount: amount to send through this hop. + * @capacity: The amount the channel was funded with. * @delay: total cltv delay at this hop. */ struct route_hop { @@ -25,6 +26,7 @@ struct route_hop { int direction; struct node_id node_id; struct amount_msat amount; + struct amount_msat capacity; u32 delay; }; diff --git a/common/test/Makefile b/common/test/Makefile index 720ce84e5bd1..3313a10577c6 100644 --- a/common/test/Makefile +++ b/common/test/Makefile @@ -45,6 +45,7 @@ common/test/run-route common/test/run-route-specific common/test/run-route-inflo common/node_id.o \ common/pseudorand.o \ common/route.o \ + gossipd/gossip_store_wiregen.o \ wire/fromwire.o \ wire/peer_wiregen.o \ wire/towire.o diff --git a/common/test/run-route-specific.c b/common/test/run-route-specific.c index d276ff896477..77ffb8da411a 100644 --- a/common/test/run-route-specific.c +++ b/common/test/run-route-specific.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -139,6 +140,13 @@ static void add_connection(int store_fd, ids[0], ids[1], &dummy_key, &dummy_key); write_to_store(store_fd, msg); + tal_free(msg); + + /* Also needs a hint as to the funding size. */ + struct amount_sat capacity = AMOUNT_SAT(100000000); + msg = towire_gossip_store_channel_amount(tmpctx, capacity); + write_to_store(store_fd, msg); + tal_free(msg); update_connection(store_fd, from, to, shortid, min, max, base_fee, proportional_fee, diff --git a/common/test/run-route.c b/common/test/run-route.c index 438aed60c4ed..638c688afe39 100644 --- a/common/test/run-route.c +++ b/common/test/run-route.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -129,8 +130,13 @@ static void add_connection(int store_fd, &dummy_key, &dummy_key); write_to_store(store_fd, msg); - update_connection(store_fd, from, to, base_fee, proportional_fee, - delay, false); + /* Also needs a hint as to the funding size. */ + struct amount_sat capacity = AMOUNT_SAT(100000000); + msg = towire_gossip_store_channel_amount(tmpctx, capacity); + write_to_store(store_fd, msg); + tal_free(msg); + update_connection(store_fd, from, to, base_fee, proportional_fee, delay, + false); } static bool channel_is_between(const struct gossmap *gossmap, diff --git a/plugins/Makefile b/plugins/Makefile index e236af31798d..23c8d1492b72 100644 --- a/plugins/Makefile +++ b/plugins/Makefile @@ -27,8 +27,14 @@ PLUGIN_LIB_SRC := plugins/libplugin.c PLUGIN_LIB_HEADER := plugins/libplugin.h PLUGIN_LIB_OBJS := $(PLUGIN_LIB_SRC:.c=.o) -PLUGIN_PAY_LIB_SRC := plugins/libplugin-pay.c -PLUGIN_PAY_LIB_HEADER := plugins/libplugin-pay.h +PLUGIN_PAY_LIB_SRC := \ + plugins/channel_hint.c \ + plugins/libplugin-pay.c + +PLUGIN_PAY_LIB_HEADER := \ + plugins/channel_hint.h \ + plugins/libplugin-pay.h + PLUGIN_PAY_LIB_OBJS := $(PLUGIN_PAY_LIB_SRC:.c=.o) PLUGIN_OFFERS_SRC := plugins/offers.c plugins/offers_offer.c plugins/offers_invreq_hook.c plugins/offers_inv_hook.c plugins/establish_onion_path.c plugins/fetchinvoice.c diff --git a/plugins/channel_hint.c b/plugins/channel_hint.c new file mode 100644 index 000000000000..4dc065410b20 --- /dev/null +++ b/plugins/channel_hint.c @@ -0,0 +1,199 @@ +#include "config.h" +#include + +void channel_hint_to_json(const char *name, const struct channel_hint *hint, + struct json_stream *dest) +{ + json_object_start(dest, name); + json_add_u32(dest, "timestamp", hint->timestamp); + json_add_short_channel_id_dir(dest, "scid", hint->scid); + json_add_amount_msat(dest, "estimated_capacity_msat", + hint->estimated_capacity); + json_add_amount_msat(dest, "total_capacity_msat", hint->capacity); + json_add_bool(dest, "enabled", hint->enabled); + json_object_end(dest); +} + +/* How long until even a channel whose estimate is down at 0msat will + * be considered fully refilled. The refill rate is the inverse of + * this times the channel size. The refilling is a linear + * approximation, with a small hysteresis applied in order to prevent + * a single payment relaxing its own constraints thus causing it to + * prematurely retry an already attempted channel. + */ +#define PAY_REFILL_TIME 7200 + +/* Add an artificial delay before accepting updates. This ensures we + * don't actually end up relaxing a tight constraint inbetween the + * attempt that added it and the next retry. If we were to relax right + * away, then we could end up retrying the exact same path we just + * failed at. If the `time_between_attempts * refill > 1msat`, we'd + * end up not actually constraining at all, because we set the + * estimate to `attempt - 1msat`. This also results in the updates + * being limited to once every minute, and causes a stairway + * pattern. The hysteresis has to be >60s otherwise a single payment + * can already end up retrying a previously excluded channel. + */ +#define PAY_REFILL_HYSTERESIS 60 +/** + * Update the `channel_hint` in place, return whether it should be kept. + * + * This computes the refill-rate based on the overall capacity, and + * the time elapsed since the last update and relaxes the upper bound + * on the capacity, and resets the enabled flag if appropriate. If the + * hint is no longer useful, i.e., it does not provide any additional + * information on top of the structural information we've learned from + * the gossip, then we return `false` to signal that the + * `channel_hint` may be removed. + */ +bool channel_hint_update(const struct timeabs now, struct channel_hint *hint) +{ + /* Precision is not required here, so integer division is good + * enough. But keep the order such that we do not round down + * too much. We do so by first multiplying, before + * dividing. The formula is `current = last + delta_t * + * overall / refill_rate`. + */ + struct amount_msat refill; + struct amount_msat capacity = hint->capacity; + + if (now.ts.tv_sec < hint->timestamp + PAY_REFILL_HYSTERESIS) + return true; + + u64 seconds = now.ts.tv_sec - hint->timestamp; + if (!amount_msat_mul(&refill, capacity, seconds)) + abort(); + + refill = amount_msat_div(refill, PAY_REFILL_TIME); + if (!amount_msat_add(&hint->estimated_capacity, + hint->estimated_capacity, refill)) + abort(); + + /* Clamp the value to the `overall_capacity` */ + if (amount_msat_greater(hint->estimated_capacity, capacity)) + hint->estimated_capacity = capacity; + + /* TODO This is rather coarse. We could map the disabled flag + to having 0msat capacity, and then relax from there. But it'd + likely be too slow of a relaxation.*/ + if (seconds > 60) + hint->enabled = true; + + /* Since we update in-place we should make sure that we can + * just call update again and the result is stable, if no time + * has passed. */ + hint->timestamp = now.ts.tv_sec; + + /* We report this hint as useless, if the hint does not + * restrict the channel, i.e., if it is enabled and the + * estimate is the same as the overall capacity. */ + return !hint->enabled || + amount_msat_greater(capacity, hint->estimated_capacity); +} + +struct channel_hint *channel_hint_set_find(struct channel_hint_set *self, + const struct short_channel_id_dir *scidd) +{ + for (size_t i=0; ihints); i++) { + struct channel_hint *hint = &self->hints[i]; + if (short_channel_id_dir_eq(&hint->scid, scidd)) + return hint; + } + return NULL; +} + +/* See header */ +struct channel_hint * +channel_hint_set_add(struct channel_hint_set *self, u32 timestamp, + const struct short_channel_id_dir *scidd, bool enabled, + const struct amount_msat *estimated_capacity, + const struct amount_msat capacity, u16 *htlc_budget) +{ + struct channel_hint *copy, *old, *newhint; + + /* If the channel is marked as enabled it must have an estimate. */ + assert(!enabled || estimated_capacity != NULL); + + /* If there was no hint, add the new one, if there was one, + * pick the one with the newer timestamp. */ + old = channel_hint_set_find(self, scidd); + copy = tal_dup(tmpctx, struct channel_hint, old); + if (old == NULL) { + newhint = tal(tmpctx, struct channel_hint); + newhint->enabled = enabled; + newhint->scid = *scidd; + newhint->capacity = capacity; + if (estimated_capacity != NULL) + newhint->estimated_capacity = *estimated_capacity; + newhint->local = NULL; + newhint->timestamp = timestamp; + tal_arr_expand(&self->hints, *newhint); + return &self->hints[tal_count(self->hints) - 1]; + } else if (old->timestamp <= timestamp) { + /* `local` is kept, since we do not pass in those + * annotations here. */ + old->enabled = enabled; + old->timestamp = timestamp; + if (estimated_capacity != NULL) + old->estimated_capacity = *estimated_capacity; + + /* We always pick the larger of the capacities we are + * being told. This is because in some cases, such as + * routehints, we're not actually being told the total + * capacity, just lower values. */ + if (amount_msat_greater(capacity, old->capacity)) + old->capacity = capacity; + + return copy; + } else { + return NULL; + } +} + +/** + * Load a channel_hint from its JSON representation. + * + * @return The initialized `channel_hint` or `NULL` if we encountered a parsing + * error. + */ +struct channel_hint *channel_hint_from_json(const tal_t *ctx, + const char *buffer, + const jsmntok_t *toks) +{ + const char *ret; + const jsmntok_t *payload = json_get_member(buffer, toks, "payload"), + *jhint = + json_get_member(buffer, payload, "channel_hint"); + struct channel_hint *hint = tal(ctx, struct channel_hint); + + ret = json_scan(ctx, buffer, jhint, + "{timestamp:%,scid:%,estimated_capacity_msat:%,total_capacity_msat:%,enabled:%}", + JSON_SCAN(json_to_u32, &hint->timestamp), + JSON_SCAN(json_to_short_channel_id_dir, &hint->scid), + JSON_SCAN(json_to_msat, &hint->estimated_capacity), + JSON_SCAN(json_to_msat, &hint->capacity), + JSON_SCAN(json_to_bool, &hint->enabled)); + + if (ret != NULL) + hint = tal_free(hint); + return hint; +} + +struct channel_hint_set *channel_hint_set_new(const tal_t *ctx) +{ + struct channel_hint_set *set = tal(ctx, struct channel_hint_set); + set->hints = tal_arr(set, struct channel_hint, 0); + return set; +} + +void channel_hint_set_update(struct channel_hint_set *set, + const struct timeabs now) +{ + for (size_t i = 0; i < tal_count(set->hints); i++) + channel_hint_update(time_now(), &set->hints[i]); +} + +size_t channel_hint_set_count(const struct channel_hint_set *set) +{ + return tal_count(set->hints); +} diff --git a/plugins/channel_hint.h b/plugins/channel_hint.h new file mode 100644 index 000000000000..c7cc9cca8474 --- /dev/null +++ b/plugins/channel_hint.h @@ -0,0 +1,95 @@ +#ifndef LIGHTNING_PLUGINS_CHANNEL_HINT_H +#define LIGHTNING_PLUGINS_CHANNEL_HINT_H + +#include "config.h" +#include +#include +#include +#include +#include +#include + +/* Information about channels we inferred from a) looking at our channels, and + * b) from failures encountered during attempts to perform a payment. These + * are attached to the root payment, since that information is + * global. Attempts update the estimated channel capacities when starting, and + * get remove on failure. Success keeps the capacities, since the capacities + * changed due to the successful HTLCs. */ +struct channel_hint { + /* The timestamp this observation was made. Used to let the + * constraint expressed by this hint decay over time, until it + * is fully relaxed, at which point we can forget about it + * (the structural information is the best we can do in that + * case). + */ + u32 timestamp; + /* The short_channel_id we're going to use when referring to + * this channel. This can either be the real scid, or the + * local alias. The `pay` algorithm doesn't really care which + * one it is, but we'll prefer the scid as that's likely more + * readable than the alias. */ + struct short_channel_id_dir scid; + + /* Upper bound on remove channels inferred from payment failures. */ + struct amount_msat estimated_capacity; + + /* Is the channel enabled? */ + bool enabled; + + /* Non-null if we are one endpoint of this channel */ + struct local_hint *local; + + /* The total `amount_msat` that were used to fund the + * channel. This is always smaller gte the estimated_capacity + * (after normalization) */ + struct amount_msat capacity; +}; + +/* A collection of channel_hint instances, allowing us to handle and + * update them more easily. */ +struct channel_hint_set { + /* tal_arr of channel_hints. */ + struct channel_hint *hints; +}; + +bool channel_hint_update(const struct timeabs now, + struct channel_hint *hint); + +void channel_hint_to_json(const char *name, const struct channel_hint *hint, + struct json_stream *dest); + +struct channel_hint *channel_hint_from_json(const tal_t *ctx, + const char *buffer, + const jsmntok_t *toks); + +struct channel_hint_set *channel_hint_set_new(const tal_t *ctx); + +/* Relax all channel_hints in this set, based on the time that has elapsed. */ +void channel_hint_set_update(struct channel_hint_set *set, const struct timeabs now); + +/** + * Look up a `channel_hint` from a `channel_hint_set` for a scidd. + */ +struct channel_hint *channel_hint_set_find(struct channel_hint_set *self, + const struct short_channel_id_dir *scidd); + +/** + * Add a new observation to the `channel_hint_set` + * + * This either adds a new entry, or updates an existing one in the set. + * @return A new channel_hint, if the addition resulted in changes. + */ +struct channel_hint *channel_hint_set_add(struct channel_hint_set *self, + u32 timestamp, + const struct short_channel_id_dir *scidd, + bool enabled, + const struct amount_msat *estimated_capacity, + const struct amount_msat overall_capacity, + u16 *htlc_budget); + +/** + * Count how many channel_hints the set contains. + */ +size_t channel_hint_set_count(const struct channel_hint_set *set); + +#endif /* LIGHTNING_PLUGINS_CHANNEL_HINT_H */ diff --git a/plugins/keysend.c b/plugins/keysend.c index 99d46c1bb8ac..75821bf0c650 100644 --- a/plugins/keysend.c +++ b/plugins/keysend.c @@ -17,6 +17,7 @@ static unsigned int maxdelay_default; static struct node_id my_id; static u64 *accepted_extra_tlvs; +static struct channel_hint_set *global_hints; /***************************************************************************** * Keysend modifier @@ -159,6 +160,8 @@ static const char *init(struct plugin *p, const char *buf UNUSED, rpc_scan(p, "getinfo", take(json_out_obj(NULL, NULL, NULL)), "{id:%}", JSON_SCAN(json_to_node_id, &my_id)); + global_hints = notleak_with_children(channel_hint_set_new(p)); + accepted_extra_tlvs = notleak(tal_arr(NULL, u64, 0)); /* BOLT #4: * ## `max_htlc_cltv` Selection @@ -241,7 +244,7 @@ static struct command_result *json_keysend(struct command *cmd, const char *buf, NULL)) return command_param_failed(); - p = payment_new(cmd, cmd, NULL /* No parent */, pay_mods); + p = payment_new(cmd, cmd, NULL /* No parent */, global_hints, pay_mods); p->local_id = &my_id; p->json_buffer = tal_dup_talarr(p, const char, buf); p->json_toks = params; diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 572fcc9e3751..2e406337f760 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1,5 +1,6 @@ #include "config.h" #include +#include #include #include #include @@ -70,6 +71,7 @@ int libplugin_pay_poll(struct pollfd *fds, nfds_t nfds, int timeout) struct payment *payment_new(tal_t *ctx, struct command *cmd, struct payment *parent, + struct channel_hint_set *channel_hints, struct payment_modifier **mods) { struct payment *p = tal(ctx, struct payment); @@ -132,7 +134,6 @@ 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->excluded_nodes = tal_arr(p, struct node_id, 0); p->id = next_id++; p->description = NULL; @@ -142,6 +143,8 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, p->groupid = 0; p->mods = NULL; p->chainlag = 0; + assert(channel_hints != NULL); + p->hints = channel_hints; } /* Initialize all modifier data so we can point to the fields when @@ -387,41 +390,6 @@ void payment_start(struct payment *p) payment_start_at_blockheight(p, INVALID_BLOCKHEIGHT); } -static void channel_hint_to_json(const char *name, const struct channel_hint *hint, struct json_stream *dest) -{ - json_object_start(dest, name); - json_add_u32(dest, "timestamp", hint->timestamp); - json_add_short_channel_id_dir(dest, "scid", hint->scid); - json_add_amount_msat(dest, "capacity_msat", hint->estimated_capacity); - json_add_bool(dest, "enabled", hint->enabled); - json_object_end(dest); -} - -/** - * Load a channel_hint from its JSON representation. - * - * @return The initialized `channel_hint` or `NULL` if we encountered a parsing - * error. - */ -/* -static struct channel_hint *channel_hint_from_json(const tal_t *ctx, - const char *buffer, - const jsmntok_t *toks) -{ - const char *ret; - struct channel_hint *hint = tal(ctx, struct channel_hint); - ret = json_scan(ctx, buffer, toks, - "{timestamp:%,scid:%,capacity_msat:%,enabled:%}", - JSON_SCAN(json_to_u32, &hint->timestamp), - JSON_SCAN(json_to_short_channel_id_dir, &hint->scid), - JSON_SCAN(json_to_msat, &hint->estimated_capacity), - JSON_SCAN(json_to_bool, &hint->enabled)); - - if (ret != NULL) - hint = tal_free(hint); - return hint; -} -*/ /** * Notify subscribers of the `channel_hint` topic about a changed hint * @@ -443,114 +411,43 @@ static void channel_hints_update(struct payment *p, const struct short_channel_id scid, int direction, bool enabled, bool local, const struct amount_msat *estimated_capacity, + const struct amount_msat overall_capacity, u16 *htlc_budget) { struct payment *root = payment_root(p); - struct channel_hint newhint; - u32 timestamp = time_now().ts.tv_sec; - - /* If the channel is marked as enabled it must have an estimate. */ - assert(!enabled || estimated_capacity != NULL); + struct short_channel_id_dir *scidd = + tal(tmpctx, struct short_channel_id_dir); + struct channel_hint *hint; + scidd->scid = scid; + scidd->dir = direction; - /* 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; - } + /* Local channels must have an HTLC budget */ + assert(!local || htlc_budget != NULL); - /* 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; - } + channel_hint_set_add(root->hints, time_now().ts.tv_sec, scidd, enabled, + estimated_capacity, overall_capacity, htlc_budget); - 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; - } - } + hint = channel_hint_set_find(root->hints, scidd); - /* No hint found, create one. */ - newhint.enabled = enabled; - newhint.timestamp = timestamp; - newhint.scid.scid = scid; - newhint.scid.dir = direction; if (local) { - newhint.local = tal(root->channel_hints, struct local_hint); - assert(htlc_budget); - newhint.local->htlc_budget = *htlc_budget; - } else - newhint.local = NULL; - if (estimated_capacity != NULL) - newhint.estimated_capacity = *estimated_capacity; - - tal_arr_expand(&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); -} - -static void payment_exclude_most_expensive(struct payment *p) -{ - struct route_hop *e = &p->route[0]; - struct amount_msat fee, worst = AMOUNT_MSAT(0); - - for (size_t i = 0; i < tal_count(p->route)-1; i++) { - if (!amount_msat_sub(&fee, p->route[i].amount, p->route[i+1].amount)) - paymod_err(p, "Negative fee in a route."); - - if (amount_msat_greater_eq(fee, worst)) { - e = &p->route[i]; - worst = fee; - } + hint->local = tal_free(hint->local); + hint->local = tal(root->hints, struct local_hint); + hint->local->htlc_budget = *htlc_budget; } - channel_hints_update(p, e->scid, e->direction, false, false, - NULL, NULL); -} -static void payment_exclude_longest_delay(struct payment *p) -{ - struct route_hop *e = &p->route[0]; - u32 delay, worst = 0; + /* If the channel is marked as enabled it must have an estimate. */ + assert(!enabled || estimated_capacity != NULL); - for (size_t i = 0; i < tal_count(p->route)-1; i++) { - delay = p->route[i].delay - p->route[i+1].delay; - if (delay >= worst) { - e = &p->route[i]; - worst = delay; - } + if (hint != NULL) { + 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); } - channel_hints_update(p, e->scid, e->direction, false, false, - NULL, NULL); } static struct amount_msat payment_route_fee(struct payment *p) @@ -591,15 +488,8 @@ 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 scidd = {.scid = h->scid, .dir = h->direction}; + return channel_hint_set_find(root->hints, &scidd); } /* Given a route and a couple of channel hints, apply the route to the channel @@ -725,8 +615,8 @@ 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]; + for (size_t i = 0; i < tal_count(root->hints->hints); i++) { + hint = &root->hints->hints[i]; if (!hint->enabled) tal_arr_expand(&res, hint->scid); @@ -800,10 +690,15 @@ static bool payment_route_check(const struct gossmap *gossmap, return false; scid = gossmap_chan_scid(gossmap, c); - hint = find_hint(payment_root(p)->channel_hints, scid, dir); + hint = find_hint(payment_root(p)->hints->hints, scid, dir); if (!hint) return true; + paymod_log(p, LOG_DBG, + "Checking hint {.scid=%s, .enabled=%d, .estimate=%s}", + fmt_short_channel_id_dir(tmpctx, &hint->scid), hint->enabled, + fmt_amount_msat(tmpctx, hint->estimated_capacity)); + if (!hint->enabled) return false; @@ -1023,7 +918,6 @@ static struct command_result *payment_getroute(struct payment *p) /* Ensure that our fee and CLTV budgets are respected. */ if (amount_msat_greater(fee, p->constraints.fee_budget)) { - payment_exclude_most_expensive(p); p->route = tal_free(p->route); payment_fail( p, "Fee exceeds our fee budget: %s > %s, discarding route", @@ -1034,7 +928,6 @@ static struct command_result *payment_getroute(struct payment *p) if (p->route[0].delay > p->constraints.cltv_budget) { u32 delay = p->route[0].delay; - payment_exclude_longest_delay(p); p->route = tal_free(p->route); payment_fail(p, "CLTV delay exceeds our CLTV budget: %d > %d", delay, p->constraints.cltv_budget); @@ -1496,7 +1389,7 @@ handle_final_failure(struct command *cmd, case WIRE_TEMPORARY_NODE_FAILURE: case WIRE_REQUIRED_NODE_FEATURE_MISSING: case WIRE_INVALID_ONION_BLINDING: - case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: case WIRE_MPP_TIMEOUT: goto error; } @@ -1548,9 +1441,9 @@ handle_intermediate_failure(struct command *cmd, *... * - MUST return a `final_incorrect_htlc_amount` error. */ - case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: - case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: - case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: + case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: /* FIXME: Document in BOLT that intermediates must not return this! */ case WIRE_MPP_TIMEOUT: goto strange_error; @@ -1560,12 +1453,13 @@ handle_intermediate_failure(struct command *cmd, case WIRE_UNKNOWN_NEXT_PEER: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: /* All of these result in the channel being marked as disabled. */ - channel_hints_update(root, errchan->scid, - errchan->direction, false, false, NULL, + channel_hints_update(root, errchan->scid, errchan->direction, + false, false, NULL, errchan->capacity, NULL); break; case WIRE_TEMPORARY_CHANNEL_FAILURE: { + memcheck(&errchan->amount, sizeof(struct amount_msat)); estimated = errchan->amount; /* Subtract one msat more, since we know that the amount did not @@ -1577,9 +1471,9 @@ handle_intermediate_failure(struct command *cmd, /* These are an indication that the capacity was insufficient, * remember the amount we tried as an estimate. */ - channel_hints_update(root, errchan->scid, - errchan->direction, true, false, - &estimated, NULL); + channel_hints_update(root, errchan->scid, errchan->direction, + true, false, &estimated, + errchan->capacity, NULL); goto error; } @@ -1699,13 +1593,13 @@ static u8 *channel_update_from_onion_error(const tal_t *ctx, onion_message, &unused_msat, &channel_update) && !fromwire_fee_insufficient(ctx, - onion_message, &unused_msat, + onion_message, &unused_msat, &channel_update) && !fromwire_incorrect_cltv_expiry(ctx, - onion_message, &unused32, + onion_message, &unused32, &channel_update) && !fromwire_expiry_too_soon(ctx, - onion_message, + onion_message, &channel_update)) /* No channel update. */ return NULL; @@ -2633,7 +2527,7 @@ static inline void retry_step_cb(struct retry_mod_data *rd, /* If the failure was not final, and we tried a route, try again. */ if (rdata->retries > 0) { payment_set_step(p, PAYMENT_STEP_RETRY); - subpayment = payment_new(p, NULL, p, p->modifiers); + subpayment = payment_new(p, NULL, p, NULL, p->modifiers); payment_start(subpayment); subpayment->why = tal_fmt(subpayment, "Still have %d attempts left", @@ -2659,6 +2553,7 @@ local_channel_hints_listpeerchannels(struct command *cmd, const char *buffer, const jsmntok_t *toks, struct payment *p) { struct listpeers_channel **chans; + struct amount_msat capacity; chans = json_to_listpeers_channels(tmpctx, buffer, toks); @@ -2683,6 +2578,8 @@ local_channel_hints_listpeerchannels(struct command *cmd, const char *buffer, else htlc_budget = chans[i]->max_accepted_htlcs - chans[i]->num_htlcs; + capacity = chans[i]->total_msat; + /* If we have both a scid and a local alias we want to * use the scid, and mark the alias as * unusable. Otherwise `getroute` might return the @@ -2694,21 +2591,33 @@ local_channel_hints_listpeerchannels(struct command *cmd, const char *buffer, if (chans[i]->scid != NULL) { channel_hints_update( p, *chans[i]->scid, chans[i]->direction, enabled, - true, &chans[i]->spendable_msat, &htlc_budget); + true, &chans[i]->spendable_msat, + capacity, &htlc_budget); if (chans[i]->alias[LOCAL] != NULL) channel_hints_update(p, *chans[i]->alias[LOCAL], chans[i]->direction, false /* not enabled */, true, &AMOUNT_MSAT(0), + capacity, &htlc_budget); } else { - channel_hints_update(p, *chans[i]->alias[LOCAL], - chans[i]->direction, enabled, true, - &chans[i]->spendable_msat, - &htlc_budget); + channel_hints_update( + p, *chans[i]->alias[LOCAL], chans[i]->direction, + enabled, true, &chans[i]->spendable_msat, + capacity, &htlc_budget); } } + /* And now we iterate through all of the hints, and update + * them. This relaxes any constraints coming from prior + * observations, and should re-enable some channels that would + * otherwise start out as excluded and remain so until + * forever. */ + + struct channel_hint *hints = payment_root(p)->hints->hints; + for (size_t i = 0; i < tal_count(hints); i++) + channel_hint_update(time_now(), &hints[i]); + payment_continue(p); return command_still_pending(cmd); } @@ -2848,7 +2757,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_set *hints = payment_root(p)->hints; /* Note that we ignore direction here: in theory, we could have * found that one direction of a channel is unavailable, but they @@ -2889,12 +2798,12 @@ static bool routehint_excluded(struct payment *p, * 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)) + if (!short_channel_id_eq(hints->hints[j].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)) + hints->hints[j].estimated_capacity)) return true; } } @@ -3160,7 +3069,15 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) routehint_pre_getroute(d, p); } else if (p->step == PAYMENT_STEP_GOT_ROUTE && d->current_routehint != NULL) { /* Now it's time to stitch the two partial routes together. */ - struct amount_msat dest_amount; + struct amount_msat dest_amount, estimate; + /* We do not have the exact final amount, however we + * know that we should be able to use the channel in + * the routehint, so let's fake it as being 2x the + * amount we want to route. */ + + if(!amount_msat_mul(&estimate, p->final_amount, 2)) + abort(); + struct route_info *routehint = d->current_routehint; struct route_hop *prev_hop; for (ssize_t i = 0; i < tal_count(routehint); i++) { @@ -3178,6 +3095,7 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) hop.amount = dest_amount; hop.delay = route_cltv(d->final_cltv, routehint + i + 1, tal_count(routehint) - i - 1); + hop.capacity = estimate; /* Should we get a failure inside the routehint we'll * need the direction so we can exclude it. Luckily @@ -3528,15 +3446,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_set_find(root->hints, d->chan); if (hint && hint->enabled && amount_msat_greater(hint->estimated_capacity, p->our_amount)) { /* Now build a route that consists only of this single hop */ @@ -3546,6 +3456,7 @@ static void direct_pay_override(struct payment *p) { p->route[0].scid = hint->scid.scid; p->route[0].direction = hint->scid.dir; p->route[0].node_id = *p->route_destination; + p->route[0].capacity = hint->capacity; paymod_log(p, LOG_DBG, "Found a direct channel (%s) with sufficient " "capacity, skipping route computation.", @@ -3637,11 +3548,12 @@ REGISTER_PAYMENT_MODIFIER(directpay, struct direct_pay_data *, direct_pay_init, static u32 payment_max_htlcs(const struct payment *p) { + return 10000;/* 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]; + for (size_t i = 0; i < tal_count(p->hints->hints); i++) { + h = &p->hints->hints[i]; if (h->local && h->enabled) res += h->local->htlc_budget; } @@ -3650,7 +3562,7 @@ static u32 payment_max_htlcs(const struct payment *p) root = root->parent; if (res > root->max_htlcs) res = root->max_htlcs; - return res; + return res;*/ } /** payment_lower_max_htlcs @@ -3775,8 +3687,8 @@ static void adaptive_splitter_cb(struct adaptive_split_mod_data *d, struct payme } p->step = PAYMENT_STEP_SPLIT; - a = payment_new(p, NULL, p, p->modifiers); - b = payment_new(p, NULL, p, p->modifiers); + a = payment_new(p, NULL, p, NULL, p->modifiers); + b = payment_new(p, NULL, p, NULL, p->modifiers); a->our_amount.millisatoshis = mid; /* Raw: split. */ b->our_amount.millisatoshis -= mid; /* Raw: split. */ @@ -3951,9 +3863,14 @@ static void route_exclusions_step_cb(struct route_exclusions_data *d, struct route_exclusion **exclusions = d->exclusions; for (size_t i = 0; i < tal_count(exclusions); i++) { struct route_exclusion *e = exclusions[i]; + + /* We don't need the details if we skip anyway. */ + struct amount_msat total = AMOUNT_MSAT(0); + if (e->type == EXCLUDE_CHANNEL) { - channel_hints_update(p, e->u.chan_id.scid, e->u.chan_id.dir, - false, false, NULL, NULL); + channel_hints_update(p, e->u.chan_id.scid, + e->u.chan_id.dir, false, false, + NULL, total, NULL); } else { if (node_id_eq(&e->u.node_id, p->route_destination)) { payment_abort(p, PAY_USER_ERROR, "Payee is manually excluded"); diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index 283c312fa5fe..efa253623ce9 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -61,38 +62,6 @@ struct local_hint { u16 htlc_budget; }; -/* Information about channels we inferred from a) looking at our channels, and - * b) from failures encountered during attempts to perform a payment. These - * are attached to the root payment, since that information is - * global. Attempts update the estimated channel capacities when starting, and - * get remove on failure. Success keeps the capacities, since the capacities - * changed due to the successful HTLCs. */ -struct channel_hint { - /* The timestamp this observation was made. Used to let the - * constraint expressed by this hint decay over time, until it - * is fully relaxed, at which point we can forget about it - * (the structural information is the best we can do in that - * case). - */ - u32 timestamp; - /* The short_channel_id we're going to use when referring to - * this channel. This can either be the real scid, or the - * local alias. The `pay` algorithm doesn't really care which - * one it is, but we'll prefer the scid as that's likely more - * readable than the alias. */ - struct short_channel_id_dir scid; - - /* Upper bound on remove channels inferred from payment failures. */ - struct amount_msat estimated_capacity; - - /* Is the channel enabled? */ - bool enabled; - - /* Non-null if we are one endpoint of this channel */ - struct local_hint *local; - -}; - /* Each payment goes through a number of steps that are always processed in * the same order, and some modifiers are called with the payment, and the * modifier's data before and after certain steps, allowing customization. The @@ -270,7 +239,7 @@ struct payment { /* tal_arr of channel_hints we incrementally learn while performing * payment attempts. */ - struct channel_hint *channel_hints; + struct channel_hint_set *hints; struct node_id *excluded_nodes; /* Optional temporarily excluded channels/nodes (i.e. this routehint) */ @@ -477,6 +446,7 @@ REGISTER_PAYMENT_MODIFIER_HEADER(route_exclusions, struct route_exclusions_data) struct payment *payment_new(tal_t *ctx, struct command *cmd, struct payment *parent, + struct channel_hint_set *channel_hints, struct payment_modifier **mods); void payment_start(struct payment *p); diff --git a/plugins/pay.c b/plugins/pay.c index b0b29e9e538c..967d97123fb0 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -14,7 +14,9 @@ #include #include #include +#include #include +#include #include /* Public key of this node. */ @@ -22,6 +24,7 @@ static struct node_id my_id; static unsigned int maxdelay_default; static bool exp_offers; static bool disablempp = false; +static struct channel_hint_set *global_hints; static LIST_HEAD(payments); @@ -582,14 +585,16 @@ static const char *init(struct plugin *p, */ /* FIXME: Typo in spec for CLTV in descripton! But it breaks our spelling check, so we omit it above */ maxdelay_default = 2016; - /* max-locktime-blocks deprecated in v24.05, but still grab it! */ - rpc_scan(p, "listconfigs", - take(json_out_obj(NULL, NULL, NULL)), - "{configs:" - "{max-locktime-blocks?:{value_int:%}," - "experimental-offers:{set:%}}}", - JSON_SCAN(json_to_number, &maxdelay_default), - JSON_SCAN(json_to_bool, &exp_offers)); + + global_hints = notleak_with_children(channel_hint_set_new(p)); + + /* max-locktime-blocks deprecated in v24.05, but still grab it! */ + rpc_scan(p, "listconfigs", take(json_out_obj(NULL, NULL, NULL)), + "{configs:" + "{max-locktime-blocks?:{value_int:%}," + "experimental-offers:{set:%}}}", + JSON_SCAN(json_to_number, &maxdelay_default), + JSON_SCAN(json_to_bool, &exp_offers)); plugin_set_memleak_handler(p, memleak_mark_payments); return NULL; @@ -1254,7 +1259,7 @@ static struct command_result *json_pay(struct command *cmd, NULL)) return command_param_failed(); - p = payment_new(cmd, cmd, NULL /* No parent */, paymod_mods); + p = payment_new(cmd, cmd, NULL /* No parent */, global_hints, paymod_mods); p->invstring = tal_steal(p, b11str); p->description = tal_steal(p, description); /* Overridded by bolt12 if present */ @@ -1471,6 +1476,26 @@ static struct command_result *json_pay(struct command *cmd, return send_outreq(cmd->plugin, req); } +static struct command_result *handle_channel_hint_update(struct command *cmd, + const char *buf, + const jsmntok_t *param) +{ + struct channel_hint *hint = channel_hint_from_json(NULL, buf, param); + + plugin_log(cmd->plugin, LOG_DBG, + "Received a channel_hint {.scid = %s, .enabled = %d, " + ".estimate = %s, .capacity = %s }", + fmt_short_channel_id_dir(tmpctx, &hint->scid), hint->enabled, + fmt_amount_msat(tmpctx, hint->estimated_capacity), + fmt_amount_msat(tmpctx, hint->capacity) + ); + channel_hint_set_add(global_hints, time_now().ts.tv_sec, &hint->scid, + hint->enabled, &hint->estimated_capacity, + hint->capacity, NULL); + tal_free(hint); + return notification_handled(cmd); +} + static const struct plugin_command commands[] = { { "paystatus", @@ -1491,15 +1516,23 @@ static const char *notification_topics[] = { "channel_hint_update", }; +static const struct plugin_notification notification_subs[] = { + { + "channel_hint_update", + handle_channel_hint_update, + }, +}; + int main(int argc, char *argv[]) { setup_locale(); plugin_main(argv, init, NULL, PLUGIN_RESTARTABLE, true, NULL, commands, - ARRAY_SIZE(commands), NULL, 0, NULL, 0, - notification_topics, ARRAY_SIZE(notification_topics), + ARRAY_SIZE(commands), notification_subs, + ARRAY_SIZE(notification_subs), NULL, 0, notification_topics, + ARRAY_SIZE(notification_topics), plugin_option("disable-mpp", "flag", - "Disable multi-part payments.", - flag_option, flag_jsonfmt, &disablempp), + "Disable multi-part payments.", flag_option, + flag_jsonfmt, &disablempp), NULL); io_poll_override(libplugin_pay_poll); } diff --git a/plugins/test/Makefile b/plugins/test/Makefile index 2ecba481c65a..3b5712d5ac09 100644 --- a/plugins/test/Makefile +++ b/plugins/test/Makefile @@ -20,14 +20,16 @@ plugins/test/run-route-overlong: \ common/gossmap.o \ common/node_id.o \ common/route.o \ - gossipd/gossip_store_wiregen.o + gossipd/gossip_store_wiregen.o \ + plugins/channel_hint.o plugins/test/run-route-calc: \ common/fp16.o \ common/gossmap.o \ common/node_id.o \ common/route.o \ - gossipd/gossip_store_wiregen.o + gossipd/gossip_store_wiregen.o \ + plugins/channel_hint.o $(PLUGIN_TEST_PROGRAMS): $(BITCOIN_OBJS) $(WIRE_OBJS) $(PLUGIN_TEST_COMMON_OBJS) diff --git a/plugins/test/run-route-calc.c b/plugins/test/run-route-calc.c index 7a8d23a18fbf..cf14bf78694a 100644 --- a/plugins/test/run-route-calc.c +++ b/plugins/test/run-route-calc.c @@ -1,4 +1,5 @@ #include "config.h" +#include "plugins/channel_hint.h" #define TESTING #include "../../common/dijkstra.c" #include "../libplugin-pay.c" @@ -83,6 +84,12 @@ void json_add_amount_msat(struct json_stream *result UNNEEDED, struct amount_msat msat) { fprintf(stderr, "json_add_amount_msat called!\n"); abort(); } +/* Generated stub for json_add_amount_sat */ +void json_add_amount_sat(struct json_stream *result UNNEEDED, + const char *satfieldname UNNEEDED, + struct amount_sat sat) + +{ fprintf(stderr, "json_add_amount_sat called!\n"); abort(); } /* Generated stub for json_add_bool */ void json_add_bool(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, bool value UNNEEDED) @@ -215,6 +222,10 @@ bool json_to_sat(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, bool json_to_short_channel_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, struct short_channel_id *scid UNNEEDED) { fprintf(stderr, "json_to_short_channel_id called!\n"); abort(); } +/* Generated stub for json_to_short_channel_id_dir */ +bool json_to_short_channel_id_dir(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, + struct short_channel_id_dir *scidd UNNEEDED) +{ fprintf(stderr, "json_to_short_channel_id_dir called!\n"); abort(); } /* Generated stub for json_to_u16 */ bool json_to_u16(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, uint16_t *num UNNEEDED) @@ -419,12 +430,14 @@ int main(int argc, char *argv[]) { struct payment *p; struct payment_modifier **mods; + struct channel_hint_set *hints; common_setup(argv[0]); chainparams = chainparams_for_network("regtest"); + hints = channel_hint_set_new(tmpctx); mods = tal_arrz(tmpctx, struct payment_modifier *, 1); - p = payment_new(mods, tal(tmpctx, struct command), NULL, mods); + p = payment_new(mods, tal(tmpctx, struct command), NULL, hints, mods); /* We want to permute order of channels between each node, to * avoid "it works because it chooses the first one!" */ diff --git a/plugins/test/run-route-overlong.c b/plugins/test/run-route-overlong.c index 9655104413ed..27a5904181d0 100644 --- a/plugins/test/run-route-overlong.c +++ b/plugins/test/run-route-overlong.c @@ -81,6 +81,12 @@ void json_add_amount_msat(struct json_stream *result UNNEEDED, struct amount_msat msat) { fprintf(stderr, "json_add_amount_msat called!\n"); abort(); } +/* Generated stub for json_add_amount_sat */ +void json_add_amount_sat(struct json_stream *result UNNEEDED, + const char *satfieldname UNNEEDED, + struct amount_sat sat) + +{ fprintf(stderr, "json_add_amount_sat called!\n"); abort(); } /* Generated stub for json_add_bool */ void json_add_bool(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, bool value UNNEEDED) @@ -213,6 +219,10 @@ bool json_to_sat(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, bool json_to_short_channel_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, struct short_channel_id *scid UNNEEDED) { fprintf(stderr, "json_to_short_channel_id called!\n"); abort(); } +/* Generated stub for json_to_short_channel_id_dir */ +bool json_to_short_channel_id_dir(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, + struct short_channel_id_dir *scidd UNNEEDED) +{ fprintf(stderr, "json_to_short_channel_id_dir called!\n"); abort(); } /* Generated stub for json_to_u16 */ bool json_to_u16(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, uint16_t *num UNNEEDED) @@ -384,12 +394,16 @@ static void add_connection(int store_fd, ids[0], ids[1], &dummy_key, &dummy_key); write_to_store(store_fd, msg); - msg = towire_gossip_store_channel_amount(tmpctx, AMOUNT_SAT(10000)); + tal_free(msg); + + /* Also needs a hint as to the funding size. */ + struct amount_sat capacity = AMOUNT_SAT(100000000); + msg = towire_gossip_store_channel_amount(tmpctx, capacity); write_to_store(store_fd, msg); + tal_free(msg); - update_connection(store_fd, from, to, scid, min, max, - base_fee, proportional_fee, - delay, false); + update_connection(store_fd, from, to, scid, min, max, base_fee, + proportional_fee, delay, false); } static void node_id_from_privkey(const struct privkey *p, struct node_id *id) @@ -412,6 +426,7 @@ int main(int argc, char *argv[]) struct payment_modifier **mods; char gossip_version = 10; char *gossipfilename; + struct channel_hint_set *hints = channel_hint_set_new(tmpctx); common_setup(argv[0]); chainparams = chainparams_for_network("regtest"); @@ -428,7 +443,7 @@ int main(int argc, char *argv[]) } mods = tal_arrz(tmpctx, struct payment_modifier *, 1); - p = payment_new(mods, tal(tmpctx, struct command), NULL, mods); + p = payment_new(mods, tal(tmpctx, struct command), NULL, hints, mods); for (size_t i = 1; i < NUM_NODES; i++) { struct short_channel_id scid; @@ -477,6 +492,7 @@ int main(int argc, char *argv[]) assert(tal_count(r) == 2); } + tal_free(hints); common_shutdown(); return 0; } diff --git a/tests/test_connection.py b/tests/test_connection.py index 2dd29645e2d5..ce29926ad6f2 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -4376,7 +4376,11 @@ def test_mutual_reconnect_race(node_factory, executor, bitcoind): def send_many_payments(): for i in range(20): time.sleep(0.5) - inv = l2.rpc.invoice(100, "label-" + str(i), "desc")['bolt11'] + inv = l2.rpc.invoice( + 100 - i, # Ensure prior chanhints don't block us + "label-" + str(i), + "desc" + )['bolt11'] try: l1.rpc.pay(inv) except RpcError: diff --git a/tests/test_pay.py b/tests/test_pay.py index 296750e69316..4a0f847942dc 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1939,7 +1939,16 @@ def listpays_nofail(b11): @pytest.mark.slow_test def test_pay_routeboost(node_factory, bitcoind): - """Make sure we can use routeboost information. """ + """Make sure we can use routeboost information. + + ```dot + graph { + l1 -- l2 -- l3 + l3 -- l4 [style="dotted"] + l4 -- l5 [style="dotted"] + } + ``` + """ # l1->l2->l3--private-->l4 l1, l2 = node_factory.line_graph(2, announce_channels=True, wait_for_announce=True) l3, l4, l5 = node_factory.line_graph(3, announce_channels=False, wait_for_announce=False) @@ -2016,10 +2025,24 @@ def test_pay_routeboost(node_factory, bitcoind): assert 'success' in attempts[0] # Finally, it should fall back to second routehint if first fails. - # (Note, this is not public because it's not 6 deep) + # (Note, this is not public because it's not 6 deep). To test this + # we add another edge to the graph, resulting in: + # + # ```dot + # graph { + # rankdir=LR + # l1 -- l2 -- l3 + # l4 [label="l4 (offline)",style="dashed"] + # l3 -- l4 [style="dotted"] + # l4 -- l5 [style="dotted"] + # l3 -- l5 [style="dotted"] + # } + # ``` l3.rpc.connect(l5.info['id'], 'localhost', l5.port) scid35, _ = l3.fundchannel(l5, 10**6) l4.stop() + + # Now that we have the channels ready, let's build the routehints through l3l5 routel3l5 = [{'id': l3.info['id'], 'short_channel_id': scid35, 'fee_base_msat': 1000, @@ -2040,10 +2063,11 @@ def test_pay_routeboost(node_factory, bitcoind): assert 'local_exclusions' not in only_one(status['pay']) attempts = only_one(status['pay'])['attempts'] - # First one fails, second one succeeds, no routehint would come last. - assert len(attempts) == 2 + # First routehint in the invoice fails, we may retry that one + # unsuccessfully before switching, hence the >2 instead of =2 + assert len(attempts) >= 2 assert 'success' not in attempts[0] - assert 'success' in attempts[1] + assert 'success' in attempts[-1] # TODO Add assertion on the routehint once we add them to the pay # output @@ -5120,7 +5144,14 @@ def test_listpays_with_filter_by_status(node_factory, bitcoind): def test_sendpay_grouping(node_factory, bitcoind): - """Paying an invoice multiple times, listpays should list them individually + """`listpays` should be smart enough to group repeated `pay` calls. + + We always use slightly decreasing values for the payment, in order + to avoid having to adjust the channel_hints that are being + remembered across attempts. In case of a failure the + `channel_hint` will be `attempted amount - 1msat` so use that as + the next payment's amount. + """ l1, l2, l3 = node_factory.line_graph( 3, @@ -5140,13 +5171,13 @@ def test_sendpay_grouping(node_factory, bitcoind): assert(len(l1.rpc.listpays()['pays']) == 0) with pytest.raises(RpcError, match=r'Ran out of routes to try after [0-9]+ attempts'): - l1.rpc.pay(inv, amount_msat='100000msat') + l1.rpc.pay(inv, amount_msat='100002msat') # After this one invocation we have one entry in `listpays` assert(len(l1.rpc.listpays()['pays']) == 1) with pytest.raises(RpcError, match=r'Ran out of routes to try after [0-9]+ attempts'): - l1.rpc.pay(inv, amount_msat='200000msat') + l1.rpc.pay(inv, amount_msat='100001msat') # Surprise: we should have 2 entries after 2 invocations assert(len(l1.rpc.listpays()['pays']) == 2) @@ -5159,7 +5190,7 @@ def test_sendpay_grouping(node_factory, bitcoind): wait_for(lambda: only_one(l3.rpc.listpeers()['peers'])['connected'] is True) scid = l3.rpc.listpeerchannels()['channels'][0]['short_channel_id'] wait_for(lambda: [c['active'] for c in l1.rpc.listchannels(scid)['channels']] == [True, True]) - l1.rpc.pay(inv, amount_msat='420000msat') + l1.rpc.pay(inv, amount_msat='10000msat') # And finally we should have all 3 attempts to pay the invoice pays = l1.rpc.listpays()['pays'] @@ -5996,3 +6027,79 @@ def test_enableoffer(node_factory): # Can't enable unknown. with pytest.raises(RpcError, match="Unknown offer"): l1.rpc.enableoffer(offer_id=offer1['offer_id']) + + +def diamond_network(node_factory): + """Build a diamond, with a cheap route, that is exhausted. The + first payment should try that route first, learn it's exhausted, + and then succeed over the other leg. The second, unrelated, + payment should immediately skip the exhausted leg and go for the + more expensive one. + + ```mermaid + graph LR + Sender -- "propfee=1" --> Forwarder1 + Forwarder1 -- "propfee="1\nexhausted --> Recipient + Sender -- "propfee=1" --> Forwarder2 + Forwarder2 -- "propfee=5" --> Recipient + ``` + """ + opts = [ + {'fee-per-satoshi': 0, 'fee-base': 0}, # Sender + {'fee-per-satoshi': 0, 'fee-base': 0}, # Low fee, but exhausted channel + {'fee-per-satoshi': 5000, 'fee-base': 0}, # Disincentivize using fw2 + {'fee-per-satoshi': 0, 'fee-base': 0}, # Recipient + ] + + sender, fw1, fw2, recipient, = node_factory.get_nodes(4, opts=opts) + + # And now wire them all up: notice that all channels, except the + # recipent <> fw1 are created in the direction of the planned + # from, meaning we won't be able to forward through there, causing + # a `channel_hint` to be created, disincentivizing usage of this + # channel on the second payment. + node_factory.join_nodes( + [sender, fw2, recipient, fw1], + wait_for_announce=True, + announce_channels=True, + ) + # And we complete the diamond by adding the edge from sender to fw1 + node_factory.join_nodes( + [sender, fw1], + wait_for_announce=True, + announce_channels=True + ) + return [sender, fw1, fw2, recipient] + + +def test_pay_remember_hint(node_factory): + """Using a diamond graph, with inferred `channel_hint`s, see if we remember + """ + sender, fw1, fw2, recipient, = diamond_network(node_factory) + + inv = recipient.rpc.invoice( + 4200000, + "lbl1", + "desc1", + exposeprivatechannels=[], # suppress routehints, so fees alone control route + )['bolt11'] + + p = sender.rpc.pay(inv) + + # Ensure we failed the first, cheap, path, and then tried the successful one. + assert(p['parts'] == 2) + + # Now for the final trick: a new payment should remember the + # previous failure, and go directly for the successful route + # through fw2 + + inv = recipient.rpc.invoice( + 4200000, + "lbl2", + "desc2", + exposeprivatechannels=[], # suppress routehints, so fees alone control route + )['bolt11'] + + # We should not have touched fw1, and should succeed after a single call + p = sender.rpc.pay(inv) + assert(p['parts'] == 1)