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.

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

Jump to bottom

channel type support #4616

Merged
merged 14 commits into from
Sep 13, 2021
Merged

channel type support #4616

merged 14 commits into from
Sep 13, 2021

Conversation

rustyrussell
Copy link
Contributor

Based on #4615

@rustyrussell rustyrussell added protocol These issues are protocol level issues that should be discussed on the protocol spec repo openingd labels Jun 24, 2021
@rustyrussell rustyrussell requested a review from cdecker as a code owner June 24, 2021 05:39
@rustyrussell
Copy link
Contributor Author

Trivial rebase now #4615 is merged

vincenzopalazzo
vincenzopalazzo approved these changes Jul 3, 2021
View reviewed changes
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ack 2c138ea

I think we need to add only the feature supports_open_accept_channel_types in the python test.

P.S: Unfortunately we need another rebase here

@rustyrussell
Copy link
Contributor Author

This is updated to match the new spec, finally!

@rustyrussell rustyrussell force-pushed the guilt/channel_type branch 2 times, most recently from 57e2dfd to e340364 Compare July 12, 2021 07:05
@t-bast
Copy link

t-bast commented Jul 20, 2021

I've tested e340364 against eclair (ACINQ/eclair#1867) and I've found some issues (sorry!).

I have activated anchor outputs in both c-lightning and eclair to be able to test a wide range of cases.
When either eclair or c-lightning proposes the anchor outputs channel type, everything works well.
However, when eclair proposes a different channel type, it doesn't work:

  • If eclair proposes only static_remotekey (feature bit 12), c-lightning rejects the open_channel and says that this is an unknown channel type
  • If eclair proposes a standard channel (empty feature bytes, but channel_type tlv is present), c-lightning acts as if the tlv was missing and sends an accept_channel with the anchor outputs channel type, which doesn't match the channel type in open_channel.

@t-bast
Copy link

t-bast commented Aug 9, 2021

@rustyrussell do you think you will have time this week to fix the issues I reported earlier? It would be great to finalize compatibility testing and be able to merge lightning/bolts#880 during the next spec meeting!

@t-bast t-bast mentioned this pull request Aug 17, 2021
Merged
@cdecker cdecker marked this pull request as draft August 30, 2021 14:46
@rustyrussell rustyrussell marked this pull request as ready for review September 6, 2021 03:03
@rustyrussell rustyrussell force-pushed the guilt/channel_type branch 2 times, most recently from f9d568e to 3379cb2 Compare September 6, 2021 03:30
@rustyrussell rustyrussell changed the title channel type EXPERIMENTAL support channel type support Sep 8, 2021
@rustyrussell
openingd: don't hand redundant feature flags.
16dc3ea 
Openingd can query them itself (as dualopend already does).  And move
the two feature args next to each other on the wire.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
tools: handle generating print templates for CSV without tlvs, messages
9c41420 
We want to use this to handle the simple description for channel_type.

It also needs to handle variable-size types (just like subtypes).

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common/channel_type: wrapper for generated 'struct channel_type'.
74e71f7 
We make it a first-class citizen internally, even though we won't use
it over the wire (at least, non-experimental builds).  This scheme
follows the latest draft, in which features are flagged compulsory.

We also add several helper functions.

Since uses the *even* bits (as per latest spec), not the *odd* bits,
we have some other fixups.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common/initial_channel: use channel_type instead of individual option…
71aa238 
… bools.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: use channel_type, pass to-and-from channeld.
ee2b57d 
Instead of explicit option_static_remotekey and option_anchor_outputs flags.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
openingd: tell lightningd what channel type we negotiated.
d562086 
Currently we actually insist it's the default, but in future it could be
different.

We also need to tell openingd what the channel_type was, if we resume
via openingd_funder_complete().

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
openingd: EXPERIMENTAL_FEATURES provide and use channel_types.
3dd37ec 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
openingd: channel_type TLV no longer experimental.
f86a5a1 
It was merged (but this doesn't update the BOLT quotes, that's in another patch).

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Protocol: We now send and support channel_type in channel open (not dual-funding though).
@rustyrussell
openingd: allow channel_type to *downgrade* from default.
610fcf7 
e.g. you can negotiate anchor_outputs, but still ask for a
non-anchor-output channel.

If/when we make those features compulsory, downgrade will
not be allowed.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: print out support for channel_types in --list-features.
d95b72a 
It's not a separate option, but lnprototest needs it to know to expect
the tlvs.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
openingd: tell them channel_type if signature is bad.
86686ea 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
closingd: fix BOLT quotes to match final spec version.
ec687ab 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common/channel_type: fix up bolt quotes now channel-types is merged.
43bfe74 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
openingd: clean up state properly when a hook says to reject opening.
707164f 
This was just a minor leak, found by CI for
test_openchannel_hook_chaining.  We didn't call negotiation_aborted
which frees various fields: negotiation_failed() does that for us.

```
 MEMLEAK: 0x55b0f2d5f3c8
   label=common/channel_type.c:19:struct channel_type
   backtrace:
     ccan/ccan/tal/tal.c:442 (tal_alloc_)
     common/channel_type.c:19 (channel_type_none)
     common/channel_type.c:27 (channel_type_static_remotekey)
     common/channel_type.c:136 (channel_type_accept)
     openingd/openingd.c:844 (fundee_channel)
     openingd/openingd.c:1240 (handle_peer_in)
     openingd/openingd.c:1510 (main)
   parents:
     openingd/openingd.c:1414:struct state
```

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author 

* If eclair proposes only `static_remotekey` (feature bit 12), c-lightning rejects the `open_channel` and says that this is an unknown channel type

This is now fixed. Initially we only allowed the default type, now we allow downgrades.

* If eclair proposes a standard channel (empty feature bytes, but `channel_type` tlv is present), c-lightning acts as if the tlv was missing and sends an `accept_channel` with the anchor outputs channel type, which doesn't match the channel type in `open_channel`.

This is also fixed: we correctly respond with the empty channel (at least for now: we may required static_remotekey soon).

cdecker
cdecker approved these changes Sep 13, 2021
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Very nice cleanup, I'm starting to like the channel_type proposal after all 😉

ACK 707164f

lightningd/opening_control.c
&supports_shutdown_script)) {
if (!fromwire_openingd_funder_start_reply(fc, resp,
&fc->funding_scriptpubkey,
&supports_shutdown_script,
Copy link
Member

Choose a reason for hiding this comment

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

I take it that the upfront shutdown script is negotiated independently of the channel_type? Or could it be removed and passed via the channel_type instead?

@cdecker cdecker merged commit 88adbbd into ElementsProject:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

@cdecker cdecker cdecker approved these changes

Assignees
No one assigned
Labels
openingd protocol These issues are protocol level issues that should be discussed on the protocol spec repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rustyrussell @t-bast @cdecker @vincenzopalazzo