From 13a278258aef90a55260295e7dae663580a1fb83 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:38:31 +1030 Subject: [PATCH 01/15] common: add amount_msat_deduct / amount_msat_deduct_sub. I added amount_msat_accumulate for the "a+=b" case, but I was struggling with a name for the subtractive equivalent. After some prompting, ChatGPT suggested deduct. Signed-off-by: Rusty Russell --- channeld/full_channel.c | 14 +++++++------- common/amount.c | 12 ++++++++++++ common/amount.h | 8 ++++++++ common/initial_commit_tx.c | 2 +- devtools/mkclose.c | 2 +- lightningd/invoice.c | 2 +- lightningd/peer_control.c | 16 ++++++++-------- lightningd/peer_htlcs.c | 3 +-- plugins/askrene/askrene.c | 2 +- plugins/askrene/mcf.c | 7 +++---- plugins/askrene/refine.c | 18 ++++++++---------- plugins/askrene/reserve.c | 2 +- plugins/bkpr/channelsapy.c | 6 ++---- plugins/bkpr/onchain_fee.c | 2 +- plugins/libplugin-pay.c | 15 ++++++--------- plugins/renepay/chan_extra.c | 8 ++++---- plugins/renepay/mcf.c | 2 +- plugins/renepay/mods.c | 2 +- plugins/renepay/routebuilder.c | 5 ++--- plugins/renepay/routetracker.c | 9 ++++----- tests/plugins/channeld_fakenet.c | 5 ++--- 21 files changed, 75 insertions(+), 67 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index eca0ee5228a8..c3df6ee66900 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -416,7 +416,7 @@ static bool get_room_above_reserve(const struct channel *channel, return false; } - if (!amount_msat_sub_sat(remainder, *remainder, reserve)) { + if (!amount_msat_deduct_sat(remainder, reserve)) { status_debug("%s cannot afford htlc: would make balance %s" " below reserve %s", side_to_str(side), @@ -504,7 +504,7 @@ static bool htlc_dust(const struct channel *channel, side, &trim_rmvd)) return false; - return amount_msat_sub(trim_total, *trim_total, trim_rmvd); + return amount_msat_deduct(trim_total, trim_rmvd); } /* @@ -773,7 +773,7 @@ static enum channel_add_err add_htlc(struct channel *channel, */ if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx) && channel->opener == sender - && !amount_msat_sub_sat(&remainder, remainder, AMOUNT_SAT(660))) + && !amount_msat_deduct_sat(&remainder, AMOUNT_SAT(660))) return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; if (channel->opener== sender) { @@ -805,7 +805,7 @@ static enum channel_add_err add_htlc(struct channel *channel, if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx) && channel->opener != sender - && !amount_msat_sub_sat(&remainder, remainder, AMOUNT_SAT(660))) + && !amount_msat_deduct_sat(&remainder, AMOUNT_SAT(660))) return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; /* Should be able to afford both their own commit tx @@ -1257,12 +1257,12 @@ u32 approx_max_feerate(const struct channel *channel) * `to_remote`). */ if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx) - && !amount_msat_sub_sat(&avail, avail, AMOUNT_SAT(660))) { + && !amount_msat_deduct_sat(&avail, AMOUNT_SAT(660))) { avail = AMOUNT_MSAT(0); } else { /* We should never go below reserve. */ - if (!amount_msat_sub_sat(&avail, avail, - channel->config[!channel->opener].channel_reserve)) + if (!amount_msat_deduct_sat(&avail, + channel->config[!channel->opener].channel_reserve)) avail = AMOUNT_MSAT(0); } diff --git a/common/amount.c b/common/amount.c index 0f1867369acd..cf872d0cb598 100644 --- a/common/amount.c +++ b/common/amount.c @@ -263,6 +263,18 @@ WARN_UNUSED_RESULT bool amount_msat_accumulate(struct amount_msat *a, return amount_msat_add(a, *a, b); } +WARN_UNUSED_RESULT bool amount_msat_deduct(struct amount_msat *a, + struct amount_msat b) +{ + return amount_msat_sub(a, *a, b); +} + +WARN_UNUSED_RESULT bool amount_msat_deduct_sat(struct amount_msat *a, + struct amount_sat b) +{ + return amount_msat_sub_sat(a, *a, b); +} + WARN_UNUSED_RESULT bool amount_msat_sub(struct amount_msat *val, struct amount_msat a, struct amount_msat b) diff --git a/common/amount.h b/common/amount.h index 11551b913a94..04965889b758 100644 --- a/common/amount.h +++ b/common/amount.h @@ -105,6 +105,14 @@ WARN_UNUSED_RESULT bool amount_sat_add_sat_s64(struct amount_sat *val, WARN_UNUSED_RESULT bool amount_msat_accumulate(struct amount_msat *a, struct amount_msat b); +/* a -= b */ +WARN_UNUSED_RESULT bool amount_msat_deduct(struct amount_msat *a, + struct amount_msat b); + +/* a -= b */ +WARN_UNUSED_RESULT bool amount_msat_deduct_sat(struct amount_msat *a, + struct amount_sat b); + /* returns floor(msat/div) */ struct amount_msat amount_msat_div(struct amount_msat msat, u64 div); diff --git a/common/initial_commit_tx.c b/common/initial_commit_tx.c index 07e3c8909bea..fb585513bc5a 100644 --- a/common/initial_commit_tx.c +++ b/common/initial_commit_tx.c @@ -40,7 +40,7 @@ bool try_subtract_fee(enum side opener, enum side side, else opener_amount = other; - if (amount_msat_sub_sat(opener_amount, *opener_amount, base_fee)) + if (amount_msat_deduct_sat(opener_amount, base_fee)) return true; *opener_amount = AMOUNT_MSAT(0); diff --git a/devtools/mkclose.c b/devtools/mkclose.c index de51c7f64f25..d32aa2a6d9eb 100644 --- a/devtools/mkclose.c +++ b/devtools/mkclose.c @@ -121,7 +121,7 @@ int main(int argc, char *argv[]) if (option_anchor_outputs && !amount_sat_add(&fee, fee, AMOUNT_SAT(660))) errx(1, "Can't afford anchors"); - if (!amount_msat_sub_sat(&local_msat, local_msat, fee)) + if (!amount_msat_deduct_sat(&local_msat, fee)) errx(1, "Can't afford fee %s", fmt_amount_sat(NULL, fee)); if (!amount_sat_sub_msat(&remote_msat, funding_amount, local_msat)) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 49933a932cc3..1fc0db374c04 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -570,7 +570,7 @@ static struct route_info **select_inchan(const tal_t *ctx, candidates[i].c->our_config.channel_reserve, candidates[i].c->channel_info.their_config.channel_reserve) || !amount_sat_to_msat(&capacity, candidates[i].c->funding_sats) - || !amount_msat_sub_sat(&capacity, capacity, cumulative_reserve)) { + || !amount_msat_deduct_sat(&capacity, cumulative_reserve)) { log_broken(ld->log, "Channel %s capacity overflow!", fmt_short_channel_id(tmpctx, *candidates[i].c->scid)); continue; diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f5b064ca81ce..de2c79714f65 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -721,7 +721,7 @@ static void subtract_offered_htlcs(const struct channel *channel, hout = htlc_out_map_next(ld->htlcs_out, &outi)) { if (hout->key.channel != channel) continue; - if (!amount_msat_sub(amount, *amount, hout->msat)) + if (!amount_msat_deduct(amount, hout->msat)) *amount = AMOUNT_MSAT(0); } } @@ -738,7 +738,7 @@ static void subtract_received_htlcs(const struct channel *channel, hin = htlc_in_map_next(ld->htlcs_in, &ini)) { if (hin->key.channel != channel) continue; - if (!amount_msat_sub(amount, *amount, hin->msat)) + if (!amount_msat_deduct(amount, hin->msat)) *amount = AMOUNT_MSAT(0); } } @@ -759,9 +759,9 @@ struct amount_msat channel_amount_spendable(const struct channel *channel) /* If we're opener, subtract txfees we'll need to spend this */ if (channel->opener == LOCAL) { - if (!amount_msat_sub_sat(&spendable, spendable, - commit_txfee(channel, spendable, - LOCAL))) + if (!amount_msat_deduct_sat(&spendable, + commit_txfee(channel, spendable, + LOCAL))) return AMOUNT_MSAT(0); } @@ -803,9 +803,9 @@ struct amount_msat channel_amount_receivable(const struct channel *channel) /* If they're opener, subtract txfees they'll need to spend this */ if (channel->opener == REMOTE) { - if (!amount_msat_sub_sat(&receivable, receivable, - commit_txfee(channel, - receivable, REMOTE))) + if (!amount_msat_deduct_sat(&receivable, + commit_txfee(channel, + receivable, REMOTE))) return AMOUNT_MSAT(0); } diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 7c818782bed0..6e6853fe55b9 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -2006,8 +2006,7 @@ static void remove_htlc_out(struct channel *channel, struct htlc_out *hout) const struct channel_coin_mvt *mvt; struct amount_msat oldamt = channel->our_msat; /* We paid for this HTLC, so deduct balance. */ - if (!amount_msat_sub(&channel->our_msat, channel->our_msat, - hout->msat)) { + if (!amount_msat_deduct(&channel->our_msat, hout->msat)) { channel_internal_error(channel, "Underflow our_msat %s - HTLC %s", fmt_amount_msat(tmpctx, diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index d76afa68d153..985932364191 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -1057,7 +1057,7 @@ static struct command_result *json_askrene_inform_channel(struct command *cmd, if (!reserve_accumulate(askrene->reserved, scidd, amount)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Amount overflow with reserves"); - if (!amount_msat_sub(amount, *amount, AMOUNT_MSAT(1))) + if (!amount_msat_deduct(amount, AMOUNT_MSAT(1))) *amount = AMOUNT_MSAT(0); if (command_check_only(cmd)) return command_check_done(cmd); diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index e41bfb05730e..3acf0a332f72 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -345,7 +345,7 @@ static double pickhardt_richter_probability(struct amount_msat low, struct amount_msat all_states, good_states; if (amount_msat_greater_eq(amount, high)) return 0.0; - if (!amount_msat_sub(&amount, amount, low)) + if (!amount_msat_deduct(&amount, low)) return 1.0; if (!amount_msat_sub(&all_states, high, low)) abort(); // we expect high > low @@ -1587,9 +1587,8 @@ linear_routes(const tal_t *ctx, struct route_query *rq, } } - if (!amount_msat_sub(&feebudget, feebudget, all_fees) || - !amount_msat_sub(&amount_to_deliver, amount_to_deliver, - all_deliver)) { + if (!amount_msat_deduct(&feebudget, all_fees) || + !amount_msat_deduct(&amount_to_deliver, all_deliver)) { error_message = rq_log(ctx, rq, LOG_BROKEN, "%s: unexpected arithmetic operation " diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 00ee4f75ced9..12743ebbc1d7 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -372,10 +372,10 @@ static struct amount_msat remove_excess(struct flow **flows, /* Remove the smaller parts if they deliver less than the * excess. */ for (int i = tal_count(*flows_index) - 1; i >= 0; i--) { - if (!amount_msat_sub(&excess, excess, + if (!amount_msat_deduct(&excess, flows[(*flows_index)[i]]->delivers)) break; - if (!amount_msat_sub(&all_deliver, all_deliver, + if (!amount_msat_deduct(&all_deliver, flows[(*flows_index)[i]]->delivers)) abort(); tal_arr_remove(flows_index, i); @@ -397,21 +397,19 @@ static struct amount_msat remove_excess(struct flow **flows, /* rounding errors: don't remove more than excess */ remove = amount_msat_min(remove, excess); - if (!amount_msat_sub(&excess, excess, remove)) + if (!amount_msat_deduct(&excess, remove)) abort(); - if (!amount_msat_sub(&all_deliver, all_deliver, remove) || - !amount_msat_sub(&flows[(*flows_index)[i]]->delivers, - flows[(*flows_index)[i]]->delivers, - remove)) + if (!amount_msat_deduct(&all_deliver, remove) || + !amount_msat_deduct(&flows[(*flows_index)[i]]->delivers, + remove)) abort(); } /* any rounding error left, take it from the first */ assert(tal_count(*flows_index) > 0); - if (!amount_msat_sub(&all_deliver, all_deliver, excess) || - !amount_msat_sub(&flows[(*flows_index)[0]]->delivers, - flows[(*flows_index)[0]]->delivers, excess)) + if (!amount_msat_deduct(&all_deliver, excess) || + !amount_msat_deduct(&flows[(*flows_index)[0]]->delivers, excess)) abort(); return all_deliver; } diff --git a/plugins/askrene/reserve.c b/plugins/askrene/reserve.c index 738577fc2bc4..de88091c2c0c 100644 --- a/plugins/askrene/reserve.c +++ b/plugins/askrene/reserve.c @@ -103,7 +103,7 @@ void reserve_sub(const struct reserve_htable *reserved, for (r = reserve_htable_getfirst(reserved, scidd, &rit); r; r = reserve_htable_getnext(reserved, scidd, &rit)) { - if (!amount_msat_sub(amount, *amount, r->rhop.amount)) + if (!amount_msat_deduct(amount, r->rhop.amount)) *amount = AMOUNT_MSAT(0); } } diff --git a/plugins/bkpr/channelsapy.c b/plugins/bkpr/channelsapy.c index f4fa55a0437c..898b3175c734 100644 --- a/plugins/bkpr/channelsapy.c +++ b/plugins/bkpr/channelsapy.c @@ -126,11 +126,9 @@ static void fillin_apy_acct_details(const struct bkpr *bkpr, /* If there is any push_out or lease_fees_out, we subtract * from starting balance */ - ok = amount_msat_sub(&apy->our_start_bal, apy->our_start_bal, - apy->push_out); + ok = amount_msat_deduct(&apy->our_start_bal, apy->push_out); assert(ok); - ok = amount_msat_sub(&apy->our_start_bal, apy->our_start_bal, - apy->lease_out); + ok = amount_msat_deduct(&apy->our_start_bal, apy->lease_out); assert(ok); /* we add values in to starting balance */ diff --git a/plugins/bkpr/onchain_fee.c b/plugins/bkpr/onchain_fee.c index 6cdce78ab3ca..8cfc0c136360 100644 --- a/plugins/bkpr/onchain_fee.c +++ b/plugins/bkpr/onchain_fee.c @@ -288,7 +288,7 @@ static void insert_chain_fees_diff(struct command *cmd, if (!amount_msat_accumulate(¤t_amt, ofs[i]->credit)) plugin_err(cmd->plugin, "Overflow when adding onchain fees"); - if (!amount_msat_sub(¤t_amt, current_amt, ofs[i]->debit)) + if (!amount_msat_deduct(¤t_amt, ofs[i]->debit)) plugin_err(cmd->plugin, "Underflow when subtracting onchain fees"); if (ofs[i]->update_count > max_update_count) max_update_count = ofs[i]->update_count; diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 919ca7de3527..6c78de12cec5 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -482,7 +482,7 @@ payment_constraints_update(struct payment_constraints *cons, return false; /* amount_msat_sub performs a check before actually subtracting. */ - if (!amount_msat_sub(&cons->fee_budget, cons->fee_budget, delta_fee)) + if (!amount_msat_deduct(&cons->fee_budget, delta_fee)) return false; cons->cltv_budget -= delta_cltv; @@ -571,9 +571,8 @@ static bool payment_chanhints_apply_route(struct payment *p) curhint->local->htlc_budget--; } - if (!amount_msat_sub(&curhint->estimated_capacity, - curhint->estimated_capacity, - curhop->amount)) { + if (!amount_msat_deduct(&curhint->estimated_capacity, + curhop->amount)) { /* Given our preemptive test * above, this should never * happen either. */ @@ -1447,7 +1446,7 @@ handle_intermediate_failure(struct command *cmd, * work. This allows us to then allow on equality, this is for * example necessary for local channels where exact matches * should be allowed. */ - if (!amount_msat_sub(&estimated, estimated, AMOUNT_MSAT(1))) + if (!amount_msat_deduct(&estimated, AMOUNT_MSAT(1))) abort(); /* These are an indication that the capacity was insufficient, @@ -3407,10 +3406,8 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, * destination could otherwise cause the route to be too * expensive, while really being ok. If any of these fail then * the above checks are insufficient. */ - if (!amount_msat_sub(&d->constraints.fee_budget, - d->constraints.fee_budget, best_fee) || - !amount_msat_sub(&p->constraints.fee_budget, - p->constraints.fee_budget, best_fee)) + if (!amount_msat_deduct(&d->constraints.fee_budget, best_fee) || + !amount_msat_deduct(&p->constraints.fee_budget, best_fee)) paymod_err(p, "Could not update fee constraints " "for shadow route extension. " diff --git a/plugins/renepay/chan_extra.c b/plugins/renepay/chan_extra.c index 931463ca53d2..d4ca301975d3 100644 --- a/plugins/renepay/chan_extra.c +++ b/plugins/renepay/chan_extra.c @@ -128,7 +128,7 @@ enum renepay_errorcode channel_liquidity(struct amount_msat *liquidity, if (!h) return RENEPAY_CHANNEL_NOT_FOUND; struct amount_msat value_liquidity = h->known_max; - if (!amount_msat_sub(&value_liquidity, value_liquidity, h->htlc_total)) + if (!amount_msat_deduct(&value_liquidity, h->htlc_total)) return RENEPAY_AMOUNT_OVERFLOW; *liquidity = value_liquidity; return RENEPAY_NOERROR; @@ -616,12 +616,12 @@ double edge_probability(struct amount_msat min, struct amount_msat max, goto function_fail; // in_flight cannot be greater than max - if (!amount_msat_sub(&B, B, in_flight)) + if (!amount_msat_deduct(&B, in_flight)) goto function_fail; struct amount_msat A = min; // = MAX(0,min-in_flight); - if (!amount_msat_sub(&A, A, in_flight)) + if (!amount_msat_deduct(&A, in_flight)) A = AMOUNT_MSAT(0); struct amount_msat denominator; // = B-A @@ -655,7 +655,7 @@ chan_extra_remove_htlc(struct chan_extra_map *chan_extra_map, if (h->num_htlcs <= 0) return RENEPAY_PRECONDITION_ERROR; - if (!amount_msat_sub(&h->htlc_total, h->htlc_total, amount)) + if (!amount_msat_deduct(&h->htlc_total, amount)) return RENEPAY_AMOUNT_OVERFLOW; h->num_htlcs--; return RENEPAY_NOERROR; diff --git a/plugins/renepay/mcf.c b/plugins/renepay/mcf.c index 7d2f1490300a..8fdd067b4e24 100644 --- a/plugins/renepay/mcf.c +++ b/plugins/renepay/mcf.c @@ -1440,7 +1440,7 @@ get_flow_paths(const tal_t *ctx, const struct gossmap *gossmap, // substract the excess of msats for not having msat // accuracy struct amount_msat delivered = amount_msat(delta*1000); - if (!amount_msat_sub(&delivered, delivered, excess)) { + if (!amount_msat_deduct(&delivered, excess)) { if (fail) *fail = tal_fmt( ctx, "unable to substract excess"); diff --git a/plugins/renepay/mods.c b/plugins/renepay/mods.c index 34e505816538..0fdd45a95059 100644 --- a/plugins/renepay/mods.c +++ b/plugins/renepay/mods.c @@ -679,7 +679,7 @@ static struct command_result *compute_routes_cb(struct payment *payment) __func__); /* Remaining fee budget. */ - if (!amount_msat_sub(&feebudget, feebudget, fees_spent)) + if (!amount_msat_deduct(&feebudget, fees_spent)) feebudget = AMOUNT_MSAT(0); /* How much are we still trying to send? */ diff --git a/plugins/renepay/routebuilder.c b/plugins/renepay/routebuilder.c index 4e9d15142326..d4c940f77187 100644 --- a/plugins/renepay/routebuilder.c +++ b/plugins/renepay/routebuilder.c @@ -364,7 +364,7 @@ struct route **get_routes(const tal_t *ctx, } // update the fee budget - if (!amount_msat_sub(&feebudget, feebudget, fee)) { + if (!amount_msat_deduct(&feebudget, fee)) { // should never happen tal_report_error( ctx, ecode, fail, PLUGIN_ERROR, @@ -377,8 +377,7 @@ struct route **get_routes(const tal_t *ctx, } // update the amount that we deliver - if (!amount_msat_sub(&amount_to_deliver, - amount_to_deliver, delivering)) { + if (!amount_msat_deduct(&amount_to_deliver, delivering)) { // should never happen tal_report_error( ctx, ecode, fail, PLUGIN_ERROR, diff --git a/plugins/renepay/routetracker.c b/plugins/renepay/routetracker.c index 2e8b9ba2b08d..b30a4a54423f 100644 --- a/plugins/renepay/routetracker.c +++ b/plugins/renepay/routetracker.c @@ -330,11 +330,10 @@ void payment_collect_results(struct payment *payment, *final_msg = tal_strdup(tmpctx, r->final_msg); } - if (!amount_msat_sub(&payment->total_delivering, - payment->total_delivering, - route_delivers(r)) || - !amount_msat_sub(&payment->total_sent, payment->total_sent, - route_sends(r))) { + if (!amount_msat_deduct(&payment->total_delivering, + route_delivers(r)) || + !amount_msat_deduct(&payment->total_sent, + route_sends(r))) { plugin_err(pay_plugin->plugin, "%s: routes do not add up to " "payment total amount.", diff --git a/tests/plugins/channeld_fakenet.c b/tests/plugins/channeld_fakenet.c index d56c5824fb01..c8281671b85d 100644 --- a/tests/plugins/channeld_fakenet.c +++ b/tests/plugins/channeld_fakenet.c @@ -657,9 +657,8 @@ static struct amount_msat calc_capacity(struct info *info, if (!short_channel_id_dir_eq(&info->reservations[i]->scidd, scidd)) continue; /* We should never use more that we have! */ - if (!amount_msat_sub(&dynamic_capacity, - dynamic_capacity, - info->reservations[i]->amount)) + if (!amount_msat_deduct(&dynamic_capacity, + info->reservations[i]->amount)) abort(); status_debug("... minus reservation %s", fmt_amount_msat(tmpctx, info->reservations[i]->amount)); From c0e3fde39c5019efce11ba0ccb1fde2c8f811e12 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:39:31 +1030 Subject: [PATCH 02/15] askrene: remove overzealous cache of channel_data. This is not worth optimizing that I can see. Using a non-debug build I get the following times for tests/test_askrene.py::test_real_data Before: 143 seconds After: 141 seconds. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 175 ++++++++++++++++----------------------- 1 file changed, 70 insertions(+), 105 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 12743ebbc1d7..79bc26e30512 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -8,14 +8,6 @@ #include #include -/* Channel data for fast retrieval. */ -struct channel_data { - struct amount_msat htlc_min, htlc_max, liquidity_max; - u32 fee_base_msat, fee_proportional_millionths; - struct short_channel_id_dir scidd; - u32 idx; -}; - /* We (ab)use the reservation system to place temporary reservations * on channels while we are refining each flow. This has the effect * of making flows aware of each other. */ @@ -146,12 +138,12 @@ bool create_flow_reservations_verify(const struct route_query *rq, /* We use an fp16_t approximatin for htlc_max/min: this gets the exact value. */ static struct amount_msat get_chan_htlc_max(const struct route_query *rq, const struct gossmap_chan *c, - const struct short_channel_id_dir *scidd) + int dir) { struct amount_msat htlc_max; gossmap_chan_get_update_details(rq->gossmap, - c, scidd->dir, + c, dir, NULL, NULL, NULL, NULL, NULL, NULL, NULL, &htlc_max); return htlc_max; @@ -159,12 +151,12 @@ static struct amount_msat get_chan_htlc_max(const struct route_query *rq, static struct amount_msat get_chan_htlc_min(const struct route_query *rq, const struct gossmap_chan *c, - const struct short_channel_id_dir *scidd) + int dir) { struct amount_msat htlc_min; gossmap_chan_get_update_details(rq->gossmap, - c, scidd->dir, + c, dir, NULL, NULL, NULL, NULL, NULL, NULL, &htlc_min, NULL); return htlc_min; @@ -175,50 +167,6 @@ enum why_capped { CAPPED_CAPACITY, }; -/* Cache channel data along the path used by this flow. */ -static struct channel_data *new_channel_path_cache(const tal_t *ctx, - struct route_query *rq, - struct flow *flow) -{ - const size_t pathlen = tal_count(flow->path); - struct channel_data *path = tal_arr(ctx, struct channel_data, pathlen); - - for (size_t i = 0; i < pathlen; i++) { - /* knowledge on liquidity bounds */ - struct amount_msat known_min, known_max; - const struct half_chan *h = flow_edge(flow, i); - struct short_channel_id_dir scidd; - - get_scidd(rq->gossmap, flow, i, &scidd); - get_constraints(rq, flow->path[i], flow->dirs[i], &known_min, - &known_max); - - path[i].htlc_min = get_chan_htlc_min(rq, flow->path[i], &scidd); - path[i].htlc_max = get_chan_htlc_max(rq, flow->path[i], &scidd); - path[i].fee_base_msat = h->base_fee; - path[i].fee_proportional_millionths = h->proportional_fee; - path[i].liquidity_max = known_max; - path[i].scidd = scidd; - path[i].idx = scidd.dir + - 2 * gossmap_chan_idx(rq->gossmap, flow->path[i]); - } - return path; -} - -/* Cache channel data along multiple paths. */ -static struct channel_data **new_channel_mpp_cache(const tal_t *ctx, - struct route_query *rq, - struct flow **flows) -{ - const size_t npaths = tal_count(flows); - struct channel_data **paths = - tal_arr(ctx, struct channel_data *, npaths); - for (size_t i = 0; i < npaths; i++) { - paths[i] = new_channel_path_cache(paths, rq, flows[i]); - } - return paths; -} - /* Reverse order: bigger first */ static int revcmp_flows(const size_t *a, const size_t *b, struct flow **flows) { @@ -231,28 +179,36 @@ static int revcmp_flows(const size_t *a, const size_t *b, struct flow **flows) // TODO: unit test: // -> make a path -// -> compute x = path_max_deliverable +// -> compute x = flow_max_deliverable // -> check that htlc_max are all satisfied // -> check that (x+1) at least one htlc_max is violated /* Given the channel constraints, return the maximum amount that can be * delivered. Sets *bottleneck_idx to one of the contraining channels' idx, if non-NULL */ -static struct amount_msat path_max_deliverable(struct channel_data *path, +static struct amount_msat flow_max_deliverable(const struct route_query *rq, + const struct flow *flow, u32 *bottleneck_idx) { struct amount_msat deliver = AMOUNT_MSAT(-1); - for (size_t i = 0; i < tal_count(path); i++) { - deliver = - amount_msat_sub_fee(deliver, path[i].fee_base_msat, - path[i].fee_proportional_millionths); - if (amount_msat_greater(deliver, path[i].htlc_max)) { + for (size_t i = 0; i < tal_count(flow->path); i++) { + const struct half_chan *hc = &flow->path[i]->half[flow->dirs[i]]; + struct amount_msat unused, known_max, htlc_max; + size_t idx = flow->dirs[i] + + 2 * gossmap_chan_idx(rq->gossmap, flow->path[i]); + + deliver = amount_msat_sub_fee(deliver, hc->base_fee, + hc->proportional_fee); + htlc_max = get_chan_htlc_max(rq, flow->path[i], flow->dirs[i]); + if (amount_msat_greater(deliver, htlc_max)) { if (bottleneck_idx) - *bottleneck_idx = path[i].idx; - deliver = path[i].htlc_max; + *bottleneck_idx = idx; + deliver = htlc_max; } - if (amount_msat_greater(deliver, path[i].liquidity_max)) { + get_constraints(rq, flow->path[i], flow->dirs[i], + &unused, &known_max); + if (amount_msat_greater(deliver, known_max)) { if (bottleneck_idx) - *bottleneck_idx = path[i].idx; - deliver = path[i].liquidity_max; + *bottleneck_idx = idx; + deliver = known_max; } } return deliver; @@ -265,28 +221,36 @@ static struct amount_msat path_max_deliverable(struct channel_data *path, // -> check that (x-1) at least one htlc_min is violated /* The least amount that we can deliver at the destination such that when one * computes the hop amounts backwards the htlc_min are always met. */ -static struct amount_msat path_min_deliverable(struct channel_data *path) +static struct amount_msat flow_min_deliverable(const struct route_query *rq, + const struct flow *flow) { struct amount_msat least_send = AMOUNT_MSAT(1); - const size_t pathlen = tal_count(path); + const size_t pathlen = tal_count(flow->path); + for (size_t i = pathlen - 1; i < pathlen; i--) { - least_send = amount_msat_max(least_send, path[i].htlc_min); - if (!amount_msat_add_fee(&least_send, path[i].fee_base_msat, - path[i].fee_proportional_millionths)) + const struct half_chan *hc = &flow->path[i]->half[flow->dirs[i]]; + struct amount_msat htlc_min = get_chan_htlc_min(rq, flow->path[i], flow->dirs[i]); + + least_send = amount_msat_max(least_send, htlc_min); + if (!amount_msat_add_fee(&least_send, hc->base_fee, + hc->proportional_fee)) abort(); } + /* least_send: is the least amount we can send in order to deliver at * least 1 msat at the destination. */ struct amount_msat least_destination = least_send; for (size_t i = 0; i < pathlen; i++) { + const struct half_chan *hc = &flow->path[i]->half[flow->dirs[i]]; + struct amount_msat htlc_min = get_chan_htlc_min(rq, flow->path[i], flow->dirs[i]); struct amount_msat in_value = least_destination; struct amount_msat out_value = - amount_msat_sub_fee(in_value, path[i].fee_base_msat, - path[i].fee_proportional_millionths); - assert(amount_msat_greater_eq(out_value, path[i].htlc_min)); + amount_msat_sub_fee(in_value, hc->base_fee, + hc->proportional_fee); + assert(amount_msat_greater_eq(out_value, htlc_min)); struct amount_msat x = out_value; - if (!amount_msat_add_fee(&x, path[i].fee_base_msat, - path[i].fee_proportional_millionths)) + if (!amount_msat_add_fee(&x, hc->base_fee, + hc->proportional_fee)) abort(); /* if the in_value computed from the out_value is smaller than * it should, then we add 1msat */ @@ -294,11 +258,11 @@ static struct amount_msat path_min_deliverable(struct channel_data *path) !amount_msat_accumulate(&out_value, AMOUNT_MSAT(1))) abort(); /* check conditions */ - assert(amount_msat_greater_eq(out_value, path[i].htlc_min)); + assert(amount_msat_greater_eq(out_value, htlc_min)); x = out_value; assert( - amount_msat_add_fee(&x, path[i].fee_base_msat, - path[i].fee_proportional_millionths) && + amount_msat_add_fee(&x, hc->base_fee, + hc->proportional_fee) && amount_msat_greater_eq(x, in_value)); least_destination = out_value; } @@ -307,27 +271,34 @@ static struct amount_msat path_min_deliverable(struct channel_data *path) static const char * remove_htlc_min_violations(const tal_t *ctx, struct route_query *rq, - const struct flow *flow, - const struct channel_data *channels) + const struct flow *flow) { const char *error_message = NULL; struct amount_msat msat = flow->delivers; for (size_t i = tal_count(flow->path) - 1; i < tal_count(flow->path); i--) { - if (amount_msat_less(msat, channels[i].htlc_min)) { + struct amount_msat htlc_min = get_chan_htlc_min(rq, flow->path[i], flow->dirs[i]); + const struct half_chan *hc = &flow->path[i]->half[flow->dirs[i]]; + if (amount_msat_less(msat, htlc_min)) { + struct short_channel_id_dir scidd; + /* FIXME: hoist this! */ + size_t idx = flow->dirs[i] + + 2 * gossmap_chan_idx(rq->gossmap, flow->path[i]); + + get_scidd(rq->gossmap, flow, i, &scidd); rq_log( ctx, rq, LOG_INFORM, "Sending %s across %s would violate htlc_min " "(~%s), disabling this channel", fmt_amount_msat(ctx, msat), - fmt_short_channel_id_dir(ctx, &channels[i].scidd), - fmt_amount_msat(ctx, channels[i].htlc_min)); - bitmap_set_bit(rq->disabled_chans, channels[i].idx); + fmt_short_channel_id_dir(ctx, &scidd), + fmt_amount_msat(ctx, htlc_min)); + bitmap_set_bit(rq->disabled_chans, idx); break; } if (!amount_msat_add_fee( - &msat, channels[i].fee_base_msat, - channels[i].fee_proportional_millionths)) { + &msat, hc->base_fee, + hc->proportional_fee)) { error_message = rq_log(ctx, rq, LOG_BROKEN, "%s: Adding fee to amount", __func__); @@ -492,22 +463,18 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, const char *error_message = NULL; struct amount_msat *max_deliverable; struct amount_msat *min_deliverable; - struct channel_data **channel_mpp_cache; size_t *flows_index; - /* we might need to access this data multiple times, so we cache - * it */ - channel_mpp_cache = new_channel_mpp_cache(working_ctx, rq, *flows); max_deliverable = tal_arrz(working_ctx, struct amount_msat, - tal_count(channel_mpp_cache)); + tal_count(*flows)); min_deliverable = tal_arrz(working_ctx, struct amount_msat, - tal_count(channel_mpp_cache)); + tal_count(*flows)); flows_index = tal_arrz(working_ctx, size_t, tal_count(*flows)); - for (size_t i = 0; i < tal_count(channel_mpp_cache); i++) { - // FIXME: does path_max_deliverable work for a single + for (size_t i = 0; i < tal_count(*flows); i++) { + // FIXME: does flow_max_deliverable work for a single // channel with 0 fees? - max_deliverable[i] = path_max_deliverable(channel_mpp_cache[i], bottleneck_idx); - min_deliverable[i] = path_min_deliverable(channel_mpp_cache[i]); + max_deliverable[i] = flow_max_deliverable(rq, (*flows)[i], bottleneck_idx); + min_deliverable[i] = flow_min_deliverable(rq, (*flows)[i]); /* We use an array of indexes to keep track of the order * of the flows. Likewise flows can be removed by simply * shrinking the flows_index array. */ @@ -539,7 +506,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, /* htlc_min is not met for this flow */ tal_arr_remove(&flows_index, i); error_message = remove_htlc_min_violations( - working_ctx, rq, (*flows)[k], channel_mpp_cache[k]); + working_ctx, rq, (*flows)[k]); if (error_message) goto fail; } @@ -575,17 +542,15 @@ void squash_flows(const tal_t *ctx, struct route_query *rq, const tal_t *working_ctx = tal(ctx, tal_t); size_t *flows_index = tal_arrz(working_ctx, size_t, tal_count(*flows)); char **paths_str = tal_arrz(working_ctx, char *, tal_count(*flows)); - struct channel_data **channel_mpp_cache = - new_channel_mpp_cache(working_ctx, rq, *flows); struct amount_msat *max_deliverable = tal_arrz( - working_ctx, struct amount_msat, tal_count(channel_mpp_cache)); + working_ctx, struct amount_msat, tal_count(*flows)); for (size_t i = 0; i < tal_count(flows_index); i++) { - struct flow *flow = (*flows)[i]; + const struct flow *flow = (*flows)[i]; struct short_channel_id_dir scidd; flows_index[i] = i; paths_str[i] = tal_strdup(working_ctx, ""); - max_deliverable[i] = path_max_deliverable(channel_mpp_cache[i], NULL); + max_deliverable[i] = flow_max_deliverable(rq, flow, NULL); for (size_t j = 0; j < tal_count(flow->path); j++) { scidd.scid = From bccf7f3156c7f2f11e2158016e53dc11333c518e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:40:31 +1030 Subject: [PATCH 03/15] askrene: fix error path if we fail sanity checks. We've already freed the working_ctx, and the fail path does that again. Signed-off-by: Rusty Russell --- plugins/askrene/mcf.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 3acf0a332f72..44cdfc728895 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -1618,24 +1618,21 @@ linear_routes(const tal_t *ctx, struct route_query *rq, rq_log(rq, rq, LOG_BROKEN, "%s: check_htlc_min_limits failed", __func__); *flows = tal_free(*flows); - goto fail; + return error_message; } if (!check_htlc_max_limits(rq, *flows)) { - error_message = - rq_log(rq, rq, LOG_BROKEN, - "%s: check_htlc_max_limits failed", __func__); *flows = tal_free(*flows); - goto fail; + return rq_log(rq, rq, LOG_BROKEN, + "%s: check_htlc_max_limits failed", __func__); } if (tal_count(*flows) > rq->maxparts) { - error_message = rq_log( - rq, rq, LOG_BROKEN, - "%s: the number of flows (%zu) exceeds the limit set " - "on payment parts (%" PRIu32 - "), please submit a bug report", - __func__, tal_count(*flows), rq->maxparts); + size_t num_flows = tal_count(*flows); *flows = tal_free(*flows); - goto fail; + return rq_log(rq, rq, LOG_BROKEN, + "%s: the number of flows (%zu) exceeds the limit set " + "on payment parts (%" PRIu32 + "), please submit a bug report", + __func__, num_flows, rq->maxparts); } return NULL; From 36616286d43a1699100e35609467f0f5d6831f29 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:41:31 +1030 Subject: [PATCH 04/15] askrene: fix use-after-free if remove_htlc_min_violations fails. It can only fail on overflow, but if it did, the fail path frees working_ctx and returns "error_message". Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 79bc26e30512..64ce3221f2e2 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -506,7 +506,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, /* htlc_min is not met for this flow */ tal_arr_remove(&flows_index, i); error_message = remove_htlc_min_violations( - working_ctx, rq, (*flows)[k]); + ctx, rq, (*flows)[k]); if (error_message) goto fail; } From 90b6d78cacf48e549edd42888d6b4ec5ed3f3ddd Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:42:31 +1030 Subject: [PATCH 05/15] askrene: remove max_deliverable cache from increase_flows. Make it calculate on demand. This will be useful when we call it from elsewhere. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 64ce3221f2e2..4e5e87052285 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -389,11 +389,11 @@ static struct amount_msat remove_excess(struct flow **flows, * flow beyond the tolerance fraction. It doesn't increase any flow above its * max_deliverable value. * Returns the total delivery amount. */ -static struct amount_msat increase_flows(struct flow **flows, +static struct amount_msat increase_flows(const struct route_query *rq, + struct flow **flows, size_t **flows_index, struct amount_msat deliver, - double tolerance, - struct amount_msat *max_deliverable) + double tolerance) { if (tal_count(flows) == 0) return AMOUNT_MSAT(0); @@ -422,7 +422,7 @@ static struct amount_msat increase_flows(struct flow **flows, can_add = amount_msat_min(can_add, amt); /* no more than max_deliverable */ - if (!amount_msat_sub(&amt, max_deliverable[index], + if (!amount_msat_sub(&amt, flow_max_deliverable(rq, flow, NULL), flow->delivers)) continue; else @@ -461,19 +461,15 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, { const tal_t *working_ctx = tal(ctx, tal_t); const char *error_message = NULL; - struct amount_msat *max_deliverable; struct amount_msat *min_deliverable; size_t *flows_index; - max_deliverable = tal_arrz(working_ctx, struct amount_msat, - tal_count(*flows)); min_deliverable = tal_arrz(working_ctx, struct amount_msat, tal_count(*flows)); flows_index = tal_arrz(working_ctx, size_t, tal_count(*flows)); for (size_t i = 0; i < tal_count(*flows); i++) { // FIXME: does flow_max_deliverable work for a single // channel with 0 fees? - max_deliverable[i] = flow_max_deliverable(rq, (*flows)[i], bottleneck_idx); min_deliverable[i] = flow_min_deliverable(rq, (*flows)[i]); /* We use an array of indexes to keep track of the order * of the flows. Likewise flows can be removed by simply @@ -485,15 +481,14 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, for (size_t i = 0; i < tal_count(flows_index); i++) { (*flows)[flows_index[i]]->delivers = amount_msat_min((*flows)[flows_index[i]]->delivers, - max_deliverable[flows_index[i]]); + flow_max_deliverable(rq, (*flows)[flows_index[i]], bottleneck_idx)); } /* remove excess from MCF granularity if any */ remove_excess(*flows, &flows_index, deliver); /* increase flows if necessary to meet the target */ - increase_flows(*flows, &flows_index, deliver, /* tolerance = */ 0.02, - max_deliverable); + increase_flows(rq, *flows, &flows_index, deliver, /* tolerance = */ 0.02); /* detect htlc_min violations */ for (size_t i = 0; i < tal_count(flows_index);) { From 0ff499b5fc53b147a26a09f3d178432d521231b9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:43:31 +1030 Subject: [PATCH 06/15] askrene: use flows array directly in remove_excess. We don't need the indexes array, we can use this directly. We still set up the indexes array (for now) after we call this. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 68 +++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 4e5e87052285..efe9c7443d96 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -177,6 +177,15 @@ static int revcmp_flows(const size_t *a, const size_t *b, struct flow **flows) return 1; } +static int revcmp_flows_noidx(struct flow *const *a, struct flow *const *b, void *unused) +{ + if (amount_msat_eq((*a)->delivers, (*b)->delivers)) + return 0; + if (amount_msat_greater((*a)->delivers, (*b)->delivers)) + return -1; + return 1; +} + // TODO: unit test: // -> make a path // -> compute x = flow_max_deliverable @@ -320,36 +329,46 @@ static struct amount_msat sum_all_deliver(struct flow **flows, return all_deliver; } +static struct amount_msat sum_all_deliver_noidx(struct flow **flows) +{ + struct amount_msat all_deliver = AMOUNT_MSAT(0); + for (size_t i = 0; i < tal_count(flows); i++) { + if (!amount_msat_accumulate(&all_deliver, + flows[i]->delivers)) + abort(); + } + return all_deliver; +} + /* It reduces the amount of the flows and/or removes some flows in order to * deliver no more than max_deliver. It will leave at least one flow. * Returns the total delivery amount. */ -static struct amount_msat remove_excess(struct flow **flows, - size_t **flows_index, +static struct amount_msat remove_excess(struct flow ***flows, struct amount_msat max_deliver) { - if (tal_count(flows) == 0) + if (tal_count(*flows) == 0) return AMOUNT_MSAT(0); struct amount_msat all_deliver, excess; - all_deliver = sum_all_deliver(flows, *flows_index); + all_deliver = sum_all_deliver_noidx(*flows); /* early exit: there is no excess */ if (!amount_msat_sub(&excess, all_deliver, max_deliver) || amount_msat_is_zero(excess)) return all_deliver; - asort(*flows_index, tal_count(*flows_index), revcmp_flows, flows); + asort(*flows, tal_count(*flows), revcmp_flows_noidx, NULL); /* Remove the smaller parts if they deliver less than the * excess. */ - for (int i = tal_count(*flows_index) - 1; i >= 0; i--) { + for (int i = tal_count(*flows) - 1; i >= 0; i--) { if (!amount_msat_deduct(&excess, - flows[(*flows_index)[i]]->delivers)) + (*flows)[i]->delivers)) break; if (!amount_msat_deduct(&all_deliver, - flows[(*flows_index)[i]]->delivers)) + (*flows)[i]->delivers)) abort(); - tal_arr_remove(flows_index, i); + tal_arr_remove(flows, i); } /* If we still have some excess, remove it from the @@ -357,9 +376,9 @@ static struct amount_msat remove_excess(struct flow **flows, * total. */ struct amount_msat old_excess = excess; struct amount_msat old_deliver = all_deliver; - for (size_t i = 0; i < tal_count(*flows_index); i++) { + for (size_t i = 0; i < tal_count(*flows); i++) { double fraction = amount_msat_ratio( - flows[(*flows_index)[i]]->delivers, old_deliver); + (*flows)[i]->delivers, old_deliver); struct amount_msat remove; if (!amount_msat_scale(&remove, old_excess, fraction)) @@ -372,15 +391,14 @@ static struct amount_msat remove_excess(struct flow **flows, abort(); if (!amount_msat_deduct(&all_deliver, remove) || - !amount_msat_deduct(&flows[(*flows_index)[i]]->delivers, - remove)) + !amount_msat_deduct(&(*flows)[i]->delivers, remove)) abort(); } /* any rounding error left, take it from the first */ - assert(tal_count(*flows_index) > 0); + assert(tal_count(*flows) > 0); if (!amount_msat_deduct(&all_deliver, excess) || - !amount_msat_deduct(&flows[(*flows_index)[0]]->delivers, excess)) + !amount_msat_deduct(&(*flows)[0]->delivers, excess)) abort(); return all_deliver; } @@ -464,6 +482,16 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, struct amount_msat *min_deliverable; size_t *flows_index; + /* do not deliver more than HTLC_MAX allow us */ + for (size_t i = 0; i < tal_count(*flows); i++) { + (*flows)[i]->delivers = + amount_msat_min((*flows)[i]->delivers, + flow_max_deliverable(rq, (*flows)[i], bottleneck_idx)); + } + + /* remove excess from MCF granularity if any */ + remove_excess(flows, deliver); + min_deliverable = tal_arrz(working_ctx, struct amount_msat, tal_count(*flows)); flows_index = tal_arrz(working_ctx, size_t, tal_count(*flows)); @@ -477,16 +505,6 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, flows_index[i] = i; } - /* do not deliver more than HTLC_MAX allow us */ - for (size_t i = 0; i < tal_count(flows_index); i++) { - (*flows)[flows_index[i]]->delivers = - amount_msat_min((*flows)[flows_index[i]]->delivers, - flow_max_deliverable(rq, (*flows)[flows_index[i]], bottleneck_idx)); - } - - /* remove excess from MCF granularity if any */ - remove_excess(*flows, &flows_index, deliver); - /* increase flows if necessary to meet the target */ increase_flows(rq, *flows, &flows_index, deliver, /* tolerance = */ 0.02); From d0e5a123507377b65d335cf02549b7af78f4ebc5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:44:31 +1030 Subject: [PATCH 07/15] askrene: remove indexes from refine_flows except for increase_flows() This removes the index array from code after increase_flows()m, so we use the flows array directly. The next step will be to make increase_flows() use the flows array, and remove the index array indirection entirely. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index efe9c7443d96..0072f366b8ae 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -508,33 +508,32 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, /* increase flows if necessary to meet the target */ increase_flows(rq, *flows, &flows_index, deliver, /* tolerance = */ 0.02); + /* finally write the remaining flows */ + write_selected_flows(working_ctx, flows_index, flows); + /* detect htlc_min violations */ - for (size_t i = 0; i < tal_count(flows_index);) { - size_t k = flows_index[i]; - if (amount_msat_greater_eq((*flows)[k]->delivers, - min_deliverable[k])) { + for (size_t i = 0; i < tal_count(*flows);) { + if (amount_msat_greater_eq((*flows)[i]->delivers, + flow_min_deliverable(rq, (*flows)[i]))) { i++; continue; } - /* htlc_min is not met for this flow */ - tal_arr_remove(&flows_index, i); error_message = remove_htlc_min_violations( - ctx, rq, (*flows)[k]); + ctx, rq, (*flows)[i]); if (error_message) goto fail; + /* htlc_min is not met for this flow */ + tal_arr_remove(flows, i); } /* remove 0 amount flows if any */ - asort(flows_index, tal_count(flows_index), revcmp_flows, *flows); - for (int i = tal_count(flows_index) - 1; i >= 0; i--) { - if (!amount_msat_is_zero((*flows)[flows_index[i]]->delivers)) + asort(*flows, tal_count(*flows), revcmp_flows_noidx, NULL); + for (int i = tal_count(*flows) - 1; i >= 0; i--) { + if (!amount_msat_is_zero((*flows)[i]->delivers)) break; - tal_arr_remove(&flows_index, i); + tal_arr_remove(flows, i); } - /* finally write the remaining flows */ - write_selected_flows(working_ctx, flows_index, flows); - tal_free(working_ctx); return NULL; From d19e0abc1f52b9f98dba1553f3c99d0ac9dbc700 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:45:31 +1030 Subject: [PATCH 08/15] askrene: make increase_flows() use the raw flows array. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 35 +++++------------------------------ 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 0072f366b8ae..2f44b17ae464 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -168,15 +168,6 @@ enum why_capped { }; /* Reverse order: bigger first */ -static int revcmp_flows(const size_t *a, const size_t *b, struct flow **flows) -{ - if (amount_msat_eq(flows[*a]->delivers, flows[*b]->delivers)) - return 0; - if (amount_msat_greater(flows[*a]->delivers, flows[*b]->delivers)) - return -1; - return 1; -} - static int revcmp_flows_noidx(struct flow *const *a, struct flow *const *b, void *unused) { if (amount_msat_eq((*a)->delivers, (*b)->delivers)) @@ -317,17 +308,6 @@ remove_htlc_min_violations(const tal_t *ctx, struct route_query *rq, return error_message; } -static struct amount_msat sum_all_deliver(struct flow **flows, - size_t *flows_index) -{ - struct amount_msat all_deliver = AMOUNT_MSAT(0); - for (size_t i = 0; i < tal_count(flows_index); i++) { - if (!amount_msat_accumulate(&all_deliver, - flows[flows_index[i]]->delivers)) - abort(); - } - return all_deliver; -} static struct amount_msat sum_all_deliver_noidx(struct flow **flows) { @@ -409,7 +389,6 @@ static struct amount_msat remove_excess(struct flow ***flows, * Returns the total delivery amount. */ static struct amount_msat increase_flows(const struct route_query *rq, struct flow **flows, - size_t **flows_index, struct amount_msat deliver, double tolerance) { @@ -417,20 +396,19 @@ static struct amount_msat increase_flows(const struct route_query *rq, return AMOUNT_MSAT(0); struct amount_msat all_deliver, defect; - all_deliver = sum_all_deliver(flows, *flows_index); + all_deliver = sum_all_deliver_noidx(flows); /* early exit: target is already met */ if (!amount_msat_sub(&defect, deliver, all_deliver) || amount_msat_is_zero(defect)) return all_deliver; - asort(*flows_index, tal_count(*flows_index), revcmp_flows, flows); + asort(flows, tal_count(flows), revcmp_flows_noidx, NULL); all_deliver = AMOUNT_MSAT(0); for (size_t i = 0; - i < tal_count(*flows_index) && !amount_msat_is_zero(defect); i++) { - const size_t index = (*flows_index)[i]; - struct flow *flow = flows[index]; + i < tal_count(flows) && !amount_msat_is_zero(defect); i++) { + struct flow *flow = flows[i]; struct amount_msat can_add = defect, amt; /* no more than tolerance */ @@ -506,10 +484,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, } /* increase flows if necessary to meet the target */ - increase_flows(rq, *flows, &flows_index, deliver, /* tolerance = */ 0.02); - - /* finally write the remaining flows */ - write_selected_flows(working_ctx, flows_index, flows); + increase_flows(rq, *flows, deliver, /* tolerance = */ 0.02); /* detect htlc_min violations */ for (size_t i = 0; i < tal_count(*flows);) { From df22985f2b2a453e4074fe84d3c0589e7eaa00f1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:46:31 +1030 Subject: [PATCH 09/15] askrene: Remove index indirection from squash_flows, simplify sorting. We don't need to convert to strings, we can compare directly. This removes the final use of the index arrays. This of course changes the order of returned routes, which alters test_real_biases, since that biases against the final channel in the *first* route. Took me far too long to diagnose that! Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 89 ++++++++++++---------------------------- tests/test_askrene.py | 4 +- 2 files changed, 29 insertions(+), 64 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 2f44b17ae464..5d84fcb9406f 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -433,24 +433,6 @@ static struct amount_msat increase_flows(const struct route_query *rq, return all_deliver; } -static void write_selected_flows(const tal_t *ctx, size_t *flows_index, - struct flow ***flows) -{ - struct flow **tmp_flows = tal_arr(ctx, struct flow *, 0); - for (size_t i = 0; i < tal_count(flows_index); i++) { - tal_arr_expand(&tmp_flows, (*flows)[flows_index[i]]); - (*flows)[flows_index[i]] = NULL; - } - for (size_t i = 0; i < tal_count(*flows); i++) { - (*flows)[i] = tal_free((*flows)[i]); - } - tal_resize(flows, 0); - for (size_t i = 0; i < tal_count(tmp_flows); i++) { - tal_arr_expand(flows, tmp_flows[i]); - } - tal_free(tmp_flows); -} - const char *refine_flows(const tal_t *ctx, struct route_query *rq, struct amount_msat deliver, struct flow ***flows, u32 *bottleneck_idx) @@ -517,64 +499,47 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, return error_message; } -/* Order of flows according to path string */ -static int cmppath_flows(const size_t *a, const size_t *b, char **paths_str) +/* Order of flows in lexicographic order */ +static int cmppath_flows(struct flow *const *a, struct flow *const *b, void *unused) { - return strcmp(paths_str[*a], paths_str[*b]); + const struct flow *fa = *a, *fb = *b; + for (size_t i = 0; i < tal_count(fa->path); i++) { + /* Shorter comes first */ + if (i >= tal_count(fb->path)) + return 1; + if (fa->path[i] < fb->path[i]) + return -1; + if (fa->path[i] > fb->path[i]) + return 1; + } + /* fa equal to fb, but is fb longer? */ + if (tal_count(fb->path) > tal_count(fa->path)) + return -1; + /* equal */ + return 0; } void squash_flows(const tal_t *ctx, struct route_query *rq, struct flow ***flows) { - const tal_t *working_ctx = tal(ctx, tal_t); - size_t *flows_index = tal_arrz(working_ctx, size_t, tal_count(*flows)); - char **paths_str = tal_arrz(working_ctx, char *, tal_count(*flows)); - struct amount_msat *max_deliverable = tal_arrz( - working_ctx, struct amount_msat, tal_count(*flows)); - - for (size_t i = 0; i < tal_count(flows_index); i++) { - const struct flow *flow = (*flows)[i]; - struct short_channel_id_dir scidd; - flows_index[i] = i; - paths_str[i] = tal_strdup(working_ctx, ""); - max_deliverable[i] = flow_max_deliverable(rq, flow, NULL); - - for (size_t j = 0; j < tal_count(flow->path); j++) { - scidd.scid = - gossmap_chan_scid(rq->gossmap, flow->path[j]); - scidd.dir = flow->dirs[j]; - tal_append_fmt( - &paths_str[i], "%s%s", j > 0 ? "->" : "", - fmt_short_channel_id_dir(working_ctx, &scidd)); - } - } - - asort(flows_index, tal_count(flows_index), cmppath_flows, paths_str); - for (size_t i = 0; i < tal_count(flows_index); i++) { - const size_t j = i + 1; - struct amount_msat combined; - struct amount_msat max = max_deliverable[flows_index[i]]; + asort(*flows, tal_count(*flows), cmppath_flows, NULL); + for (size_t i = 0; i < tal_count(*flows); i++) { + struct flow *flow = (*flows)[i]; /* same path? We merge */ - while (j < tal_count(flows_index) && - cmppath_flows(&flows_index[i], - &flows_index[j], - paths_str) == 0) { - if (!amount_msat_add( - &combined, (*flows)[flows_index[i]]->delivers, - (*flows)[flows_index[j]]->delivers)) + while (i + 1 < tal_count(*flows) && + cmppath_flows(&flow, &(*flows)[i+1], NULL) == 0) { + struct amount_msat combined, max = flow_max_deliverable(rq, flow, NULL); + + if (!amount_msat_add(&combined, flow->delivers, (*flows)[i+1]->delivers)) abort(); /* do we break any HTLC max limits */ if (amount_msat_greater(combined, max)) break; - (*flows)[flows_index[i]]->delivers = combined; - tal_arr_remove(&flows_index, j); + flow->delivers = combined; + tal_arr_remove(flows, i+1); } } - - write_selected_flows(working_ctx, flows_index, flows); - - tal_free(working_ctx); } double flows_probability(const tal_t *ctx, struct route_query *rq, diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 46229650545f..59e0eb8720b2 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -1563,10 +1563,10 @@ def test_real_biases(node_factory, bitcoind): # CI, it's slow. if SLOW_MACHINE: limit = 25 - expected = ({1: 6, 2: 6, 4: 7, 8: 10, 16: 15, 32: 20, 64: 25, 100: 25}, 0) + expected = ({1: 6, 2: 6, 4: 7, 8: 12, 16: 14, 32: 19, 64: 25, 100: 25}, 0) else: limit = 100 - expected = ({1: 19, 2: 27, 4: 36, 8: 48, 16: 71, 32: 83, 64: 95, 100: 96}, 0) + expected = ({1: 22, 2: 25, 4: 36, 8: 53, 16: 69, 32: 80, 64: 96, 100: 96}, 0) l1.rpc.askrene_create_layer('biases') num_changed = {} From 12c4f16291ad5abf846eef6622aa9cbedc92f4ba Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:47:31 +1030 Subject: [PATCH 10/15] askrene: clean up renamed functions. We added _noidx versions of the sort functions, but now they're the only ones, we can rename them to the old names. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 5d84fcb9406f..2b9f1b1a2ea3 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -168,7 +168,7 @@ enum why_capped { }; /* Reverse order: bigger first */ -static int revcmp_flows_noidx(struct flow *const *a, struct flow *const *b, void *unused) +static int revcmp_flows(struct flow *const *a, struct flow *const *b, void *unused) { if (amount_msat_eq((*a)->delivers, (*b)->delivers)) return 0; @@ -309,7 +309,7 @@ remove_htlc_min_violations(const tal_t *ctx, struct route_query *rq, } -static struct amount_msat sum_all_deliver_noidx(struct flow **flows) +static struct amount_msat sum_all_deliver(struct flow **flows) { struct amount_msat all_deliver = AMOUNT_MSAT(0); for (size_t i = 0; i < tal_count(flows); i++) { @@ -330,14 +330,14 @@ static struct amount_msat remove_excess(struct flow ***flows, return AMOUNT_MSAT(0); struct amount_msat all_deliver, excess; - all_deliver = sum_all_deliver_noidx(*flows); + all_deliver = sum_all_deliver(*flows); /* early exit: there is no excess */ if (!amount_msat_sub(&excess, all_deliver, max_deliver) || amount_msat_is_zero(excess)) return all_deliver; - asort(*flows, tal_count(*flows), revcmp_flows_noidx, NULL); + asort(*flows, tal_count(*flows), revcmp_flows, NULL); /* Remove the smaller parts if they deliver less than the * excess. */ @@ -396,14 +396,14 @@ static struct amount_msat increase_flows(const struct route_query *rq, return AMOUNT_MSAT(0); struct amount_msat all_deliver, defect; - all_deliver = sum_all_deliver_noidx(flows); + all_deliver = sum_all_deliver(flows); /* early exit: target is already met */ if (!amount_msat_sub(&defect, deliver, all_deliver) || amount_msat_is_zero(defect)) return all_deliver; - asort(flows, tal_count(flows), revcmp_flows_noidx, NULL); + asort(flows, tal_count(flows), revcmp_flows, NULL); all_deliver = AMOUNT_MSAT(0); for (size_t i = 0; @@ -484,7 +484,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, } /* remove 0 amount flows if any */ - asort(*flows, tal_count(*flows), revcmp_flows_noidx, NULL); + asort(*flows, tal_count(*flows), revcmp_flows, NULL); for (int i = tal_count(*flows) - 1; i >= 0; i--) { if (!amount_msat_is_zero((*flows)[i]->delivers)) break; From 90cfa2e27281dd71fac3d22ea438adaa65732cf5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:48:31 +1030 Subject: [PATCH 11/15] askrene: make increase_flows function more generic. Rewrite it, so it properly takes into account interactions between flows by using reservations. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 112 ++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 36 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 2b9f1b1a2ea3..052d125bf3ef 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -383,54 +383,94 @@ static struct amount_msat remove_excess(struct flow ***flows, return all_deliver; } +/* Return true (and set shortage) if flow doesn't deliver this much */ +static bool flows_short(struct flow **flows, + struct amount_msat deliver, + struct amount_msat *shortage) +{ + return amount_msat_sub(shortage, deliver, sum_all_deliver(flows)) + && !amount_msat_is_zero(*shortage); +} + /* It increases the flows to meet the deliver target. It does not increase any - * flow beyond the tolerance fraction. It doesn't increase any flow above its - * max_deliverable value. - * Returns the total delivery amount. */ -static struct amount_msat increase_flows(const struct route_query *rq, - struct flow **flows, - struct amount_msat deliver, - double tolerance) + * flow beyond the tolerance fraction (unless negative). + * Returns true if it managed to increase total amount to "deliver". */ +static bool increase_flows(const struct route_query *rq, + struct flow **flows, + struct amount_msat deliver, + double tolerance) { - if (tal_count(flows) == 0) - return AMOUNT_MSAT(0); + const tal_t *working_ctx = tal(NULL, tal_t); + struct amount_msat shortage, *ceiling; - struct amount_msat all_deliver, defect; - all_deliver = sum_all_deliver(flows); + /* Record max we can deliver for each flow, so we don't exceed it */ + ceiling = tal_arr(working_ctx, struct amount_msat, tal_count(flows)); + for (size_t i = 0; i < tal_count(flows); i++) { + if (tolerance < 0) + ceiling[i] = deliver; + else if (!amount_msat_scale(&ceiling[i], flows[i]->delivers, 1.0 + tolerance)) + abort(); + } - /* early exit: target is already met */ - if (!amount_msat_sub(&defect, deliver, all_deliver) || - amount_msat_is_zero(defect)) - return all_deliver; + /* This is naive, but since flows can overlap, increasing one + * can alter the remaining capacity of the others! */ + while (flows_short(flows, deliver, &shortage)) { + size_t best_flownum = 0; + struct amount_msat best_remaining = AMOUNT_MSAT(0); + struct reserve_hop **reservations; + struct amount_msat addition; + + /* Because flows can interact, we reserve them all, removing one at a time. */ + reservations = tal_arr(NULL, struct reserve_hop *, tal_count(flows)); + for (size_t i = 0; i < tal_count(flows); i++) { + reservations[i] = new_reservations(reservations, rq); + create_flow_reservations(rq, &reservations[i], flows[i]); + } - asort(flows, tal_count(flows), revcmp_flows, NULL); + /* Find flow with most excess capacity. */ + for (size_t i = 0; i < tal_count(flows); i++) { + struct amount_msat capacity, remaining; - all_deliver = AMOUNT_MSAT(0); - for (size_t i = 0; - i < tal_count(flows) && !amount_msat_is_zero(defect); i++) { - struct flow *flow = flows[i]; - struct amount_msat can_add = defect, amt; + /* flow_max_deliverable considers reservations *and* + * htlc_max. So remove this reservation, to get the + * real maximum for one flow, then replace it. */ + tal_free(reservations[i]); + capacity = flow_max_deliverable(rq, flows[i], NULL); + reservations[i] = new_reservations(reservations, rq); + create_flow_reservations(rq, &reservations[i], flows[i]); - /* no more than tolerance */ - if (!amount_msat_scale(&amt, flow->delivers, tolerance)) - continue; - else - can_add = amount_msat_min(can_add, amt); + /* Don't go above our tolerance */ + if (amount_msat_greater(capacity, ceiling[i])) + capacity = ceiling[i]; - /* no more than max_deliverable */ - if (!amount_msat_sub(&amt, flow_max_deliverable(rq, flow, NULL), - flow->delivers)) - continue; + if (!amount_msat_sub(&remaining, capacity, flows[i]->delivers)) + abort(); + if (amount_msat_greater(remaining, best_remaining)) { + best_flownum = i; + best_remaining = remaining; + } + } + tal_free(reservations); + + /* Add 1/n of the remainder, or all we can if that's less than 10 sats. */ + if (amount_msat_less_sat(shortage, AMOUNT_SAT(10))) + addition = shortage; else - can_add = amount_msat_min(can_add, amt); + addition = amount_msat_div_ceil(shortage, tal_count(flows)); - if (!amount_msat_add(&flow->delivers, flow->delivers, - can_add) || - !amount_msat_sub(&defect, defect, can_add) || - !amount_msat_accumulate(&all_deliver, flow->delivers)) + /* Can't add it? */ + if (amount_msat_less(best_remaining, addition)) { + tal_free(working_ctx); + return false; + } + + if (!amount_msat_accumulate(&flows[best_flownum]->delivers, addition)) + abort(); + if (!amount_msat_deduct(&shortage, addition)) abort(); } - return all_deliver; + tal_free(working_ctx); + return true; } const char *refine_flows(const tal_t *ctx, struct route_query *rq, From ccbdea3247d528b622238e5f4b5ef3f5ee4ae969 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 13 Nov 2025 15:49:31 +1030 Subject: [PATCH 12/15] askrene: handle maxparts parameter values 1 and 0. For 1, we use single-path. For 0, reject. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 985932364191..f04f3e11f29f 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -639,6 +639,12 @@ static struct command_result *do_getroutes(struct command *cmd, "Layer no_mpp_support is active we switch to a " "single path algorithm."); } + if (rq->maxparts == 1 && + info->dev_algo != ALGO_SINGLE_PATH) { + info->dev_algo = ALGO_SINGLE_PATH; + rq_log(tmpctx, rq, LOG_DBG, + "maxparts == 1: switching to a single path algorithm."); + } /* Compute the routes. At this point we might select between multiple * algorithms. Right now there is only one algorithm available. */ @@ -830,6 +836,11 @@ static struct command_result *json_getroutes(struct command *cmd, "amount must be non-zero"); } + if (maxparts == 0) { + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "maxparts must be non-zero"); + } + if (*maxdelay > maxdelay_allowed) { return command_fail(cmd, PAY_USER_ERROR, "maximum delay allowed is %d", From 92a7e494dd2c7c8f860520b55bcc3ae6a31ec1d1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 16 Nov 2025 15:36:13 +1030 Subject: [PATCH 13/15] askrene: implement reduce_num_flows in refine, using increase_flows(). Now we simply call it at the end. We need to check it hasn't violated fee maxima, but otherwise it's simple. Signed-off-by: Rusty Russell Changelog-Fixed: Plugins: `askrene` now handles limits on number of htlcs much more gracefully. --- plugins/askrene/mcf.c | 79 +++++++++++++++------------------------- plugins/askrene/refine.c | 61 +++++++++++++++++++------------ plugins/askrene/refine.h | 9 +++-- tests/test_askrene.py | 21 +++++------ 4 files changed, 83 insertions(+), 87 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 44cdfc728895..c04f72022887 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -1377,9 +1377,6 @@ linear_routes(const tal_t *ctx, struct route_query *rq, struct reserve_hop *reservations = new_reservations(working_ctx, rq); while (!amount_msat_is_zero(amount_to_deliver)) { - size_t num_parts, parts_slots, excess_parts; - u32 bottleneck_idx; - if (timemono_after(time_mono(), deadline)) { error_message = rq_log(ctx, rq, LOG_BROKEN, "%s: timed out after deadline", @@ -1387,24 +1384,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq, goto fail; } - /* FIXME: This algorithm to limit the number of parts is dumb - * for two reasons: - * 1. it does not take into account that several loop - * iterations here may produce two flows along the same - * path that after "squash_flows" become a single flow. - * 2. limiting the number of "slots" to 1 makes us fail to - * see some solutions that use more than one of those - * existing paths. - * - * A better approach could be to run MCF, remove the excess - * paths and then recompute a MCF on a network where each arc is - * one of the previously remaining paths, ie. redistributing the - * payment amount among the selected paths in a cost-efficient - * way. */ new_flows = tal_free(new_flows); - num_parts = tal_count(*flows); - assert(num_parts < rq->maxparts); - parts_slots = rq->maxparts - num_parts; /* If the amount_to_deliver is very small we better use a single * path computation because: @@ -1415,8 +1395,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq, * do not deliver the entire payment amount by just a small * amount. */ if (amount_msat_less_eq(amount_to_deliver, - SINGLE_PATH_THRESHOLD) || - parts_slots == 1) { + SINGLE_PATH_THRESHOLD)) { new_flows = single_path_flow(working_ctx, rq, srcnode, dstnode, amount_to_deliver, mu, delay_feefactor); @@ -1433,7 +1412,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq, } error_message = - refine_flows(ctx, rq, amount_to_deliver, &new_flows, &bottleneck_idx); + refine_flows(ctx, rq, amount_to_deliver, &new_flows, NULL); if (error_message) goto fail; @@ -1457,32 +1436,6 @@ linear_routes(const tal_t *ctx, struct route_query *rq, assert(!amount_msat_is_zero(new_flows[i]->delivers)); } - if (tal_count(new_flows) > parts_slots) { - /* Remove the excees of parts and leave one slot for the - * next round of computations. */ - excess_parts = 1 + tal_count(new_flows) - parts_slots; - } else if (tal_count(new_flows) == parts_slots && - amount_msat_less(all_deliver, amount_to_deliver)) { - /* Leave exactly 1 slot for the next round of - * computations. */ - excess_parts = 1; - } else - excess_parts = 0; - if (excess_parts > 0) { - /* If we removed all the flows we found, avoid selecting them again, - * by disabling one. */ - if (excess_parts == tal_count(new_flows)) - bitmap_set_bit(rq->disabled_chans, bottleneck_idx); - if (!remove_flows(&new_flows, excess_parts)) { - error_message = rq_log(ctx, rq, LOG_BROKEN, - "%s: failed to remove %zu" - " flows from a set of %zu", - __func__, excess_parts, - tal_count(new_flows)); - goto fail; - } - } - /* Is this set of flows too expensive? * We can check if the new flows are within the fee budget, * however in some cases we have discarded some flows at this @@ -1606,6 +1559,34 @@ linear_routes(const tal_t *ctx, struct route_query *rq, /* all set! Now squash flows that use the same path */ squash_flows(ctx, rq, flows); + /* If we're over the number of parts, try to cram excess into the + * largest-capacity parts */ + if (tal_count(*flows) > rq->maxparts) { + struct amount_msat fee; + + error_message = reduce_num_flows(rq, rq, flows, amount, rq->maxparts); + if (error_message) { + *flows = tal_free(*flows); + return error_message; + } + + /* Check fee budget! */ + fee = flowset_fee(rq->plugin, *flows); + if (amount_msat_greater(fee, maxfee)) { + error_message = rq_log(rq, rq, LOG_INFORM, + "After reducing the flows to %zu (i.e. maxparts)," + " we had a fee of %s, greater than " + "max of %s.", + tal_count(*flows), + fmt_amount_msat(tmpctx, fee), + fmt_amount_msat(tmpctx, maxfee)); + if (error_message) { + *flows = tal_free(*flows); + return error_message; + } + } + } + /* flows_probability re-does a temporary reservation so we need to call * it after we have cleaned the reservations we used to build the flows * hence after we freed working_ctx. */ diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 052d125bf3ef..2babc216a4cf 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -1,5 +1,6 @@ #include "config.h" #include +#include #include #include #include @@ -597,29 +598,43 @@ double flows_probability(const tal_t *ctx, struct route_query *rq, return probability; } -/* Compare flows by deliver amount */ -static int reverse_cmp_flows(struct flow *const *fa, struct flow *const *fb, - void *unused UNUSED) +const char *reduce_num_flows(const tal_t *ctx, + const struct route_query *rq, + struct flow ***flows, + struct amount_msat deliver, + size_t num_parts) { - if (amount_msat_eq((*fa)->delivers, (*fb)->delivers)) - return 0; - if (amount_msat_greater((*fa)->delivers, (*fb)->delivers)) - return -1; - return 1; -} + /* Keep the largest flows (not as I originally implemented, the largest + * capacity flows). Here's Lagrang3's analysis: + * + * I think it is better to keep the largest-deliver flows. If we only + * go for the highest capacity we may throw away the low cost benefits + * of the MCF. + + * Hypothetical scenario: MCF finds 3 flows but maxparts=2, + * flow 1: deliver=10, cost=0, capacity=0 + * flow 2: deliver=7, cost=1, capacity=5 + * flow 3: deliver=1, cost=10, capacity=100 + * + * It is better to keep flows 1 and 2 by accomodating 1 more unit of + * flow in flow2 at 1 value expense (per flow), than to keep flows 2 and + * 3 by accomodating 5 more units of flow in flow2 at cost 1 and 5 in + * flow3 at cost 100. + * + * The trade-off is: if we prioritize the delivery value already + * computed by MCF then we find better solutions, but we might fail to + * find feasible solutions sometimes. If we prioritize capacity then we + * generally find bad solutions though we find feasibility more often + * than the alternative. + */ + size_t orig_num_flows = tal_count(*flows); + asort(*flows, orig_num_flows, revcmp_flows, NULL); + tal_resize(flows, num_parts); + + if (!increase_flows(rq, *flows, deliver, -1.0)) + return rq_log(ctx, rq, LOG_INFORM, + "Failed to reduce %zu flows down to maxparts (%zu)", + orig_num_flows, num_parts); -bool remove_flows(struct flow ***flows, u32 n) -{ - if (n == 0) - goto fail; - if (n > tal_count(*flows)) - goto fail; - asort(*flows, tal_count(*flows), reverse_cmp_flows, NULL); - for (size_t count = tal_count(*flows); n > 0; n--, count--) { - assert(count > 0); - tal_arr_remove(flows, count - 1); - } - return true; -fail: - return false; + return NULL; } diff --git a/plugins/askrene/refine.h b/plugins/askrene/refine.h index fcd64be21be6..eeed8f28eef6 100644 --- a/plugins/askrene/refine.h +++ b/plugins/askrene/refine.h @@ -38,7 +38,10 @@ void squash_flows(const tal_t *ctx, struct route_query *rq, double flows_probability(const tal_t *ctx, struct route_query *rq, struct flow ***flows); -/* Helper function: removes n flows from the set. It will remove those flows - * with the lowest amount values. */ -bool remove_flows(struct flow ***flows, u32 n); +/* Modify flows so only N remain, if we can. Returns an error if we cannot. */ +const char *reduce_num_flows(const tal_t *ctx, + const struct route_query *rq, + struct flow ***flows, + struct amount_msat deliver, + size_t num_parts); #endif /* LIGHTNING_PLUGINS_ASKRENE_REFINE_H */ diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 59e0eb8720b2..08b8705473fc 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -1782,8 +1782,7 @@ def test_simple_dummy_channel(node_factory): def test_maxparts_infloop(node_factory, bitcoind): # Three paths from l1 -> l5. - # FIXME: enhance explain_failure! - l1, l2, l3, l4, l5 = node_factory.get_nodes(5, opts=[{'broken_log': 'plugin-cln-askrene.*the obvious route'}] + [{}] * 4) + l1, l2, l3, l4, l5 = node_factory.get_nodes(5) for intermediate in (l2, l3, l4): node_factory.join_nodes([l1, intermediate, l5]) @@ -1805,16 +1804,14 @@ def test_maxparts_infloop(node_factory, bitcoind): final_cltv=5) assert len(route['routes']) == 3 - # Now with maxparts == 2. Usually askrene can't figure out why it failed, - # but sometimes it gets a theory. - with pytest.raises(RpcError): - l1.rpc.getroutes(source=l1.info['id'], - destination=l5.info['id'], - amount_msat=amount, - layers=[], - maxfee_msat=amount, - final_cltv=5, - maxparts=2) + route = l1.rpc.getroutes(source=l1.info['id'], + destination=l5.info['id'], + amount_msat=amount, + layers=[], + maxfee_msat=amount, + final_cltv=5, + maxparts=2) + assert len(route['routes']) == 2 def test_askrene_timeout(node_factory, bitcoind): From d733a7827249604b5753d93faad84e8b3041c80a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 16 Nov 2025 15:36:15 +1030 Subject: [PATCH 14/15] askrene: remove now-unused bottleneck_idx from flow_max_deliverable. Signed-off-by: Rusty Russell --- plugins/askrene/mcf.c | 2 +- plugins/askrene/refine.c | 28 +++++++++------------------- plugins/askrene/refine.h | 5 +---- 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index c04f72022887..a59c6feb6b05 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -1412,7 +1412,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq, } error_message = - refine_flows(ctx, rq, amount_to_deliver, &new_flows, NULL); + refine_flows(ctx, rq, amount_to_deliver, &new_flows); if (error_message) goto fail; diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 2babc216a4cf..3e2f9af82a2a 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -184,33 +184,24 @@ static int revcmp_flows(struct flow *const *a, struct flow *const *b, void *unus // -> check that htlc_max are all satisfied // -> check that (x+1) at least one htlc_max is violated /* Given the channel constraints, return the maximum amount that can be - * delivered. Sets *bottleneck_idx to one of the contraining channels' idx, if non-NULL */ + * delivered. */ static struct amount_msat flow_max_deliverable(const struct route_query *rq, - const struct flow *flow, - u32 *bottleneck_idx) + const struct flow *flow) { struct amount_msat deliver = AMOUNT_MSAT(-1); for (size_t i = 0; i < tal_count(flow->path); i++) { const struct half_chan *hc = &flow->path[i]->half[flow->dirs[i]]; struct amount_msat unused, known_max, htlc_max; - size_t idx = flow->dirs[i] - + 2 * gossmap_chan_idx(rq->gossmap, flow->path[i]); - deliver = amount_msat_sub_fee(deliver, hc->base_fee, hc->proportional_fee); htlc_max = get_chan_htlc_max(rq, flow->path[i], flow->dirs[i]); - if (amount_msat_greater(deliver, htlc_max)) { - if (bottleneck_idx) - *bottleneck_idx = idx; + if (amount_msat_greater(deliver, htlc_max)) deliver = htlc_max; - } + get_constraints(rq, flow->path[i], flow->dirs[i], &unused, &known_max); - if (amount_msat_greater(deliver, known_max)) { - if (bottleneck_idx) - *bottleneck_idx = idx; + if (amount_msat_greater(deliver, known_max)) deliver = known_max; - } } return deliver; } @@ -436,7 +427,7 @@ static bool increase_flows(const struct route_query *rq, * htlc_max. So remove this reservation, to get the * real maximum for one flow, then replace it. */ tal_free(reservations[i]); - capacity = flow_max_deliverable(rq, flows[i], NULL); + capacity = flow_max_deliverable(rq, flows[i]); reservations[i] = new_reservations(reservations, rq); create_flow_reservations(rq, &reservations[i], flows[i]); @@ -475,8 +466,7 @@ static bool increase_flows(const struct route_query *rq, } const char *refine_flows(const tal_t *ctx, struct route_query *rq, - struct amount_msat deliver, struct flow ***flows, - u32 *bottleneck_idx) + struct amount_msat deliver, struct flow ***flows) { const tal_t *working_ctx = tal(ctx, tal_t); const char *error_message = NULL; @@ -487,7 +477,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, for (size_t i = 0; i < tal_count(*flows); i++) { (*flows)[i]->delivers = amount_msat_min((*flows)[i]->delivers, - flow_max_deliverable(rq, (*flows)[i], bottleneck_idx)); + flow_max_deliverable(rq, (*flows)[i])); } /* remove excess from MCF granularity if any */ @@ -570,7 +560,7 @@ void squash_flows(const tal_t *ctx, struct route_query *rq, /* same path? We merge */ while (i + 1 < tal_count(*flows) && cmppath_flows(&flow, &(*flows)[i+1], NULL) == 0) { - struct amount_msat combined, max = flow_max_deliverable(rq, flow, NULL); + struct amount_msat combined, max = flow_max_deliverable(rq, flow); if (!amount_msat_add(&combined, flow->delivers, (*flows)[i+1]->delivers)) abort(); diff --git a/plugins/askrene/refine.h b/plugins/askrene/refine.h index eeed8f28eef6..7c1f1ecbd3d5 100644 --- a/plugins/askrene/refine.h +++ b/plugins/askrene/refine.h @@ -23,12 +23,9 @@ bool create_flow_reservations_verify(const struct route_query *rq, /* Modify flows to meet HTLC min/max requirements. * It takes into account the exact value of the fees expected at each hop. - * If we reduce flows because it's too large for one channel, *bottleneck_idx - * is set to the idx of a channel which caused a reduction (if non-NULL). */ const char *refine_flows(const tal_t *ctx, struct route_query *rq, - struct amount_msat deliver, struct flow ***flows, - u32 *bottleneck_idx); + struct amount_msat deliver, struct flow ***flows); /* Duplicated flows are merged into one. This saves in base fee and HTLC fees. */ From 60dc918bcef28102b9eb8f6ceb0060f11c397d95 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 16 Nov 2025 15:40:02 +1030 Subject: [PATCH 15/15] askrene: neated flow array handling, by freeing flows we discard. Pointed out by @Lagrang3; he's right, while it's a temporary leak the way we use flows, it's still a trap. Signed-off-by: Rusty Russell --- plugins/askrene/refine.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/plugins/askrene/refine.c b/plugins/askrene/refine.c index 3e2f9af82a2a..4308e63a9ac9 100644 --- a/plugins/askrene/refine.c +++ b/plugins/askrene/refine.c @@ -312,6 +312,13 @@ static struct amount_msat sum_all_deliver(struct flow **flows) return all_deliver; } +/* Remove and free the flow */ +static void del_flow_from_arr(struct flow ***flows, size_t i) +{ + tal_free((*flows)[i]); + tal_arr_remove(flows, i); +} + /* It reduces the amount of the flows and/or removes some flows in order to * deliver no more than max_deliver. It will leave at least one flow. * Returns the total delivery amount. */ @@ -340,7 +347,7 @@ static struct amount_msat remove_excess(struct flow ***flows, if (!amount_msat_deduct(&all_deliver, (*flows)[i]->delivers)) abort(); - tal_arr_remove(flows, i); + del_flow_from_arr(flows, i); } /* If we still have some excess, remove it from the @@ -511,7 +518,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, if (error_message) goto fail; /* htlc_min is not met for this flow */ - tal_arr_remove(flows, i); + del_flow_from_arr(flows, i); } /* remove 0 amount flows if any */ @@ -519,7 +526,7 @@ const char *refine_flows(const tal_t *ctx, struct route_query *rq, for (int i = tal_count(*flows) - 1; i >= 0; i--) { if (!amount_msat_is_zero((*flows)[i]->delivers)) break; - tal_arr_remove(flows, i); + del_flow_from_arr(flows, i); } tal_free(working_ctx); @@ -568,7 +575,7 @@ void squash_flows(const tal_t *ctx, struct route_query *rq, if (amount_msat_greater(combined, max)) break; flow->delivers = combined; - tal_arr_remove(flows, i+1); + del_flow_from_arr(flows, i+1); } } }