From e5a57d4d606d244e4fa424a2fa58144c04f25258 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 9 Oct 2024 03:57:14 +1030 Subject: [PATCH 01/22] pyln-testing: understand `gossip_store_file` arg in get_nodes. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 9d1ac4a77d45..ab8eb4c3302e 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -1544,6 +1544,7 @@ def split_options(self, opts): 'wait_for_bitcoind_sync', 'allow_bad_gossip', 'start', + 'gossip_store_file', ] node_opts = {k: v for k, v in opts.items() if k in node_opt_keys} cli_opts = {k: v for k, v in opts.items() if k not in node_opt_keys} From 23fc7929e996fa9686720884db0219ce283941e0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:26 +1030 Subject: [PATCH 02/22] askrene: give notifications back to caller as we go. And unify logging for better debugging. Signed-off-by: Rusty Russell Changelog-EXPERIMENTAL: `askrene` now has better logging, gives notifications of progress. --- plugins/askrene/askrene.c | 73 ++++++++++++++++++++++++-- plugins/askrene/askrene.h | 8 +++ plugins/askrene/explain_failure.c | 17 ++++--- plugins/askrene/refine.c | 85 ++++++++++++++----------------- 4 files changed, 124 insertions(+), 59 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index db720f0d96ba..d91b03d12986 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -225,6 +225,50 @@ struct amount_msat get_additional_per_htlc_cost(const struct route_query *rq, return AMOUNT_MSAT(0); } +const char *rq_log(const tal_t *ctx, + const struct route_query *rq, + enum log_level level, + const char *fmt, + ...) +{ + va_list args; + const char *msg; + + va_start(args, fmt); + msg = tal_vfmt(ctx, fmt, args); + va_end(args); + + plugin_notify_message(rq->cmd, level, "%s", msg); + + /* Notifications already get logged at debug. Otherwise reduce + * severity. */ + if (level != LOG_DBG) + plugin_log(rq->plugin, + level == LOG_BROKEN ? level : level - 1, + "%s: %s", rq->cmd->id, msg); + return msg; +} + +static const char *fmt_route(const tal_t *ctx, + const struct route *route, + struct amount_msat delivers, + u32 final_cltv) +{ + char *str = tal_strdup(ctx, ""); + + for (size_t i = 0; i < tal_count(route->hops); i++) { + struct short_channel_id_dir scidd; + scidd.scid = route->hops[i].scid; + scidd.dir = route->hops[i].direction; + tal_append_fmt(&str, "%s/%u %s -> ", + fmt_amount_msat(tmpctx, route->hops[i].amount), + route->hops[i].delay, + fmt_short_channel_id_dir(tmpctx, &scidd)); + } + tal_append_fmt(&str, "%s/%u", + fmt_amount_msat(tmpctx, delivers), final_cltv); + return str; +} /* Returns an error message, or sets *routes */ static const char *get_routes(const tal_t *ctx, @@ -300,13 +344,17 @@ static const char *get_routes(const tal_t *ctx, srcnode = gossmap_find_node(askrene->gossmap, source); if (!srcnode) { - ret = tal_fmt(ctx, "Unknown source node %s", fmt_node_id(tmpctx, source)); + ret = rq_log(ctx, rq, LOG_INFORM, + "Unknown source node %s", + fmt_node_id(tmpctx, source)); goto fail; } dstnode = gossmap_find_node(askrene->gossmap, dest); if (!dstnode) { - ret = tal_fmt(ctx, "Unknown destination node %s", fmt_node_id(tmpctx, dest)); + ret = rq_log(ctx, rq, LOG_INFORM, + "Unknown destination node %s", + fmt_node_id(tmpctx, dest)); goto fail; } @@ -344,10 +392,14 @@ static const char *get_routes(const tal_t *ctx, /* FIXME: Typo in spec for CLTV in descripton! But it breaks our spelling check, so we omit it above */ while (finalcltv + flows_worst_delay(flows) > 2016) { delay_feefactor *= 2; + rq_log(tmpctx, rq, LOG_UNUSUAL, + "The worst flow delay is %"PRIu64" (> %i), retrying with delay_feefactor %f...", + flows_worst_delay(flows), 2016 - finalcltv, delay_feefactor); flows = minflow(rq, rq, srcnode, dstnode, amount, mu, delay_feefactor, base_fee_penalty, prob_cost_factor); if (!flows || delay_feefactor > 10) { - ret = tal_fmt(ctx, "Could not find route without excessive delays"); + ret = rq_log(ctx, rq, LOG_UNUSUAL, + "Could not find route without excessive delays"); goto fail; } } @@ -355,16 +407,23 @@ static const char *get_routes(const tal_t *ctx, /* Too expensive? */ while (amount_msat_greater(flowset_fee(rq->plugin, flows), maxfee)) { mu += 10; + rq_log(tmpctx, rq, LOG_UNUSUAL, + "The flows had a fee of %s, greater than max of %s, retrying with mu of %u%%...", + fmt_amount_msat(tmpctx, flowset_fee(rq->plugin, flows)), + fmt_amount_msat(tmpctx, maxfee), + mu); flows = minflow(rq, rq, srcnode, dstnode, amount, mu, delay_feefactor, base_fee_penalty, prob_cost_factor); if (!flows || mu == 100) { - ret = tal_fmt(ctx, "Could not find route without excessive cost"); + ret = rq_log(ctx, rq, LOG_UNUSUAL, + "Could not find route without excessive cost"); goto fail; } } if (finalcltv + flows_worst_delay(flows) > 2016) { - ret = tal_fmt(ctx, "Could not find route without excessive cost or delays"); + ret = rq_log(ctx, rq, LOG_UNUSUAL, + "Could not find route without excessive cost or delays"); goto fail; } @@ -409,10 +468,14 @@ static const char *get_routes(const tal_t *ctx, rh->delay = delay; } (*amounts)[i] = flows[i]->delivers; + rq_log(tmpctx, rq, LOG_INFORM, "Flow %zu/%zu: %s", + i, tal_count(flows), + fmt_route(tmpctx, r, (*amounts)[i], finalcltv)); } *probability = flowset_probability(flows, rq); gossmap_remove_localmods(askrene->gossmap, localmods); + return NULL; /* Explicit failure path keeps the compiler (gcc version 12.3.0 -O3) from diff --git a/plugins/askrene/askrene.h b/plugins/askrene/askrene.h index 5ae9a4511ace..e8ad8a907ceb 100644 --- a/plugins/askrene/askrene.h +++ b/plugins/askrene/askrene.h @@ -64,6 +64,14 @@ void get_constraints(const struct route_query *rq, struct amount_msat *min, struct amount_msat *max); +/* Say something about this route_query */ +const char *rq_log(const tal_t *ctx, + const struct route_query *rq, + enum log_level level, + const char *fmt, + ...) + PRINTF_FMT(4, 5); + /* Is there a known additional per-htlc cost for this channel? */ struct amount_msat get_additional_per_htlc_cost(const struct route_query *rq, const struct short_channel_id_dir *scidd); diff --git a/plugins/askrene/explain_failure.c b/plugins/askrene/explain_failure.c index b8ac907d132d..d4cd7cb01854 100644 --- a/plugins/askrene/explain_failure.c +++ b/plugins/askrene/explain_failure.c @@ -236,7 +236,8 @@ const char *explain_failure(const tal_t *ctx, hops = route_from_dijkstra(tmpctx, rq->gossmap, dij, srcnode, AMOUNT_MSAT(0), 0); if (!hops) - return tal_fmt(ctx, "There is no connection between source and destination at all"); + return rq_log(ctx, rq, LOG_INFORM, + "There is no connection between source and destination at all"); /* Description of shortest path */ path = tal_strdup(tmpctx, ""); @@ -276,15 +277,15 @@ const char *explain_failure(const tal_t *ctx, else continue; - return tal_fmt(ctx, - NO_USABLE_PATHS_STRING - " The shortest path is %s, but %s %s", - path, - fmt_short_channel_id_dir(tmpctx, &scidd), - explanation); + return rq_log(ctx, rq, LOG_INFORM, + NO_USABLE_PATHS_STRING + " The shortest path is %s, but %s %s", + path, + fmt_short_channel_id_dir(tmpctx, &scidd), + explanation); } - return tal_fmt(ctx, + return rq_log(ctx, rq, LOG_BROKEN, "Actually, I'm not sure why we didn't find the" " obvious route %s: perhaps this is a bug?", path); diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 18596281594f..e7b257cb0373 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -103,12 +103,12 @@ static const char *constrain_flow(const tal_t *ctx, /* If amount is > max, we decrease and add note it in * case something goes wrong later. */ if (amount_msat_greater(msat, max)) { - plugin_log(rq->plugin, LOG_DBG, - "Decreased %s to %s%s across %s", - fmt_amount_msat(tmpctx, msat), - max_cause, - fmt_amount_msat(tmpctx, max), - fmt_flows_step_scid(tmpctx, rq, flow, i)); + rq_log(tmpctx, rq, LOG_INFORM, + "Had to decreased amount %s to %s%s across %s", + fmt_amount_msat(tmpctx, msat), + max_cause, + fmt_amount_msat(tmpctx, max), + fmt_flows_step_scid(tmpctx, rq, flow, i)); msat = max; decreased = i; why_decreased = max_cause; @@ -139,28 +139,31 @@ static const char *constrain_flow(const tal_t *ctx, /* These failures are incredibly unlikely, but possible */ if (amount_msat_is_zero(next)) { - return tal_fmt(ctx, "Amount %s cannot pay its own fees across %s", - fmt_amount_msat(tmpctx, msat), - fmt_flows_step_scid(tmpctx, rq, flow, i)); + return rq_log(ctx, rq, LOG_UNUSUAL, + "Amount %s cannot pay its own fees across %s", + fmt_amount_msat(tmpctx, msat), + fmt_flows_step_scid(tmpctx, rq, flow, i)); } /* Does happen if we try to pay 1 msat, and all paths have 1000msat min */ if (amount_msat_less(next, min)) { - return tal_fmt(ctx, "Amount %s below minimum across %s", - fmt_amount_msat(tmpctx, next), - fmt_flows_step_scid(tmpctx, rq, flow, i)); + return rq_log(ctx, rq, LOG_UNUSUAL, + "Amount %s below minimum across %s", + fmt_amount_msat(tmpctx, next), + fmt_flows_step_scid(tmpctx, rq, flow, i)); } msat = next; } if (!amount_msat_eq(flow->delivers, msat)) { - plugin_log(rq->plugin, LOG_DBG, "Flow changed to deliver %s not %s, because max constrained by %s%s", - fmt_amount_msat(tmpctx, msat), - fmt_amount_msat(tmpctx, flow->delivers), - why_decreased ? why_decreased : NULL, - decreased == -1 ? "none" - : fmt_flows_step_scid(tmpctx, rq, flow, decreased)); + rq_log(tmpctx, rq, LOG_INFORM, + "Flow changed to deliver %s not %s, because max constrained by %s%s", + fmt_amount_msat(tmpctx, msat), + fmt_amount_msat(tmpctx, flow->delivers), + why_decreased ? why_decreased : NULL, + decreased == -1 ? "none" + : fmt_flows_step_scid(tmpctx, rq, flow, decreased)); flow->delivers = msat; } @@ -244,7 +247,6 @@ static struct flow *pick_most_likely_flow(struct route_query *rq, continue; best_prob = prob; best_flow = flows[i]; - plugin_log(rq->plugin, LOG_DBG, "Best flow is #%zu!", i); } return best_flow; @@ -260,15 +262,14 @@ static const char *flow_violates_min(const tal_t *ctx, const struct half_chan *h = flow_edge(flow, i); struct amount_msat min = amount_msat(fp16_to_u64(h->htlc_min)); - plugin_log(rq->plugin, LOG_DBG, "flow_violates_min: %u/%zu amt=%s, min=%s", - i, tal_count(flow->path), fmt_amount_msat(tmpctx, msat), fmt_amount_msat(tmpctx, min)); if (amount_msat_less(msat, min)) { struct short_channel_id_dir scidd; get_scidd(rq->gossmap, flow, i, &scidd); - return tal_fmt(ctx, "Sending %s across %s would violate htlc_min (~%s)", - fmt_amount_msat(tmpctx, msat), - fmt_short_channel_id_dir(tmpctx, &scidd), - fmt_amount_msat(tmpctx, min)); + return rq_log(ctx, rq, LOG_UNUSUAL, + "Sending %s across %s would violate htlc_min (~%s)", + fmt_amount_msat(tmpctx, msat), + fmt_short_channel_id_dir(tmpctx, &scidd), + fmt_amount_msat(tmpctx, min)); } if (!amount_msat_add_fee(&msat, h->base_fee, h->proportional_fee)) plugin_err(rq->plugin, "Adding fee to amount"); @@ -290,24 +291,14 @@ refine_with_fees_and_limits(const tal_t *ctx, for (size_t i = 0; i < tal_count(*flows);) { struct flow *flow = (*flows)[i]; - plugin_log(rq->plugin, LOG_DBG, "Constraining flow %zu: %s", - i, fmt_amount_msat(tmpctx, flow->delivers)); - for (size_t j = 0; j < tal_count(flow->path); j++) { - struct amount_msat min, max; - get_constraints(rq, flow->path[j], flow->dirs[j], &min, &max); - plugin_log(rq->plugin, LOG_DBG, "->%s(max %s)", - fmt_flows_step_scid(tmpctx, rq, flow, j), - fmt_amount_msat(tmpctx, max)); - } - flow_constraint_error = constrain_flow(tmpctx, rq, flow, &reservations); if (!flow_constraint_error) { i++; continue; } - plugin_log(rq->plugin, LOG_DBG, "Flow was too constrained: %s", - flow_constraint_error); + rq_log(tmpctx, rq, LOG_UNUSUAL, "Flow %zu/%zu was too constrained (%s) so removing it", + i, tal_count(*flows), flow_constraint_error); /* This flow was reduced to 0 / impossible, remove */ tal_arr_remove(flows, i); } @@ -324,10 +315,10 @@ refine_with_fees_and_limits(const tal_t *ctx, for (size_t i = 0; i < tal_count(*flows); i++) { if (amount_msat_sub(&(*flows)[i]->delivers, (*flows)[i]->delivers, excess)) { const char *err; - plugin_log(rq->plugin, LOG_DBG, - "Flows delivered %s extra, trimming %zu/%zu", - fmt_amount_msat(tmpctx, excess), - i, tal_count(*flows)); + rq_log(tmpctx, rq, LOG_DBG, + "Flow %zu/%zu delivered %s extra, trimming", + i, tal_count(*flows), + fmt_amount_msat(tmpctx, excess)); /* In theory, this can violate min_htlc! Thanks @Lagrang3! */ err = flow_violates_min(tmpctx, rq, (*flows)[i]); if (err) { @@ -346,15 +337,16 @@ refine_with_fees_and_limits(const tal_t *ctx, * was deleted instead for being below minimum */ if (!amount_msat_sub(&more_to_deliver, deliver, flowset_delivers(rq->plugin, *flows))) { - ret = tal_fmt(ctx, + ret = rq_log(ctx, rq, LOG_UNUSUAL, "Flowset delivers %s instead of %s, can't shed excess?", fmt_amount_msat(tmpctx, flowset_delivers(rq->plugin, *flows)), fmt_amount_msat(tmpctx, deliver)); goto out; } - plugin_log(rq->plugin, LOG_DBG, "After dealing with excess, more_to_deliver=%s", - fmt_amount_msat(tmpctx, more_to_deliver)); + rq_log(tmpctx, rq, LOG_DBG, + "After dealing with excess, more_to_deliver=%s", + fmt_amount_msat(tmpctx, more_to_deliver)); } /* The residual is minimal. In theory we could add one msat at a time @@ -375,8 +367,9 @@ refine_with_fees_and_limits(const tal_t *ctx, * could try MCF again for the remainder. */ f = pick_most_likely_flow(rq, *flows, extra); if (!f) { - ret = tal_fmt(ctx, "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", - fmt_amount_msat(tmpctx, more_to_deliver)); + ret = rq_log(ctx, rq, LOG_BROKEN, + "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", + fmt_amount_msat(tmpctx, more_to_deliver)); goto out; } From a79e5d6b84430274cc79f7b546023aa4b5ce3fea Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:37 +1030 Subject: [PATCH 03/22] askrene: populate auto.localchans layer properly. Rather than adding to the gossmap modifications directly, populate the layer and have the normal layer application logic do it. This is consistent when we query layers in the next patch. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index d91b03d12986..17aafba60261 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -309,7 +309,7 @@ static const char *get_routes(const tal_t *ctx, rq->capacities = tal_dup_talarr(rq, fp16_t, askrene->capacities); rq->additional_costs = additional_costs; - /* Layers don't have to exist: they might be empty! */ + /* Layers must exist, but might be special ones! */ for (size_t i = 0; i < tal_count(layers); i++) { const struct layer *l = find_layer(askrene, layers[i]); if (!l) { @@ -599,9 +599,14 @@ static void add_localchan(struct gossmap_localmods *mods, const char *opener; const char *err; - gossmod_add_localchan(mods, self, peer, scidd, capacity_msat, htlcmin, htlcmax, - spendable, fee_base, fee_proportional, cltv_delta, enabled, - buf, chantok, info->local_layer); + /* We get called twice, once in each direction: only create once. */ + if (!layer_find_local_channel(info->local_layer, scidd->scid)) + layer_add_local_channel(info->local_layer, + self, peer, scidd->scid, capacity_msat); + layer_add_update_channel(info->local_layer, scidd, + &enabled, + &htlcmin, &htlcmax, + &fee_base, &fee_proportional, &cltv_delta); /* We also need to know the feerate and opener, so we can calculate per-HTLC cost */ feerate = 0; /* Can be unset on unconfirmed channels */ From ea074bf34b86335c0e7bcc9a5baa171aa3846633 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:38 +1030 Subject: [PATCH 04/22] askrene: make `auto.sourcefree` a real layer, too. Signed-off-by: Rusty Russell Changelog-EXPERIMENTAL: `getroutes` now applies `auto.sourcefree` layer in the order specified, so doesn't alter channels changed in later layers. --- contrib/msggen/msggen/schema.json | 2 +- doc/schemas/lightning-getroutes.json | 2 +- plugins/askrene/askrene.c | 40 +++++++++++----------------- tests/test_askrene.py | 4 +-- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/contrib/msggen/msggen/schema.json b/contrib/msggen/msggen/schema.json index 66058efe5f9b..4b1f7678993e 100644 --- a/contrib/msggen/msggen/schema.json +++ b/contrib/msggen/msggen/schema.json @@ -14647,7 +14647,7 @@ "", "Layers are generally maintained by plugins, either to contain persistent information about capacities which have been discovered, or to contain transient information for this particular payment (such as blinded paths or routehints).", "", - "There are two automatic layers: *auto.localchans* contains information on local channels from this node (including non-public ones), and their exact current spendable capacities, and *auto.sourcefree* overrides all channels leading out of the *source* to be zero fee and zero delay. These are both useful in the case where the source is the current node." + "There are two automatic layers: *auto.localchans* contains information on local channels from this node (including non-public ones), and their exact current spendable capacities, and *auto.sourcefree* overrides all channels (including those from previous layers) leading out of the *source* to be zero fee and zero delay. These are both useful in the case where the source is the current node." ], "categories": [ "readonly" diff --git a/doc/schemas/lightning-getroutes.json b/doc/schemas/lightning-getroutes.json index aeaeb2216b8a..12cabd461969 100644 --- a/doc/schemas/lightning-getroutes.json +++ b/doc/schemas/lightning-getroutes.json @@ -14,7 +14,7 @@ "", "Layers are generally maintained by plugins, either to contain persistent information about capacities which have been discovered, or to contain transient information for this particular payment (such as blinded paths or routehints).", "", - "There are two automatic layers: *auto.localchans* contains information on local channels from this node (including non-public ones), and their exact current spendable capacities, and *auto.sourcefree* overrides all channels leading out of the *source* to be zero fee and zero delay. These are both useful in the case where the source is the current node." + "There are two automatic layers: *auto.localchans* contains information on local channels from this node (including non-public ones), and their exact current spendable capacities, and *auto.sourcefree* overrides all channels (including those from previous layers) leading out of the *source* to be zero fee and zero delay. These are both useful in the case where the source is the current node." ], "categories": [ "readonly" diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 17aafba60261..c3a38ae4ef39 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -172,21 +172,20 @@ static fp16_t *get_capacities(const tal_t *ctx, /* If we're the payer, we don't add delay or fee to our own outgoing * channels. This wouldn't be right if we looped back through ourselves, * but we won't. */ -/* FIXME: We could cache this until gossmap changes... */ -static void add_free_source(struct plugin *plugin, - struct gossmap *gossmap, - struct gossmap_localmods *localmods, - const struct node_id *source) +/* FIXME: We could cache this until gossmap/layer changes... */ +static struct layer *source_free_layer(const tal_t *ctx, + struct gossmap *gossmap, + const struct node_id *source, + struct gossmap_localmods *localmods) { - /* We apply existing localmods, save up mods we want, then append - * them: it's not safe to modify localmods while they are applied! */ + /* We apply existing localmods so we see *all* channels */ const struct gossmap_node *srcnode; const struct amount_msat zero_base_fee = AMOUNT_MSAT(0); const u16 zero_delay = 0; const u32 zero_prop_fee = 0; - struct short_channel_id_dir *scidds - = tal_arr(tmpctx, struct short_channel_id_dir, 0); + struct layer *layer = new_temp_layer(ctx, "auto.sourcefree"); + /* We apply this so we see any created channels */ gossmap_apply_localmods(gossmap, localmods); /* If we're not in map, we complain later */ @@ -198,20 +197,14 @@ static void add_free_source(struct plugin *plugin, c = gossmap_nth_chan(gossmap, srcnode, i, &scidd.dir); scidd.scid = gossmap_chan_scid(gossmap, c); - tal_arr_expand(&scidds, scidd); + layer_add_update_channel(layer, &scidd, + NULL, NULL, NULL, + &zero_base_fee, &zero_prop_fee, + &zero_delay); } gossmap_remove_localmods(gossmap, localmods); - /* Now we can update localmods: we only change fee levels and delay */ - for (size_t i = 0; i < tal_count(scidds); i++) { - if (!gossmap_local_updatechan(localmods, - &scidds[i], - NULL, NULL, NULL, - &zero_base_fee, &zero_prop_fee, - &zero_delay)) - plugin_err(plugin, "Could not zero fee/delay on %s", - fmt_short_channel_id_dir(tmpctx, &scidds[i])); - } + return layer; } struct amount_msat get_additional_per_htlc_cost(const struct route_query *rq, @@ -319,7 +312,8 @@ static const char *get_routes(const tal_t *ctx, } else { /* Handled below, after other layers */ assert(streq(layers[i], "auto.sourcefree")); - continue; + plugin_log(rq->plugin, LOG_DBG, "Adding auto.sourcefree"); + l = source_free_layer(layers, askrene->gossmap, source, localmods); } } @@ -332,10 +326,6 @@ static const char *get_routes(const tal_t *ctx, layer_clear_overridden_capacities(l, askrene->gossmap, rq->capacities); } - /* This also looks into localmods, to zero them */ - if (have_layer(layers, "auto.sourcefree")) - add_free_source(rq->plugin, askrene->gossmap, localmods, source); - /* Clear scids with reservations, too, so we don't have to look up * all the time! */ reserves_clear_capacities(askrene->reserved, askrene->gossmap, rq->capacities); diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 5a7e7abe7c2a..62e7e0b156d8 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -671,7 +671,7 @@ def test_sourcefree_on_mods(node_factory, bitcoind): check_route_as_expected(routes, [[{'short_channel_id_dir': '0x3x3/1', 'amount_msat': 1000000, 'delay': 99}]]) - # Same if we specify layers in the other order! + # NOT if we specify layers in the other order! routes = l1.rpc.getroutes(source=nodemap[0], destination=l1.info['id'], amount_msat=1000000, @@ -680,7 +680,7 @@ def test_sourcefree_on_mods(node_factory, bitcoind): final_cltv=99)['routes'] # Expect no fee. check_route_as_expected(routes, [[{'short_channel_id_dir': '0x3x3/1', - 'amount_msat': 1000000, 'delay': 99}]]) + 'amount_msat': 1003000, 'delay': 117}]]) def test_live_spendable(node_factory, bitcoind): From e7ac294526df1467c892b129d0d28554d4ff498e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:38 +1030 Subject: [PATCH 05/22] gossmap: include cltv_expiry_delta in gossmap_chan_get_update_details for completeness. Signed-off-by: Rusty Russell --- common/gossmap.c | 3 +++ common/gossmap.h | 1 + common/test/run-gossmap_canned.c | 5 +++++ common/test/run-gossmap_local.c | 9 +++++++++ connectd/queries.c | 2 +- connectd/test/run-crc32_of_update.c | 1 + devtools/gossmap-compress.c | 8 ++++---- gossipd/gossmap_manage.c | 2 +- gossipd/seeker.c | 2 +- gossipd/test/run-next_block_range.c | 1 + plugins/test/run-route-calc.c | 6 ------ plugins/test/run-route-overlong.c | 6 ------ plugins/topology.c | 1 + 13 files changed, 28 insertions(+), 19 deletions(-) diff --git a/common/gossmap.c b/common/gossmap.c index 976fa397d3ba..22f9ea05dcb9 100644 --- a/common/gossmap.c +++ b/common/gossmap.c @@ -1485,6 +1485,7 @@ void gossmap_chan_get_update_details(const struct gossmap *map, u32 *timestamp, u8 *message_flags, u8 *channel_flags, + u16 *cltv_expiry_delta, u32 *fee_base_msat, u32 *fee_proportional_millionths, struct amount_msat *htlc_minimum_msat, @@ -1511,6 +1512,8 @@ void gossmap_chan_get_update_details(const struct gossmap *map, *channel_flags = map_u8(map, channel_flags_off); if (message_flags) *message_flags = map_u8(map, message_flags_off); + if (cltv_expiry_delta) + *cltv_expiry_delta = map_be16(map, cltv_expiry_delta_off); if (fee_base_msat) *fee_base_msat = map_be32(map, fee_base_off); if (fee_proportional_millionths) diff --git a/common/gossmap.h b/common/gossmap.h index b942c46614a5..01b40ea50c7a 100644 --- a/common/gossmap.h +++ b/common/gossmap.h @@ -230,6 +230,7 @@ void gossmap_chan_get_update_details(const struct gossmap *map, u32 *timestamp, u8 *message_flags, u8 *channel_flags, + u16 *cltv_expiry_delta, u32 *fee_base_msat, u32 *fee_proportional_millionths, struct amount_msat *htlc_minimum_msat, diff --git a/common/test/run-gossmap_canned.c b/common/test/run-gossmap_canned.c index e7f0662284cd..eef6467cfe9d 100644 --- a/common/test/run-gossmap_canned.c +++ b/common/test/run-gossmap_canned.c @@ -317,6 +317,7 @@ int main(int argc, char *argv[]) struct amount_msat capacity; u32 timestamp, fee_base_msat, fee_proportional_millionths; u8 message_flags, channel_flags; + u16 cltv_expiry_delta; struct amount_msat htlc_minimum_msat, htlc_maximum_msat; u8 *cann; @@ -345,6 +346,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -352,6 +354,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115313); assert(message_flags == 1); assert(channel_flags == 0); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); @@ -362,6 +365,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -369,6 +373,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115313); assert(message_flags == 1); assert(channel_flags == 1); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); diff --git a/common/test/run-gossmap_local.c b/common/test/run-gossmap_local.c index b583495c0439..218175724e63 100644 --- a/common/test/run-gossmap_local.c +++ b/common/test/run-gossmap_local.c @@ -338,6 +338,7 @@ int main(int argc, char *argv[]) struct gossmap_localmods *mods; struct amount_msat capacity; u32 timestamp, fee_base_msat, fee_proportional_millionths; + u16 cltv_expiry_delta; u8 message_flags, channel_flags; struct amount_msat htlc_minimum_msat, htlc_maximum_msat; u8 *cann, *nann; @@ -375,6 +376,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -382,6 +384,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115301); assert(message_flags == 1); assert(channel_flags == 0); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); @@ -392,6 +395,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -399,6 +403,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115311); assert(message_flags == 1); assert(channel_flags == 1); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); @@ -409,6 +414,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -416,6 +422,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115313); assert(message_flags == 1); assert(channel_flags == 0); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); @@ -426,6 +433,7 @@ int main(int argc, char *argv[]) ×tamp, &message_flags, &channel_flags, + &cltv_expiry_delta, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, @@ -433,6 +441,7 @@ int main(int argc, char *argv[]) assert(timestamp == 1700115313); assert(message_flags == 1); assert(channel_flags == 1); + assert(cltv_expiry_delta == 6); assert(fee_base_msat == 20); assert(fee_proportional_millionths == 1000); assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(0))); diff --git a/connectd/queries.c b/connectd/queries.c index ef8c0ca672e9..3a680cb75bbd 100644 --- a/connectd/queries.c +++ b/connectd/queries.c @@ -484,7 +484,7 @@ static u32 get_timestamp(struct gossmap *gossmap, return 0; gossmap_chan_get_update_details(gossmap, chan, dir, ×tamp, - NULL, NULL, NULL, NULL, NULL, NULL); + NULL, NULL, NULL, NULL, NULL, NULL, NULL); return timestamp; } diff --git a/connectd/test/run-crc32_of_update.c b/connectd/test/run-crc32_of_update.c index 2b65565cfa40..efcaaf4792d7 100644 --- a/connectd/test/run-crc32_of_update.c +++ b/connectd/test/run-crc32_of_update.c @@ -124,6 +124,7 @@ void gossmap_chan_get_update_details(const struct gossmap *map UNNEEDED, u32 *timestamp UNNEEDED, u8 *message_flags UNNEEDED, u8 *channel_flags UNNEEDED, + u16 *cltv_expiry_delta UNNEEDED, u32 *fee_base_msat UNNEEDED, u32 *fee_proportional_millionths UNNEEDED, struct amount_msat *htlc_minimum_msat UNNEEDED, diff --git a/devtools/gossmap-compress.c b/devtools/gossmap-compress.c index d41672a54b25..fa873022c858 100644 --- a/devtools/gossmap-compress.c +++ b/devtools/gossmap-compress.c @@ -225,7 +225,7 @@ static u64 get_htlc_min(struct gossmap *gossmap, { struct amount_msat msat; gossmap_chan_get_update_details(gossmap, chan, dir, - NULL, NULL, NULL, NULL, NULL, &msat, NULL); + NULL, NULL, NULL, NULL, NULL, NULL, &msat, NULL); return msat.millisatoshis; /* Raw: compressed format */ } @@ -237,7 +237,7 @@ static u64 get_htlc_max(struct gossmap *gossmap, capacity_msat = gossmap_chan_get_capacity(gossmap, chan); gossmap_chan_get_update_details(gossmap, chan, dir, - NULL, NULL, NULL, NULL, NULL, NULL, &msat); + NULL, NULL, NULL, NULL, NULL, NULL, NULL, &msat); /* Special value for the common case of "max_htlc == capacity" */ if (amount_msat_eq(msat, capacity_msat)) { @@ -257,7 +257,7 @@ static u64 get_basefee(struct gossmap *gossmap, { u32 basefee; gossmap_chan_get_update_details(gossmap, chan, dir, - NULL, NULL, NULL, &basefee, NULL, NULL, NULL); + NULL, NULL, NULL, NULL, &basefee, NULL, NULL, NULL); return basefee; } @@ -267,7 +267,7 @@ static u64 get_propfee(struct gossmap *gossmap, { u32 propfee; gossmap_chan_get_update_details(gossmap, chan, dir, - NULL, NULL, NULL, NULL, &propfee, NULL, NULL); + NULL, NULL, NULL, NULL, NULL, &propfee, NULL, NULL); return propfee; } diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 20534a5c987d..996f259c4a0b 100644 --- a/gossipd/gossmap_manage.c +++ b/gossipd/gossmap_manage.c @@ -332,7 +332,7 @@ static u32 get_timestamp(struct gossmap *gossmap, gossmap_chan_get_update_details(gossmap, chan, dir, ×tamp, - NULL, NULL, NULL, NULL, NULL, NULL); + NULL, NULL, NULL, NULL, NULL, NULL, NULL); return timestamp; } diff --git a/gossipd/seeker.c b/gossipd/seeker.c index cee9dda5cf58..7a8fcfc2fabd 100644 --- a/gossipd/seeker.c +++ b/gossipd/seeker.c @@ -622,7 +622,7 @@ static bool want_update(struct gossmap *gossmap, return timestamp != 0; gossmap_chan_get_update_details(gossmap, chan, dir, &our_timestamp, - NULL, NULL, NULL, NULL, NULL, NULL); + NULL, NULL, NULL, NULL, NULL, NULL, NULL); if (timestamp <= our_timestamp) return false; diff --git a/gossipd/test/run-next_block_range.c b/gossipd/test/run-next_block_range.c index be5859db3f68..8f2e7db7e9a8 100644 --- a/gossipd/test/run-next_block_range.c +++ b/gossipd/test/run-next_block_range.c @@ -45,6 +45,7 @@ void gossmap_chan_get_update_details(const struct gossmap *map UNNEEDED, u32 *timestamp UNNEEDED, u8 *message_flags UNNEEDED, u8 *channel_flags UNNEEDED, + u16 *cltv_expiry_delta UNNEEDED, u32 *fee_base_msat UNNEEDED, u32 *fee_proportional_millionths UNNEEDED, struct amount_msat *htlc_minimum_msat UNNEEDED, diff --git a/plugins/test/run-route-calc.c b/plugins/test/run-route-calc.c index 88021f81703f..8e4eda56a9b1 100644 --- a/plugins/test/run-route-calc.c +++ b/plugins/test/run-route-calc.c @@ -84,12 +84,6 @@ 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) diff --git a/plugins/test/run-route-overlong.c b/plugins/test/run-route-overlong.c index 52a50c1c97e4..308361c7c4c7 100644 --- a/plugins/test/run-route-overlong.c +++ b/plugins/test/run-route-overlong.c @@ -81,12 +81,6 @@ 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) diff --git a/plugins/topology.c b/plugins/topology.c index 92425a04ab72..51565cb2f363 100644 --- a/plugins/topology.c +++ b/plugins/topology.c @@ -279,6 +279,7 @@ static void json_add_halfchan(struct json_stream *response, ×tamp, &message_flags, &channel_flags, + NULL, &fee_base_msat, &fee_proportional_millionths, &htlc_minimum_msat, From 373e45ed215b779f7b76e44a737e3da3de9ed451 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:38 +1030 Subject: [PATCH 06/22] gossmap: allow gossmap_chan_get_update_details on locally-modified channels. In particular, this lets you find the exact htlc_maximum_msat/htlc_minimum_msat values. This means we actually create real channel_updates for local mods, which requires a second "local" scratch region. Signed-off-by: Rusty Russell --- common/gossmap.c | 289 +++++++++++++++++++------------- common/gossmap.h | 3 +- common/test/run-gossmap_local.c | 40 +++++ 3 files changed, 217 insertions(+), 115 deletions(-) diff --git a/common/gossmap.c b/common/gossmap.c index 22f9ea05dcb9..d769ced6afc4 100644 --- a/common/gossmap.c +++ b/common/gossmap.c @@ -82,8 +82,10 @@ struct gossmap { /* Linked list of freed ones, if any. */ u32 freed_nodes, freed_chans; - /* local messages, if any. */ - const u8 *local; + /* local channel_announce messages, if any. */ + const u8 *local_announces; + /* local channel_update messages, if any. */ + u8 *local_updates; /* Callbacks for different events: return false to fail. */ void (*cupdate_fail)(struct gossmap *map, @@ -104,10 +106,17 @@ struct gossmap { static void map_copy(const struct gossmap *map, u64 offset, void *dst, size_t len) { + /* After mmap, we place local channel_announcements then channel_updates */ if (offset >= map->map_size) { - u64 localoff = offset - map->map_size; - assert(localoff + len <= tal_bytelen(map->local)); - memcpy(dst, map->local + localoff, len); + u64 localannoff = offset - map->map_size; + if (localannoff < tal_bytelen(map->local_announces)) { + assert(localannoff + len <= tal_bytelen(map->local_announces)); + memcpy(dst, map->local_announces + localannoff, len); + } else { + u64 localchanoff = localannoff - tal_bytelen(map->local_announces); + assert(localchanoff + len <= tal_bytelen(map->local_updates)); + memcpy(dst, map->local_updates + localchanoff, len); + } } else { assert(offset + len <= map->map_size); if (map->mmap) @@ -508,6 +517,61 @@ static struct gossmap_chan *add_channel(struct gossmap *map, return chan; } +/* Does not set hc->nodeidx! */ +static void fill_from_update(struct gossmap *map, + struct short_channel_id_dir *scidd, + struct half_chan *hc, + u64 cupdate_off, + void (*cupdate_fail)(struct gossmap *map, + const struct short_channel_id_dir *scidd, + u16 cltv_expiry_delta, + u32 fee_base_msat, + u32 fee_proportional_millionths, + void *cb_arg), + void *cb_arg) +{ + /* Note that first two bytes are message type */ + const u64 scid_off = cupdate_off + 2 + (64 + 32); + const u64 message_flags_off = scid_off + 8 + 4; + const u64 channel_flags_off = message_flags_off + 1; + const u64 cltv_expiry_delta_off = channel_flags_off + 1; + const u64 htlc_minimum_off = cltv_expiry_delta_off + 2; + const u64 fee_base_off = htlc_minimum_off + 8; + const u64 fee_prop_off = fee_base_off + 4; + const u64 htlc_maximum_off = fee_prop_off + 4; + u8 chanflags; + u32 base_fee, proportional_fee; + u16 delay; + + /* We round this *down*, since too-low min is more conservative */ + hc->htlc_min = u64_to_fp16(map_be64(map, htlc_minimum_off), false); + hc->htlc_max = u64_to_fp16(map_be64(map, htlc_maximum_off), true); + + scidd->scid.u64 = map_be64(map, scid_off); + chanflags = map_u8(map, channel_flags_off); + scidd->dir = (chanflags & ROUTING_FLAGS_DIRECTION); + hc->enabled = !(chanflags & ROUTING_FLAGS_DISABLED); + base_fee = map_be32(map, fee_base_off); + proportional_fee = map_be32(map, fee_prop_off); + delay = map_be16(map, cltv_expiry_delta_off); + + hc->base_fee = base_fee; + hc->proportional_fee = proportional_fee; + hc->delay = delay; + + /* Check they fit: we turn off if not, call optional callback. */ + if (hc->base_fee != base_fee + || hc->proportional_fee != proportional_fee + || hc->delay != delay) { + hc->htlc_max = 0; + hc->enabled = false; + if (cupdate_fail) + cupdate_fail(map, scidd, + delay, base_fee, proportional_fee, + cb_arg); + } +} + /* BOLT #7: * 1. type: 258 (`channel_update`) * 2. data: @@ -525,58 +589,21 @@ static struct gossmap_chan *add_channel(struct gossmap *map, */ static void update_channel(struct gossmap *map, u64 cupdate_off) { - /* Note that first two bytes are message type */ - const u64 scid_off = cupdate_off + 2 + (64 + 32); - const u64 message_flags_off = scid_off + 8 + 4; - const u64 channel_flags_off = message_flags_off + 1; - const u64 cltv_expiry_delta_off = channel_flags_off + 1; - const u64 htlc_minimum_off = cltv_expiry_delta_off + 2; - const u64 fee_base_off = htlc_minimum_off + 8; - const u64 fee_prop_off = fee_base_off + 4; - const u64 htlc_maximum_off = fee_prop_off + 4; struct short_channel_id_dir scidd; struct gossmap_chan *chan; struct half_chan hc; - u8 chanflags; - u32 base_fee, proportional_fee; - u16 delay; - scidd.scid.u64 = map_be64(map, scid_off); + fill_from_update(map, &scidd, &hc, cupdate_off, + map->cupdate_fail, map->cb_arg); chan = gossmap_find_chan(map, &scidd.scid); /* This can happen if channel gets deleted! */ if (!chan) return; - /* We round this *down*, since too-low min is more conservative */ - hc.htlc_min = u64_to_fp16(map_be64(map, htlc_minimum_off), false); - hc.htlc_max = u64_to_fp16(map_be64(map, htlc_maximum_off), true); - - chanflags = map_u8(map, channel_flags_off); - hc.enabled = !(chanflags & ROUTING_FLAGS_DISABLED); - scidd.dir = (chanflags & ROUTING_FLAGS_DIRECTION); - base_fee = map_be32(map, fee_base_off); - proportional_fee = map_be32(map, fee_prop_off); - delay = map_be16(map, cltv_expiry_delta_off); - - hc.base_fee = base_fee; - hc.proportional_fee = proportional_fee; - hc.delay = delay; - /* Check they fit: we turn off if not. */ - if (hc.base_fee != base_fee - || hc.proportional_fee != proportional_fee - || hc.delay != delay) { - if (map->cupdate_fail) - map->cupdate_fail(map, &scidd, - delay, base_fee, proportional_fee, - map->cb_arg); - hc.htlc_max = 0; - hc.enabled = false; - } - /* Preserve this */ - hc.nodeidx = chan->half[chanflags & 1].nodeidx; - chan->half[chanflags & 1] = hc; - chan->cupdate_off[chanflags & 1] = cupdate_off; + hc.nodeidx = chan->half[scidd.dir].nodeidx; + chan->half[scidd.dir] = hc; + chan->cupdate_off[scidd.dir] = cupdate_off; } static void remove_channel_by_deletemsg(struct gossmap *map, u64 del_off) @@ -710,7 +737,8 @@ static bool map_catchup(struct gossmap *map, bool *changed) static bool load_gossip_store(struct gossmap *map) { map->map_size = lseek(map->fd, 0, SEEK_END); - map->local = NULL; + map->local_announces = NULL; + map->local_updates = NULL; /* gossipd uses pwritev(), which is not consistent with mmap on OpenBSD! */ #ifndef __OpenBSD__ @@ -772,7 +800,7 @@ struct localmod { /* Non-NULL values mean change existing ones */ struct localmod_changes { const bool *enabled; - const fp16_t *htlc_min, *htlc_max; + const struct amount_msat *htlc_min, *htlc_max; const u32 *base_fee, *proportional_fee; const u16 *delay; } changes[2]; @@ -791,8 +819,8 @@ static bool localmod_is_local_chan(const struct localmod *mod) struct gossmap_localmods { struct localmod *mods; - /* This is the local array to be used by the gossmap */ - u8 *local; + /* This is the local announcement to be used by the gossmap */ + u8 *local_announces; }; struct gossmap_localmods *gossmap_localmods_new(const tal_t *ctx) @@ -801,18 +829,17 @@ struct gossmap_localmods *gossmap_localmods_new(const tal_t *ctx) localmods = tal(ctx, struct gossmap_localmods); localmods->mods = tal_arr(localmods, struct localmod, 0); - localmods->local = tal_arr(localmods, u8, 0); + localmods->local_announces = tal_arr(localmods, u8, 0); return localmods; } /* Create space at end of local map, return offset it was added at. */ -static size_t insert_local_space(struct gossmap_localmods *localmods, - size_t msglen) +static size_t insert_local_space(u8 **localspace, size_t msglen) { - size_t oldlen = tal_bytelen(localmods->local); + size_t oldlen = tal_bytelen(*localspace); - tal_resize(&localmods->local, oldlen + msglen); + tal_resize(localspace, oldlen + msglen); return oldlen; } @@ -856,13 +883,13 @@ bool gossmap_local_addchan(struct gossmap_localmods *localmods, memset(&mod.changes, 0, sizeof(mod.changes)); /* We create amount, then fake local channel_announcement */ - off = insert_local_space(localmods, + off = insert_local_space(&localmods->local_announces, 8 + 2 + 64 * 4 + 2 + tal_bytelen(features) + 32 + 8 + 33 + 33); /* Write amount */ be64 = be64_to_cpu(capacity.millisatoshis / 1000 /* Raw: gossmap */); - memcpy(localmods->local + off, &be64, sizeof(be64)); + memcpy(localmods->local_announces + off, &be64, sizeof(be64)); off += sizeof(be64); /* From here is a channel-announcment, with only the fields we use */ @@ -870,7 +897,7 @@ bool gossmap_local_addchan(struct gossmap_localmods *localmods, /* Set type to be kosher. */ be16 = CPU_TO_BE16(WIRE_CHANNEL_ANNOUNCEMENT); - memcpy(localmods->local + off, &be16, sizeof(be16)); + memcpy(localmods->local_announces + off, &be16, sizeof(be16)); off += sizeof(be16); /* Skip sigs */ @@ -878,11 +905,11 @@ bool gossmap_local_addchan(struct gossmap_localmods *localmods, /* Set length and features */ be16 = cpu_to_be16(tal_bytelen(features)); - memcpy(localmods->local + off, &be16, sizeof(be16)); + memcpy(localmods->local_announces + off, &be16, sizeof(be16)); off += sizeof(be16); /* Damn you, C committee! */ if (features) - memcpy(localmods->local + off, features, tal_bytelen(features)); + memcpy(localmods->local_announces + off, features, tal_bytelen(features)); off += tal_bytelen(features); /* Skip chain_hash */ @@ -890,16 +917,16 @@ bool gossmap_local_addchan(struct gossmap_localmods *localmods, /* Set scid */ be64 = be64_to_cpu(scid.u64); - memcpy(localmods->local + off, &be64, sizeof(be64)); + memcpy(localmods->local_announces + off, &be64, sizeof(be64)); off += sizeof(be64); /* set node_ids */ - memcpy(localmods->local + off, n1->k, sizeof(n1->k)); + memcpy(localmods->local_announces + off, n1->k, sizeof(n1->k)); off += sizeof(n1->k); - memcpy(localmods->local + off, n2->k, sizeof(n2->k)); + memcpy(localmods->local_announces + off, n2->k, sizeof(n2->k)); off += sizeof(n2->k); - assert(off == tal_bytelen(localmods->local)); + assert(off == tal_bytelen(localmods->local_announces)); tal_arr_expand(&localmods->mods, mod); return true; @@ -954,16 +981,12 @@ bool gossmap_local_updatechan(struct gossmap_localmods *localmods, lc->enabled = tal_dup(localmods, bool, enabled); } if (htlc_min) { - fp16_t min = u64_to_fp16(htlc_min->millisatoshis, /* Raw: to fp16 */ - false); tal_free(lc->htlc_min); - lc->htlc_min = tal_dup(localmods, fp16_t, &min); + lc->htlc_min = tal_dup(localmods, struct amount_msat, htlc_min); } if (htlc_max) { - fp16_t max = u64_to_fp16(htlc_max->millisatoshis, /* Raw: to fp16 */ - true); tal_free(lc->htlc_max); - lc->htlc_max = tal_dup(localmods, fp16_t, &max); + lc->htlc_max = tal_dup(localmods, struct amount_msat, htlc_max); } if (base_fee) { u32 base_as_u32 = base_fee->millisatoshis; /* Raw: localmod */ @@ -1005,8 +1028,9 @@ void gossmap_apply_localmods(struct gossmap *map, { size_t n = tal_count(localmods->mods); - assert(!map->local); - map->local = localmods->local; + assert(!map->local_announces); + map->local_announces = localmods->local_announces; + map->local_updates = tal_arr(map, u8, 0); for (size_t i = 0; i < n; i++) { struct localmod *mod = &localmods->mods[i]; @@ -1025,47 +1049,87 @@ void gossmap_apply_localmods(struct gossmap *map, /* Save old, update any fields they wanted to change */ for (size_t h = 0; h < 2; h++) { - bool was_set, all_changed; const struct localmod_changes *c = &mod->changes[h]; + /* Default values all zero, disabled */ + u32 timestamp = 0; + u8 message_flags = ROUTING_OPT_HTLC_MAX_MSAT, channel_flags = h | ROUTING_FLAGS_DISABLED; + u16 cltv_expiry_delta = 0; + u32 fee_base_msat = 0, fee_proportional_millionths = 0; + struct amount_msat htlc_minimum_msat = AMOUNT_MSAT(0), + htlc_maximum_msat = AMOUNT_MSAT(0); + const u8 *cupdatemsg; + secp256k1_ecdsa_signature fake_signature; + struct bitcoin_blkid fake_blkid; + struct short_channel_id_dir scidd; + u64 off; + bool any_set = false; + /* Save existing versions */ mod->orig[h] = chan->half[h]; mod->orig_cupdate_off[h] = chan->cupdate_off[h]; - was_set = gossmap_chan_set(chan, h); + /* If we have a previous one, base it on that! */ + if (gossmap_chan_set(chan, h)) { + gossmap_chan_get_update_details(map, chan, h, + ×tamp, + &message_flags, + &channel_flags, + &cltv_expiry_delta, + &fee_base_msat, + &fee_proportional_millionths, + &htlc_minimum_msat, + &htlc_maximum_msat); + any_set = true; + } /* Override specified fields. */ - all_changed = true; - if (c->enabled) - chan->half[h].enabled = *c->enabled; - else - all_changed = false; - if (c->htlc_min) - chan->half[h].htlc_min = *c->htlc_min; - else - all_changed = false; - if (c->htlc_max) - chan->half[h].htlc_max = *c->htlc_max; - else - all_changed = false; - if (c->base_fee) - chan->half[h].base_fee = *c->base_fee; - else - all_changed = false; - if (c->proportional_fee) - chan->half[h].proportional_fee = *c->proportional_fee; - else - all_changed = false; - if (c->delay) - chan->half[h].delay = *c->delay; - else - all_changed = false; - - /* Is it all defined? - * This controls gossmap_chan_set(chan, h); */ - if (was_set || all_changed) - chan->cupdate_off[h] = 0xFFFFFFFFFFFFFFFFULL; - else - chan->cupdate_off[h] = 0; + if (c->enabled) { + any_set = true; + if (!*c->enabled) + channel_flags |= ROUTING_FLAGS_DISABLED; + else + channel_flags &= ~ROUTING_FLAGS_DISABLED; + } + if (c->htlc_min) { + any_set = true; + htlc_minimum_msat = *c->htlc_min; + } + if (c->htlc_max) { + any_set = true; + htlc_maximum_msat = *c->htlc_max; + } + if (c->base_fee) { + any_set = true; + fee_base_msat = *c->base_fee; + } + if (c->proportional_fee) { + any_set = true; + fee_proportional_millionths = *c->proportional_fee; + } + if (c->delay) { + any_set = true; + cltv_expiry_delta = *c->delay; + } + + /* Leave unset */ + if (!any_set) + continue; + + /* We create fake local channel_update */ + memset(&fake_signature, 0, sizeof(fake_signature)); + memset(&fake_blkid, 0, sizeof(fake_blkid)); + cupdatemsg = towire_channel_update(tmpctx, &fake_signature, &fake_blkid, mod->scid, + timestamp, message_flags, channel_flags, cltv_expiry_delta, + htlc_minimum_msat, fee_base_msat, fee_proportional_millionths, + htlc_maximum_msat); + off = insert_local_space(&map->local_updates, tal_bytelen(cupdatemsg)); + memcpy(map->local_updates + off, cupdatemsg, tal_bytelen(cupdatemsg)); + chan->cupdate_off[h] = map->map_size + tal_bytelen(map->local_announces) + off; + fill_from_update(map, &scidd, &chan->half[h], chan->cupdate_off[h], NULL, NULL); + + /* We wrote the right update, correct? */ + assert(short_channel_id_eq(scidd.scid, mod->scid)); + assert(scidd.dir == h); } } } @@ -1075,7 +1139,7 @@ void gossmap_remove_localmods(struct gossmap *map, { size_t n = tal_count(localmods->mods); - assert(map->local == localmods->local); + assert(map->local_announces == localmods->local_announces); for (size_t i = 0; i < n; i++) { const struct localmod *mod = &localmods->mods[i]; @@ -1096,15 +1160,16 @@ void gossmap_remove_localmods(struct gossmap *map, } } } - map->local = NULL; + map->local_announces = NULL; + map->local_updates = tal_free(map->local_updates); } bool gossmap_refresh_mayfail(struct gossmap *map, bool *updated) { off_t len; - /* You must remove local updates before this. */ - assert(!map->local); + /* You must remove local modifications before this. */ + assert(!map->local_announces); /* If file has gotten larger, try rereading */ len = lseek(map->fd, 0, SEEK_END); @@ -1503,8 +1568,6 @@ void gossmap_chan_get_update_details(const struct gossmap *map, const u64 htlc_maximum_off = fee_prop_off + 4; assert(gossmap_chan_set(chan, dir)); - /* Not allowed on local updates! */ - assert(chan->cann_off < map->map_size); if (timestamp) *timestamp = map_be32(map, timestamp_off); diff --git a/common/gossmap.h b/common/gossmap.h index 01b40ea50c7a..e56ba9464ef7 100644 --- a/common/gossmap.h +++ b/common/gossmap.h @@ -222,8 +222,7 @@ u8 *gossmap_chan_get_update(const tal_t *ctx, bool gossmap_scidd_pubkey(struct gossmap *gossmap, struct sciddir_or_pubkey *sciddpk); -/* Returns details from channel_update (must be gossmap_chan_set, and - * does not work for local_updatechan)! */ +/* Returns details from channel_update (must be gossmap_chan_set)! */ void gossmap_chan_get_update_details(const struct gossmap *map, const struct gossmap_chan *chan, int dir, diff --git a/common/test/run-gossmap_local.c b/common/test/run-gossmap_local.c index 218175724e63..b9ad6808d3f4 100644 --- a/common/test/run-gossmap_local.c +++ b/common/test/run-gossmap_local.c @@ -528,6 +528,26 @@ int main(int argc, char *argv[]) assert(chan->half[0].proportional_fee == 3); assert(chan->half[0].delay == 4); + /* We can query update_details on locally-generated chans */ + gossmap_chan_get_update_details(map, chan, + 0, + ×tamp, + &message_flags, + &channel_flags, + &cltv_expiry_delta, + &fee_base_msat, + &fee_proportional_millionths, + &htlc_minimum_msat, + &htlc_maximum_msat); + assert(timestamp == 0); + assert(message_flags == ROUTING_OPT_HTLC_MAX_MSAT); + assert(channel_flags == 0); + assert(cltv_expiry_delta == 4); + assert(fee_base_msat == 2); + assert(fee_proportional_millionths == 3); + assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(1))); + assert(amount_msat_eq(htlc_maximum_msat, AMOUNT_MSAT(100000))); + assert(!gossmap_find_chan(map, &scid_nonexisting)); chan = gossmap_find_chan(map, &scid23); @@ -538,6 +558,26 @@ int main(int argc, char *argv[]) assert(chan->half[0].proportional_fee == 102); assert(chan->half[0].delay == 103); + /* We can query update_details on locally-modified chans */ + gossmap_chan_get_update_details(map, chan, + 0, + ×tamp, + &message_flags, + &channel_flags, + &cltv_expiry_delta, + &fee_base_msat, + &fee_proportional_millionths, + &htlc_minimum_msat, + &htlc_maximum_msat); + assert(timestamp == 1700115301); + assert(message_flags == ROUTING_OPT_HTLC_MAX_MSAT); + assert(channel_flags == 0); + assert(cltv_expiry_delta == 103); + assert(fee_base_msat == 101); + assert(fee_proportional_millionths == 102); + assert(amount_msat_eq(htlc_minimum_msat, AMOUNT_MSAT(99))); + assert(amount_msat_eq(htlc_maximum_msat, AMOUNT_MSAT(100))); + /* Cleanup leaves everything previous intact */ gossmap_remove_localmods(map, mods); From 19ee27cabac845fd0b0fc4c5097543a7cee9c8f1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:38 +1030 Subject: [PATCH 07/22] askrene: constrain to exact htlc_min/htlc_max values. The fp16_t values are approximations (overestimate for htlc_max, underestimate for htlc_min), so in the refinement step we should use the exact values. This also fixes a logic bug: flow_remaining_capacity returned the total capacity, not the additional capacity! Signed-off-by: Rusty Russell Changelog-EXPERIMENTAL: `askrene` now honors exact htlc_maximum_msat limits. --- plugins/askrene/askrene.c | 8 ++ plugins/askrene/refine.c | 241 +++++++++++++++++++++++--------------- tests/test_askrene.py | 2 +- 3 files changed, 153 insertions(+), 98 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index c3a38ae4ef39..a035e83b4a8b 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -395,6 +395,7 @@ static const char *get_routes(const tal_t *ctx, } /* Too expensive? */ +too_expensive: while (amount_msat_greater(flowset_fee(rq->plugin, flows), maxfee)) { mu += 10; rq_log(tmpctx, rq, LOG_UNUSUAL, @@ -425,6 +426,13 @@ static const char *get_routes(const tal_t *ctx, if (ret) goto fail; + /* Again, a tiny corner case: refine step can make us exceed maxfee */ + if (amount_msat_greater(flowset_fee(rq->plugin, flows), maxfee)) { + rq_log(tmpctx, rq, LOG_UNUSUAL, + "After final refinement, fee was excessive: retrying"); + goto too_expensive; + } + /* Convert back into routes, with delay and other information fixed */ *routes = tal_arr(ctx, struct route *, tal_count(flows)); *amounts = tal_arr(ctx, struct amount_msat, tal_count(flows)); diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index e7b257cb0373..be2ccc4ac886 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -62,6 +62,93 @@ static void add_reservation(struct reserve_hop **reservations, tal_arr_expand(reservations, rhop); } +/* We aren't allowed to ask for update_details on locally-generated channels, + * so go to the source in that case */ +static struct amount_msat get_chan_htlc_max(const struct route_query *rq, + const struct gossmap_chan *c, + const struct short_channel_id_dir *scidd) +{ + struct amount_msat htlc_max; + + gossmap_chan_get_update_details(rq->gossmap, + c, scidd->dir, + NULL, NULL, NULL, NULL, NULL, NULL, + NULL, &htlc_max); + return htlc_max; +} + +static struct amount_msat get_chan_htlc_min(const struct route_query *rq, + const struct gossmap_chan *c, + const struct short_channel_id_dir *scidd) +{ + struct amount_msat htlc_min; + + gossmap_chan_get_update_details(rq->gossmap, + c, scidd->dir, + NULL, NULL, NULL, NULL, NULL, NULL, + &htlc_min, NULL); + return htlc_min; +} + +enum why_capped { + CAPPED_HTLC_MAX, + CAPPED_CAPACITY, +}; + +/* Get exact maximum we can deliver with this flow. Returns reason + * why this is the limit (max_hltc or capacity), and optionally sets scidd */ +static enum why_capped flow_max_capacity(const struct route_query *rq, + const struct flow *flow, + struct amount_msat *deliverable, + struct short_channel_id_dir *scidd_why, + struct amount_msat *amount_why) +{ + struct amount_msat max_msat = AMOUNT_MSAT(-1ULL); + enum why_capped why_capped = CAPPED_CAPACITY; + + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat min, max, htlc_max; + struct short_channel_id_dir scidd; + + get_scidd(rq->gossmap, flow, i, &scidd); + /* We can pass constraints due to addition of fees! */ + get_constraints(rq, flow->path[i], flow->dirs[i], &min, &max); + + if (amount_msat_greater(max_msat, max)) { + why_capped = CAPPED_CAPACITY; + if (scidd_why) + *scidd_why = scidd; + if (amount_why) + *amount_why = max; + max_msat = max; + } + + htlc_max = get_chan_htlc_max(rq, flow->path[i], &scidd); + if (amount_msat_greater(max_msat, htlc_max)) { + why_capped = CAPPED_HTLC_MAX; + if (scidd_why) + *scidd_why = scidd; + if (amount_why) + *amount_why = htlc_max; + max_msat = htlc_max; + } + if (!amount_msat_add_fee(&max_msat, h->base_fee, h->proportional_fee)) + max_msat = AMOUNT_MSAT(-1ULL); + } + + /* Calculate deliverable max */ + *deliverable = max_msat; + for (size_t i = 0; i < tal_count(flow->path); i++) { + const struct half_chan *h = flow_edge(flow, i); + *deliverable = amount_msat_sub_fee(*deliverable, + h->base_fee, + h->proportional_fee); + } + return why_capped; +} + + /* We have a basic set of flows, but we need to add fees, and take into * account that "spendable" estimates are for a single HTLC. This can * push us again over capacity or htlc_maximum_msat. @@ -78,95 +165,67 @@ static const char *constrain_flow(const tal_t *ctx, struct flow *flow, struct reserve_hop **reservations) { - struct amount_msat msat; - int decreased = -1; - const char *why_decreased = NULL; + struct amount_msat deliverable, msat, amount_capped; + enum why_capped why_capped; + struct short_channel_id_dir scidd_capped; + + why_capped = flow_max_capacity(rq, flow, &deliverable, + &scidd_capped, &amount_capped); + if (amount_msat_less(deliverable, flow->delivers)) { + rq_log(tmpctx, rq, LOG_INFORM, + "Flow reduced to deliver %s not %s, because %s %s %s", + fmt_amount_msat(tmpctx, deliverable), + fmt_amount_msat(tmpctx, flow->delivers), + fmt_short_channel_id_dir(tmpctx, &scidd_capped), + why_capped == CAPPED_HTLC_MAX + ? "advertizes htlc_maximum_msat" + : "has remaining capacity", + fmt_amount_msat(tmpctx, amount_capped)); + flow->delivers = deliverable; + } - /* Walk backwards, adding fees and testing for htlc_max and - * capacity limits. */ + /* Now, check if any of them violate htlc_min */ msat = flow->delivers; for (int i = tal_count(flow->path) - 1; i >= 0; i--) { const struct half_chan *h = flow_edge(flow, i); - struct amount_msat min, max, amount_to_reserve; + struct amount_msat min; struct short_channel_id_dir scidd; - const char *max_cause; - /* We can pass constraints due to addition of fees! */ - get_constraints(rq, flow->path[i], flow->dirs[i], &min, &max); - if (amount_msat_less(amount_msat(fp16_to_u64(h->htlc_max)), max)) { - max_cause = "htlc_maximum_msat of "; - max = amount_msat(fp16_to_u64(h->htlc_max)); - } else { - max_cause = "channel capacity of "; - } + get_scidd(rq->gossmap, flow, i, &scidd); + min = get_chan_htlc_min(rq, flow->path[i], &scidd); - /* If amount is > max, we decrease and add note it in - * case something goes wrong later. */ - if (amount_msat_greater(msat, max)) { - rq_log(tmpctx, rq, LOG_INFORM, - "Had to decreased amount %s to %s%s across %s", - fmt_amount_msat(tmpctx, msat), - max_cause, - fmt_amount_msat(tmpctx, max), - fmt_flows_step_scid(tmpctx, rq, flow, i)); - msat = max; - decreased = i; - why_decreased = max_cause; + if (amount_msat_less(msat, min)) { + return rq_log(ctx, rq, LOG_UNUSUAL, + "Amount %s below minimum %s across %s", + fmt_amount_msat(tmpctx, msat), + fmt_amount_msat(tmpctx, min), + fmt_short_channel_id_dir(tmpctx, &scidd)); } + if (!amount_msat_add_fee(&msat, + h->base_fee, h->proportional_fee)) + plugin_err(rq->plugin, "Adding fee to amount"); + } + + /* Finally, reserve so next flow sees reduced capacity. */ + msat = flow->delivers; + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat amount_to_reserve; + struct short_channel_id_dir scidd; - /* Reserve more for local channels if it reduces capacity */ get_scidd(rq->gossmap, flow, i, &scidd); + + /* Reserve more for local channels if it reduces capacity */ if (!amount_msat_add(&amount_to_reserve, msat, get_additional_per_htlc_cost(rq, &scidd))) abort(); - /* Reserve it, so if the next flow asks about the same channel, - it will see the reduced capacity from this one. */ add_reservation(reservations, rq, flow, i, amount_to_reserve); - - if (!amount_msat_add_fee(&msat, h->base_fee, h->proportional_fee)) + if (!amount_msat_add_fee(&msat, + h->base_fee, h->proportional_fee)) plugin_err(rq->plugin, "Adding fee to amount"); } - /* Now we know how much we could send, figure out how much would be - * actually delivered. Here we also check for min_htlc violations. */ - for (size_t i = 0; i < tal_count(flow->path); i++) { - const struct half_chan *h = flow_edge(flow, i); - struct amount_msat next, min = amount_msat(fp16_to_u64(h->htlc_min)); - - next = amount_msat_sub_fee(msat, - h->base_fee, h->proportional_fee); - - /* These failures are incredibly unlikely, but possible */ - if (amount_msat_is_zero(next)) { - return rq_log(ctx, rq, LOG_UNUSUAL, - "Amount %s cannot pay its own fees across %s", - fmt_amount_msat(tmpctx, msat), - fmt_flows_step_scid(tmpctx, rq, flow, i)); - } - - /* Does happen if we try to pay 1 msat, and all paths have 1000msat min */ - if (amount_msat_less(next, min)) { - return rq_log(ctx, rq, LOG_UNUSUAL, - "Amount %s below minimum across %s", - fmt_amount_msat(tmpctx, next), - fmt_flows_step_scid(tmpctx, rq, flow, i)); - } - - msat = next; - } - - if (!amount_msat_eq(flow->delivers, msat)) { - rq_log(tmpctx, rq, LOG_INFORM, - "Flow changed to deliver %s not %s, because max constrained by %s%s", - fmt_amount_msat(tmpctx, msat), - fmt_amount_msat(tmpctx, flow->delivers), - why_decreased ? why_decreased : NULL, - decreased == -1 ? "none" - : fmt_flows_step_scid(tmpctx, rq, flow, decreased)); - flow->delivers = msat; - } - return NULL; } @@ -201,36 +260,22 @@ static void add_to_flow(struct flow *flow, } } -/* Check out remaining capacity for this flow. Changes as other flows get - * increased (which sets reservations) */ -static struct amount_msat flow_remaining_capacity(struct route_query *rq, +static struct amount_msat flow_remaining_capacity(const struct route_query *rq, const struct flow *flow) { - struct amount_msat max_msat = AMOUNT_MSAT(-1ULL); - for (int i = tal_count(flow->path) - 1; i >= 0; i--) { - const struct half_chan *h = flow_edge(flow, i); - struct amount_msat min, max; - - /* We can pass constraints due to addition of fees! */ - get_constraints(rq, flow->path[i], flow->dirs[i], &min, &max); - max = amount_msat_min(max, amount_msat(fp16_to_u64(h->htlc_max))); + struct amount_msat max, diff; + flow_max_capacity(rq, flow, &max, NULL, NULL); - max_msat = amount_msat_min(max_msat, max); - if (!amount_msat_add_fee(&max_msat, h->base_fee, h->proportional_fee)) - max_msat = AMOUNT_MSAT(-1ULL); - } + if (!amount_msat_sub(&diff, max, flow->delivers)) + plugin_err(rq->plugin, "Flow delivers %s but max only %s", + fmt_amount_msat(tmpctx, flow->delivers), + fmt_amount_msat(tmpctx, max)); - /* Calculate deliverable max */ - for (size_t i = 0; i < tal_count(flow->path); i++) { - const struct half_chan *h = flow_edge(flow, i); - max_msat = amount_msat_sub_fee(max_msat, - h->base_fee, h->proportional_fee); - } - return max_msat; + return diff; } /* What's the "best" flow to add to? */ -static struct flow *pick_most_likely_flow(struct route_query *rq, +static struct flow *pick_most_likely_flow(const struct route_query *rq, struct flow **flows, struct amount_msat additional) { @@ -260,11 +305,12 @@ static const char *flow_violates_min(const tal_t *ctx, struct amount_msat msat = flow->delivers; for (int i = tal_count(flow->path) - 1; i >= 0; i--) { const struct half_chan *h = flow_edge(flow, i); - struct amount_msat min = amount_msat(fp16_to_u64(h->htlc_min)); + struct amount_msat min; + struct short_channel_id_dir scidd; + get_scidd(rq->gossmap, flow, i, &scidd); + min = get_chan_htlc_min(rq, flow->path[i], &scidd); if (amount_msat_less(msat, min)) { - struct short_channel_id_dir scidd; - get_scidd(rq->gossmap, flow, i, &scidd); return rq_log(ctx, rq, LOG_UNUSUAL, "Sending %s across %s would violate htlc_min (~%s)", fmt_amount_msat(tmpctx, msat), @@ -391,6 +437,7 @@ refine_with_fees_and_limits(const tal_t *ctx, fmt_amount_msat(tmpctx, flowset_delivers(rq->plugin, *flows)), fmt_amount_msat(tmpctx, deliver)); } + ret = NULL; out: diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 62e7e0b156d8..f533986e7368 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -861,7 +861,7 @@ def test_min_htlc(node_factory, bitcoind): GenChannel(0, 1, capacity_sats=20_000)]) l1 = node_factory.get_node(gossip_store_file=gsfile.name) - with pytest.raises(RpcError, match="Amount 1000msat below minimum across 0x1x0/1"): + with pytest.raises(RpcError, match="Amount 1000msat below minimum 2000msat across 0x1x0/1"): l1.rpc.getroutes(source=nodemap[0], destination=nodemap[1], amount_msat=1000, From 9f8c0bed0a9f5b109f2d89c98155990d109b8cf1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:39 +1030 Subject: [PATCH 08/22] askrene: don't give up if we hit htlc_max and have no other flows. This happens in the coming "real network" test! We add fees and hit htlc_max, but don't have another flow to add to. Rather than MCF again, we split the flow into two. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 53 +++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index be2ccc4ac886..b02a7217a304 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -323,6 +323,35 @@ static const char *flow_violates_min(const tal_t *ctx, return NULL; } +/* If one flow is constrained by htlc_max, we might be able to simply + * duplicate it. This is naive: it could still fail due to total + * capacity, but it is a corner case anyway. */ +static bool duplicate_one_flow(const struct route_query *rq, + struct flow ***flows) +{ + for (size_t i = 0; i < tal_count(*flows); i++) { + struct flow *flow = (*flows)[i], *new_flow; + struct amount_msat max; + if (flow_max_capacity(rq, flow, &max, NULL, NULL) + != CAPPED_HTLC_MAX) + continue; + + new_flow = tal(*flows, struct flow); + new_flow->path = tal_dup_talarr(new_flow, + const struct gossmap_chan *, + flow->path); + new_flow->dirs = tal_dup_talarr(new_flow, int, + flow->dirs); + new_flow->delivers = amount_msat_div(flow->delivers, 2); + if (!amount_msat_sub(&flow->delivers, + flow->delivers, new_flow->delivers)) + abort(); + tal_arr_expand(flows, new_flow); + return true; + } + return false; +} + const char * refine_with_fees_and_limits(const tal_t *ctx, struct route_query *rq, @@ -333,6 +362,7 @@ refine_with_fees_and_limits(const tal_t *ctx, struct amount_msat more_to_deliver; const char *flow_constraint_error = NULL; const char *ret; + bool tried_split_flow = false; for (size_t i = 0; i < tal_count(*flows);) { struct flow *flow = (*flows)[i]; @@ -398,25 +428,30 @@ refine_with_fees_and_limits(const tal_t *ctx, /* The residual is minimal. In theory we could add one msat at a time * to the most probably flow which has capacity. For speed, we break it * into the number of flows, then assign each one. */ - for (size_t i = 0; i < tal_count(*flows) && !amount_msat_is_zero(more_to_deliver); i++) { + while (!amount_msat_is_zero(more_to_deliver) && tal_count(*flows)) { struct flow *f; struct amount_msat extra; /* How much more do we deliver? Round up if we can */ - extra = amount_msat_div(more_to_deliver, tal_count(*flows) - i); + extra = amount_msat_div(more_to_deliver, tal_count(*flows)); if (amount_msat_less(extra, more_to_deliver)) { if (!amount_msat_accumulate(&extra, AMOUNT_MSAT(1))) abort(); } - /* In theory, this can happen. If it ever does, we - * could try MCF again for the remainder. */ + /* This happens when we have a single flow, and hit + * htlc_max. For this corner case, we split into an + * additional flow, but only once! */ f = pick_most_likely_flow(rq, *flows, extra); if (!f) { - ret = rq_log(ctx, rq, LOG_BROKEN, - "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", - fmt_amount_msat(tmpctx, more_to_deliver)); - goto out; + if (tried_split_flow || !duplicate_one_flow(rq, flows)) { + ret = rq_log(ctx, rq, LOG_BROKEN, + "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", + fmt_amount_msat(tmpctx, more_to_deliver)); + goto out; + } + tried_split_flow = true; + continue; } /* Make this flow deliver +extra, and modify reservations */ @@ -430,7 +465,7 @@ refine_with_fees_and_limits(const tal_t *ctx, if (!amount_msat_eq(deliver, flowset_delivers(rq->plugin, *flows))) { /* This should only happen if there were no flows */ if (tal_count(*flows) == 0) { - ret = flow_constraint_error; + ret = tal_steal(ctx, flow_constraint_error); goto out; } plugin_err(rq->plugin, "Flows delivered only %s of %s?", From 8b2bd8be3b42d10fe3dfaf9a8cab3754921467ce Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Fri, 11 Oct 2024 21:30:39 +1030 Subject: [PATCH 09/22] askrene: detect and cancel flow cycles Flow cycles can occur if we have arc zero arc costs. The previous path construction from the flow in the network assumed the absence of such cycles and would enter an infinite loop if it hit one. With his patch wee add cycle detection and removal during the path construction phase. Reported-by: Rusty Russell Signed-off-by: Lagrang3 Changelog-EXPERIMENTAL: `askrene` infinite loop fixed --- plugins/askrene/mcf.c | 243 ++++++++++++++++++++++++++---------------- 1 file changed, 154 insertions(+), 89 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index aaa914b03afe..7e803470d47f 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -998,8 +998,10 @@ struct chan_flow }; /* Search in the network a path of positive flow until we reach a node with - * positive balance. */ -static u32 find_positive_balance( + * positive balance (returns a node idx with positive balance) + * or we discover a cycle (returns a node idx with 0 balance). + * */ +static u32 find_path_or_cycle( const struct gossmap *gossmap, const struct chan_flow *chan_flow, const u32 start_idx, @@ -1009,42 +1011,34 @@ static u32 find_positive_balance( int *prev_dir, u32 *prev_idx) { + const size_t max_num_nodes = gossmap_max_node_idx(gossmap); + bitmap *visited = + tal_arrz(tmpctx, bitmap, BITMAP_NWORDS(max_num_nodes)); u32 final_idx = start_idx; + bitmap_set_bit(visited, start_idx); - /* TODO(eduardo) - * This is guaranteed to halt if there are no directed flow cycles. - * There souldn't be any. In fact if cost is strickly - * positive, then flow cycles do not exist at all in the - * MCF solution. But if cost is allowed to be zero for - * some arcs, then we might have flow cyles in the final - * solution. We must somehow ensure that the MCF - * algorithm does not come up with spurious flow cycles. */ - while(balance[final_idx]<=0) - { - // printf("%s: node = %d\n",__PRETTY_FUNCTION__,final_idx); - u32 updated_idx=INVALID_INDEX; - struct gossmap_node *cur - = gossmap_node_byidx(gossmap,final_idx); + /* It is guaranteed to halt, because we either find a node with + * balance[]>0 or we hit a node twice and we stop. */ + while (balance[final_idx] <= 0) { + u32 updated_idx = INVALID_INDEX; + struct gossmap_node *cur = + gossmap_node_byidx(gossmap, final_idx); - for(size_t i=0;inum_chans;++i) - { + for (size_t i = 0; i < cur->num_chans; ++i) { int dir; - const struct gossmap_chan *c - = gossmap_nth_chan(gossmap, - cur,i,&dir); + const struct gossmap_chan *c = + gossmap_nth_chan(gossmap, cur, i, &dir); - if (!gossmap_chan_set(c, dir)) + if (!gossmap_chan_set(c, dir) || !c->half[dir].enabled) continue; - const u32 c_idx = gossmap_chan_idx(gossmap,c); - - // follow the flow - if(chan_flow[c_idx].half[dir]>0) - { - const struct gossmap_node *next - = gossmap_nth_node(gossmap,c,!dir); - u32 next_idx = gossmap_node_idx(gossmap,next); + const u32 c_idx = gossmap_chan_idx(gossmap, c); + /* follow the flow */ + if (chan_flow[c_idx].half[dir] > 0) { + const struct gossmap_node *next = + gossmap_nth_node(gossmap, c, !dir); + u32 next_idx = gossmap_node_idx(gossmap, next); prev_dir[next_idx] = dir; prev_chan[next_idx] = c; @@ -1055,10 +1049,17 @@ static u32 find_positive_balance( } } - assert(updated_idx!=INVALID_INDEX); - assert(updated_idx!=final_idx); - + assert(updated_idx != INVALID_INDEX); + assert(updated_idx != final_idx); final_idx = updated_idx; + + if (bitmap_test_bit(visited, updated_idx)) { + /* We have seen this node before, we've found a cycle. + */ + assert(balance[updated_idx] <= 0); + break; + } + bitmap_set_bit(visited, updated_idx); } return final_idx; } @@ -1069,6 +1070,108 @@ struct list_data struct flow *flow_path; }; +/* Given a path from a node with negative balance to a node with positive + * balance, compute the bigest flow and substract it from the nodes balance and + * the channels allocation. */ +static struct flow *substract_flow(const tal_t *ctx, + const struct gossmap *gossmap, + const u32 start_idx, const u32 final_idx, + s64 *balance, struct chan_flow *chan_flow, + const u32 *prev_idx, const int *prev_dir, + const struct gossmap_chan *const *prev_chan) +{ + assert(balance[start_idx] < 0); + assert(balance[final_idx] > 0); + s64 delta = -balance[start_idx]; + size_t length = 0; + delta = MIN(delta, balance[final_idx]); + + /* We can only walk backwards, now get me the legth of the path and the + * max flow we can send through this route. */ + for (u32 cur_idx = final_idx; cur_idx != start_idx; + cur_idx = prev_idx[cur_idx]) { + assert(cur_idx != INVALID_INDEX); + const int dir = prev_dir[cur_idx]; + const struct gossmap_chan *const chan = prev_chan[cur_idx]; + + /* we could optimize here by caching the idx of the channels in + * the path, but the bottleneck of the algorithm is the MCF + * computation not here. */ + const u32 chan_idx = gossmap_chan_idx(gossmap, chan); + + delta = MIN(delta, chan_flow[chan_idx].half[dir]); + length++; + } + + struct flow *f = tal(ctx, struct flow); + f->path = tal_arr(f, const struct gossmap_chan *, length); + f->dirs = tal_arr(f, int, length); + + /* Walk again and substract the flow value (delta). */ + assert(delta > 0); + balance[start_idx] += delta; + balance[final_idx] -= delta; + for (u32 cur_idx = final_idx; cur_idx != start_idx; + cur_idx = prev_idx[cur_idx]) { + const int dir = prev_dir[cur_idx]; + const struct gossmap_chan *const chan = prev_chan[cur_idx]; + const u32 chan_idx = gossmap_chan_idx(gossmap, chan); + + length--; + /* f->path and f->dirs contain the channels in the path in the + * correct order. */ + f->path[length] = chan; + f->dirs[length] = dir; + + chan_flow[chan_idx].half[dir] -= delta; + } + f->delivers = amount_msat(delta * 1000); + return f; +} + +/* Substract a flow cycle from the channel allocation. */ +static void substract_cycle(const struct gossmap *gossmap, const u32 final_idx, + struct chan_flow *chan_flow, const u32 *prev_idx, + const int *prev_dir, + const struct gossmap_chan *const *prev_chan) +{ + s64 delta = INFINITE; + u32 cur_idx; + + /* Compute greatest flow in this cycle. */ + for (cur_idx = final_idx; cur_idx!=INVALID_INDEX;) { + const int dir = prev_dir[cur_idx]; + const struct gossmap_chan *const chan = prev_chan[cur_idx]; + const u32 chan_idx = gossmap_chan_idx(gossmap, chan); + + delta = MIN(delta, chan_flow[chan_idx].half[dir]); + + cur_idx = prev_idx[cur_idx]; + if (cur_idx == final_idx) + /* we have come back full circle */ + break; + } + assert(cur_idx==final_idx); + + /* Walk again and substract the flow value (delta). */ + assert(delta < INFINITE); + assert(delta > 0); + + for (cur_idx = final_idx;cur_idx!=INVALID_INDEX;) { + const int dir = prev_dir[cur_idx]; + const struct gossmap_chan *const chan = prev_chan[cur_idx]; + const u32 chan_idx = gossmap_chan_idx(gossmap, chan); + + chan_flow[chan_idx].half[dir] -= delta; + + cur_idx = prev_idx[cur_idx]; + if (cur_idx == final_idx) + /* we have come back full circle */ + break; + } + assert(cur_idx==final_idx); +} + /* Given a flow in the residual network, build a set of payment flows in the * gossmap that corresponds to this flow. */ static struct flow ** @@ -1092,6 +1195,9 @@ get_flow_paths(const tal_t *ctx, int *prev_dir = tal_arr(tmpctx,int,max_num_nodes); u32 *prev_idx = tal_arr(tmpctx,u32,max_num_nodes); + for (u32 node_idx = 0; node_idx < max_num_nodes; node_idx++) + prev_idx[node_idx] = INVALID_INDEX; + // Convert the arc based residual network flow into a flow in the // directed channel network. // Compute balance on the nodes. @@ -1117,75 +1223,34 @@ get_flow_paths(const tal_t *ctx, } - // Select all nodes with negative balance and find a flow that reaches a // positive balance node. for(u32 node_idx=0;node_idxgossmap, chan_flow, node_idx, balance, prev_chan, prev_dir, prev_idx); - s64 delta=-balance[node_idx]; - int length = 0; - delta = MIN(delta,balance[final_idx]); - - // walk backwards, get me the length and the max flow we - // can send. - for(u32 cur_idx = final_idx; - cur_idx!=node_idx; - cur_idx=prev_idx[cur_idx]) + if (balance[final_idx] > 0) + /* case 1. found a path */ { - assert(cur_idx!=INVALID_INDEX); - - const int dir = prev_dir[cur_idx]; - const struct gossmap_chan *const c = prev_chan[cur_idx]; - const u32 c_idx = gossmap_chan_idx(rq->gossmap,c); - - delta=MIN(delta,chan_flow[c_idx].half[dir]); - length++; - } - - struct flow *fp = tal(flows,struct flow); - fp->path = tal_arr(fp,const struct gossmap_chan *,length); - fp->dirs = tal_arr(fp,int,length); - - balance[node_idx] += delta; - balance[final_idx]-= delta; - - // walk backwards, substract flow - for(u32 cur_idx = final_idx; - cur_idx!=node_idx; - cur_idx=prev_idx[cur_idx]) + struct flow *fp = substract_flow( + flows, rq->gossmap, node_idx, final_idx, + balance, chan_flow, prev_idx, prev_dir, + prev_chan); + + tal_arr_expand(&flows, fp); + } else + /* case 2. found a cycle */ { - assert(cur_idx!=INVALID_INDEX); - - const int dir = prev_dir[cur_idx]; - const struct gossmap_chan *const c = prev_chan[cur_idx]; - const u32 c_idx = gossmap_chan_idx(rq->gossmap,c); - - length--; - fp->path[length]=c; - fp->dirs[length]=dir; - // notice: fp->path and fp->dirs have the path - // in the correct order. - - chan_flow[c_idx].half[prev_dir[cur_idx]]-=delta; + substract_cycle(rq->gossmap, final_idx, + chan_flow, prev_idx, prev_dir, + prev_chan); } - - assert(delta>0); - fp->delivers = amount_msat(delta*1000); - - // add fp to flows - tal_arr_expand(&flows, fp); } } return flows; From f895a260c6b32e734e797d50bad7bc998fa09577 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:39 +1030 Subject: [PATCH 10/22] askrene: don't use tmpctx in minflow() I tested with a really large gossmap (hacked to be 4GB), and when we keep retrying to minimize cost (calling minflow 11 times), and we don't free tmpctx. Due to an issue with how gossmap estimates the index sizes, we ended up running out of memory. This fixes it. Signed-off-by: Rusty Russell --- plugins/askrene/mcf.c | 63 ++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 7e803470d47f..e60d8316e37e 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -688,14 +688,17 @@ init_linear_network(const tal_t *ctx, const struct pay_parameters *params) * residual network with capacity greater than 0. * The path is encoded into prev, which contains the idx of the arcs that are * traversed. */ + +/* Note we eschew tmpctx here, as this can be called multiple times! */ static bool -find_admissible_path(const struct linear_network *linear_network, +find_admissible_path(const tal_t *working_ctx, + const struct linear_network *linear_network, const struct residual_network *residual_network, const u32 source, const u32 target, struct arc *prev) { bool target_found = false; /* Simple linear queue of node indexes */ - u32 *queue = tal_arr(tmpctx, u32, linear_network->max_num_arcs); + u32 *queue = tal_arr(working_ctx, u32, linear_network->max_num_arcs); size_t qstart, qend, prev_len = tal_count(prev); for(size_t i=0;imax_num_nodes); + struct arc *prev = tal_arr(working_ctx,struct arc,linear_network->max_num_nodes); while(amount>0) { // find a path from source to target - if (!find_admissible_path(linear_network, + if (!find_admissible_path(working_ctx, + linear_network, residual_network, source, target, prev)) { return false; @@ -846,7 +851,8 @@ static bool find_feasible_flow(const struct linear_network *linear_network, // TODO(eduardo): unit test this /* Similar to `find_admissible_path` but use Dijkstra to optimize the distance * label. Stops when the target is hit. */ -static bool find_optimal_path(struct dijkstra *dijkstra, +static bool find_optimal_path(const tal_t *working_ctx, + struct dijkstra *dijkstra, const struct linear_network *linear_network, const struct residual_network *residual_network, const u32 source, const u32 target, @@ -854,7 +860,7 @@ static bool find_optimal_path(struct dijkstra *dijkstra, { bool target_found = false; - bitmap *visited = tal_arrz(tmpctx, bitmap, + bitmap *visited = tal_arrz(working_ctx, bitmap, BITMAP_NWORDS(linear_network->max_num_nodes)); for(size_t i=0;i=0); zero_flow(linear_network,residual_network); - struct arc *prev = tal_arr(tmpctx,struct arc,linear_network->max_num_nodes); + struct arc *prev = tal_arr(working_ctx,struct arc,linear_network->max_num_nodes); const s64 *const distance = dijkstra_distance_data(dijkstra); @@ -955,7 +962,7 @@ static bool optimize_mcf(struct dijkstra *dijkstra, while(remaining_amount>0) { - if (!find_optimal_path(dijkstra, linear_network, + if (!find_optimal_path(working_ctx, dijkstra, linear_network, residual_network, source, target, prev)) { return false; } @@ -1002,6 +1009,7 @@ struct chan_flow * or we discover a cycle (returns a node idx with 0 balance). * */ static u32 find_path_or_cycle( + const tal_t *working_ctx, const struct gossmap *gossmap, const struct chan_flow *chan_flow, const u32 start_idx, @@ -1013,7 +1021,7 @@ static u32 find_path_or_cycle( { const size_t max_num_nodes = gossmap_max_node_idx(gossmap); bitmap *visited = - tal_arrz(tmpctx, bitmap, BITMAP_NWORDS(max_num_nodes)); + tal_arrz(working_ctx, bitmap, BITMAP_NWORDS(max_num_nodes)); u32 final_idx = start_idx; bitmap_set_bit(visited, start_idx); @@ -1176,6 +1184,7 @@ static void substract_cycle(const struct gossmap *gossmap, const u32 final_idx, * gossmap that corresponds to this flow. */ static struct flow ** get_flow_paths(const tal_t *ctx, + const tal_t *working_ctx, const struct route_query *rq, const struct linear_network *linear_network, const struct residual_network *residual_network) @@ -1183,17 +1192,17 @@ get_flow_paths(const tal_t *ctx, struct flow **flows = tal_arr(ctx,struct flow*,0); const size_t max_num_chans = gossmap_max_chan_idx(rq->gossmap); - struct chan_flow *chan_flow = tal_arrz(tmpctx,struct chan_flow,max_num_chans); + struct chan_flow *chan_flow = tal_arrz(working_ctx,struct chan_flow,max_num_chans); const size_t max_num_nodes = gossmap_max_node_idx(rq->gossmap); - s64 *balance = tal_arrz(tmpctx,s64,max_num_nodes); + s64 *balance = tal_arrz(working_ctx,s64,max_num_nodes); const struct gossmap_chan **prev_chan - = tal_arr(tmpctx,const struct gossmap_chan *,max_num_nodes); + = tal_arr(working_ctx,const struct gossmap_chan *,max_num_nodes); - int *prev_dir = tal_arr(tmpctx,int,max_num_nodes); - u32 *prev_idx = tal_arr(tmpctx,u32,max_num_nodes); + int *prev_dir = tal_arr(working_ctx,int,max_num_nodes); + u32 *prev_idx = tal_arr(working_ctx,u32,max_num_nodes); for (u32 node_idx = 0; node_idx < max_num_nodes; node_idx++) prev_idx[node_idx] = INVALID_INDEX; @@ -1232,6 +1241,7 @@ get_flow_paths(const tal_t *ctx, { prev_chan[node_idx] = NULL; u32 final_idx = find_path_or_cycle( + working_ctx, rq->gossmap, chan_flow, node_idx, balance, prev_chan, prev_dir, prev_idx); @@ -1278,8 +1288,10 @@ struct flow **minflow(const tal_t *ctx, u32 prob_cost_factor) { struct flow **flow_paths; - - struct pay_parameters *params = tal(tmpctx,struct pay_parameters); + /* We allocate everything off this, and free it at the end, + * as we can be called multiple times without cleaning tmpctx! */ + tal_t *working_ctx = tal(NULL, char); + struct pay_parameters *params = tal(working_ctx, struct pay_parameters); struct dijkstra *dijkstra; params->rq = rq; @@ -1306,11 +1318,11 @@ struct flow **minflow(const tal_t *ctx, params->prob_cost_factor = prob_cost_factor; // build the uncertainty network with linearization and residual arcs - struct linear_network *linear_network= init_linear_network(tmpctx, params); + struct linear_network *linear_network= init_linear_network(working_ctx, params); struct residual_network *residual_network = - alloc_residual_network(tmpctx, linear_network->max_num_nodes, + alloc_residual_network(working_ctx, linear_network->max_num_nodes, linear_network->max_num_arcs); - dijkstra = dijkstra_new(tmpctx, gossmap_max_node_idx(rq->gossmap)); + dijkstra = dijkstra_new(working_ctx, gossmap_max_node_idx(rq->gossmap)); const u32 target_idx = gossmap_node_idx(rq->gossmap,target); const u32 source_idx = gossmap_node_idx(rq->gossmap,source); @@ -1333,23 +1345,26 @@ struct flow **minflow(const tal_t *ctx, * flow units. */ const u64 pay_amount_sats = (params->amount.millisatoshis + 999)/1000; /* Raw: minflow */ - if (!find_feasible_flow(linear_network, residual_network, + if (!find_feasible_flow(working_ctx, linear_network, residual_network, source_idx, target_idx, pay_amount_sats)) { + tal_free(working_ctx); return NULL; } combine_cost_function(linear_network, residual_network, mu); /* We solve a linear MCF problem. */ - if(!optimize_mcf(dijkstra,linear_network,residual_network, + if(!optimize_mcf(working_ctx, dijkstra,linear_network,residual_network, source_idx,target_idx,pay_amount_sats)) { + tal_free(working_ctx); return NULL; } /* We dissect the solution of the MCF into payment routes. * Actual amounts considering fees are computed for every * channel in the routes. */ - flow_paths = get_flow_paths(tmpctx, rq, + flow_paths = get_flow_paths(ctx, working_ctx, rq, linear_network, residual_network); + tal_free(working_ctx); return flow_paths; } From 3b019e2baa45d114e6ef7b1beaee760d0cfdd216 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:39 +1030 Subject: [PATCH 11/22] pytest: test askrene on real network data. I checked the failures, they seem real. Signed-off-by: Rusty Russell --- tests/test_askrene.py | 53 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/test_askrene.py b/tests/test_askrene.py index f533986e7368..7f17c1f60dfd 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -5,7 +5,9 @@ sync_blockheight, wait_for ) import pytest +import subprocess import time +import tempfile def direction(src, dst): @@ -882,3 +884,54 @@ def test_min_htlc_after_excess(node_factory, bitcoind): layers=[], maxfee_msat=20_000_000, final_cltv=10) + + +@pytest.mark.slow_test +def test_real_data(node_factory, bitcoind): + # Route from Rusty's node to the top 100. + # From tests/data/gossip-store-2024-09-22-node-map.xz: + # Me: 3301:024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605:BLUEIRON + # So we make l2 node 3301. + outfile = tempfile.NamedTemporaryFile(prefix='gossip-store-') + nodeids = subprocess.check_output(['devtools/gossmap-compress', + 'decompress', + '--node-map=3301=022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59', + 'tests/data/gossip-store-2024-09-22.compressed', + outfile.name]).decode('utf-8').splitlines() + + # l2 complains being given bad gossip from l1 + l1, l2 = node_factory.line_graph(2, + opts=[{'gossip_store_file': outfile.name, + 'allow_warning': True}, + {'allow_warning': True}]) + + # These were obviously having a bad day at the time of the snapshot: + badnodes = { + # 62:03dbe3fedd4f6e7f7020c69e6d01453d5a69f9faa1382901cf3028f1e997ef2814:BTC_👽👽👽👽👽👽 + 62: " marked disabled by gossip message", + # This one has 151 channels, with peers each only connected to it. An island! + # 76:0298906458987af756e2a43b208c03499c4d2bde630d4868dda0ea6a184f87c62a:0298906458987af756e2 + 76: "There is no connection between source and destination at all", + # 80:02d246c519845e7b23b02684d64ca23b750958e0307f9519849ee2535e3637999a:SLIMYRAGE- + 80: " marked disabled by gossip message", + # 97:034a5fdb2df3ce1bfd2c2aca205ce9cfeef1a5f4af21b0b5e81c453080c30d7683:🚶LightningTransact + 97: r"We could not find a usable set of paths\. The shortest path is 103x1x0->0x3301x1646->0x1281x2323->97x1281x33241, but 97x1281x33241/1 isn't big enough to carry 100000000msat\.", + } + for n in range(0, 100): + if n in badnodes: + with pytest.raises(RpcError, match=badnodes[n]): + l1.rpc.getroutes(source=l1.info['id'], + destination=nodeids[n], + amount_msat=100000000, + layers=['auto.sourcefree', 'auto.localchans'], + maxfee_msat=10000000, + final_cltv=18) + continue + + routes = l1.rpc.getroutes(source=l1.info['id'], + destination=nodeids[n], + amount_msat=100000000, + layers=['auto.sourcefree', 'auto.localchans'], + maxfee_msat=10000000, + final_cltv=18) + print(f"{n} route has {len(routes['routes'])} paths") From 302fde522015dadc9e3d085d616ab882353ed356 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:40 +1030 Subject: [PATCH 12/22] pytest: test askrene with worse maxfee argument. We ask it again, but reduce fees by 1msat from the previous answer. This is really nasty, as it frequently exercises the case where we only go over fee when we do the refinement step. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 3 ++ tests/test_askrene.py | 82 +++++++++++++++++++++++++++++++++------ 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index a035e83b4a8b..722f574c801c 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -433,6 +433,9 @@ static const char *get_routes(const tal_t *ctx, goto too_expensive; } + rq_log(tmpctx, rq, LOG_DBG, "Final answer has %zu flows with mu=%u", + tal_count(flows), mu); + /* Convert back into routes, with delay and other information fixed */ *routes = tal_arr(ctx, struct route *, tal_count(flows)); *amounts = tal_arr(ctx, struct amount_msat, tal_count(flows)); diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 7f17c1f60dfd..aa3db5805759 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -899,10 +899,15 @@ def test_real_data(node_factory, bitcoind): 'tests/data/gossip-store-2024-09-22.compressed', outfile.name]).decode('utf-8').splitlines() - # l2 complains being given bad gossip from l1 - l1, l2 = node_factory.line_graph(2, + # This is in msat, but is also the size of channel we create. + AMOUNT = 100000000 + + # l2 complains being given bad gossip from l1, throttle to reduce + # the sheer amount of log noise. + l1, l2 = node_factory.line_graph(2, fundamount=AMOUNT, opts=[{'gossip_store_file': outfile.name, - 'allow_warning': True}, + 'allow_warning': True, + 'dev-throttle-gossip': None}, {'allow_warning': True}]) # These were obviously having a bad day at the time of the snapshot: @@ -917,21 +922,74 @@ def test_real_data(node_factory, bitcoind): # 97:034a5fdb2df3ce1bfd2c2aca205ce9cfeef1a5f4af21b0b5e81c453080c30d7683:🚶LightningTransact 97: r"We could not find a usable set of paths\. The shortest path is 103x1x0->0x3301x1646->0x1281x2323->97x1281x33241, but 97x1281x33241/1 isn't big enough to carry 100000000msat\.", } + + fees = {} for n in range(0, 100): + print(f"XXX: {n}") + # 0.5% is the norm + MAX_FEE = AMOUNT // 200 + if n in badnodes: with pytest.raises(RpcError, match=badnodes[n]): l1.rpc.getroutes(source=l1.info['id'], destination=nodeids[n], - amount_msat=100000000, + amount_msat=AMOUNT, layers=['auto.sourcefree', 'auto.localchans'], - maxfee_msat=10000000, + maxfee_msat=MAX_FEE, final_cltv=18) + fees[n] = [] continue - routes = l1.rpc.getroutes(source=l1.info['id'], - destination=nodeids[n], - amount_msat=100000000, - layers=['auto.sourcefree', 'auto.localchans'], - maxfee_msat=10000000, - final_cltv=18) - print(f"{n} route has {len(routes['routes'])} paths") + try: + prev = l1.rpc.getroutes(source=l1.info['id'], + destination=nodeids[n], + amount_msat=AMOUNT, + layers=['auto.sourcefree', 'auto.localchans'], + maxfee_msat=MAX_FEE, + final_cltv=18) + except RpcError: + fees[n] = [] + continue + + # Now stress it, by asking it to spend 1msat less! + fees[n] = [sum([r['path'][0]['amount_msat'] for r in prev['routes']]) - AMOUNT] + + while True: + # Keep making it harder... + try: + routes = l1.rpc.getroutes(source=l1.info['id'], + destination=nodeids[n], + amount_msat=AMOUNT, + layers=['auto.sourcefree', 'auto.localchans'], + maxfee_msat=fees[n][-1] - 1, + final_cltv=18) + except RpcError: + break + + fee = sum([r['path'][0]['amount_msat'] for r in routes['routes']]) - AMOUNT + # Should get less expensive + assert fee < fees[n][-1] + + # Should get less likely (Note! This is violated because once we care + # about fees, the total is reduced, leading to better prob!). +# assert routes['probability_ppm'] < prev['probability_ppm'] + + fees[n].append(fee) + prev = routes + + # Which succeeded in improving + improved = [n for n in fees if len(fees[n]) > 1] + total_first_fee = sum([fees[n][0] for n in improved]) + total_final_fee = sum([fees[n][-1] for n in improved]) + + if total_first_fee != 0: + percent_fee_reduction = 100 - int(total_final_fee * 100 / total_first_fee) + else: + percent_fee_reduction = 0 + + best = 0 + for n in fees: + if len(fees[n]) > len(fees[best]): + best = n + + assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (9, 91, 20917688, 5254665, 75) From ff7dbdfd16569518ddc970980e79e3dbc1d82430 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:40 +1030 Subject: [PATCH 13/22] mcf: simplify mu -> cost translation. The current prob_cost_factor setting does not seem to make mu very effective, in fact, it gives strange results: plugin-cln-askrene: notify msg unusual: The flows had a fee of 151950msat, greater than max of 53697msat, retrying with mu of 10%... plugin-cln-askrene: notify msg unusual: The flows had a fee of 220126msat, greater than max of 53697msat, retrying with mu of 20%... We would expect increasing mu to *reduce* the fee! As a first step, simplify (it can't be infinite, and the -1 are weird). Signed-off-by: Rusty Russell --- plugins/askrene/mcf.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index e60d8316e37e..7a97b0a7ea5a 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -193,7 +193,7 @@ static const double CHANNEL_PIVOTS[]={0,0.5,0.8,0.95}; static const s64 INFINITE = INT64_MAX; static const u32 INVALID_INDEX = 0xffffffff; -static const s64 MU_MAX = 101; +static const s64 MU_MAX = 100; /* Let's try this encoding of arcs: * Each channel `c` has two possible directions identified by a bit @@ -524,14 +524,11 @@ static void combine_cost_function( continue; const s64 pcost = linear_network->arc_prob_cost[arc.idx], - fcost = linear_network->arc_fee_cost[arc.idx]; + fcost = linear_network->arc_fee_cost[arc.idx]; - const s64 combined = pcost==INFINITE || fcost==INFINITE ? INFINITE : - mu*fcost + (MU_MAX-1-mu)*pcost; - - residual_network->cost[arc.idx] - = mu==0 ? pcost : - (mu==(MU_MAX-1) ? fcost : combined); + assert(pcost != INFINITE); + assert(fcost != INFINITE); + residual_network->cost[arc.idx] = fcost*mu + (MU_MAX-mu)*pcost; } } From 514c2c4727218f404fde677aa1b121b5f905a159 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:40 +1030 Subject: [PATCH 14/22] askrene: calculate prob_cost_factor using ratio of typical mainnet channel. During "test_real_data", then only successes with reduced fees were 92 on "mu=10", and only 1 on "mu=30": the rest went to mu=100 and failed. I tried numerous approaches, and in the end, opted for the simplest: The typical range of probability costs looks likes: min = 0, max = 924196240, mean = 10509.4, stddev = 1.9e+06 The typical range of linear fee costs looks like: min = 0, max = 101000000, mean = 81894.6, stddev = 2.6e+06 This implies a k factor of 8 makes the two comparable. This makes the two numbers comparable, and thus makes "mu" much more effective. Here are the number of different mu values we succeeded at: 87 mu=0 90 mu=10 42 mu=20 24 mu=30 17 mu=40 19 mu=50 19 mu=60 11 mu=70 95 mu=80 19 mu=90 Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 22 +++---------- plugins/askrene/mcf.c | 67 ++++++++++----------------------------- plugins/askrene/mcf.h | 27 ++-------------- tests/test_askrene.py | 6 ++-- 4 files changed, 27 insertions(+), 95 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 722f574c801c..cace4bbb3145 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -285,7 +285,7 @@ static const char *get_routes(const tal_t *ctx, const struct gossmap_node *srcnode, *dstnode; double delay_feefactor; double base_fee_penalty; - u32 prob_cost_factor, mu; + u32 mu; const char *ret; if (gossmap_refresh(askrene->gossmap, NULL)) { @@ -351,22 +351,10 @@ static const char *get_routes(const tal_t *ctx, delay_feefactor = 1.0/1000000; base_fee_penalty = 10.0; - /* From mcf.c: The input parameter `prob_cost_factor` in the function - * `minflow` is defined as the PPM from the delivery amount `T` we are - * *willing to pay* to increase the prob. of success by 0.1% */ - - /* This value is somewhat implied by our fee budget: say we would pay - * the entire budget for 100% probability, that means prob_cost_factor - * is (fee / amount) / 1000, or in PPM: (fee / amount) * 1000 */ - if (amount_msat_is_zero(amount)) - prob_cost_factor = 0; - else - prob_cost_factor = amount_msat_ratio(maxfee, amount) * 1000; - /* First up, don't care about fees. */ mu = 0; flows = minflow(rq, rq, srcnode, dstnode, amount, - mu, delay_feefactor, base_fee_penalty, prob_cost_factor); + mu, delay_feefactor, base_fee_penalty); if (!flows) { ret = explain_failure(ctx, rq, srcnode, dstnode, amount); goto fail; @@ -386,7 +374,7 @@ static const char *get_routes(const tal_t *ctx, "The worst flow delay is %"PRIu64" (> %i), retrying with delay_feefactor %f...", flows_worst_delay(flows), 2016 - finalcltv, delay_feefactor); flows = minflow(rq, rq, srcnode, dstnode, amount, - mu, delay_feefactor, base_fee_penalty, prob_cost_factor); + mu, delay_feefactor, base_fee_penalty); if (!flows || delay_feefactor > 10) { ret = rq_log(ctx, rq, LOG_UNUSUAL, "Could not find route without excessive delays"); @@ -404,8 +392,8 @@ static const char *get_routes(const tal_t *ctx, fmt_amount_msat(tmpctx, maxfee), mu); flows = minflow(rq, rq, srcnode, dstnode, amount, - mu, delay_feefactor, base_fee_penalty, prob_cost_factor); - if (!flows || mu == 100) { + mu > 100 ? 100 : mu, delay_feefactor, base_fee_penalty); + if (!flows || mu >= 100) { ret = rq_log(ctx, rq, LOG_UNUSUAL, "Could not find route without excessive cost"); goto fail; diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 7a97b0a7ea5a..92988e27093f 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -134,38 +134,7 @@ * However we propose to scale the prob. cost by a global factor k that * translates into the monetization of prob. cost. * - * k/1000, for instance, becomes the equivalent monetary cost - * of increasing the probability of success by 0.1% for P~100%. - * - * The input parameter `prob_cost_factor` in the function `minflow` is defined - * as the PPM from the delivery amount `T` we are *willing to pay* to increase the - * prob. of success by 0.1%: - * - * k_microsat = floor(1000*prob_cost_factor * T_sat) - * - * Is this enough to make integer prob. cost per unit flow? - * For `prob_cost_factor=10`; i.e. we pay 10ppm for increasing the prob. by - * 0.1%, we get that - * - * -> any arc with (b-a) > 10000 T, will have zero prob. cost, which is - * reasonable because even if all the flow passes through that arc, we get - * a 1.3 T/(b-a) ~ 0.01% prob. of failure at most. - * - * -> if (b-a) ~ 10000 T, then the arc will have unit cost, or just that we - * pay 1 microsat for every sat we send through this arc. - * - * -> it would be desirable to have a high proportional fee when (b-a)~T, - * because prob. of failure start to become very high. - * In this case we get to pay 10000 microsats for every sat. - * - * Once `k` is fixed then we can combine the linear prob. and fee costs, both - * are in monetary units. - * - * Note: with costs in microsats, because slopes represent ppm and flows are in - * sats, then our integer bounds with 64 bits are such that we can move as many - * as 10'000 BTC without overflow: - * - * 10^6 (max ppm) * 10^8 (sats per BTC) * 10^4 = 10^18 + * This was chosen empirically from examination of typical network values. * * # References * @@ -324,7 +293,7 @@ struct pay_parameters { double delay_feefactor; double base_fee_penalty; - u32 prob_cost_factor; + double k_factor; }; /* Representation of the linear MCF network. @@ -470,7 +439,7 @@ static void linearize_channel(const struct pay_parameters *params, cost[i] = params->cost_fraction[i] *params->amount.millisatoshis /* Raw: linearize_channel */ - *params->prob_cost_factor*1.0/(b-a); + *params->k_factor/(b-a); } } @@ -561,17 +530,13 @@ static void linear_network_add_adjacenct_arc( * use `base_fee_penalty` to weight the base fee and `delay_feefactor` to * weight the CLTV delay. * */ -static s64 linear_fee_cost( - const struct gossmap_chan *c, - const int dir, - double base_fee_penalty, - double delay_feefactor) +static s64 linear_fee_cost(u32 base_fee, u32 proportional_fee, u16 cltv_delta, + double base_fee_penalty, + double delay_feefactor) { - assert(c); - assert(dir==0 || dir==1); - s64 pfee = c->half[dir].proportional_fee, - bfee = c->half[dir].base_fee, - delay = c->half[dir].delay; + s64 pfee = proportional_fee, + bfee = base_fee, + delay = cltv_delta; return pfee + bfee* base_fee_penalty+ delay*delay_feefactor; } @@ -644,9 +609,12 @@ init_linear_network(const tal_t *ctx, const struct pay_parameters *params) // that are outgoing to `node` linearize_channel(params, c, half, capacity, prob_cost); - const s64 fee_cost = linear_fee_cost(c,half, - params->base_fee_penalty, - params->delay_feefactor); + const s64 fee_cost = linear_fee_cost( + c->half[half].base_fee, + c->half[half].proportional_fee, + c->half[half].delay, + params->base_fee_penalty, + params->delay_feefactor); // let's subscribe the 4 parts of the channel direction // (c,half), the dual of these guys will be subscribed @@ -1281,8 +1249,7 @@ struct flow **minflow(const tal_t *ctx, const struct gossmap_node *target, struct amount_msat amount, u32 mu, - double delay_feefactor, double base_fee_penalty, - u32 prob_cost_factor) + double delay_feefactor, double base_fee_penalty) { struct flow **flow_paths; /* We allocate everything off this, and free it at the end, @@ -1312,7 +1279,7 @@ struct flow **minflow(const tal_t *ctx, params->delay_feefactor = delay_feefactor; params->base_fee_penalty = base_fee_penalty; - params->prob_cost_factor = prob_cost_factor; + params->k_factor = 8.0; // build the uncertainty network with linearization and residual arcs struct linear_network *linear_network= init_linear_network(working_ctx, params); diff --git a/plugins/askrene/mcf.h b/plugins/askrene/mcf.h index eae5bd506ab0..4f9e03e788ab 100644 --- a/plugins/askrene/mcf.h +++ b/plugins/askrene/mcf.h @@ -8,22 +8,10 @@ struct route_query; -enum { - RENEPAY_ERR_OK, - // No feasible flow found, either there is not enough known liquidity (or capacity) - // in the channels to complete the payment - RENEPAY_ERR_NOFEASIBLEFLOW, - // There is at least one feasible flow, but the the cheapest solution that we - // found is too expensive, we return the result anyways. - RENEPAY_ERR_NOCHEAPFLOW -}; - - - /** * optimal_payment_flow - API for min cost flow function(s). * @ctx: context to allocate returned flows from - * @gossmap: the gossip map + * @rq: the route_query we're processing (for logging) * @source: the source to start from * @target: the target to pay * @amount: the amount we want to reach @target @@ -39,16 +27,6 @@ enum { * * effective_ppm = proportional_fee + base_fee_msat * base_fee_penalty * - * @prob_cost_factor: factor used to monetize the probability cost. It is - * defined as the number of ppm (parts per million of the total payment) we - * are willing to pay to improve the probability of success by 0.1%. - * - * k_microsat = floor(1000*prob_cost_factor * payment_sat) - * - * this k is used to compute a prob. cost in units of microsats - * - * cost(payment) = - k_microsat * log Prob(payment) - * * Return a series of subflows which deliver amount to target, or NULL. */ struct flow **minflow(const tal_t *ctx, @@ -58,6 +36,5 @@ struct flow **minflow(const tal_t *ctx, struct amount_msat amount, u32 mu, double delay_feefactor, - double base_fee_penalty, - u32 prob_cost_factor); + double base_fee_penalty); #endif /* LIGHTNING_PLUGINS_ASKRENE_MCF_H */ diff --git a/tests/test_askrene.py b/tests/test_askrene.py index aa3db5805759..e0fe4c961117 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -433,11 +433,11 @@ def test_getroutes(node_factory): 10000000, [[{'short_channel_id_dir': '0x2x1/1', 'next_node_id': nodemap[2], - 'amount_msat': 500000, + 'amount_msat': 4500004, 'delay': 99 + 6}], [{'short_channel_id_dir': '0x2x3/1', 'next_node_id': nodemap[2], - 'amount_msat': 9500009, + 'amount_msat': 5500005, 'delay': 99 + 6}]]) @@ -992,4 +992,4 @@ def test_real_data(node_factory, bitcoind): if len(fees[n]) > len(fees[best]): best = n - assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (9, 91, 20917688, 5254665, 75) + assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (10, 96, 19969585, 801613, 96) From 545146ff3738630743df642849361f010affff89 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:40 +1030 Subject: [PATCH 15/22] askrene: fix base fee. I noticed this in the logs: plugin-cln-askrene: notify msg unusual: The flows had a fee of 151950msat, greater than max of 53697msat, retrying with mu of 10%... plugin-cln-askrene: notify msg unusual: The flows had a fee of 220126msat, greater than max of 53697msat, retrying with mu of 20%... We would expect increasing mu to *reduce* the fee! Turns out that our linear fee is a bad terrible approximation, because I was using base_fee_penalty of 10.0. | | / __ <- real fee, with base: fee = base + propfee * amount. | / __/ | _// | __/ | __/_/ |/ _/ | _/ <- linearized fee: fee = linear * amount |/ +----------------------------------- These cross over where linear = propfee + base / amount. Assume we split the payment into 10 parts, this implies that the base_fee_penalty should be 10 / amount (this gives a slight penalty to the normal case, but that's ok). This gives better results, too: we get down to 650099 sats in fees, vs 801613 before. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 8 +++----- plugins/askrene/mcf.c | 12 ++++++++++-- plugins/askrene/mcf.h | 9 +-------- tests/test_askrene.py | 2 +- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index cace4bbb3145..6b59cad304d6 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -284,7 +284,6 @@ static const char *get_routes(const tal_t *ctx, struct flow **flows; const struct gossmap_node *srcnode, *dstnode; double delay_feefactor; - double base_fee_penalty; u32 mu; const char *ret; @@ -349,12 +348,11 @@ static const char *get_routes(const tal_t *ctx, } delay_feefactor = 1.0/1000000; - base_fee_penalty = 10.0; /* First up, don't care about fees. */ mu = 0; flows = minflow(rq, rq, srcnode, dstnode, amount, - mu, delay_feefactor, base_fee_penalty); + mu, delay_feefactor); if (!flows) { ret = explain_failure(ctx, rq, srcnode, dstnode, amount); goto fail; @@ -374,7 +372,7 @@ static const char *get_routes(const tal_t *ctx, "The worst flow delay is %"PRIu64" (> %i), retrying with delay_feefactor %f...", flows_worst_delay(flows), 2016 - finalcltv, delay_feefactor); flows = minflow(rq, rq, srcnode, dstnode, amount, - mu, delay_feefactor, base_fee_penalty); + mu, delay_feefactor); if (!flows || delay_feefactor > 10) { ret = rq_log(ctx, rq, LOG_UNUSUAL, "Could not find route without excessive delays"); @@ -392,7 +390,7 @@ static const char *get_routes(const tal_t *ctx, fmt_amount_msat(tmpctx, maxfee), mu); flows = minflow(rq, rq, srcnode, dstnode, amount, - mu > 100 ? 100 : mu, delay_feefactor, base_fee_penalty); + mu > 100 ? 100 : mu, delay_feefactor); if (!flows || mu >= 100) { ret = rq_log(ctx, rq, LOG_UNUSUAL, "Could not find route without excessive cost"); diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 92988e27093f..76f50225367e 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -541,6 +541,14 @@ static s64 linear_fee_cost(u32 base_fee, u32 proportional_fee, u16 cltv_delta, return pfee + bfee* base_fee_penalty+ delay*delay_feefactor; } +/* This is inversely proportional to the amount we expect to send. Let's + * assume we will send ~10th of the total amount per path. But note + * that it converts to parts per million! */ +static double base_fee_penalty_estimate(struct amount_msat amount) +{ + return amount_msat_ratio(AMOUNT_MSAT(10000000), amount); +} + static struct linear_network * init_linear_network(const tal_t *ctx, const struct pay_parameters *params) { @@ -1249,7 +1257,7 @@ struct flow **minflow(const tal_t *ctx, const struct gossmap_node *target, struct amount_msat amount, u32 mu, - double delay_feefactor, double base_fee_penalty) + double delay_feefactor) { struct flow **flow_paths; /* We allocate everything off this, and free it at the end, @@ -1278,7 +1286,7 @@ struct flow **minflow(const tal_t *ctx, } params->delay_feefactor = delay_feefactor; - params->base_fee_penalty = base_fee_penalty; + params->base_fee_penalty = base_fee_penalty_estimate(amount); params->k_factor = 8.0; // build the uncertainty network with linearization and residual arcs diff --git a/plugins/askrene/mcf.h b/plugins/askrene/mcf.h index 4f9e03e788ab..2b2a885a1b33 100644 --- a/plugins/askrene/mcf.h +++ b/plugins/askrene/mcf.h @@ -21,12 +21,6 @@ struct route_query; * fee. So if a CLTV delay on a node is 5 blocks, that's treated as if it * were a fee of 5 * @delay_feefactor. * - * @base_fee_penalty: factor to compute additional proportional cost from each - * unit of base fee. So #base_fee_penalty will be added to the effective - * proportional fee for each msat of base fee. - * - * effective_ppm = proportional_fee + base_fee_msat * base_fee_penalty - * * Return a series of subflows which deliver amount to target, or NULL. */ struct flow **minflow(const tal_t *ctx, @@ -35,6 +29,5 @@ struct flow **minflow(const tal_t *ctx, const struct gossmap_node *target, struct amount_msat amount, u32 mu, - double delay_feefactor, - double base_fee_penalty); + double delay_feefactor); #endif /* LIGHTNING_PLUGINS_ASKRENE_MCF_H */ diff --git a/tests/test_askrene.py b/tests/test_askrene.py index e0fe4c961117..58aa2c9f4bd2 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -992,4 +992,4 @@ def test_real_data(node_factory, bitcoind): if len(fees[n]) > len(fees[best]): best = n - assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (10, 96, 19969585, 801613, 96) + assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (9, 96, 19961361, 650099, 97) From 5763c7d50089d0807f1c9cd88b6aadb6ffc8b006 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 11 Oct 2024 21:30:40 +1030 Subject: [PATCH 16/22] askrene: debug and check we actually reduce fees when mu increase. Even after the previous fix, we still occasionally increase fees when my increases. This is due to the difference between MCF's linear fees, and actual fees, and is unavoidable, but add a check if it somehow happens. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 81 ++++++++++++++++++++++++++++++++++++++- plugins/askrene/mcf.c | 20 ++++++++++ plugins/askrene/mcf.h | 7 ++++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 6b59cad304d6..e7e993a2508d 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -263,6 +263,56 @@ static const char *fmt_route(const tal_t *ctx, return str; } +static const char *fmt_flow_full(const tal_t *ctx, + const struct route_query *rq, + const struct flow *flow, + struct amount_msat total_delivered, + double delay_feefactor) +{ + struct amount_msat amt = flow->delivers; + char *str = tal_fmt(ctx, "%s (linear cost %s)", + fmt_amount_msat(tmpctx, amt), + fmt_amount_msat(tmpctx, linear_flow_cost(flow, + total_delivered, + delay_feefactor))); + + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + struct short_channel_id_dir scidd; + struct amount_msat min, max; + scidd.scid = gossmap_chan_scid(rq->gossmap, flow->path[i]); + scidd.dir = flow->dirs[i]; + if (!amount_msat_add_fee(&amt, + flow->path[i]->half[scidd.dir].base_fee, + flow->path[i]->half[scidd.dir].proportional_fee)) + abort(); + get_constraints(rq, flow->path[i], scidd.dir, &min, &max); + tal_append_fmt(&str, " <- %s %s (cap=%s,fee=%u+%u,delay=%u)", + fmt_amount_msat(tmpctx, amt), + fmt_short_channel_id_dir(tmpctx, &scidd), + fmt_amount_msat(tmpctx, max), + flow->path[i]->half[scidd.dir].base_fee, + flow->path[i]->half[scidd.dir].proportional_fee, + flow->path[i]->half[scidd.dir].delay); + } + return str; +} + +static struct amount_msat linear_flows_cost(struct flow **flows, + struct amount_msat total_amount, + double delay_feefactor) +{ + struct amount_msat total = AMOUNT_MSAT(0); + + for (size_t i = 0; i < tal_count(flows); i++) { + if (!amount_msat_accumulate(&total, + linear_flow_cost(flows[i], + total_amount, + delay_feefactor))) + abort(); + } + return total; +} + /* Returns an error message, or sets *routes */ static const char *get_routes(const tal_t *ctx, struct command *cmd, @@ -383,19 +433,46 @@ static const char *get_routes(const tal_t *ctx, /* Too expensive? */ too_expensive: while (amount_msat_greater(flowset_fee(rq->plugin, flows), maxfee)) { + struct flow **new_flows; + mu += 10; rq_log(tmpctx, rq, LOG_UNUSUAL, "The flows had a fee of %s, greater than max of %s, retrying with mu of %u%%...", fmt_amount_msat(tmpctx, flowset_fee(rq->plugin, flows)), fmt_amount_msat(tmpctx, maxfee), mu); - flows = minflow(rq, rq, srcnode, dstnode, amount, - mu > 100 ? 100 : mu, delay_feefactor); + new_flows = minflow(rq, rq, srcnode, dstnode, amount, + mu > 100 ? 100 : mu, delay_feefactor); if (!flows || mu >= 100) { ret = rq_log(ctx, rq, LOG_UNUSUAL, "Could not find route without excessive cost"); goto fail; } + + /* This is possible, because MCF's linear fees are not the same. */ + if (amount_msat_greater(flowset_fee(rq->plugin, new_flows), + flowset_fee(rq->plugin, flows))) { + struct amount_msat old_cost = linear_flows_cost(flows, amount, delay_feefactor); + struct amount_msat new_cost = linear_flows_cost(new_flows, amount, delay_feefactor); + if (amount_msat_greater_eq(new_cost, old_cost)) { + rq_log(tmpctx, rq, LOG_BROKEN, "Old flows cost %s:", + fmt_amount_msat(tmpctx, old_cost)); + for (size_t i = 0; i < tal_count(flows); i++) { + rq_log(tmpctx, rq, LOG_BROKEN, + "Flow %zu/%zu: %s", i, tal_count(flows), + fmt_flow_full(tmpctx, rq, flows[i], amount, delay_feefactor)); + } + rq_log(tmpctx, rq, LOG_BROKEN, "Old flows cost %s:", + fmt_amount_msat(tmpctx, new_cost)); + for (size_t i = 0; i < tal_count(new_flows); i++) { + rq_log(tmpctx, rq, LOG_BROKEN, + "Flow %zu/%zu: %s", i, tal_count(new_flows), + fmt_flow_full(tmpctx, rq, new_flows[i], amount, delay_feefactor)); + } + } + } + tal_free(flows); + flows = new_flows; } if (finalcltv + flows_worst_delay(flows) > 2016) { diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 76f50225367e..4de5a9282f20 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -549,6 +549,26 @@ static double base_fee_penalty_estimate(struct amount_msat amount) return amount_msat_ratio(AMOUNT_MSAT(10000000), amount); } +struct amount_msat linear_flow_cost(const struct flow *flow, + struct amount_msat total_amount, + double delay_feefactor) +{ + struct amount_msat msat_cost; + s64 cost = 0; + double base_fee_penalty = base_fee_penalty_estimate(total_amount); + + for (size_t i = 0; i < tal_count(flow->path); i++) { + const struct half_chan *h = &flow->path[i]->half[flow->dirs[i]]; + + cost += linear_fee_cost(h->base_fee, h ->proportional_fee, h->delay, + base_fee_penalty, delay_feefactor); + } + + if (!amount_msat_mul(&msat_cost, flow->delivers, cost)) + abort(); + return msat_cost; +} + static struct linear_network * init_linear_network(const tal_t *ctx, const struct pay_parameters *params) { diff --git a/plugins/askrene/mcf.h b/plugins/askrene/mcf.h index 2b2a885a1b33..2a1b8fcf0cd2 100644 --- a/plugins/askrene/mcf.h +++ b/plugins/askrene/mcf.h @@ -30,4 +30,11 @@ struct flow **minflow(const tal_t *ctx, struct amount_msat amount, u32 mu, double delay_feefactor); + +/* To sanity check: this is the approximation mcf uses for the cost + * of each channel. */ +struct amount_msat linear_flow_cost(const struct flow *flow, + struct amount_msat total_amount, + double delay_feefactor); + #endif /* LIGHTNING_PLUGINS_ASKRENE_MCF_H */ From 6786294ed265fe89d6539f28fd2426d90e7d8e72 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 12 Oct 2024 12:50:41 +1030 Subject: [PATCH 17/22] askrene: calculate `k` value dynamically, using medians. While the `k=8` value worked for the current main network tests with the amounts in those tests, it wasn't robust across a wider range of values (as demonstrated when other test changes broke tests!). Time to do this properly: calculate the ratio at the time we combine them, using median values. Signed-off-by: Rusty Russell --- plugins/askrene/mcf.c | 77 +++++++++++++++++++++++++++++++++++-------- tests/test_askrene.py | 2 +- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 4de5a9282f20..36eff2b8b480 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -1,10 +1,12 @@ #include "config.h" #include +#include #include #include #include #include #include +#include #include #include #include @@ -293,7 +295,6 @@ struct pay_parameters { double delay_feefactor; double base_fee_penalty; - double k_factor; }; /* Representation of the linear MCF network. @@ -311,7 +312,8 @@ struct linear_network struct arc *node_adjacency_first_arc; // probability and fee cost associated to an arc - s64 *arc_prob_cost, *arc_fee_cost; + double *arc_prob_cost; + s64 *arc_fee_cost; s64 *capacity; size_t max_num_arcs,max_num_nodes; @@ -413,7 +415,7 @@ static void set_capacity(s64 *capacity, u64 value, u64 *cap_on_capacity) /* Split a directed channel into parts with linear cost function. */ static void linearize_channel(const struct pay_parameters *params, const struct gossmap_chan *c, const int dir, - s64 *capacity, s64 *cost) + s64 *capacity, double *cost) { struct amount_msat mincap, maxcap; @@ -439,7 +441,7 @@ static void linearize_channel(const struct pay_parameters *params, cost[i] = params->cost_fraction[i] *params->amount.millisatoshis /* Raw: linearize_channel */ - *params->k_factor/(b-a); + /(b-a); } } @@ -482,22 +484,71 @@ static void init_residual_network( } } +static int cmp_u64(const u64 *a, const u64 *b, void *unused) +{ + if (*a < *b) + return -1; + if (*a > *b) + return 1; + return 0; +} + +static int cmp_double(const double *a, const double *b, void *unused) +{ + if (*a < *b) + return -1; + if (*a > *b) + return 1; + return 0; +} + +static double get_median_ratio(const tal_t *working_ctx, + const struct linear_network* linear_network) +{ + u64 *u64_arr = tal_arr(working_ctx, u64, linear_network->max_num_arcs/2); + double *double_arr = tal_arr(working_ctx, double, linear_network->max_num_arcs/2); + size_t n = 0; + + for (struct arc arc = {0};arc.idx < linear_network->max_num_arcs; ++arc.idx) { + if (arc_is_dual(arc)) + continue; + assert(n < linear_network->max_num_arcs/2); + u64_arr[n] = linear_network->arc_fee_cost[arc.idx]; + double_arr[n] = linear_network->arc_prob_cost[arc.idx]; + n++; + } + asort(u64_arr, n, cmp_u64, NULL); + asort(double_arr, n, cmp_double, NULL); + + /* Empty network, or tiny probability, nobody cares */ + if (n == 0 || double_arr[n/2] < 0.001) + return 1; + + /* You need to scale arc_prob_cost by this to match arc_fee_cost */ + return u64_arr[n/2] / double_arr[n/2]; +} + static void combine_cost_function( + const tal_t *working_ctx, const struct linear_network* linear_network, struct residual_network *residual_network, s64 mu) { + /* probabilty and fee costs are not directly comparable! + * Scale by ratio of (positive) medians. */ + const double k = get_median_ratio(working_ctx, linear_network); + for(struct arc arc = {0};arc.idx < linear_network->max_num_arcs; ++arc.idx) { if(arc_tail(linear_network,arc)==INVALID_INDEX) continue; - const s64 pcost = linear_network->arc_prob_cost[arc.idx], - fcost = linear_network->arc_fee_cost[arc.idx]; + const double pcost = linear_network->arc_prob_cost[arc.idx]; + const s64 fcost = linear_network->arc_fee_cost[arc.idx]; - assert(pcost != INFINITE); assert(fcost != INFINITE); - residual_network->cost[arc.idx] = fcost*mu + (MU_MAX-mu)*pcost; + assert(pcost != DBL_MAX); + residual_network->cost[arc.idx] = fcost*mu + (MU_MAX-mu)*pcost*k; } } @@ -594,9 +645,9 @@ init_linear_network(const tal_t *ctx, const struct pay_parameters *params) for(size_t i=0;inode_adjacency_first_arc[i].idx=INVALID_INDEX; - linear_network->arc_prob_cost = tal_arr(linear_network,s64,max_num_arcs); + linear_network->arc_prob_cost = tal_arr(linear_network,double,max_num_arcs); for(size_t i=0;iarc_prob_cost[i]=INFINITE; + linear_network->arc_prob_cost[i]=DBL_MAX; linear_network->arc_fee_cost = tal_arr(linear_network,s64,max_num_arcs); for(size_t i=0;idelay_feefactor = delay_feefactor; params->base_fee_penalty = base_fee_penalty_estimate(amount); - params->k_factor = 8.0; // build the uncertainty network with linearization and residual arcs struct linear_network *linear_network= init_linear_network(working_ctx, params); @@ -1342,7 +1393,7 @@ struct flow **minflow(const tal_t *ctx, tal_free(working_ctx); return NULL; } - combine_cost_function(linear_network, residual_network, mu); + combine_cost_function(working_ctx, linear_network, residual_network, mu); /* We solve a linear MCF problem. */ if(!optimize_mcf(working_ctx, dijkstra,linear_network,residual_network, diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 58aa2c9f4bd2..3b1b61808b8a 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -992,4 +992,4 @@ def test_real_data(node_factory, bitcoind): if len(fees[n]) > len(fees[best]): best = n - assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (9, 96, 19961361, 650099, 97) + assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (8, 95, 19384459, 564997, 98) From 948466e5b498862a584326d895e9895a4a905bbe Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 12 Oct 2024 17:23:42 +1030 Subject: [PATCH 18/22] askrene: don't *completely* ignore fees to start. I noticed that increasing mu a little bit sometimes made a big difference, because by completely ignoring fees we were choosing the worst of two channels in some cases. Start at 1% fees; this saves a lot on initial fees in this test! Here's the new stats on mu levels: 96 mu=1 90 mu=10 41 mu=20 30 mu=30 24 mu=40 19 mu=50 22 mu=60 8 mu=70 95 mu=80 19 mu=90 Signed-off-by: Rusty Russell Changelog-EXPERIMENTAL: `askrene` is now better at finding low-fee paths. --- plugins/askrene/askrene.c | 9 ++++++--- tests/test_askrene.py | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index e7e993a2508d..a5ad545ba9eb 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -399,8 +399,8 @@ static const char *get_routes(const tal_t *ctx, delay_feefactor = 1.0/1000000; - /* First up, don't care about fees. */ - mu = 0; + /* First up, don't care about fees (well, just enough to tiebreak!) */ + mu = 1; flows = minflow(rq, rq, srcnode, dstnode, amount, mu, delay_feefactor); if (!flows) { @@ -435,7 +435,10 @@ static const char *get_routes(const tal_t *ctx, while (amount_msat_greater(flowset_fee(rq->plugin, flows), maxfee)) { struct flow **new_flows; - mu += 10; + if (mu == 1) + mu = 10; + else + mu += 10; rq_log(tmpctx, rq, LOG_UNUSUAL, "The flows had a fee of %s, greater than max of %s, retrying with mu of %u%%...", fmt_amount_msat(tmpctx, flowset_fee(rq->plugin, flows)), diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 3b1b61808b8a..d403d6e33a60 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -992,4 +992,4 @@ def test_real_data(node_factory, bitcoind): if len(fees[n]) > len(fees[best]): best = n - assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (8, 95, 19384459, 564997, 98) + assert (len(fees[best]), len(improved), total_first_fee, total_final_fee, percent_fee_reduction) == (8, 95, 6007785, 564997, 91) From 8b83d1237a842112497b80c945569ecd94db677a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 12 Oct 2024 17:23:54 +1030 Subject: [PATCH 19/22] askrene: fix bug with reservations used during refinement. We were trying to get the max capacity of a flow to see if we could add some more sats, and hit an assertion: tests/test_askrene.py:707: ``` DEBUG plugin-cln-askrene: notify msg info: Flow reduced to deliver 88070161msat not 90008000msat, because 107x1x0/1 has remaining capacity 88071042msat DEBUG plugin-cln-askrene: notify msg info: Flow reduced to deliver 284138158msat not 284787000msat, because 108x1x0/1 has remaining capacity 284141000msat **BROKEN** plugin-cln-askrene: Flow delivers 129565000msat but max only 56506138msat INFO plugin-cln-askrene: Killing plugin: exited during normal operation ``` We need to *unreserve* our flow before asking for max capacity. We were also missing a few less important cases where we altered flows without altering the reservation, so fix those too. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 241 ++++++++++++++++++++++++--------------- 1 file changed, 151 insertions(+), 90 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index b02a7217a304..1232f21d7243 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -36,25 +36,43 @@ static struct reserve_hop *new_reservations(const tal_t *ctx, return rhops; } -/* Add reservation: we (ab)use this to temporarily avoid over-usage as +static struct reserve_hop *find_reservation(struct reserve_hop *rhops, + const struct short_channel_id_dir *scidd) +{ + for (size_t i = 0; i < tal_count(rhops); i++) { + if (short_channel_id_dir_eq(scidd, &rhops[i].scidd)) + return &rhops[i]; + } + return NULL; +} + +/* Add/update reservation: we (ab)use this to temporarily avoid over-usage as * we refine. */ static void add_reservation(struct reserve_hop **reservations, - struct route_query *rq, - const struct flow *flow, - size_t i, + const struct route_query *rq, + const struct gossmap_chan *chan, + const struct short_channel_id_dir *scidd, struct amount_msat amt) { - struct reserve_hop rhop; + struct reserve_hop rhop, *prev; struct askrene *askrene = get_askrene(rq->plugin); size_t idx; - get_scidd(rq->gossmap, flow, i, &rhop.scidd); + /* Update in-place if possible */ + prev = find_reservation(*reservations, scidd); + if (prev) { + reserve_remove(askrene->reserved, prev); + if (!amount_msat_accumulate(&prev->amount, amt)) + abort(); + reserve_add(askrene->reserved, prev, rq->cmd->id); + return; + } + rhop.scidd = *scidd; rhop.amount = amt; - reserve_add(askrene->reserved, &rhop, rq->cmd->id); /* Set capacities entry to 0 so it get_constraints() looks in reserve. */ - idx = gossmap_chan_idx(rq->gossmap, flow->path[i]); + idx = gossmap_chan_idx(rq->gossmap, chan); if (idx < tal_count(rq->capacities)) rq->capacities[idx] = 0; @@ -62,8 +80,88 @@ static void add_reservation(struct reserve_hop **reservations, tal_arr_expand(reservations, rhop); } -/* We aren't allowed to ask for update_details on locally-generated channels, - * so go to the source in that case */ +static void subtract_reservation(struct reserve_hop **reservations, + const struct route_query *rq, + const struct gossmap_chan *chan, + const struct short_channel_id_dir *scidd, + struct amount_msat amt) +{ + struct reserve_hop *prev; + struct askrene *askrene = get_askrene(rq->plugin); + + prev = find_reservation(*reservations, scidd); + assert(prev); + + reserve_remove(askrene->reserved, prev); + if (!amount_msat_sub(&prev->amount, prev->amount, amt)) + abort(); + /* Adding a zero reserve is weird, but legal and easy! */ + reserve_add(askrene->reserved, prev, rq->cmd->id); +} + +static void create_flow_reservations(const struct route_query *rq, + struct reserve_hop **reservations, + const struct flow *flow) +{ + struct amount_msat msat; + + msat = flow->delivers; + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat amount_to_reserve; + struct short_channel_id_dir scidd; + + get_scidd(rq->gossmap, flow, i, &scidd); + + /* Reserve more for local channels if it reduces capacity */ + if (!amount_msat_add(&amount_to_reserve, msat, + get_additional_per_htlc_cost(rq, &scidd))) + abort(); + + add_reservation(reservations, rq, flow->path[i], &scidd, + amount_to_reserve); + if (!amount_msat_add_fee(&msat, + h->base_fee, h->proportional_fee)) + plugin_err(rq->plugin, "Adding fee to amount"); + } +} + +static void remove_flow_reservations(const struct route_query *rq, + struct reserve_hop **reservations, + const struct flow *flow) +{ + struct amount_msat msat = flow->delivers; + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat amount_to_reserve; + struct short_channel_id_dir scidd; + + get_scidd(rq->gossmap, flow, i, &scidd); + + /* Reserve more for local channels if it reduces capacity */ + if (!amount_msat_add(&amount_to_reserve, msat, + get_additional_per_htlc_cost(rq, &scidd))) + abort(); + + subtract_reservation(reservations, rq, flow->path[i], &scidd, + amount_to_reserve); + if (!amount_msat_add_fee(&msat, + h->base_fee, h->proportional_fee)) + plugin_err(rq->plugin, "Adding fee to amount"); + } +} + +static void change_flow_delivers(const struct route_query *rq, + struct flow *flow, + struct reserve_hop **reservations, + struct amount_msat new) +{ + remove_flow_reservations(rq, reservations, flow); + flow->delivers = new; + create_flow_reservations(rq, reservations, flow); +} + +/* We use an fp16_t approximatin for htlc_max/min: this gets the exact value. */ static struct amount_msat get_chan_htlc_max(const struct route_query *rq, const struct gossmap_chan *c, const struct short_channel_id_dir *scidd) @@ -162,8 +260,7 @@ static enum why_capped flow_max_capacity(const struct route_query *rq, */ static const char *constrain_flow(const tal_t *ctx, struct route_query *rq, - struct flow *flow, - struct reserve_hop **reservations) + struct flow *flow) { struct amount_msat deliverable, msat, amount_capped; enum why_capped why_capped; @@ -205,66 +302,20 @@ static const char *constrain_flow(const tal_t *ctx, h->base_fee, h->proportional_fee)) plugin_err(rq->plugin, "Adding fee to amount"); } - - /* Finally, reserve so next flow sees reduced capacity. */ - msat = flow->delivers; - for (int i = tal_count(flow->path) - 1; i >= 0; i--) { - const struct half_chan *h = flow_edge(flow, i); - struct amount_msat amount_to_reserve; - struct short_channel_id_dir scidd; - - get_scidd(rq->gossmap, flow, i, &scidd); - - /* Reserve more for local channels if it reduces capacity */ - if (!amount_msat_add(&amount_to_reserve, msat, - get_additional_per_htlc_cost(rq, &scidd))) - abort(); - - add_reservation(reservations, rq, flow, i, amount_to_reserve); - if (!amount_msat_add_fee(&msat, - h->base_fee, h->proportional_fee)) - plugin_err(rq->plugin, "Adding fee to amount"); - } - return NULL; } -/* Flow is now delivering `extra` additional msat, so modify reservations */ -static void add_to_flow(struct flow *flow, - struct route_query *rq, - struct reserve_hop **reservations, - struct amount_msat extra) -{ - struct amount_msat orig, updated; - - orig = flow->delivers; - if (!amount_msat_add(&updated, orig, extra)) - abort(); - - flow->delivers = updated; - - /* Now add reservations accordingly (effects constraints on other flows) */ - for (int i = tal_count(flow->path) - 1; i >= 0; i--) { - const struct half_chan *h = flow_edge(flow, i); - struct amount_msat diff; - - /* Can't happen, since updated >= orig */ - if (!amount_msat_sub(&diff, updated, orig)) - abort(); - add_reservation(reservations, rq, flow, i, diff); - - if (!amount_msat_add_fee(&orig, h->base_fee, h->proportional_fee)) - abort(); - if (!amount_msat_add_fee(&updated, h->base_fee, h->proportional_fee)) - abort(); - } -} - static struct amount_msat flow_remaining_capacity(const struct route_query *rq, + struct reserve_hop **reservations, const struct flow *flow) { struct amount_msat max, diff; + + /* Remove ourselves from reservation temporarily, so we don't + * accidentally cap! */ + remove_flow_reservations(rq, reservations, flow); flow_max_capacity(rq, flow, &max, NULL, NULL); + create_flow_reservations(rq, reservations, flow); if (!amount_msat_sub(&diff, max, flow->delivers)) plugin_err(rq->plugin, "Flow delivers %s but max only %s", @@ -277,6 +328,7 @@ static struct amount_msat flow_remaining_capacity(const struct route_query *rq, /* What's the "best" flow to add to? */ static struct flow *pick_most_likely_flow(const struct route_query *rq, struct flow **flows, + struct reserve_hop **reservations, struct amount_msat additional) { double best_prob = 0; @@ -287,7 +339,7 @@ static struct flow *pick_most_likely_flow(const struct route_query *rq, double prob = flow_probability(flows[i], rq); if (prob < best_prob) continue; - cap = flow_remaining_capacity(rq, flows[i]); + cap = flow_remaining_capacity(rq, reservations, flows[i]); if (amount_msat_less(cap, additional)) continue; best_prob = prob; @@ -327,11 +379,12 @@ static const char *flow_violates_min(const tal_t *ctx, * duplicate it. This is naive: it could still fail due to total * capacity, but it is a corner case anyway. */ static bool duplicate_one_flow(const struct route_query *rq, + struct reserve_hop **reservations, struct flow ***flows) { for (size_t i = 0; i < tal_count(*flows); i++) { struct flow *flow = (*flows)[i], *new_flow; - struct amount_msat max; + struct amount_msat max, new_amount; if (flow_max_capacity(rq, flow, &max, NULL, NULL) != CAPPED_HTLC_MAX) continue; @@ -343,9 +396,12 @@ static bool duplicate_one_flow(const struct route_query *rq, new_flow->dirs = tal_dup_talarr(new_flow, int, flow->dirs); new_flow->delivers = amount_msat_div(flow->delivers, 2); - if (!amount_msat_sub(&flow->delivers, + create_flow_reservations(rq, reservations, new_flow); + + if (!amount_msat_sub(&new_amount, flow->delivers, new_flow->delivers)) abort(); + change_flow_delivers(rq, flow, reservations, new_amount); tal_arr_expand(flows, new_flow); return true; } @@ -367,8 +423,9 @@ refine_with_fees_and_limits(const tal_t *ctx, for (size_t i = 0; i < tal_count(*flows);) { struct flow *flow = (*flows)[i]; - flow_constraint_error = constrain_flow(tmpctx, rq, flow, &reservations); + flow_constraint_error = constrain_flow(tmpctx, rq, flow); if (!flow_constraint_error) { + create_flow_reservations(rq, &reservations, flow); i++; continue; } @@ -389,24 +446,27 @@ refine_with_fees_and_limits(const tal_t *ctx, deliver)) abort(); for (size_t i = 0; i < tal_count(*flows); i++) { - if (amount_msat_sub(&(*flows)[i]->delivers, (*flows)[i]->delivers, excess)) { - const char *err; - rq_log(tmpctx, rq, LOG_DBG, - "Flow %zu/%zu delivered %s extra, trimming", - i, tal_count(*flows), - fmt_amount_msat(tmpctx, excess)); - /* In theory, this can violate min_htlc! Thanks @Lagrang3! */ - err = flow_violates_min(tmpctx, rq, (*flows)[i]); - if (err) { - /* This flow was reduced to 0 / impossible, remove */ - tal_arr_remove(flows, i); - i--; - /* If this causes failure, indicate why! */ - flow_constraint_error = err; - continue; - } - break; + const char *err; + struct amount_msat trimmed; + if (!amount_msat_sub(&trimmed, (*flows)[i]->delivers, excess)) + continue; + + rq_log(tmpctx, rq, LOG_DBG, + "%s extra, trimming flow %zu/%zu", + fmt_amount_msat(tmpctx, excess), i, tal_count(*flows)); + change_flow_delivers(rq, (*flows)[i], &reservations, trimmed); + /* In theory, this can violate min_htlc! Thanks @Lagrang3! */ + err = flow_violates_min(tmpctx, rq, (*flows)[i]); + if (err) { + /* This flow was reduced to 0 / impossible, remove */ + remove_flow_reservations(rq, &reservations, (*flows)[i]); + tal_arr_remove(flows, i); + i--; + /* If this causes failure, indicate why! */ + flow_constraint_error = err; + continue; } + break; } /* Usually this should shed excess, *BUT* maybe one @@ -430,7 +490,7 @@ refine_with_fees_and_limits(const tal_t *ctx, * into the number of flows, then assign each one. */ while (!amount_msat_is_zero(more_to_deliver) && tal_count(*flows)) { struct flow *f; - struct amount_msat extra; + struct amount_msat extra, new_delivers; /* How much more do we deliver? Round up if we can */ extra = amount_msat_div(more_to_deliver, tal_count(*flows)); @@ -442,9 +502,9 @@ refine_with_fees_and_limits(const tal_t *ctx, /* This happens when we have a single flow, and hit * htlc_max. For this corner case, we split into an * additional flow, but only once! */ - f = pick_most_likely_flow(rq, *flows, extra); + f = pick_most_likely_flow(rq, *flows, &reservations, extra); if (!f) { - if (tried_split_flow || !duplicate_one_flow(rq, flows)) { + if (tried_split_flow || !duplicate_one_flow(rq, &reservations, flows)) { ret = rq_log(ctx, rq, LOG_BROKEN, "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", fmt_amount_msat(tmpctx, more_to_deliver)); @@ -454,8 +514,9 @@ refine_with_fees_and_limits(const tal_t *ctx, continue; } - /* Make this flow deliver +extra, and modify reservations */ - add_to_flow(f, rq, &reservations, extra); + if (!amount_msat_add(&new_delivers, f->delivers, extra)) + abort(); + change_flow_delivers(rq, f, &reservations, new_delivers); /* Should not happen, since extra comes from div... */ if (!amount_msat_sub(&more_to_deliver, more_to_deliver, extra)) From 2fd767c03fd802e0ca4c4ab861de74a098608e33 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 13 Oct 2024 09:47:37 +1030 Subject: [PATCH 20/22] askrene: use refine step to calculate flowset probability. Since we know the total reservations on each hop, we can more easily determine probabilities than using flowset_probability() which has to replicate this collision detection. We leave both in place for now, to check. The results are not identical, due to slightly different calculation methods. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 8 +++++- plugins/askrene/flow.c | 10 ++++++- plugins/askrene/refine.c | 55 ++++++++++++++++++++++++++++++++++++++- plugins/askrene/refine.h | 3 ++- 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index a5ad545ba9eb..2697053225fc 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -336,6 +337,7 @@ static const char *get_routes(const tal_t *ctx, double delay_feefactor; u32 mu; const char *ret; + double flowset_prob; if (gossmap_refresh(askrene->gossmap, NULL)) { /* FIXME: gossmap_refresh callbacks to we can update in place */ @@ -488,7 +490,7 @@ static const char *get_routes(const tal_t *ctx, * fees, so we try to adjust now. We could re-run MCF if this * fails, but failure basically never happens where payment is * still possible */ - ret = refine_with_fees_and_limits(ctx, rq, amount, &flows); + ret = refine_with_fees_and_limits(ctx, rq, amount, &flows, &flowset_prob); if (ret) goto fail; @@ -541,6 +543,10 @@ static const char *get_routes(const tal_t *ctx, } *probability = flowset_probability(flows, rq); + if (fabs(*probability - flowset_prob) > 0.000001) { + rq_log(tmpctx, rq, LOG_BROKEN, "Probability %f != expected %f", + *probability, flowset_prob); + } gossmap_remove_localmods(askrene->gossmap, localmods); return NULL; diff --git a/plugins/askrene/flow.c b/plugins/askrene/flow.c index f2bb09096423..31212f029bc5 100644 --- a/plugins/askrene/flow.c +++ b/plugins/askrene/flow.c @@ -130,7 +130,15 @@ double flowset_probability(struct flow **flows, const struct amount_msat deliver = amounts[j]; get_constraints(rq, f->path[j], c_dir, &mincap, &maxcap); - + struct short_channel_id_dir scidd; + scidd.scid = gossmap_chan_scid(rq->gossmap, f->path[j]); + scidd.dir = c_dir; + + rq_log(tmpctx, rq, LOG_DBG, "flow: edge_probability for %s: min=%s, max=%s, sent=%s", + fmt_short_channel_id_dir(tmpctx, &scidd), + fmt_amount_msat(tmpctx, mincap), + fmt_amount_msat(tmpctx, maxcap), + fmt_amount_msat(tmpctx, in_flight[c_idx].half[c_dir])); prob *= edge_probability(deliver, mincap, maxcap, in_flight[c_idx].half[c_dir]); diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 1232f21d7243..d66b4e4d8b5b 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -408,11 +408,53 @@ static bool duplicate_one_flow(const struct route_query *rq, return false; } +/* Stolen whole-cloth from @Lagrang3 in renepay's flow.c. Wrong + * because of htlc overhead in reservations! */ +static double edge_probability(const struct route_query *rq, + const struct short_channel_id_dir *scidd, + struct amount_msat sent) +{ + struct amount_msat numerator, denominator; + struct amount_msat mincap, maxcap, additional; + const struct gossmap_chan *c = gossmap_find_chan(rq->gossmap, &scidd->scid); + + get_constraints(rq, c, scidd->dir, &mincap, &maxcap); + + /* We add an extra per-htlc reservation for the *next* HTLC, so we "over-reserve" + * on local channels. Undo that! */ + additional = get_additional_per_htlc_cost(rq, scidd); + if (!amount_msat_accumulate(&mincap, additional) + || !amount_msat_accumulate(&maxcap, additional)) + abort(); + + rq_log(tmpctx, rq, LOG_DBG, "refine: edge_probability for %s: min=%s, max=%s, sent=%s", + fmt_short_channel_id_dir(tmpctx, scidd), + fmt_amount_msat(tmpctx, mincap), + fmt_amount_msat(tmpctx, maxcap), + fmt_amount_msat(tmpctx, sent)); + if (amount_msat_less_eq(sent, mincap)) + return 1.0; + else if (amount_msat_greater(sent, maxcap)) + return 0.0; + + /* Linear probability: 1 - (spend - min) / (max - min) */ + + /* spend > mincap, from above. */ + if (!amount_msat_sub(&numerator, sent, mincap)) + abort(); + /* This can only fail is maxcap was < mincap, + * so we would be captured above */ + if (!amount_msat_sub(&denominator, maxcap, mincap)) + abort(); + return 1.0 - amount_msat_ratio(numerator, denominator); +} + const char * refine_with_fees_and_limits(const tal_t *ctx, struct route_query *rq, struct amount_msat deliver, - struct flow ***flows) + struct flow ***flows, + double *flowset_probability) { struct reserve_hop *reservations = new_reservations(NULL, rq); struct amount_msat more_to_deliver; @@ -536,6 +578,17 @@ refine_with_fees_and_limits(const tal_t *ctx, ret = NULL; + /* Total flowset probability is now easily calculated given reservations + * contains the total amounts through each channel (once we remove them) */ + destroy_reservations(reservations, get_askrene(rq->plugin)); + tal_add_destructor2(reservations, destroy_reservations, get_askrene(rq->plugin)); + + *flowset_probability = 1.0; + for (size_t i = 0; i < tal_count(reservations); i++) { + const struct reserve_hop *r = &reservations[i]; + *flowset_probability *= edge_probability(rq, &r->scidd, r->amount); + } + out: tal_free(reservations); return ret; diff --git a/plugins/askrene/refine.h b/plugins/askrene/refine.h index 0ccc4809e1dd..66befec9b1e1 100644 --- a/plugins/askrene/refine.h +++ b/plugins/askrene/refine.h @@ -21,5 +21,6 @@ const char * refine_with_fees_and_limits(const tal_t *ctx, struct route_query *rq, struct amount_msat deliver, - struct flow ***flows); + struct flow ***flows, + double *flowset_probability); #endif /* LIGHTNING_PLUGINS_ASKRENE_REFINE_H */ From 2eb37d75aa041c287d22c5e776efc564910d245c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 13 Oct 2024 09:52:45 +1030 Subject: [PATCH 21/22] askrene: remove flowset_probability() now refine step calculates it. Now we've checked it gives the same answers, we can remove a lot of work in flow.c. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 9 +--- plugins/askrene/flow.c | 92 --------------------------------------- plugins/askrene/flow.h | 4 -- plugins/askrene/refine.c | 5 --- 4 files changed, 1 insertion(+), 109 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 2697053225fc..8bb8dc97a18e 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -16,7 +16,6 @@ #include #include #include -#include #include #include #include @@ -337,7 +336,6 @@ static const char *get_routes(const tal_t *ctx, double delay_feefactor; u32 mu; const char *ret; - double flowset_prob; if (gossmap_refresh(askrene->gossmap, NULL)) { /* FIXME: gossmap_refresh callbacks to we can update in place */ @@ -490,7 +488,7 @@ static const char *get_routes(const tal_t *ctx, * fees, so we try to adjust now. We could re-run MCF if this * fails, but failure basically never happens where payment is * still possible */ - ret = refine_with_fees_and_limits(ctx, rq, amount, &flows, &flowset_prob); + ret = refine_with_fees_and_limits(ctx, rq, amount, &flows, probability); if (ret) goto fail; @@ -542,11 +540,6 @@ static const char *get_routes(const tal_t *ctx, fmt_route(tmpctx, r, (*amounts)[i], finalcltv)); } - *probability = flowset_probability(flows, rq); - if (fabs(*probability - flowset_prob) > 0.000001) { - rq_log(tmpctx, rq, LOG_BROKEN, "Probability %f != expected %f", - *probability, flowset_prob); - } gossmap_remove_localmods(askrene->gossmap, localmods); return NULL; diff --git a/plugins/askrene/flow.c b/plugins/askrene/flow.c index 31212f029bc5..f6a7ac1f7e34 100644 --- a/plugins/askrene/flow.c +++ b/plugins/askrene/flow.c @@ -16,29 +16,6 @@ #define SUPERVERBOSE_ENABLED 1 #endif -static struct amount_msat *flow_amounts(const tal_t *ctx, - struct plugin *plugin, - const struct flow *flow) -{ - const size_t pathlen = tal_count(flow->path); - struct amount_msat *amounts = tal_arr(ctx, struct amount_msat, pathlen); - amounts[pathlen - 1] = flow->delivers; - - for (int i = (int)pathlen - 2; i >= 0; i--) { - const struct half_chan *h = flow_edge(flow, i + 1); - amounts[i] = amounts[i + 1]; - if (!amount_msat_add_fee(&amounts[i], h->base_fee, - h->proportional_fee)) { - plugin_err(plugin, "Could not add fee %u/%u to amount %s in %i/%zu", - h->base_fee, h->proportional_fee, - fmt_amount_msat(tmpctx, amounts[i+1]), - i, pathlen); - } - } - - return amounts; -} - /* How much do we deliver to destination using this set of routes */ struct amount_msat flowset_delivers(struct plugin *plugin, struct flow **flows) @@ -84,75 +61,6 @@ static double edge_probability(struct amount_msat sent, return 1.0 - amount_msat_ratio(numerator, denominator); } -/* Compute the prob. of success of a set of concurrent set of flows. - * - * IMPORTANT: this is not simply the multiplication of the prob. of success of - * all of them, because they're not independent events. A flow that passes - * through a channel c changes that channel's liquidity and then if another flow - * passes through that same channel the previous liquidity change must be taken - * into account. - * - * P(A and B) != P(A) * P(B), - * - * but - * - * P(A and B) = P(A) * P(B | A) - * - * also due to the linear form of P() we have - * - * P(A and B) = P(A + B) - * */ -struct chan_inflight_flow -{ - struct amount_msat half[2]; -}; - -double flowset_probability(struct flow **flows, - const struct route_query *rq) -{ - double prob = 1.0; - - // TODO(eduardo): should it be better to use a map instead of an array - // here? - const size_t max_num_chans = gossmap_max_chan_idx(rq->gossmap); - struct chan_inflight_flow *in_flight = - tal_arrz(tmpctx, struct chan_inflight_flow, max_num_chans); - - for (size_t i = 0; i < tal_count(flows); ++i) { - const struct flow *f = flows[i]; - const size_t pathlen = tal_count(f->path); - struct amount_msat *amounts = flow_amounts(tmpctx, rq->plugin, f); - - for (size_t j = 0; j < pathlen; ++j) { - struct amount_msat mincap, maxcap; - const int c_dir = f->dirs[j]; - const u32 c_idx = gossmap_chan_idx(rq->gossmap, f->path[j]); - const struct amount_msat deliver = amounts[j]; - - get_constraints(rq, f->path[j], c_dir, &mincap, &maxcap); - struct short_channel_id_dir scidd; - scidd.scid = gossmap_chan_scid(rq->gossmap, f->path[j]); - scidd.dir = c_dir; - - rq_log(tmpctx, rq, LOG_DBG, "flow: edge_probability for %s: min=%s, max=%s, sent=%s", - fmt_short_channel_id_dir(tmpctx, &scidd), - fmt_amount_msat(tmpctx, mincap), - fmt_amount_msat(tmpctx, maxcap), - fmt_amount_msat(tmpctx, in_flight[c_idx].half[c_dir])); - prob *= edge_probability(deliver, mincap, maxcap, - in_flight[c_idx].half[c_dir]); - - if (!amount_msat_accumulate(&in_flight[c_idx].half[c_dir], - deliver)) { - plugin_err(rq->plugin, "Could not add %s to inflight %s", - fmt_amount_msat(tmpctx, deliver), - fmt_amount_msat(tmpctx, in_flight[c_idx].half[c_dir])); - } - } - } - return prob; -} - struct amount_msat flow_spend(struct plugin *plugin, const struct flow *flow) { const size_t pathlen = tal_count(flow->path); diff --git a/plugins/askrene/flow.h b/plugins/askrene/flow.h index 4a9006566c8d..44de4312efdf 100644 --- a/plugins/askrene/flow.h +++ b/plugins/askrene/flow.h @@ -39,10 +39,6 @@ double flow_edge_cost(const struct gossmap *gossmap, double flow_probability(const struct flow *flow, const struct route_query *rq); -/* Compute the prob. of success of a set of concurrent set of flows. */ -double flowset_probability(struct flow **flows, - const struct route_query *rq); - /* How much do we need to send to make this flow arrive. */ struct amount_msat flow_spend(struct plugin *plugin, const struct flow *flow); diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index d66b4e4d8b5b..d52ab076bda0 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -427,11 +427,6 @@ static double edge_probability(const struct route_query *rq, || !amount_msat_accumulate(&maxcap, additional)) abort(); - rq_log(tmpctx, rq, LOG_DBG, "refine: edge_probability for %s: min=%s, max=%s, sent=%s", - fmt_short_channel_id_dir(tmpctx, scidd), - fmt_amount_msat(tmpctx, mincap), - fmt_amount_msat(tmpctx, maxcap), - fmt_amount_msat(tmpctx, sent)); if (amount_msat_less_eq(sent, mincap)) return 1.0; else if (amount_msat_greater(sent, maxcap)) From 2d3c00496b540f9bd35740bb840d586627e6bf66 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 13 Oct 2024 09:53:45 +1030 Subject: [PATCH 22/22] askrene: more logging in explain_failure. Lagrang3 doesn't like the logging in here at all, but he suggested we at least be consistent! Signed-off-by: Rusty Russell --- plugins/askrene/explain_failure.c | 46 +++++++++++++++---------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/plugins/askrene/explain_failure.c b/plugins/askrene/explain_failure.c index d4cd7cb01854..626be3fe9191 100644 --- a/plugins/askrene/explain_failure.c +++ b/plugins/askrene/explain_failure.c @@ -133,33 +133,33 @@ static const char *check_capacity(const tal_t *ctx, node_stats(rq, node, node_direction, &stats); if (amount_msat_greater(amount, stats.total.capacity)) { - return tal_fmt(ctx, - NO_USABLE_PATHS_STRING - " Total %s capacity is only %s" - " (in %zu channels).", - name, - fmt_amount_msat(tmpctx, stats.total.capacity), - stats.total.num_channels); + return rq_log(ctx, rq, LOG_DBG, + NO_USABLE_PATHS_STRING + " Total %s capacity is only %s" + " (in %zu channels).", + name, + fmt_amount_msat(tmpctx, stats.total.capacity), + stats.total.num_channels); } if (amount_msat_greater(amount, stats.gossip_known.capacity)) { - return tal_fmt(ctx, - NO_USABLE_PATHS_STRING - " Missing gossip for %s: only known %zu/%zu channels, leaving capacity only %s of %s.", - name, - stats.gossip_known.num_channels, - stats.total.num_channels, - fmt_amount_msat(tmpctx, stats.gossip_known.capacity), - fmt_amount_msat(tmpctx, stats.total.capacity)); + return rq_log(ctx, rq, LOG_DBG, + NO_USABLE_PATHS_STRING + " Missing gossip for %s: only known %zu/%zu channels, leaving capacity only %s of %s.", + name, + stats.gossip_known.num_channels, + stats.total.num_channels, + fmt_amount_msat(tmpctx, stats.gossip_known.capacity), + fmt_amount_msat(tmpctx, stats.total.capacity)); } if (amount_msat_greater(amount, stats.enabled.capacity)) { - return tal_fmt(ctx, - NO_USABLE_PATHS_STRING - " The %s has disabled %zu of %zu channels, leaving capacity only %s of %s.", - name, - stats.total.num_channels - stats.enabled.num_channels, - stats.total.num_channels, - fmt_amount_msat(tmpctx, stats.enabled.capacity), - fmt_amount_msat(tmpctx, stats.total.capacity)); + return rq_log(ctx, rq, LOG_DBG, + NO_USABLE_PATHS_STRING + " The %s has disabled %zu of %zu channels, leaving capacity only %s of %s.", + name, + stats.total.num_channels - stats.enabled.num_channels, + stats.total.num_channels, + fmt_amount_msat(tmpctx, stats.enabled.capacity), + fmt_amount_msat(tmpctx, stats.total.capacity)); } return NULL; }