Fix askrene 0 amount crash (and hopefully stop xpay calling that!) and over capacity crash

common/daemon.c

@@ -32,6 +32,9 @@ void send_backtrace(const char *why)
 	if (backtrace_state)
 		backtrace_print(backtrace_state, 0, stderr);
 
+	if (!bt_print)
+		return;
+
 	/* Now send to parent. */
 	bt_print("%s (version %s)", why, version());
 	if (backtrace_state)
@@ -75,7 +78,8 @@ static void crashdump(int sig)
 	send_backtrace(why);
 
 	/* Probably shouldn't return. */
-	bt_exit();
+	if (bt_exit)
+		bt_exit();
 
 	/* This time it will kill us instantly. */
 	kill(getpid(), sig);

plugins/askrene/askrene.c

@@ -265,18 +265,12 @@ 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)
+const char *fmt_flow_full(const tal_t *ctx,
+			  const struct route_query *rq,
+			  const struct flow *flow)
 {
 	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)));
+	char *str = fmt_amount_msat(ctx, flow->delivers);
 
 	for (int i = tal_count(flow->path) - 1; i >= 0; i--) {
 		struct short_channel_id_dir scidd;
@@ -465,15 +459,21 @@ static const char *get_routes(const tal_t *ctx,
 				       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));
+					       "Flow %zu/%zu: %s (linear cost %s)", i, tal_count(flows),
+					       fmt_flow_full(tmpctx, rq, flows[i]),
+					       fmt_amount_msat(tmpctx, linear_flow_cost(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));
+					       "Flow %zu/%zu: %s (linear cost %s)", i, tal_count(new_flows),
+					       fmt_flow_full(tmpctx, rq, new_flows[i]),
+					       fmt_amount_msat(tmpctx, linear_flow_cost(new_flows[i],
+											amount,
+											delay_feefactor)));
 				}
 			}
 		}
@@ -755,18 +755,26 @@ static struct command_result *json_getroutes(struct command *cmd,
 	const u32 maxdelay_allowed = 2016;
 	struct getroutes_info *info = tal(cmd, struct getroutes_info);
 
-	if (!param(cmd, buffer, params,
-		   p_req("source", param_node_id, &info->source),
-		   p_req("destination", param_node_id, &info->dest),
-		   p_req("amount_msat", param_msat, &info->amount),
-		   p_req("layers", param_layer_names, &info->layers),
-		   p_req("maxfee_msat", param_msat, &info->maxfee),
-		   p_req("final_cltv", param_u32, &info->finalcltv),
-		   p_opt_def("maxdelay", param_u32, &info->maxdelay,
-			     maxdelay_allowed),
-		   NULL))
+	if (!param_check(cmd, buffer, params,
+			 p_req("source", param_node_id, &info->source),
+			 p_req("destination", param_node_id, &info->dest),
+			 p_req("amount_msat", param_msat, &info->amount),
+			 p_req("layers", param_layer_names, &info->layers),
+			 p_req("maxfee_msat", param_msat, &info->maxfee),
+			 p_req("final_cltv", param_u32, &info->finalcltv),
+			 p_opt_def("maxdelay", param_u32, &info->maxdelay,
+				   maxdelay_allowed),
+			 NULL))
 		return command_param_failed();
 
+	if (amount_msat_is_zero(*info->amount)) {
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+				    "amount must be non-zero");
+	}
+
+	if (command_check_only(cmd))
+		return command_check_done(cmd);
+
 	if (*info->maxdelay > maxdelay_allowed) {
 		return command_fail(cmd, PAY_USER_ERROR,
 				    "maximum delay allowed is %d",

plugins/askrene/flow.h

@@ -61,4 +61,9 @@ u64 flows_worst_delay(struct flow **flows);
 const char *fmt_flows_step_scid(const tal_t *ctx,
 				const struct route_query *rq,
 				const struct flow *flow, size_t i);
+
+/* When we need to debug */
+const char *fmt_flow_full(const tal_t *ctx,
+			  const struct route_query *rq,
+			  const struct flow *flow);
 #endif /* LIGHTNING_PLUGINS_ASKRENE_FLOW_H */

plugins/askrene/refine.c

@@ -315,13 +315,25 @@ static struct amount_msat flow_remaining_capacity(const struct route_query *rq,
 	 * 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",
+	/* This seems to happen.  Which is strange, since flows should
+	   already be constrained. */
+	if (!amount_msat_sub(&diff, max, flow->delivers)) {
+		plugin_log(rq->plugin, LOG_BROKEN,
+			   "Flow delivers %s but max only %s? flow=%s",
 			   fmt_amount_msat(tmpctx, flow->delivers),
-			   fmt_amount_msat(tmpctx, max));
-
+			   fmt_amount_msat(tmpctx, max),
+			   fmt_flow_full(rq, rq, flow));
+		for (size_t i = 0; i < tal_count(*reservations); i++) {
+			plugin_log(rq->plugin, LOG_BROKEN,
+				   "Reservation #%zi: %s on %s",
+				   i,
+				   fmt_amount_msat(tmpctx, (*reservations)[i].amount),
+				   fmt_short_channel_id_dir(tmpctx, &(*reservations)[i].scidd));
+		}
+		diff = AMOUNT_MSAT(0);
+	}
+	create_flow_reservations(rq, reservations, flow);
 	return diff;
 }
 
@@ -385,6 +397,10 @@ static bool duplicate_one_flow(const struct route_query *rq,
 	for (size_t i = 0; i < tal_count(*flows); i++) {
 		struct flow *flow = (*flows)[i], *new_flow;
 		struct amount_msat max, new_amount;
+		/* Don't create 0 flow (shouldn't happen, but be sure) */
+		if (amount_msat_less(flow->delivers, AMOUNT_MSAT(2)))
+			continue;
+
 		if (flow_max_capacity(rq, flow, &max, NULL, NULL)
 		    != CAPPED_HTLC_MAX)
 			continue;

plugins/xpay/xpay.c

@@ -7,6 +7,7 @@
 #include <ccan/tal/str/str.h>
 #include <common/bolt11.h>
 #include <common/bolt12.h>
+#include <common/daemon.h>
 #include <common/dijkstra.h>
 #include <common/gossmap.h>
 #include <common/gossmods_listpeerchannels.h>
@@ -1138,6 +1139,12 @@ static struct command_result *getroutes_for(struct command *aux_cmd,
 	const struct pubkey *dst;
 	struct amount_msat maxfee;
 
+	/* I would normally assert here, but we have reports of this happening... */
+	if (amount_msat_is_zero(deliver)) {
+		payment_log(payment, LOG_BROKEN, "getroutes for 0msat!");
+		send_backtrace("getroutes for 0msat!");
+	}
+
 	/* If we get injectpaymentonion responses, they can wait */
 	payment->amount_being_routed = deliver;
 
@@ -1506,6 +1513,13 @@ static struct command_result *json_xpay_core(struct command *cmd,
 		if (msat)
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 					    "Cannot override amount for bolt12 invoices");
+		/* FIXME: This is actually spec legal, since invoice_amount is
+		 * the *minumum* it will accept.  We could change this to
+		 * 1msat if required. */
+ 		if (amount_msat_is_zero(payment->full_amount))
+			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+					    "Invalid bolt12 invoice with zero amount");
+
 		payment->route_hints = NULL;
 		payment->payment_secret = NULL;
 		payment->payment_metadata = NULL;
@@ -1572,6 +1586,9 @@ static struct command_result *json_xpay_core(struct command *cmd,
 		else
 			payment->full_amount = *msat;
 
+ 		if (amount_msat_is_zero(payment->full_amount))
+			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+					    "Cannot pay bolt11 invoice with zero amount");
 		invexpiry = b11->timestamp + b11->expiry;
 	}
 
@@ -1587,6 +1604,9 @@ static struct command_result *json_xpay_core(struct command *cmd,
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 					    "partial_msat must be less or equal to total amount %s",
 					    fmt_amount_msat(tmpctx, payment->full_amount));
+		if (amount_msat_is_zero(payment->amount))
+			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+					    "partial_msat must be non-zero");
 	} else {
 		payment->amount = payment->full_amount;
 	}

tests/test_askrene.py

@@ -986,6 +986,17 @@ def test_limits_fake_gossmap(node_factory, bitcoind):
     # Must deliver exact amount.
     assert sum(r['amount_msat'] for r in routes["routes"]) == 800_000_001
 
+    # Won't evaluate route for 0msat.
+    with pytest.raises(RpcError, match="amount must be non-zero"):
+        l1.rpc.getroutes(
+            source=nodemap[0],
+            destination=nodemap[2],
+            amount_msat=0,
+            layers=["localchans", "auto.sourcefree"],
+            maxfee_msat=50_000_000,
+            final_cltv=10,
+        )
+
 
 def test_max_htlc(node_factory, bitcoind):
     """A route which looks good isn't actually, because of max htlc limits"""