Skip to content

Commit 8b83d12

Browse files
rustyrussellrustyrussell
committed
askrene: fix bug with reservations used during refinement.
We were trying to get the max capacity of a flow to see if we could add some more sats, and hit an assertion: tests/test_askrene.py:707: ``` DEBUG plugin-cln-askrene: notify msg info: Flow reduced to deliver 88070161msat not 90008000msat, because 107x1x0/1 has remaining capacity 88071042msat DEBUG plugin-cln-askrene: notify msg info: Flow reduced to deliver 284138158msat not 284787000msat, because 108x1x0/1 has remaining capacity 284141000msat **BROKEN** plugin-cln-askrene: Flow delivers 129565000msat but max only 56506138msat INFO plugin-cln-askrene: Killing plugin: exited during normal operation ``` We need to *unreserve* our flow before asking for max capacity. We were also missing a few less important cases where we altered flows without altering the reservation, so fix those too. Signed-off-by: Rusty Russell <[email protected]>
1 parent 948466e commit 8b83d12

File tree

1 file changed

+151
-90
lines changed

1 file changed

+151
-90
lines changed

‎plugins/askrene/refine.c‎

Lines changed: 151 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -36,34 +36,132 @@ static struct reserve_hop *new_reservations(const tal_t *ctx,
3636
return rhops;
3737
}
3838

39-
/* Add reservation: we (ab)use this to temporarily avoid over-usage as
39+
static struct reserve_hop *find_reservation(struct reserve_hop *rhops,
40+
const struct short_channel_id_dir *scidd)
41+
{
42+
for (size_t i = 0; i < tal_count(rhops); i++) {
43+
if (short_channel_id_dir_eq(scidd, &rhops[i].scidd))
44+
return &rhops[i];
45+
}
46+
return NULL;
47+
}
48+
49+
/* Add/update reservation: we (ab)use this to temporarily avoid over-usage as
4050
* we refine. */
4151
static void add_reservation(struct reserve_hop **reservations,
42-
struct route_query *rq,
43-
const struct flow *flow,
44-
size_t i,
52+
const struct route_query *rq,
53+
const struct gossmap_chan *chan,
54+
const struct short_channel_id_dir *scidd,
4555
struct amount_msat amt)
4656
{
47-
struct reserve_hop rhop;
57+
struct reserve_hop rhop, *prev;
4858
struct askrene *askrene = get_askrene(rq->plugin);
4959
size_t idx;
5060

51-
get_scidd(rq->gossmap, flow, i, &rhop.scidd);
61+
/* Update in-place if possible */
62+
prev = find_reservation(*reservations, scidd);
63+
if (prev) {
64+
reserve_remove(askrene->reserved, prev);
65+
if (!amount_msat_accumulate(&prev->amount, amt))
66+
abort();
67+
reserve_add(askrene->reserved, prev, rq->cmd->id);
68+
return;
69+
}
70+
rhop.scidd = *scidd;
5271
rhop.amount = amt;
53-
5472
reserve_add(askrene->reserved, &rhop, rq->cmd->id);
5573

5674
/* Set capacities entry to 0 so it get_constraints() looks in reserve. */
57-
idx = gossmap_chan_idx(rq->gossmap, flow->path[i]);
75+
idx = gossmap_chan_idx(rq->gossmap, chan);
5876
if (idx < tal_count(rq->capacities))
5977
rq->capacities[idx] = 0;
6078

6179
/* Record so destructor will unreserve */
6280
tal_arr_expand(reservations, rhop);
6381
}
6482

65-
/* We aren't allowed to ask for update_details on locally-generated channels,
66-
* so go to the source in that case */
83+
static void subtract_reservation(struct reserve_hop **reservations,
84+
const struct route_query *rq,
85+
const struct gossmap_chan *chan,
86+
const struct short_channel_id_dir *scidd,
87+
struct amount_msat amt)
88+
{
89+
struct reserve_hop *prev;
90+
struct askrene *askrene = get_askrene(rq->plugin);
91+
92+
prev = find_reservation(*reservations, scidd);
93+
assert(prev);
94+
95+
reserve_remove(askrene->reserved, prev);
96+
if (!amount_msat_sub(&prev->amount, prev->amount, amt))
97+
abort();
98+
/* Adding a zero reserve is weird, but legal and easy! */
99+
reserve_add(askrene->reserved, prev, rq->cmd->id);
100+
}
101+
102+
static void create_flow_reservations(const struct route_query *rq,
103+
struct reserve_hop **reservations,
104+
const struct flow *flow)
105+
{
106+
struct amount_msat msat;
107+
108+
msat = flow->delivers;
109+
for (int i = tal_count(flow->path) - 1; i >= 0; i--) {
110+
const struct half_chan *h = flow_edge(flow, i);
111+
struct amount_msat amount_to_reserve;
112+
struct short_channel_id_dir scidd;
113+
114+
get_scidd(rq->gossmap, flow, i, &scidd);
115+
116+
/* Reserve more for local channels if it reduces capacity */
117+
if (!amount_msat_add(&amount_to_reserve, msat,
118+
get_additional_per_htlc_cost(rq, &scidd)))
119+
abort();
120+
121+
add_reservation(reservations, rq, flow->path[i], &scidd,
122+
amount_to_reserve);
123+
if (!amount_msat_add_fee(&msat,
124+
h->base_fee, h->proportional_fee))
125+
plugin_err(rq->plugin, "Adding fee to amount");
126+
}
127+
}
128+
129+
static void remove_flow_reservations(const struct route_query *rq,
130+
struct reserve_hop **reservations,
131+
const struct flow *flow)
132+
{
133+
struct amount_msat msat = flow->delivers;
134+
for (int i = tal_count(flow->path) - 1; i >= 0; i--) {
135+
const struct half_chan *h = flow_edge(flow, i);
136+
struct amount_msat amount_to_reserve;
137+
struct short_channel_id_dir scidd;
138+
139+
get_scidd(rq->gossmap, flow, i, &scidd);
140+
141+
/* Reserve more for local channels if it reduces capacity */
142+
if (!amount_msat_add(&amount_to_reserve, msat,
143+
get_additional_per_htlc_cost(rq, &scidd)))
144+
abort();
145+
146+
subtract_reservation(reservations, rq, flow->path[i], &scidd,
147+
amount_to_reserve);
148+
if (!amount_msat_add_fee(&msat,
149+
h->base_fee, h->proportional_fee))
150+
plugin_err(rq->plugin, "Adding fee to amount");
151+
}
152+
}
153+
154+
static void change_flow_delivers(const struct route_query *rq,
155+
struct flow *flow,
156+
struct reserve_hop **reservations,
157+
struct amount_msat new)
158+
{
159+
remove_flow_reservations(rq, reservations, flow);
160+
flow->delivers = new;
161+
create_flow_reservations(rq, reservations, flow);
162+
}
163+
164+
/* We use an fp16_t approximatin for htlc_max/min: this gets the exact value. */
67165
static struct amount_msat get_chan_htlc_max(const struct route_query *rq,
68166
const struct gossmap_chan *c,
69167
const struct short_channel_id_dir *scidd)
@@ -162,8 +260,7 @@ static enum why_capped flow_max_capacity(const struct route_query *rq,
162260
*/
163261
static const char *constrain_flow(const tal_t *ctx,
164262
struct route_query *rq,
165-
struct flow *flow,
166-
struct reserve_hop **reservations)
263+
struct flow *flow)
167264
{
168265
struct amount_msat deliverable, msat, amount_capped;
169266
enum why_capped why_capped;
@@ -205,66 +302,20 @@ static const char *constrain_flow(const tal_t *ctx,
205302
h->base_fee, h->proportional_fee))
206303
plugin_err(rq->plugin, "Adding fee to amount");
207304
}
208-
209-
/* Finally, reserve so next flow sees reduced capacity. */
210-
msat = flow->delivers;
211-
for (int i = tal_count(flow->path) - 1; i >= 0; i--) {
212-
const struct half_chan *h = flow_edge(flow, i);
213-
struct amount_msat amount_to_reserve;
214-
struct short_channel_id_dir scidd;
215-
216-
get_scidd(rq->gossmap, flow, i, &scidd);
217-
218-
/* Reserve more for local channels if it reduces capacity */
219-
if (!amount_msat_add(&amount_to_reserve, msat,
220-
get_additional_per_htlc_cost(rq, &scidd)))
221-
abort();
222-
223-
add_reservation(reservations, rq, flow, i, amount_to_reserve);
224-
if (!amount_msat_add_fee(&msat,
225-
h->base_fee, h->proportional_fee))
226-
plugin_err(rq->plugin, "Adding fee to amount");
227-
}
228-
229305
return NULL;
230306
}
231307

232-
/* Flow is now delivering `extra` additional msat, so modify reservations */
233-
static void add_to_flow(struct flow *flow,
234-
struct route_query *rq,
235-
struct reserve_hop **reservations,
236-
struct amount_msat extra)
237-
{
238-
struct amount_msat orig, updated;
239-
240-
orig = flow->delivers;
241-
if (!amount_msat_add(&updated, orig, extra))
242-
abort();
243-
244-
flow->delivers = updated;
245-
246-
/* Now add reservations accordingly (effects constraints on other flows) */
247-
for (int i = tal_count(flow->path) - 1; i >= 0; i--) {
248-
const struct half_chan *h = flow_edge(flow, i);
249-
struct amount_msat diff;
250-
251-
/* Can't happen, since updated >= orig */
252-
if (!amount_msat_sub(&diff, updated, orig))
253-
abort();
254-
add_reservation(reservations, rq, flow, i, diff);
255-
256-
if (!amount_msat_add_fee(&orig, h->base_fee, h->proportional_fee))
257-
abort();
258-
if (!amount_msat_add_fee(&updated, h->base_fee, h->proportional_fee))
259-
abort();
260-
}
261-
}
262-
263308
static struct amount_msat flow_remaining_capacity(const struct route_query *rq,
309+
struct reserve_hop **reservations,
264310
const struct flow *flow)
265311
{
266312
struct amount_msat max, diff;
313+
314+
/* Remove ourselves from reservation temporarily, so we don't
315+
* accidentally cap! */
316+
remove_flow_reservations(rq, reservations, flow);
267317
flow_max_capacity(rq, flow, &max, NULL, NULL);
318+
create_flow_reservations(rq, reservations, flow);
268319

269320
if (!amount_msat_sub(&diff, max, flow->delivers))
270321
plugin_err(rq->plugin, "Flow delivers %s but max only %s",
@@ -277,6 +328,7 @@ static struct amount_msat flow_remaining_capacity(const struct route_query *rq,
277328
/* What's the "best" flow to add to? */
278329
static struct flow *pick_most_likely_flow(const struct route_query *rq,
279330
struct flow **flows,
331+
struct reserve_hop **reservations,
280332
struct amount_msat additional)
281333
{
282334
double best_prob = 0;
@@ -287,7 +339,7 @@ static struct flow *pick_most_likely_flow(const struct route_query *rq,
287339
double prob = flow_probability(flows[i], rq);
288340
if (prob < best_prob)
289341
continue;
290-
cap = flow_remaining_capacity(rq, flows[i]);
342+
cap = flow_remaining_capacity(rq, reservations, flows[i]);
291343
if (amount_msat_less(cap, additional))
292344
continue;
293345
best_prob = prob;
@@ -327,11 +379,12 @@ static const char *flow_violates_min(const tal_t *ctx,
327379
* duplicate it. This is naive: it could still fail due to total
328380
* capacity, but it is a corner case anyway. */
329381
static bool duplicate_one_flow(const struct route_query *rq,
382+
struct reserve_hop **reservations,
330383
struct flow ***flows)
331384
{
332385
for (size_t i = 0; i < tal_count(*flows); i++) {
333386
struct flow *flow = (*flows)[i], *new_flow;
334-
struct amount_msat max;
387+
struct amount_msat max, new_amount;
335388
if (flow_max_capacity(rq, flow, &max, NULL, NULL)
336389
!= CAPPED_HTLC_MAX)
337390
continue;
@@ -343,9 +396,12 @@ static bool duplicate_one_flow(const struct route_query *rq,
343396
new_flow->dirs = tal_dup_talarr(new_flow, int,
344397
flow->dirs);
345398
new_flow->delivers = amount_msat_div(flow->delivers, 2);
346-
if (!amount_msat_sub(&flow->delivers,
399+
create_flow_reservations(rq, reservations, new_flow);
400+
401+
if (!amount_msat_sub(&new_amount,
347402
flow->delivers, new_flow->delivers))
348403
abort();
404+
change_flow_delivers(rq, flow, reservations, new_amount);
349405
tal_arr_expand(flows, new_flow);
350406
return true;
351407
}
@@ -367,8 +423,9 @@ refine_with_fees_and_limits(const tal_t *ctx,
367423
for (size_t i = 0; i < tal_count(*flows);) {
368424
struct flow *flow = (*flows)[i];
369425

370-
flow_constraint_error = constrain_flow(tmpctx, rq, flow, &reservations);
426+
flow_constraint_error = constrain_flow(tmpctx, rq, flow);
371427
if (!flow_constraint_error) {
428+
create_flow_reservations(rq, &reservations, flow);
372429
i++;
373430
continue;
374431
}
@@ -389,24 +446,27 @@ refine_with_fees_and_limits(const tal_t *ctx,
389446
deliver))
390447
abort();
391448
for (size_t i = 0; i < tal_count(*flows); i++) {
392-
if (amount_msat_sub(&(*flows)[i]->delivers, (*flows)[i]->delivers, excess)) {
393-
const char *err;
394-
rq_log(tmpctx, rq, LOG_DBG,
395-
"Flow %zu/%zu delivered %s extra, trimming",
396-
i, tal_count(*flows),
397-
fmt_amount_msat(tmpctx, excess));
398-
/* In theory, this can violate min_htlc! Thanks @Lagrang3! */
399-
err = flow_violates_min(tmpctx, rq, (*flows)[i]);
400-
if (err) {
401-
/* This flow was reduced to 0 / impossible, remove */
402-
tal_arr_remove(flows, i);
403-
i--;
404-
/* If this causes failure, indicate why! */
405-
flow_constraint_error = err;
406-
continue;
407-
}
408-
break;
449+
const char *err;
450+
struct amount_msat trimmed;
451+
if (!amount_msat_sub(&trimmed, (*flows)[i]->delivers, excess))
452+
continue;
453+
454+
rq_log(tmpctx, rq, LOG_DBG,
455+
"%s extra, trimming flow %zu/%zu",
456+
fmt_amount_msat(tmpctx, excess), i, tal_count(*flows));
457+
change_flow_delivers(rq, (*flows)[i], &reservations, trimmed);
458+
/* In theory, this can violate min_htlc! Thanks @Lagrang3! */
459+
err = flow_violates_min(tmpctx, rq, (*flows)[i]);
460+
if (err) {
461+
/* This flow was reduced to 0 / impossible, remove */
462+
remove_flow_reservations(rq, &reservations, (*flows)[i]);
463+
tal_arr_remove(flows, i);
464+
i--;
465+
/* If this causes failure, indicate why! */
466+
flow_constraint_error = err;
467+
continue;
409468
}
469+
break;
410470
}
411471

412472
/* Usually this should shed excess, *BUT* maybe one
@@ -430,7 +490,7 @@ refine_with_fees_and_limits(const tal_t *ctx,
430490
* into the number of flows, then assign each one. */
431491
while (!amount_msat_is_zero(more_to_deliver) && tal_count(*flows)) {
432492
struct flow *f;
433-
struct amount_msat extra;
493+
struct amount_msat extra, new_delivers;
434494

435495
/* How much more do we deliver? Round up if we can */
436496
extra = amount_msat_div(more_to_deliver, tal_count(*flows));
@@ -442,9 +502,9 @@ refine_with_fees_and_limits(const tal_t *ctx,
442502
/* This happens when we have a single flow, and hit
443503
* htlc_max. For this corner case, we split into an
444504
* additional flow, but only once! */
445-
f = pick_most_likely_flow(rq, *flows, extra);
505+
f = pick_most_likely_flow(rq, *flows, &reservations, extra);
446506
if (!f) {
447-
if (tried_split_flow || !duplicate_one_flow(rq, flows)) {
507+
if (tried_split_flow || !duplicate_one_flow(rq, &reservations, flows)) {
448508
ret = rq_log(ctx, rq, LOG_BROKEN,
449509
"We couldn't quite afford it, we need to send %s more for fees: please submit a bug report!",
450510
fmt_amount_msat(tmpctx, more_to_deliver));
@@ -454,8 +514,9 @@ refine_with_fees_and_limits(const tal_t *ctx,
454514
continue;
455515
}
456516

457-
/* Make this flow deliver +extra, and modify reservations */
458-
add_to_flow(f, rq, &reservations, extra);
517+
if (!amount_msat_add(&new_delivers, f->delivers, extra))
518+
abort();
519+
change_flow_delivers(rq, f, &reservations, new_delivers);
459520

460521
/* Should not happen, since extra comes from div... */
461522
if (!amount_msat_sub(&more_to_deliver, more_to_deliver, extra))

0 commit comments

Comments
 (0)