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

Wumbo channel support #3612

Merged
merged 11 commits into from
Apr 3, 2020
Merged

Conversation

rustyrussell
Copy link
Contributor

This adds a --wumbo (or, more prosiacly) a --large-channels option (off by default) which means:

  1. We advertize the new option_support_large_channel from the spec.
  2. We will open, and allow others to open, channels >= 2^24 satoshis if they also offer the feature.
  3. Fundchannel 'all' will also not cap funds, if we and the peer offer this feature.

Before merging, someone should interop this with Eclair: in particular, it's the first feature we advertize in the channel_announcement, and we can't advertize the channel if we don't both agree on how that's represented!

@rustyrussell rustyrussell added spec Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! labels Apr 1, 2020
@rustyrussell rustyrussell added this to the 0.8.2 milestone Apr 1, 2020
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.

Commit f692220 may cause a conflict with
commit [e61cfdc][e61cfdc] from the keysend PR since I moved the
plugin_feature_place_names to the features.h and features.c, but should
be relatively easy to fix up once either gets merged.

ACK e816d77

Comment on lines 157 to 158
const struct feature_set *ours,
const u8 *theirfeatures)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: inconsistent naming, should be theirs and ours, or theirfeaturesandourfeatures`.

Comment on lines +740 to +741
else
continue;
Copy link
Member

Choose a reason for hiding this comment

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

This should likely at least produce a warning that the plugin is not in sync with lightningd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure it's always legal to add a field. Though perhaps libplugin should consider itself more tightly bound?

This will help with the next patch, where we wean off using a global
for features: connectd.c has access to the feature bits.

Since connectd might now want to send a message, it needs the crypto_state
non-const, which makes this less trivial than it would otherwise be.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Turns out that unnecessary: all callers can access the feature_set,
so make it much more like a normal primitive.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Shows what features we use in various contexts, including those added
by plugins in getmanifest.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugin: `feature_set` object added to `init`
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is useful in general, but in particular it allows fundchannel to avoid YA
query to figure out if it can wumbo.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON: `connect` returns `features` of the connected peer on success.
This means we correctly reject invoices with features we don't understand.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/wumbo branch 3 times, most recently from b7fa526 to d50abc3 Compare April 2, 2020 06:46
@rustyrussell
Copy link
Contributor Author

OK, I fixed some tests, and did a sweep to rename all the feature sets to "our_features" or "their_features" where appropriate.

…arge_channel.

Note that now we check capacity once we've figured out which peer, which
broke a test (we returned "unknown peer" instead of "capacity exceeded"),
so we rework that too.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-added: `large-channels` option to negotiate opening larger channels.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…icit.

It's almost always "their_features" and "our_features" respectively, so
make those names clear.

Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor Author

Ack 6ea51f0

@rustyrussell rustyrussell merged commit 2f1502a into ElementsProject:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants