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

Compulsory modern features, and open-by-channel-type APIs #6864

Merged

Conversation

rustyrussell
Copy link
Contributor

I thought we made static_remotekey, gossip_queries and dataloss_protect required already, but we didn't. However, we still need to handle old channels without static_remotekey, which lead me to finally implement the open-by-channel-type explicit API.

@rustyrussell
Copy link
Contributor Author

@cdecker this broke the GRPC generation, and I cannot figure out why :(

I note that channel_types is missing from GRPC for the existing listpeerchannels return; seems patched out (which seems wrong!), but doing the same for Fundchannel does not seem to work. Here's the error:

node.proto:1597:18: "FundchannelChannel_typeNames" is not defined.
make: *** [Makefile:385: contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py] Error 1

@cdecker
Copy link
Member

cdecker commented Nov 13, 2023

Ok, so it turns out that msggen does not correctly handle arrays of enum values. Since this is the only time we used them, I just overrode it for this type, since it is likely we'll want to reuse the type somewhere else too (listpeerchannels, etc), and from that point of view it's very similar to HtlcState which is also manually managed.

The longer term solution for this would be to have a protobuf-like IDL that allows us to specify the API as a flat namespace of messages that may nest each other, and then generate the JSON Schema from that. That'd also remove the need for unreadable, context-related, names such as FundchannelChannel_typeNames which technically reflects the jq path fundchannel.channel_type.names[], and is generated from the full path to be unique.

@cdecker
Copy link
Member

cdecker commented Nov 13, 2023

Fixed a couple of source lints, and added the missing enum, as well as the override.
Please force-pull before making changes, otherwise my changes will get lost :-)

@rustyrussell rustyrussell force-pushed the guilt/channel-type-apis branch 2 times, most recently from c6764c1 to 2b7667f Compare December 13, 2023 05:52
@rustyrussell rustyrussell force-pushed the guilt/channel-type-apis branch 3 times, most recently from 5917f15 to 86f4ee1 Compare December 15, 2023 06:18
@rustyrussell
Copy link
Contributor Author

Note that this needs rustyrussell/lnprototest#115 so I've changed the upstream for lnprototest temporarily!

rustyrussell and others added 14 commits January 29, 2024 10:04
Otherwise it fails because lnprototest expects us to allow a non-static-channel
channel type.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It fails because this is an array of enum values, which is a
combination we hadn't seen before. Replacing this with a manually
managed enum, since it is likely we'll want to reuse it in the future.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As suggested in lightning/bolts#1092.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_data_loss_protect` is now required (advertized by all but 11 nodes)
As suggested in lightning/bolts#1092.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 11 nodes)
… offered minimum_depth=0.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: `channel_type` reflects option_zeroconf if explicitly negotiated.
We use an array of bit numbers.  We could use an array of names, but the JSON typing is then harder.

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

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

And add request schemas for openchannel_init and fundchannel_start.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `fundchannel_start` and `openchannel_init` now take an optional `channel_type` parameter.
And add a request schema for multifundchannel.

Changelog-Added: JSON-RPC: `fundchannel` and `multifundchannel` now take an optional `channel_type` parameter.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're about to make static_remotekey compulsory, but we still want to
do tests for pre-existing channels.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Let's tell the caller what channel_type they got!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `fundchannel`, `multifundchannel`, `fundchannel_start` and `openchannel_init`: new field `channel_type`.
…turned from fund/openchannel

Could have done this in an earlier commit, but that would be a messy rebase.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For example, lnprototest got the error 'You gave bad parameters: Did not support channel_type ' which doesn't make it clear that it's rejecting the empty channel type.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell merged commit 6778f32 into ElementsProject:master Jan 29, 2024
38 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