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

wait command for generic events, invoices first #6127

Merged
merged 12 commits into from
Jul 23, 2023

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Mar 26, 2023

Based on #6379 Merged, so rebased.

The first commit contains the lightning-wait(7) documentation, and the rest of it is simply delivering the promises!

If this API passes muster, we can expand it to other list commands in future releases.

@rustyrussell rustyrussell force-pushed the guilt/index-wait branch 2 times, most recently from 62be0ad to 29962cd Compare March 27, 2023 04:08
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

I think that I develop my idea of how the paginator API should look, so I share the idea with you maybe we can converge

In my mind, I would like to have a paginator macro or something that intercepts the json rpc command to check if inside the request there are parameters like limit, offset and then decode it inside a jsonrpc_paginator that I can access with cmd->paginator

I also think that having this struct around the paginator will help to implement smarter ways to filter, such as returning a list in the reverse order this is useful for UX or filter by fields by mapping json field <-> SQL colum

I had a complete history of my idea implemented here https://github.com/vincenzopalazzo/lightning/commits/macros/rpc_filtering but I am not happy with how the interceptor works!

Any thought?

P.S: my code is trash, just I was coding while thinking
P.S': I also think that having this paginator struct can help the code generation by putting inside the json schema a flag is_paginator or something and the code gen will deal with it

@@ -1248,7 +1250,7 @@ static void json_add_invoices(struct json_stream *response,
}
} else {
memset(&it, 0, sizeof(it));
while (wallet_invoice_iterate(wallet, &it)) {
while (wallet_invoice_iterate(wallet, &it, listindex, liststart)) {
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo Apr 3, 2023

Choose a reason for hiding this comment

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

Mh! we always avoid putting specific fields for specific filtering, and I think this was the right choice?

Why we do not steal your idea of filtering but inject the field inside the JSON RPC params array?

I wrote an I think modular version of the paginator here vincenzopalazzo@7401722#diff-401ba4b6734345a7b89101b868e5709aa58d0e6d6e33b6df2451bbfb488e087f by stealing your idea of filter filed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several things here:

  1. We need to drive the offset and limit all the way into the db code for efficiency.
  2. We need to have a well-defined stable order (this patch defines two in fact, creation and update order) otherwise pagination can occasionally miss entries.

Now, we could hide these, as we did for filtering, but I don't think that's a good idea, since they only apply to a small subset of commands, and you probably really care if they are ignored!

We are unlikely to implement generic output selection, simply because doing it efficiently means we have to have a powerful db engine which maintains a huge number of indexes for each possible query. Hence I expect all usage in practice will be either a single "key" query, or built by keeping your own index and using wait to detect changes, additions or deletions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are unlikely to implement generic output selection, simply because doing it efficiently means we have to have a powerful db engine which maintains a huge number of indexes for each possible query. Hence I expect all usage in practice will be either a single "key" query, or built by keeping your own index and using wait to detect changes, additions or deletions.

Yeah, I agree I was worried into abuse much of this dynamic filtering, but I think we need some way to decide a runtime if I want the result in reverse order or filtering by timestamp.

I had a use case where I would like to have the forwarding by channel id or by timestamp, and maybe have a way to query commands like listnode and listpeer in a batch way! like listnodes batch=[".....", "...."]

We need to have a well-defined stable order (this patch defines two in fact, creation and update order) otherwise pagination can occasionally miss entries.

I guess that I am missing the case where we miss an entry but maybe I am missing just the use case where this happens.

optional string label = 1;
optional string invstring = 2;
optional bytes payment_hash = 3;
optional string offer_id = 4;
optional ListinvoicesIndex index = 5;

Choose a reason for hiding this comment

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

Small question here:
Would it also be an option to remove the enum and have:

message ListinvoicesRequest {
	optional string label = 1;
	optional string invstring = 2;
	optional bytes payment_hash = 3;
	optional string offer_id = 4;
	optional uint64 start_created = 5;
	optional uint64 start_updated = 6;
	optional uint64 limit = 7;
}

Most likely I'm missing something ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are generated, I have no idea: @cdecker ?

@2Fast2BCn
Copy link

Would it be possible to add this to milestone v23.08: https://github.com/ElementsProject/lightning/milestone/30 ?
Preferably for payments and invoices first.

@vincenzopalazzo vincenzopalazzo added the rfc Request for Comments label May 9, 2023
@niteshbalusu11
Copy link

It'll be cool if this can be expanded to hooks as well where we could also respond.

@rustyrussell rustyrussell added this to the v23.08 milestone Jun 22, 2023
@rustyrussell
Copy link
Contributor Author

Would it be possible to add this to milestone v23.08: https://github.com/ElementsProject/lightning/milestone/30 ? Preferably for payments and invoices first.

I've implemented invoices, and I am hoping to get listsendpays for 23.08. listpays itself is harder, since:

  1. It's in a plugin, so infrastructure will be needed for that.
  2. It's actually a wrapper around listsendpays.

I think it might Just Work, but I could be wrong...

@rustyrussell rustyrussell marked this pull request as ready for review July 6, 2023 06:33
@rustyrussell rustyrussell changed the title [RFC] wait command for generic events wait command for generic events, invoices first Jul 6, 2023
@rustyrussell
Copy link
Contributor Author

It'll be cool if this can be expanded to hooks as well where we could also respond.

I'm not sure how that would work? In particular, hooks freeze progress, and we generally don't want that. But this replaces notifications in some cases.

@rustyrussell rustyrussell force-pushed the guilt/index-wait branch 2 times, most recently from 5a30100 to 0e3ade2 Compare July 10, 2023 02:24
This will initially be for listinvoices, but can be expanded to other
list commands.

It's documented, but it makes promises which currently don't exist:

* listinvoice does not support `index` or `start` yet.
* It doesn't actually fire when invoices change yet.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `wait`: new generic command to wait for events.
We set next_<tablename>_<indexname>_index as separate var fields.

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 do expirations inside the loop, so we can set updated_index
and trigger the callback.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And use a simple array (it's not huge).

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

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>
Now we have defined ordering, we can add a start param.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `listinvoices` has `index` and `start` parameters for listing control.
Changelog-Added: JSON-RPC: `listinvoices` has `limit` parameter for listing control.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If you miss a wait event, you can catch up by doing listinvoices and
getting the max of these fields.  It's also a good debugging clue.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell merged commit d25a8ca into ElementsProject:master Jul 23, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
json-rpc rfc Request for Comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants