Skip to content

xpay reserve clash #8685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 17, 2025
Merged

xpay reserve clash #8685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea0d86a
pytest: test to demonstrate that reservations of "private" channels o…
rustyrussell Nov 17, 2025
8b2b0f8
askrene: add optional layers to reservations.
rustyrussell Nov 17, 2025
f7857f2
xpay: don't place global reservations on generated channels.
rustyrussell Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .msggen.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,7 @@
"Askrene-reservePath": {
"AskRene-Reserve.path[].amount_msat": 3,
"AskRene-Reserve.path[].direction": 2,
"AskRene-Reserve.path[].layer": 5,
"AskRene-Reserve.path[].short_channel_id": 1,
"AskRene-Reserve.path[].short_channel_id_dir": 4
},
Expand All @@ -790,6 +791,7 @@
"Askrene-unreservePath": {
"AskRene-Unreserve.path[].amount_msat": 3,
"AskRene-Unreserve.path[].direction": 2,
"AskRene-Unreserve.path[].layer": 5,
"AskRene-Unreserve.path[].short_channel_id": 1,
"AskRene-Unreserve.path[].short_channel_id_dir": 4
},
Expand Down Expand Up @@ -4548,6 +4550,10 @@
"added": "v24.08",
"deprecated": null
},
"AskRene-Reserve.path[].layer": {
"added": "v25.12",
"deprecated": null
},
"AskRene-Reserve.path[].short_channel_id": {
"added": "v24.08",
"deprecated": null
Expand All @@ -4572,6 +4578,10 @@
"added": "v24.08",
"deprecated": null
},
"AskRene-Unreserve.path[].layer": {
"added": "v25.12",
"deprecated": null
},
"AskRene-Unreserve.path[].short_channel_id": {
"added": "v24.08",
"deprecated": null
Expand Down
2 changes: 2 additions & 0 deletions cln-grpc/proto/node.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cln-grpc/src/convert.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cln-rpc/src/model.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 15 additions & 1 deletion contrib/msggen/msggen/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,7 @@
"layer": {
"type": "string",
"description": [
"The name of the layer to apply this change to."
"The name of the layer to apply this change to. Also causes us to consider any reservations which are local to that layer (which is useful for fake channels where `layer` is set in an `askrene-reserve` `path` object)."
]
},
"short_channel_id_dir": {
Expand Down Expand Up @@ -1879,6 +1879,13 @@
"description": [
"The amount to send into this hop."
]
},
"layer": {
"added": "v25.12",
"type": "string",
"description": [
"The layer to restrict this reservation to. This is only useful for fake channels which are not uniquely identified by `short_channel_id_dir`, which would otherwise confuse multiple unrelated callers."
]
}
}
}
Expand Down Expand Up @@ -1980,6 +1987,13 @@
"description": [
"The amount to send into this hop."
]
},
"layer": {
"added": "v25.12",
"type": "string",
"description": [
"The layer to restrict this reservation to (useful for fake channels)."
]
}
}
}
Expand Down
278 changes: 139 additions & 139 deletions contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion doc/schemas/askrene-inform-channel.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"layer": {
"type": "string",
"description": [
"The name of the layer to apply this change to."
"The name of the layer to apply this change to. Also causes us to consider any reservations which are local to that layer (which is useful for fake channels where `layer` is set in an `askrene-reserve` `path` object)."
]
},
"short_channel_id_dir": {
Expand Down
7 changes: 7 additions & 0 deletions doc/schemas/askrene-reserve.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@
"description": [
"The amount to send into this hop."
]
},
"layer": {
"added": "v25.12",
"type": "string",
"description": [
"The layer to restrict this reservation to. This is only useful for fake channels which are not uniquely identified by `short_channel_id_dir`, which would otherwise confuse multiple unrelated callers."
]
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions doc/schemas/askrene-unreserve.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@
"description": [
"The amount to send into this hop."
]
},
"layer": {
"added": "v25.12",
"type": "string",
"description": [
"The layer to restrict this reservation to (useful for fake channels)."
]
}
}
}
Expand Down
28 changes: 20 additions & 8 deletions plugins/askrene/askrene.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,21 @@ static struct command_result *parse_reserve_hop(struct command *cmd,
struct reserve_hop *rhop)
{
const char *err;
const char *layername = NULL;

err = json_scan(tmpctx, buffer, tok, "{short_channel_id_dir:%,amount_msat:%}",
err = json_scan(tmpctx, buffer, tok, "{short_channel_id_dir:%,amount_msat:%,layer?:%}",
JSON_SCAN(json_to_short_channel_id_dir, &rhop->scidd),
JSON_SCAN(json_to_msat, &rhop->amount));
JSON_SCAN(json_to_msat, &rhop->amount),
JSON_SCAN_TAL(tmpctx, json_strdup, &layername));
if (err)
return command_fail_badparam(cmd, name, buffer, tok, err);
if (layername) {
rhop->layer = find_layer(get_askrene(cmd->plugin), layername);
if (!rhop->layer)
return command_fail_badparam(cmd, name, buffer, tok, "Unknown layer");
} else
rhop->layer = NULL;

return NULL;
}

Expand Down Expand Up @@ -548,8 +557,8 @@ void get_constraints(const struct route_query *rq,
*max = gossmap_chan_get_capacity(rq->gossmap, chan);

/* Finally, if any is in use, subtract that! */
reserve_sub(rq->reserved, &scidd, min);
reserve_sub(rq->reserved, &scidd, max);
reserve_sub(rq->reserved, &scidd, rq->layers, min);
reserve_sub(rq->reserved, &scidd, rq->layers, max);
}

static struct command_result *do_getroutes(struct command *cmd,
Expand Down Expand Up @@ -916,9 +925,11 @@ static struct command_result *json_askrene_unreserve(struct command *cmd,
for (size_t i = 0; i < tal_count(path); i++) {
if (!reserve_remove(askrene->reserved, &path[i])) {
return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
"Unknown reservation for %s",
"Unknown reservation for %s%s%s",
fmt_short_channel_id_dir(tmpctx,
&path[i].scidd));
&path[i].scidd),
path[i].layer ? " on layer " : "",
path[i].layer ? layer_name(path[i].layer) : "");
}
}

Expand All @@ -933,14 +944,15 @@ static struct command_result *json_askrene_listreservations(struct command *cmd,
struct askrene *askrene = get_askrene(cmd->plugin);
struct json_stream *response;

/* FIXME: We could allow layer names here? */
Copy link
Collaborator

@Lagrang3 Lagrang3 Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, why not?
The only question is: if I specify a list of layers here, would I want to get a list of reservations
in which some of them are global or not?

Copy link
Collaborator

@Lagrang3 Lagrang3 Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that get_constraints and thus getroutes fetch the reservations belonging to the request layers
plus the global reservations. I guess the answer should include the global as well.
But on the other hand it is weird to call listreservations specifying some layer and getting global
reservations mixed up in the reply.

Copy link
Contributor Author

@rustyrussell rustyrussell Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed. One reason I didn't implement it, since I can't tell how it would be used.

if (!param(cmd, buffer, params,
NULL))
return command_param_failed();
plugin_log(cmd->plugin, LOG_TRACE, "%s called: %.*s", __func__,
json_tok_full_len(params), json_tok_full(buffer, params));

response = jsonrpc_stream_success(cmd);
json_add_reservations(response, askrene->reserved, "reservations");
json_add_reservations(response, askrene->reserved, "reservations", NULL);
return command_finished(cmd, response);
}

Expand Down Expand Up @@ -1065,7 +1077,7 @@ static struct command_result *json_askrene_inform_channel(struct command *cmd,
case INFORM_CONSTRAINED:
/* It didn't pass, so minimal assumption is that reserve was all used
* then there we were one msat short. */
if (!reserve_accumulate(askrene->reserved, scidd, amount))
if (!reserve_accumulate(askrene->reserved, scidd, layer, amount))
Copy link
Collaborator

@Lagrang3 Lagrang3 Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be documented in askrene-inform-channel.
Now there is a double meaning to the layer argument: it is both the place where the constraint is stored
and the place where reservations are looked for.

I create a reservation of 5 in layer1 and a reservation of 7 in layer2 for the same channel.
If I call askrene-inform-channel ... layer=layer1 I must know that internally askrene will be assuming
5 as the total reservation and not 5+7.

Copy link
Collaborator

@Lagrang3 Lagrang3 Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion on this is that we add an extra argument reservations_layers to specify which layers contain the reservations to be
taken into account. By default only global reservations are in, this makes it backwards compatible.

For example:
I create a global reservation of 1, a reservation of 5 in layer1 and a reservation of 7 in layer2.
Calling askrene-inform-channel ... reservation_layers=[] makes the algorithm to assume the total reservation is 1.
Calling askrene-inform-channel ... reservation_layers=["layer1"] makes the algorithm to assume the total reservation is 1+5=6.
Calling askrene-inform-channel ... reservation_layers=["layer1","layer2"] makes the algorithm to assume the total reservation is 1+5+7=13.
Calling askrene-inform-channel ... reservation_layers=["layer2"] makes the algorithm to assume the total reservation is 1+7=8.

Copy link
Contributor Author

@rustyrussell rustyrussell Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've documented this, and that we only want to put the reservation in the layer when it's not in the gossmap. We are using them to fake up namespaces, which is pretty gross.

This is not a generic mechanism for per-layer reservations. Just a disambiguation one for fake channels, which are not unique.

The right answer to this whole mess is to ensure scids are unique, but that's hard to do properly. We would need to rework things to have askrene assign them. I'm not sure it's worth it (especially since we're closing on rc1).

return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
"Amount overflow with reserves");
if (!amount_msat_deduct(amount, AMOUNT_MSAT(1)))
Expand Down
2 changes: 1 addition & 1 deletion plugins/askrene/explain_failure.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ static const char *why_max_constrained(const tal_t *ctx,
fmt_amount_msat(tmpctx, max));
}

reservations = fmt_reservations(tmpctx, rq->reserved, scidd);
reservations = fmt_reservations(tmpctx, rq->reserved, scidd, rq->layers);
if (reservations) {
if (!ret)
ret = tal_strdup(ctx, "");
Expand Down
3 changes: 3 additions & 0 deletions plugins/askrene/refine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ static void add_reservation(struct reserve_hop **reservations,
}
rhop.scidd = *scidd;
rhop.amount = amt;
/* We don't have to restrict it to a layer, since it's transitory:
* nobody else will see this. */
rhop.layer = NULL;
reserve_add(askrene->reserved, &rhop, rq->cmd->id);

/* Set capacities entry to 0 so it get_constraints() looks in reserve. */
Expand Down
44 changes: 41 additions & 3 deletions plugins/askrene/reserve.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ struct reserve_htable *new_reserve_htable(const tal_t *ctx)
return new_htable(ctx, reserve_htable);
}

static void destroy_reserve(struct reserve *r, struct reserve_htable *reserved)
{
if (!reserve_htable_del(reserved, r))
abort();
}

void reserve_add(struct reserve_htable *reserved,
const struct reserve_hop *rhop,
const char *cmd_id TAKES)
Expand All @@ -49,6 +55,11 @@ void reserve_add(struct reserve_htable *reserved,
r->timestamp = time_mono();
r->cmd_id = tal_strdup(r, cmd_id);

/* If owned by a layer, clean up when layer destroyed */
if (rhop->layer) {
tal_steal(rhop->layer, r);
tal_add_destructor2(r, destroy_reserve, reserved);
}
reserve_htable_add(reserved, r);
}

Expand All @@ -65,8 +76,12 @@ bool reserve_remove(struct reserve_htable *reserved,
r = reserve_htable_getnext(reserved, &rhop->scidd, &rit)) {
if (!amount_msat_eq(r->rhop.amount, rhop->amount))
continue;
if (r->rhop.layer != rhop->layer)
continue;

reserve_htable_del(reserved, r);
/* hops on layers have a destructor which does this. */
if (r->rhop.layer == NULL)
reserve_htable_del(reserved, r);
tal_free(r);
return true;
}
Expand All @@ -93,8 +108,18 @@ void reserves_clear_capacities(struct reserve_htable *reserved,
}
}

static bool layer_in(const struct layer *layer,
const struct layer **layers)
{
for (size_t i = 0; i < tal_count(layers); i++)
if (layer == layers[i])
return true;
return false;
}

void reserve_sub(const struct reserve_htable *reserved,
const struct short_channel_id_dir *scidd,
const struct layer **layers,
struct amount_msat *amount)
{
struct reserve *r;
Expand All @@ -103,13 +128,16 @@ 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 (r->rhop.layer && !layer_in(r->rhop.layer, layers))
continue;
if (!amount_msat_deduct(amount, r->rhop.amount))
*amount = AMOUNT_MSAT(0);
}
}

bool reserve_accumulate(const struct reserve_htable *reserved,
const struct short_channel_id_dir *scidd,
const struct layer *layer,
struct amount_msat *amount)
{
struct reserve *r;
Expand All @@ -118,6 +146,10 @@ bool reserve_accumulate(const struct reserve_htable *reserved,
for (r = reserve_htable_getfirst(reserved, scidd, &rit);
r;
r = reserve_htable_getnext(reserved, scidd, &rit)) {
/* Non-layer ones always get counted. Layered ones have
* to match this layer. */
if (r->rhop.layer && r->rhop.layer != layer)
continue;
if (!amount_msat_add(amount, *amount, r->rhop.amount))
return false;
}
Expand All @@ -126,7 +158,8 @@ bool reserve_accumulate(const struct reserve_htable *reserved,

void json_add_reservations(struct json_stream *js,
const struct reserve_htable *reserved,
const char *fieldname)
const char *fieldname,
const struct layer **layers)
{
struct reserve *r;
struct reserve_htable_iter rit;
Expand All @@ -135,6 +168,8 @@ void json_add_reservations(struct json_stream *js,
for (r = reserve_htable_first(reserved, &rit);
r;
r = reserve_htable_next(reserved, &rit)) {
if (r->rhop.layer && !layer_in(r->rhop.layer, layers))
continue;
json_object_start(js, NULL);
json_add_short_channel_id_dir(js,
"short_channel_id_dir",
Expand All @@ -152,7 +187,8 @@ void json_add_reservations(struct json_stream *js,

const char *fmt_reservations(const tal_t *ctx,
const struct reserve_htable *reserved,
const struct short_channel_id_dir *scidd)
const struct short_channel_id_dir *scidd,
const struct layer **layers)
{
struct reserve *r;
struct reserve_htable_iter rit;
Expand All @@ -162,6 +198,8 @@ const char *fmt_reservations(const tal_t *ctx,
r;
r = reserve_htable_getnext(reserved, scidd, &rit)) {
u64 seconds;
if (r->rhop.layer && !layer_in(r->rhop.layer, layers))
continue;
if (!ret)
ret = tal_strdup(ctx, "");
else
Expand Down
Loading
Loading