From b8acd3b37ca2680ee0a988837f3d3c56b17e2885 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 4 Oct 2024 09:17:12 +0930 Subject: [PATCH] askrene: more code tweaks on feedback from Lagrang3. 1. describe_disabled should point out if node itself is disabled. 2. Hoist constraint check for neater if branching. 3. Use amount_msat_max/min for greater clarity. 4. Simply disable channels, don't zero htlc_min/max when node disabled. I also fixed the diagnostic of htlc_max correctly, which removes a FIXME. Signed-off-by: Rusty Russell --- plugins/askrene/explain_failure.c | 49 ++++++++++++++++--------------- plugins/askrene/layer.c | 24 +++++++++------ plugins/askrene/layer.h | 5 +++- tests/test_askrene.py | 3 +- 4 files changed, 45 insertions(+), 36 deletions(-) diff --git a/plugins/askrene/explain_failure.c b/plugins/askrene/explain_failure.c index 1369a88d2ea1..b8ac907d132d 100644 --- a/plugins/askrene/explain_failure.c +++ b/plugins/askrene/explain_failure.c @@ -69,16 +69,6 @@ static const char *why_max_constrained(const tal_t *ctx, tal_append_fmt(&ret, "already reserved %s", reservations); } - /* If that doesn't explain it, perhaps it violates htlc_max? */ - if (!ret) { - struct gossmap_chan *c = gossmap_find_chan(rq->gossmap, &scidd->scid); - fp16_t htlc_max = c->half[scidd->dir].htlc_max; - if (amount_msat_greater_fp16(amount, htlc_max)) - ret = tal_fmt(ctx, "exceeds htlc_maximum_msat ~%s", - fmt_amount_msat(tmpctx, - amount_msat(fp16_to_u64(htlc_max)))); - } - /* This seems unlikely, but don't return NULL. */ if (!ret) ret = tal_fmt(ctx, "is constrained"); @@ -176,11 +166,19 @@ static const char *check_capacity(const tal_t *ctx, /* Return description of why scidd is disabled scidd */ static const char *describe_disabled(const tal_t *ctx, - const struct route_query *rq, - const struct short_channel_id_dir *scidd) + const struct route_query *rq, + const struct gossmap_chan *c, + const struct short_channel_id_dir *scidd) { for (int i = tal_count(rq->layers) - 1; i >= 0; i--) { - if (layer_disables(rq->layers[i], scidd)) { + struct gossmap_node *dst = gossmap_nth_node(rq->gossmap, c, !scidd->dir); + struct node_id dstid; + + gossmap_node_get_id(rq->gossmap, dst, &dstid); + if (layer_disables_node(rq->layers[i], &dstid)) + return tal_fmt(ctx, "leads to node disabled by layer %s.", + layer_name(rq->layers[i])); + else if (layer_disables_chan(rq->layers[i], scidd)) { return tal_fmt(ctx, "marked disabled by layer %s.", layer_name(rq->layers[i])); } @@ -253,27 +251,30 @@ const char *explain_failure(const tal_t *ctx, const char *explanation; struct short_channel_id_dir scidd; struct gossmap_chan *c; - struct amount_msat cap_msat; + struct amount_msat cap_msat, min, max, htlc_max; scidd.scid = hops[i].scid; scidd.dir = hops[i].direction; c = gossmap_find_chan(rq->gossmap, &scidd.scid); cap_msat = gossmap_chan_get_capacity(rq->gossmap, c); + get_constraints(rq, c, scidd.dir, &min, &max); + htlc_max = amount_msat(fp16_to_u64(c->half[scidd.dir].htlc_max)); + if (!gossmap_chan_set(c, scidd.dir)) explanation = "has no gossip"; else if (!c->half[scidd.dir].enabled) - explanation = describe_disabled(tmpctx, rq, &scidd); + explanation = describe_disabled(tmpctx, rq, c, &scidd); else if (amount_msat_greater(amount, cap_msat)) explanation = describe_capacity(tmpctx, rq, &scidd, amount); - else { - struct amount_msat min, max; - get_constraints(rq, c, scidd.dir, &min, &max); - if (amount_msat_less(max, amount)) { - explanation = why_max_constrained(tmpctx, rq, - &scidd, amount); - } else - continue; - } + else if (amount_msat_greater(amount, max)) + explanation = why_max_constrained(tmpctx, rq, + &scidd, amount); + else if (amount_msat_greater(amount, htlc_max)) + explanation = tal_fmt(tmpctx, + "exceeds htlc_maximum_msat ~%s", + fmt_amount_msat(tmpctx, htlc_max)); + else + continue; return tal_fmt(ctx, NO_USABLE_PATHS_STRING diff --git a/plugins/askrene/layer.c b/plugins/askrene/layer.c index 26d602853812..17cadc8e8983 100644 --- a/plugins/askrene/layer.c +++ b/plugins/askrene/layer.c @@ -256,10 +256,8 @@ void layer_apply_constraints(const struct layer *layer, for (c = constraint_hash_getfirst(layer->constraints, scidd, &cit); c; c = constraint_hash_getnext(layer->constraints, scidd, &cit)) { - if (amount_msat_greater(c->min, *min)) - *min = c->min; - if (amount_msat_less(c->max, *max)) - *max = c->max; + *min = amount_msat_max(*min, c->min); + *max = amount_msat_min(*max, c->max); } } @@ -350,7 +348,6 @@ void layer_add_localmods(const struct layer *layer, struct short_channel_id_dir scidd; struct gossmap_chan *c; bool enabled = false; - struct amount_msat zero = AMOUNT_MSAT(0); c = gossmap_nth_chan(gossmap, node, n, &scidd.dir); scidd.scid = gossmap_chan_scid(gossmap, c); @@ -358,8 +355,7 @@ void layer_add_localmods(const struct layer *layer, gossmap_local_updatechan(localmods, &scidd, &enabled, - &zero, &zero, - NULL, NULL, NULL); + NULL, NULL, NULL, NULL, NULL); } } @@ -507,8 +503,8 @@ bool layer_created(const struct layer *layer, struct short_channel_id scid) return local_channel_hash_get(layer->local_channels, scid); } -bool layer_disables(const struct layer *layer, - const struct short_channel_id_dir *scidd) +bool layer_disables_chan(const struct layer *layer, + const struct short_channel_id_dir *scidd) { const struct local_update *lu; @@ -517,6 +513,16 @@ bool layer_disables(const struct layer *layer, return (lu && lu->enabled && *lu->enabled == false); } +bool layer_disables_node(const struct layer *layer, + const struct node_id *node) +{ + for (size_t i = 0; i < tal_count(layer->disabled_nodes); i++) { + if (node_id_eq(&layer->disabled_nodes[i], node)) + return true; + } + return false; +} + void layer_memleak_mark(struct askrene *askrene, struct htable *memtable) { struct layer *l; diff --git a/plugins/askrene/layer.h b/plugins/askrene/layer.h index b8e700fdc46e..0bac5744a62b 100644 --- a/plugins/askrene/layer.h +++ b/plugins/askrene/layer.h @@ -106,7 +106,10 @@ void json_add_constraint(struct json_stream *js, bool layer_created(const struct layer *layer, struct short_channel_id scid); /* For explain_failure: did this layer disable this channel? */ -bool layer_disables(const struct layer *layer, const struct short_channel_id_dir *scidd); +bool layer_disables_chan(const struct layer *layer, const struct short_channel_id_dir *scidd); + +/* For explain_failure: did this layer disable this node? */ +bool layer_disables_node(const struct layer *layer, const struct node_id *node); /* Scan for memleaks */ void layer_memleak_mark(struct askrene *askrene, struct htable *memtable); diff --git a/tests/test_askrene.py b/tests/test_askrene.py index fe065c1e0e5b..5a7e7abe7c2a 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -845,8 +845,7 @@ def test_max_htlc(node_factory, bitcoind): amount_msat=1, inform='constrained') - # FIXME: Better diag! - with pytest.raises(RpcError, match="Could not find route"): + with pytest.raises(RpcError, match="We could not find a usable set of paths. The shortest path is 0x1x0, but 0x1x0/1 exceeds htlc_maximum_msat ~1000448msat"): l1.rpc.getroutes(source=nodemap[0], destination=nodemap[1], amount_msat=20_000_000,