From f0331cd82e107fa1eac68eb92136ef3bf90565af Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 18 Sep 2024 17:15:27 +0930 Subject: [PATCH] askrene: add a "refining" step to add fees and handle corner cases. This is the root cause of the problem worked around in 50949b7b9ca2 "askrene: hack in some padding so we don't overflow capacities." When adding fees to flows, we didn't recheck the boundary conditions: in renepay this is done by routebuilder. Fortunately, we can use our "reservations" infrastructure to temporarily use capacity as we process flows, so we handle the cases where they are not independent correclty. My assumption is that the resulting errors are small, so we divide them between the remaining flows based on highest-to-least probability. Signed-off-by: Rusty Russell --- plugins/askrene/askrene.c | 357 ++++++++++++++++++++++++++++++++++++++ plugins/askrene/flow.c | 11 ++ plugins/askrene/flow.h | 3 + plugins/askrene/mcf.c | 7 - tests/test_askrene.py | 20 +-- 5 files changed, 381 insertions(+), 17 deletions(-) diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index 2f13e21fc7..401313aa35 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -232,6 +232,355 @@ static void add_free_source(struct plugin *plugin, } } +struct reservations { + struct short_channel_id_dir *scidds; + struct amount_msat *amounts; +}; + +static void destroy_reservations(struct reservations *r, struct askrene *askrene) +{ + assert(tal_count(r->scidds) == tal_count(r->amounts)); + if (reserves_remove(askrene->reserved, + r->scidds, r->amounts, + tal_count(r->scidds)) != tal_count(r->scidds)) { + plugin_err(askrene->plugin, "Failed to remove reservations?"); + } +} + +static struct reservations *new_reservations(const tal_t *ctx, + struct route_query *rq) +{ + struct reservations *r = tal(ctx, struct reservations); + r->scidds = tal_arr(r, struct short_channel_id_dir, 0); + r->amounts = tal_arr(r, struct amount_msat, 0); + + /* Unreserve on free */ + tal_add_destructor2(r, destroy_reservations, get_askrene(rq->plugin)); + return r; +} + +/* Add reservation: we (ab)use this to temporarily avoid over-usage as + * we refine. */ +static void add_reservation(struct reservations *r, + struct route_query *rq, + const struct flow *flow, + size_t i, + struct amount_msat amt) +{ + struct short_channel_id_dir scidd; + struct askrene *askrene = get_askrene(rq->plugin); + size_t idx; + + scidd.scid = gossmap_chan_scid(rq->gossmap, flow->path[i]); + scidd.dir = flow->dirs[i]; + + /* This should not happen, but simply don't reserve if it does */ + if (!reserves_add(askrene->reserved, &scidd, &amt, 1)) { + plugin_log(rq->plugin, LOG_BROKEN, + "Failed to reserve %s in %s", + fmt_amount_msat(tmpctx, amt), + fmt_short_channel_id_dir(tmpctx, &scidd)); + return; + } + + /* Set capacities entry to 0 so it get_constraints() looks in reserve. */ + idx = gossmap_chan_idx(rq->gossmap, flow->path[i]); + if (idx < tal_count(rq->capacities)) + rq->capacities[idx] = 0; + + /* Record so destructor will unreserve */ + tal_arr_expand(&r->scidds, scidd); + tal_arr_expand(&r->amounts, amt); +} + +/* We have a basic set of flows, but we need to add fees. This can + * push us again over capacity or htlc_maximum_msat. + * + * We may have to reduce the flow amount in response to these. + */ +static const char *constrain_flow(const tal_t *ctx, + struct route_query *rq, + struct flow *flow, + struct reservations *reservations) +{ + struct amount_msat msat; + int decreased = -1; + const char *why_decreased = NULL; + + /* Walk backwards, adding fees and testing for htlc_max and + * capacity limits. */ + msat = flow->delivers; + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat min, max; + const char *max_cause; + + /* We can pass constraints due to addition of fees! */ + get_constraints(rq, flow->path[i], flow->dirs[i], &min, &max); + if (amount_msat_less(amount_msat(fp16_to_u64(h->htlc_max)), max)) { + max_cause = "htlc_maximum_msat of "; + max = amount_msat(fp16_to_u64(h->htlc_max)); + } else { + max_cause = "channel capacity of "; + } + + /* If amount is > max, we decrease and add note it in + * case something goes wrong later. */ + if (amount_msat_greater(msat, max)) { + plugin_log(rq->plugin, LOG_DBG, + "Decreased %s to %s%s across %s", + fmt_amount_msat(tmpctx, msat), + max_cause, + fmt_amount_msat(tmpctx, max), + fmt_flows_step_scid(tmpctx, rq, flow, i)); + msat = max; + decreased = i; + why_decreased = max_cause; + } + + /* Reserve it, so if the next flow asks about the same channel, + it will see the reduced capacity from this one. */ + add_reservation(reservations, rq, flow, i, msat); + + if (!amount_msat_add_fee(&msat, h->base_fee, h->proportional_fee)) + plugin_err(rq->plugin, "Adding fee to amount"); + } + + /* Now we know how much we could send, figure out how much would be + * actually delivered. Here we also check for min_htlc violations. */ + for (size_t i = 0; i < tal_count(flow->path); i++) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat next, min = amount_msat(fp16_to_u64(h->htlc_min)); + + next = amount_msat_sub_fee(msat, + h->base_fee, h->proportional_fee); + + /* These failures are incredibly unlikely, but possible */ + if (amount_msat_is_zero(next)) { + return tal_fmt(ctx, "Amount %s cannot pay its own fees across %s", + fmt_amount_msat(tmpctx, msat), + fmt_flows_step_scid(tmpctx, rq, flow, i)); + } + + /* Does happen if we try to pay 1 msat, and all paths have 1000msat min */ + if (amount_msat_less(next, min)) { + return tal_fmt(ctx, "Amount %s below minimum across %s", + fmt_amount_msat(tmpctx, next), + fmt_flows_step_scid(tmpctx, rq, flow, i)); + } + + msat = next; + } + + if (!amount_msat_eq(flow->delivers, msat)) { + plugin_log(rq->plugin, LOG_DBG, "Flow changed to deliver %s not %s, because max constrained by %s%s", + fmt_amount_msat(tmpctx, msat), + fmt_amount_msat(tmpctx, flow->delivers), + why_decreased ? why_decreased : NULL, + decreased == -1 ? "none" + : fmt_flows_step_scid(tmpctx, rq, flow, decreased)); + flow->delivers = msat; + } + + return NULL; +} + +/* Check out remaining capacity for this flow. Changes as other flows get + * increased (which sets reservations) */ +static struct amount_msat flow_remaining_capacity(struct route_query *rq, + const struct flow *flow) +{ + struct amount_msat max_msat = AMOUNT_MSAT(-1ULL); + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat min, max; + + /* We can pass constraints due to addition of fees! */ + get_constraints(rq, flow->path[i], flow->dirs[i], &min, &max); + max = amount_msat_min(max, amount_msat(fp16_to_u64(h->htlc_max))); + + max_msat = amount_msat_min(max_msat, max); + if (!amount_msat_add_fee(&max_msat, h->base_fee, h->proportional_fee)) + max_msat = AMOUNT_MSAT(-1ULL); + } + + /* Calculate deliverable max */ + for (size_t i = 0; i < tal_count(flow->path); i++) { + const struct half_chan *h = flow_edge(flow, i); + max_msat = amount_msat_sub_fee(max_msat, + h->base_fee, h->proportional_fee); + } + return max_msat; +} + +static struct flow *pick_most_likely_flow(struct route_query *rq, + struct flow **flows, + struct amount_msat additional) +{ + double best_prob = 0; + struct flow *best_flow = NULL; + + for (size_t i = 0; i < tal_count(flows); i++) { + struct amount_msat cap; + double prob = flow_probability(flows[i], rq); + if (prob < best_prob) + continue; + cap = flow_remaining_capacity(rq, flows[i]); + if (amount_msat_less(cap, additional)) + continue; + best_prob = prob; + best_flow = flows[i]; + plugin_log(rq->plugin, LOG_DBG, "Best flow is #%zu!", i); + } + + return best_flow; +} + +/* Flow is now delivering `extra`, so modify reservations */ +static void add_to_flow(struct flow *flow, + struct route_query *rq, + struct reservations *reservations, + struct amount_msat extra) +{ + struct amount_msat orig, updated; + + orig = flow->delivers; + if (!amount_msat_add(&updated, orig, extra)) + abort(); + + flow->delivers = updated; + + /* Now add reservations accordingly (effects constraints on other flows) */ + for (int i = tal_count(flow->path) - 1; i >= 0; i--) { + const struct half_chan *h = flow_edge(flow, i); + struct amount_msat diff; + + /* Can't happen, since updated >= orig */ + if (!amount_msat_sub(&diff, updated, orig)) + abort(); + add_reservation(reservations, rq, flow, i, diff); + + if (!amount_msat_add_fee(&orig, h->base_fee, h->proportional_fee)) + abort(); + if (!amount_msat_add_fee(&updated, h->base_fee, h->proportional_fee)) + abort(); + } +} + +/* We got an answer from min-cost-flow, but we now need to add fees. + * This can cause us to hit limits, and even find that some flows are + * impossible. Returns NULL on success, or an error message.*/ +static const char * +refine_with_fees_and_limits(const tal_t *ctx, + struct route_query *rq, + struct amount_msat deliver, + struct flow ***flows) +{ + struct reservations *reservations = new_reservations(NULL, rq); + struct amount_msat more_to_deliver; + const char *flow_constraint_error = NULL; + const char *ret; + + for (size_t i = 0; i < tal_count(*flows);) { + struct flow *flow = (*flows)[i]; + + plugin_log(rq->plugin, LOG_DBG, "Constraining flow %zu: %s", + i, fmt_amount_msat(tmpctx, flow->delivers)); + for (size_t j = 0; j < tal_count(flow->path); j++) { + struct amount_msat min, max; + get_constraints(rq, flow->path[j], flow->dirs[j], &min, &max); + plugin_log(rq->plugin, LOG_DBG, "->%s(max %s)", + fmt_flows_step_scid(tmpctx, rq, flow, j), + fmt_amount_msat(tmpctx, max)); + } + + flow_constraint_error = constrain_flow(tmpctx, rq, flow, reservations); + if (!flow_constraint_error) { + i++; + continue; + } + + plugin_log(rq->plugin, LOG_DBG, "Flow was too constrained: %s", + flow_constraint_error); + /* This flow was reduced to 0 / impossible, remove */ + tal_arr_remove(flows, i); + } + + /* Due to granularity of MCF, we can deliver slightly more than expected: + * trim one in that case. */ + if (!amount_msat_sub(&more_to_deliver, deliver, + flowset_delivers(rq->plugin, *flows))) { + struct amount_msat excess; + if (!amount_msat_sub(&excess, + flowset_delivers(rq->plugin, *flows), + deliver)) + abort(); + for (size_t i = 0; i < tal_count(*flows); i++) { + if (amount_msat_sub(&(*flows)[i]->delivers, (*flows)[i]->delivers, excess)) { + plugin_log(rq->plugin, LOG_DBG, + "Flows delivered %s extra, trimming %zu/%zu", + fmt_amount_msat(tmpctx, excess), + i, tal_count(*flows)); + break; + } + } + if (!amount_msat_eq(flowset_delivers(rq->plugin, *flows), deliver)) { + plugin_err(rq->plugin, + "Flowset delivers %s, can't shed excess?", + fmt_amount_msat(tmpctx, flowset_delivers(rq->plugin, *flows)), + fmt_amount_msat(tmpctx, deliver)); + } + more_to_deliver = AMOUNT_MSAT(0); + } + + /* The residual is minimal. In theory we could add one msat at a time + * to the most probably flow which has capacity. For speed, we break it + * into the number of flows, then assign each one. */ + for (size_t i = 0; i < tal_count(*flows) && !amount_msat_is_zero(more_to_deliver); i++) { + struct flow *f; + struct amount_msat extra; + + /* How much more do we deliver? Round up if we can */ + extra = amount_msat_div(more_to_deliver, tal_count(*flows) - i); + if (amount_msat_less(extra, more_to_deliver)) { + if (!amount_msat_accumulate(&extra, AMOUNT_MSAT(1))) + abort(); + } + + /* In theory, this can happen. If it ever does, we + * could try MCF again for the remainder. */ + f = pick_most_likely_flow(rq, *flows, extra); + if (!f) { + ret = tal_fmt(ctx, "We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!", + fmt_amount_msat(tmpctx, more_to_deliver)); + goto out; + } + + /* Make this flow deliver +extra, and modify reservations */ + add_to_flow(f, rq, reservations, extra); + + /* Should not happen, since extra comes from div... */ + if (!amount_msat_sub(&more_to_deliver, more_to_deliver, extra)) + abort(); + } + + if (!amount_msat_eq(deliver, flowset_delivers(rq->plugin, *flows))) { + /* This should only happen if there were no flows */ + if (tal_count(*flows) == 0) { + ret = flow_constraint_error; + goto out; + } + plugin_err(rq->plugin, "Flows delivered only %s of %s?", + fmt_amount_msat(tmpctx, flowset_delivers(rq->plugin, *flows)), + fmt_amount_msat(tmpctx, deliver)); + } + ret = NULL; + +out: + tal_free(reservations); + return ret; +} + /* Returns an error message, or sets *routes */ static const char *get_routes(const tal_t *ctx, struct plugin *plugin, @@ -371,6 +720,14 @@ static const char *get_routes(const tal_t *ctx, goto out; } + /* The above did not take into account the extra funds to pay + * fees, so we try to adjust now. We could re-run MCF if this + * fails, but failure basically never happens where payment is + * still possible */ + ret = refine_with_fees_and_limits(ctx, rq, amount, &flows); + if (ret) + goto out; + /* Convert back into routes, with delay and other information fixed */ *routes = tal_arr(ctx, struct route *, tal_count(flows)); *amounts = tal_arr(ctx, struct amount_msat, tal_count(flows)); diff --git a/plugins/askrene/flow.c b/plugins/askrene/flow.c index 318c79e721..f2bb090964 100644 --- a/plugins/askrene/flow.c +++ b/plugins/askrene/flow.c @@ -249,6 +249,17 @@ u64 flows_worst_delay(struct flow **flows) return maxdelay; } +const char *fmt_flows_step_scid(const tal_t *ctx, + const struct route_query *rq, + const struct flow *flow, size_t i) +{ + struct short_channel_id_dir scidd; + + scidd.scid = gossmap_chan_scid(rq->gossmap, flow->path[i]); + scidd.dir = flow->dirs[i]; + return fmt_short_channel_id_dir(ctx, &scidd); +} + #ifndef SUPERVERBOSE_ENABLED #undef SUPERVERBOSE #endif diff --git a/plugins/askrene/flow.h b/plugins/askrene/flow.h index 9336f83007..4a9006566c 100644 --- a/plugins/askrene/flow.h +++ b/plugins/askrene/flow.h @@ -62,4 +62,7 @@ u64 flow_delay(const struct flow *flow); /* Max CLTV any of these flows requires */ 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); #endif /* LIGHTNING_PLUGINS_ASKRENE_FLOW_H */ diff --git a/plugins/askrene/mcf.c b/plugins/askrene/mcf.c index 737bf00785..8d454f6a82 100644 --- a/plugins/askrene/mcf.c +++ b/plugins/askrene/mcf.c @@ -454,13 +454,6 @@ static void linearize_channel(const struct pay_parameters *params, /* This takes into account any payments in progress. */ get_constraints(params->rq, c, dir, &mincap, &maxcap); - /* We seem to have some rounding error (perhaps due to our use - * of sats and fee interactions?). Since it's unusual to see - * a large unmber of flows, even if each overflows by 1 sat, - * 5 sats should be plenty. */ - if (!amount_msat_sub(&maxcap, maxcap, AMOUNT_MSAT(5000))) - maxcap = AMOUNT_MSAT(0); - /* Assume if min > max, min is wrong */ if (amount_msat_greater(mincap, maxcap)) mincap = maxcap; diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 7d6b286053..4d402199eb 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -170,8 +170,8 @@ def test_getroutes(node_factory): amount_msat=1000, layers=[], maxfee_msat=1000, - final_cltv=99) == {'probability_ppm': 999998, - 'routes': [{'probability_ppm': 999998, + final_cltv=99) == {'probability_ppm': 999999, + 'routes': [{'probability_ppm': 999999, 'final_cltv': 99, 'amount_msat': 1000, 'path': [{'short_channel_id': '0x1x0', @@ -185,8 +185,8 @@ def test_getroutes(node_factory): amount_msat=100000, layers=[], maxfee_msat=5000, - final_cltv=99) == {'probability_ppm': 999797, - 'routes': [{'probability_ppm': 999797, + final_cltv=99) == {'probability_ppm': 999798, + 'routes': [{'probability_ppm': 999798, 'final_cltv': 99, 'amount_msat': 100000, 'path': [{'short_channel_id': '0x1x0', @@ -248,11 +248,11 @@ def test_getroutes(node_factory): 10000000, [[{'short_channel_id': '0x2x1', 'next_node_id': nodemap[2], - 'amount_msat': 505000, + 'amount_msat': 500000, 'delay': 99 + 6}], [{'short_channel_id': '0x2x3', 'next_node_id': nodemap[2], - 'amount_msat': 9495009, + 'amount_msat': 9500009, 'delay': 99 + 6}]]) @@ -310,8 +310,8 @@ def test_getroutes_auto_sourcefree(node_factory): amount_msat=1000, layers=['auto.sourcefree'], maxfee_msat=1000, - final_cltv=99) == {'probability_ppm': 999998, - 'routes': [{'probability_ppm': 999998, + final_cltv=99) == {'probability_ppm': 999999, + 'routes': [{'probability_ppm': 999999, 'final_cltv': 99, 'amount_msat': 1000, 'path': [{'short_channel_id': '0x1x0', @@ -325,8 +325,8 @@ def test_getroutes_auto_sourcefree(node_factory): amount_msat=100000, layers=['auto.sourcefree'], maxfee_msat=5000, - final_cltv=99) == {'probability_ppm': 999797, - 'routes': [{'probability_ppm': 999797, + final_cltv=99) == {'probability_ppm': 999798, + 'routes': [{'probability_ppm': 999798, 'final_cltv': 99, 'amount_msat': 100000, 'path': [{'short_channel_id': '0x1x0',