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 <[email protected]>

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

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,16 +348,14 @@ 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);
 
 			/* Disabled zero-capacity on incoming */
 			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;

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);

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,