xpay reserve clash #8685
xpay reserve clash #8685
Conversation
|
@rustyrussell it is possible to backport this also on 25.05 if it is necessary? thanks! |
| struct askrene *askrene = get_askrene(cmd->plugin); | ||
| struct json_stream *response; | ||
|
|
||
| /* FIXME: We could allow layer names here? */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, indeed. One reason I didn't implement it, since I can't tell how it would be used.
| /* 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
rustyrussell
force-pushed
the
guilt/askrene-reserve-clash
branch
from
fb4fb9e to
574cb1c
Compare
…verlap. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/askrene-reserve-clash
branch
from
574cb1c to
765b209
Compare
We have the issue of aliases: xpay uses scids like 0x0x0 for routehints and blinded paths, and then can apply reservations to them. But generally, reservations are *global*, so we need to differentiate. Changelog-Added: Plugins: `askrene-reserve` and `askrene-unreserve` can take an optional `layer` inside `path` elements. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We generate fake scids for routehints and blinded paths. But then we were placing reservations on them as if they were global. If there are two xpays going at once these reservations will clash, even though the same scid refers to different channels. Reported-by: @Lagrang3 Changelog-Fixed: xpay: fixed theoretical clash with simultanous payments via routehints and blinded paths. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/askrene-reserve-clash
branch
from
765b209 to
f7857f2
Compare
rustyrussell
dismissed
Lagrang3’s stale review
I understand your objections, but in this case I am going to perform the minimal fix, without changing the base case that reservations are global.
|
@rustyrussell so if this is not fixing the #8663 this should not be linked to the issue, but ths should fix it #8675 it is already kind of hard to understand when this patch is useful, but I kind have an idea by talking with @Lagrang3 However, this is not fixing the 8663 and could be an issue when people go to review my SoC 2 change request |
|
Right, sorry, I missed that. I will add this. |
3 participants
As spotted by @Lagrang3: reservations are global, but some channels are local. This allows some reservations to be local, by having an optional
layerparameter in reservation paths.xpay then uses it.
Fixes: #8663