Skip to content
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.

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

Askrene: an EXPERIMENTAL MCF plugin and infrastructure #7517

Merged
merged 28 commits into from
Aug 7, 2024

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Aug 2, 2024

(Builds on #7495)

This steals the min-cost-flow implementation from renepay, neatens it up for standalone use, and wraps it in an API. At the heart of the API is "getroutes", but much of the API concerns managing layers: these contain additional information about topology and capacity of channels.

It doesn't do very much by itself, but it does making building a pay plugin much simpler, and it makes testing (and benchmarking) simpler too.

@rustyrussell rustyrussell added this to the v24.08 milestone Aug 2, 2024
@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 2, 2024

it seems that this builds on top of #7495 as well.

@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 2, 2024

Very nice that you added helpers gossmap_chan_htlc_max, gossmap_chan_htlc_min, amount_msat_min and amount_msat_max.

Also the plugin data field, was very much needed. I don't see any plugin yet using this,
but I will start using it right away in renepay.

plugins/askrene/askrene.c Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor Author

it seems that this builds on top of #7495 as well.

Oops, typo! Fixed.

@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 2, 2024

The schema for askrene-age is missing.

@rustyrussell rustyrussell force-pushed the guilt/pay-oracle branch 2 times, most recently from b123e51 to 4fe57e4 Compare August 4, 2024 02:39
@rustyrussell
Copy link
Contributor Author

OK, merged those fixes, thanks!

plugins/askrene/reserve.c Outdated Show resolved Hide resolved
plugins/askrene/reserve.c Outdated Show resolved Hide resolved
rpc_scan(plugin, "getinfo", take(json_out_obj(NULL, NULL, NULL)),
"{id:%}", JSON_SCAN(json_to_node_id, &askrene->my_id));

plugin_set_data(plugin, askrene);
Copy link
Collaborator

@Lagrang3 Lagrang3 Aug 5, 2024

Choose a reason for hiding this comment

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

Instead of calling plugin_set_data here, why not pass the data as argument to plugin_main?

It is not clear to me what happens if I put that set data statement in init but also pass a data pointer in plugin_main. One of the two would come last and override the first. How can we avoid the user shooting himself on the foot?
IDK, maybe plugin_set_data should be kept static, and allow the user to set the data through the plugin_main API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would override if you did both.

But the problem is that you should really do setup before allocating, so if you want to allocate it's awkward to do it before calling plugin_main. For example, leak detection setup.

Perhaps the main argument was the error, and we should have used this in init for all plugins? Any significant plugin has a main function anyway?

@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 5, 2024

I like the idea of the layers to isolate payment specific information. It makes good building blocks for a payment plugin.
I was thinking a payment plugin could have global and private layers of information: global layers would contain constraints that are relevant to any payment and private will be those bounded to a unique payment.

plugins/askrene/layer.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lagrang3 Lagrang3 left a comment

Choose a reason for hiding this comment

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

ACK

@rustyrussell
Copy link
Contributor Author

I like the idea of the layers to isolate payment specific information. It makes good building blocks for a payment plugin. I was thinking a payment plugin could have global and private layers of information: global layers would contain constraints that are relevant to any payment and private will be those bounded to a unique payment.

Yes, I was thinking a "pay" layer which all/any local pay plugins will contribute to, and "tmp.<pluginname." layers for local payment information.

This prompts the question: who maintains the pay layer? In particular, who ages it, and does anyone save and restore it across restarts?

Maybe we should change the age command to set a threshold? Or even not age layers, supply a threshold on the getroutes() command, which discards old modifications for that particular query? (If we want a different age for each layer, this gets a bit more complex though).

I think we can revisit this later, though, once we have experience.

It's experimental, so API may change, but it definitely does need explanation!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This simplifies the callers significantly: all channel_announcements now
have an amount, so gossmap_chan_get_capacity() only fails on a local
modification.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This avoids globals (and means memleak traverses the variables!): we
only change over the test plugin though, to avoid unnecessary churn.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
param_u16 (for delay).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
All the infrastructure and interfaces, but it doesn't do anything yet.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are the repositories of all information.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>


Header from folded patch 'layers-fixup.patch':

fixup! askrene: add layers infrastructure.
They tell us what paths they're using, so we can adjust capacity estimates
accordingly.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>


Header from folded patch 'reserve-fixup.patch':

fixup! askrene: reservation implementation.
We apply all the gossmods for the layers they specified, and create a
naive routine to give the capacity of a channel given those layers.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we never have to look up a local channel when asked the capacity.

We mark these dummy constraints with an MAX timestamp.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't know anything about most channels, so we create an array of
fp16_t containing them.  We zero out ones where we do know something,
and use the previous code as the slow path.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We let the caller choose mu, and iterate if necessary: it can also
check its limits for fees, etc.  Rationalize it to 0-100 inclusive for
human consumption.

This means we don't loop internally, and in fact there's only one
failure mode: we cannot find enough capacity.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Still don't actually try compiling them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
That will be done in the caller, not here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tal does not fail: the default handler (which we use) aborts.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than handling failure, simply report and exit the plugin.
Simplifies error handling.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This adapts them to their new locations, and copies a few more routines.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This make the code use askrene's "struct route_query".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In general, we should be using tmpctx unless there's a specific reason not to.
It's clear, and simplifies the code somewhat.

If tmpctx is not cleaned often enough, we can look at a per-MCF context, but this
seems like premature optimization.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now getroutes actually does something!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This marks all channels around the source node as free (no delay, no fee).  This is normally what we want, if we are calculating a path for ourselves.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…nerated nodes.

This lets us make gossip which contains "real" nodes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will allow us to call an RPC function in the middle.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This populates information on both topology (i.e. unannounced channels) and capacity for the local node using `listpeerchannels`.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell merged commit f8b259d into ElementsProject:master Aug 7, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants