From b871ea01bae6cffaecf02ca754d1b221de23d963 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:24 +1030 Subject: [PATCH 1/7] askrene: don't crash if asked to route 0 msat. But don't allow it either. ``` Jan 19 13:18:52 boltz lightningd[2259911]: cln-askrene: plugins/askrene/algorithm.c:274: simple_feasibleflow: Assertion `amount > 0' failed. Jan 19 13:18:52 boltz lightningd[2259911]: cln-askrene: FATAL SIGNAL 6 (version v24.11.1-1-ge9dbdeb) Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e212b407 send_backtrace Jan 19 13:18:52 boltz lightningd[2259911]: common/daemon.c:33 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e212b49e crashdump Jan 19 13:18:52 boltz lightningd[2259911]: common/daemon.c:75 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba9251f ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964bae69fc ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba92475 ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba787f2 ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba7871a ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba89e95 ??? Jan 19 13:18:52 boltz lightningd[2259911]: ???:0 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e211695e simple_feasibleflow Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/algorithm.c:274 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e2111495 minflow Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/mcf.c:1014 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e210bc74 get_routes Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/askrene.c:414 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e210c610 do_getroutes Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/askrene.c:615 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e210cad8 listpeerchannels_done Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/askrene.c:741 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e211b35a handle_rpc_reply Jan 19 13:18:52 boltz lightningd[2259911]: plugins/libplugin.c:1084 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e211b54c rpc_read_response_one Jan 19 13:18:52 boltz lightningd[2259911]: plugins/libplugin.c:1388 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e211b5fd rpc_conn_read_response Jan 19 13:18:52 boltz lightningd[2259911]: plugins/libplugin.c:1412 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e214fe8f next_plan Jan 19 13:18:52 boltz lightningd[2259911]: ccan/ccan/io/io.c:60 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e215036e do_plan Jan 19 13:18:52 boltz lightningd[2259911]: ccan/ccan/io/io.c:422 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e215042b io_ready Jan 19 13:18:52 boltz lightningd[2259911]: ccan/ccan/io/io.c:439 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e2151e2e io_loop Jan 19 13:18:52 boltz lightningd[2259911]: ccan/ccan/io/poll.c:455 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e211cc29 plugin_main Jan 19 13:18:52 boltz lightningd[2259911]: plugins/libplugin.c:2488 Jan 19 13:18:52 boltz lightningd[2259911]: 0x5576e210cb38 main Jan 19 13:18:52 boltz lightningd[2259911]: plugins/askrene/askrene.c:1262 Jan 19 13:18:52 boltz lightningd[2259911]: 0x7f964ba79d8f ??? ``` Signed-off-by: Rusty Russell Changelog-Fixed: JSON-RPC: `getroutes` will refuse, not crash, if asked to find a route fr 0msat. --- plugins/askrene/askrene.c | 28 ++++++++++++++++++---------- tests/test_askrene.py | 11 +++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 0808485bda1e..3fee04559609 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -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/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""" From 87a19b86e555221d35dc459917087f703b052a9f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:29 +1030 Subject: [PATCH 2/7] xpay: refuse request to pay 0msat. Signed-off-by: Rusty Russell Changelog-Fixed: JSON-RPC: `xpay` will refuse to make a 0msat payment (0msat invoice, partial payment, or manually-set on amountless invoice). Fixes: https://github.com/ElementsProject/lightning/issues/8016 --- plugins/xpay/xpay.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 59ee75df4ca0..febc1c1e608a 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -1506,6 +1506,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 +1579,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 +1597,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; } From b93f6e5311507ebb8948425e48e8bccc1fecbf9d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:29 +1030 Subject: [PATCH 3/7] common: don't crash on send_backtrace() if bt_print is NULL. This (and bt_exit) are NULL for libplugin. We don't always want to crash! Signed-off-by: Rusty Russell --- common/daemon.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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); From 2f6835283b7f7fbab8d0cf3a8af378e31ba52f71 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:29 +1030 Subject: [PATCH 4/7] xpay: make sure we report if we ever try to call getroutes with 0msat. Signed-off-by: Rusty Russell --- plugins/xpay/xpay.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index febc1c1e608a..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; From 296d5e125453a787faddc1ed920a723dc7fb1620 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:30 +1030 Subject: [PATCH 5/7] askrene: don't create 0-msat flow in corner case. Shouldn't happen, but I can't *prove* it's impossible. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index d52ab076bda0..5c1834e1305e 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -385,6 +385,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; From 1c68920694c2a1a8f8891e586e08caf35b8b8051 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:30 +1030 Subject: [PATCH 6/7] askrene: expose fmt_flow_full and make more generic. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 28 ++++++++++++++-------------- plugins/askrene/flow.h | 5 +++++ 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 3fee04559609..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))); } } } 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 */ From e8e3a58fb37bfa244c80e07344811d3075c2b3cf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Jan 2025 09:38:30 +1030 Subject: [PATCH 7/7] askrene: don't crash, just report, when a flow's remaining capacity is negative. I'm not sure why this happens, and suspect it is caused by an issue elsewhere, so add some verbose debugging, don't crash. Signed-off-by: Rusty Russell Fixes: https://github.com/ElementsProject/lightning/issues/8017 --- plugins/askrene/refine.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 5c1834e1305e..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; }