diff --git a/common/daemon.c b/common/daemon.c index 31878a55d131..c14de8b07c2a 100644 --- a/common/daemon.c +++ b/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); diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 0808485bda1e..d8c82883fde9 100644 --- a/plugins/askrene/askrene.c +++ b/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", diff --git a/plugins/askrene/flow.h b/plugins/askrene/flow.h index 44de4312efdf..28ac7627723a 100644 --- a/plugins/askrene/flow.h +++ b/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 */ diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index d52ab076bda0..51f030ade8e8 100644 --- a/plugins/askrene/refine.c +++ b/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; diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 59ee75df4ca0..2bc4c36e9602 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -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; } diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 1de2d3ce5033..44e029194a33 100644 --- a/tests/test_askrene.py +++ b/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"""