plugin: Allow plugins to register featurebits by cdecker · Pull Request #3477 · ElementsProject/lightning

cdecker · 2020-01-31T15:01:15Z

With the htlc_accepted hook merged (#2267), in conjunction with the
sendonion RPC method (#3260), and the recently merged custommsg support
(#3315), plugins can now extend the lightning protocol using custom
extensions:

The sendcustommsg method and the associated custommsg plugin hook allow
plugins to communicate directly with connected peers.
sendonion and the htlc_accepted hook allow plugins to deliver and
receive custom payloads to non-peers, over multiple hops, by encoding them
in the onion.

The missing piece in these cases is a way to signal support for these custom
protocol extensions. This is what we address in this pull request: we allow
plugins to register featurebits that should be mixed into the featurebits that
are handled in lightningd natively.

We support 3 locations for featurebits:

node_announcement
init messages
BOLT 11 invoices

In addition the specification would allow for featurebits in
the channel_announcement, however I decided against implementing that for
now, since the features would need to be negotiated between the endpoints and
I could not come up with a use-case that is not already covered by the
invoices featurebits or the node_announcement featurebits. It should be
trivial to add them after the fact if we find a use-case.

The plugin can register featurebits as part of its manifest, so it cannot
dynamically add or remove features, and the featurebits are cached in
connectd and gossipd. Starting / stopping plugins may result in
featurebits not being added to those two daemons or featurebits still being
announced despite the plugin that registered them no longer being
available. See open questions below.

Todo

Update the doc/PLUGINS.md documentation to include an example of the
new manifest fields.

Open Questions

Should we dynamically fetch the features whenever we send an init
message or create a node_announcement, or is it ok if these are a
little bit stale? The current "set-once" option is simple, but may
advertise features that are no longer available. There are a number of
ways to tackle this:
- We could push new featurebits when plugins get added or removed.
- We could pull featurebits whenever a message is created.
- We can mandate that plugins registering featurebits must be dynamic = false, preventing changes to featurebits after startup.

Depends #3479

darosior

This is great !

Open Questions

We could push new featurebits when plugins get added or removed.

If this can be done "just" with another message to connectd, I think that's preferable (I mean ratio added complexity / usability) ?

cdecker · 2020-01-31T16:35:15Z

Currently fighting against travis-ci (as usual)...

cdecker · 2020-01-31T21:26:46Z

This is great !

Open Questions

We could push new featurebits when plugins get added or removed.

If this can be done "just" with another message to connectd, I think that's preferable (I mean ratio added complexity / usability) ?

It'd also mean we need to have a way to notify lightningd/connect_control.c which adds a dependency between the two, that's more what I'm afraid of :-)

cdecker · 2020-01-31T21:27:48Z

Had to add #3479 as a dependency since secp256k1 was giving a bit of problems on Travis.

ZmnSCPxj · 2020-02-03T01:03:15Z

I suggest requiring dynamic = false for now. We can as well enable it later.

cdecker · 2020-02-06T14:11:48Z

Rebased and squashed. I also decided to punt the dynamic issue by forcing plugins to be non-dynamic if they register featurebits for now.

cdecker · 2020-02-06T15:28:24Z

Should be good to go. Ping @rustyrussell and @niftynei

niftynei

this is sweet!! my comments are mostly clarification asks.

ACK fd8d137

We could push new featurebits when plugins get added or removed.

regarding the 'should and if so how do we update the values for features' the "push on change" option strikes me as the most reasonable, given the frequency of a plugin removal/addition vs frequency of sending init messages (we'd also need a way to update node_anns)

niftynei · 2020-02-06T17:46:54Z

 }

+/* List of JSON keys matching `plugin_features_type`. */
+static const char *plugin_features_type_names[] = {"node", "init", "invoice"};


it might be nicer to co-locate these names with the enum declaration, so if the enum set changes it's easy to see/update the type names

Yes, unf. that has to be in the header, and this really wants to be here.

I'd like to keep this here, so we don't overexpose internal things. I added a comment in the header to also update this array if changing the enum.

rustyrussell

Ack, minor feedback only. Might do it myself in fact, depending on what else is needed pre rc1...

we have 4 venues in which we can add features, 3 of which are unilaterally controlled (`init`, `node_announcement`, and `invoices`) the `channel_announcement` is co-signed by both parties, so we can't add featurebits without additional coordination overhead. Each location is encoded as a key-value pair in a dict called `featurebits` in the manifest (omitted if no custom featurebits are set).

@niftynei

Feedback from @niftynei and me; nothing major, but avoids another round-trip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-02-10T02:47:01Z

Rebase and simple additional fixup commit.

cdecker · 2020-02-10T14:42:40Z

ACK 282c5f2

We'll collect the featurebits on-demand from all currently active plugins when needed.

We will be doing this when collecting featurebits from the plugins, so make this a reusable function.

The `init_featurebits` are computed at startup, and then cached indefinitely. They are then used whenever a new `init` handshake is performed. We could add a new message to push updates to `connectd` whenever a plugin is added or removed, but that's up for discussion.

This is the last venue we need to add custom featurebits to, so we also unmark the test as xfail. Changelog-Added: plugin: Plugins can now signal support for experimental protocol extensions by registering featurebits for `node_announcement`s, the connection handshake, and for invoices. For now this is limited to non-dynamic plugins only

This is in order to avoid having to update featurebits as plugins get activated and deactivated.

@niftynei

Feedback from @niftynei and me; nothing major, but avoids another round-trip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker force-pushed the plugin-features branch 2 times, most recently from f3b6c9b to 7a58594 Compare January 31, 2020 15:03

darosior reviewed Jan 31, 2020

View reviewed changes

Comment thread lightningd/plugin.c Outdated

Comment thread doc/PLUGINS.md Outdated

Comment thread doc/PLUGINS.md Outdated

cdecker force-pushed the plugin-features branch from b6af69c to 41e2174 Compare January 31, 2020 16:33

cdecker force-pushed the plugin-features branch 3 times, most recently from 637c36e to eecdccc Compare January 31, 2020 21:25

cdecker force-pushed the plugin-features branch 3 times, most recently from 1c38144 to ebe8e13 Compare February 1, 2020 15:51

cdecker force-pushed the plugin-features branch 4 times, most recently from 129354f to fd8d137 Compare February 6, 2020 14:10

niftynei reviewed Feb 6, 2020

View reviewed changes

rustyrussell added this to the 0.8.1 milestone Feb 9, 2020

rustyrussell approved these changes Feb 9, 2020

View reviewed changes

Comment thread Makefile

Comment thread common/features.c Outdated

Comment thread lightningd/invoice.c Outdated

Comment thread lightningd/invoice.c Outdated

cdecker added 2 commits February 10, 2020 13:01

pytest: Add a test for plugin featurebits

b8518a3

rustyrussell added a commit to cdecker/lightning that referenced this pull request Feb 10, 2020

Minor fixups on PR ElementsProject#3477

f9dd252

Feedback from @niftynei and me; nothing major, but avoids another round-trip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the plugin-features branch from fd8d137 to f9dd252 Compare February 10, 2020 02:46

plugin: Store a plugin's featurebits in the plugin struct

592b489

We'll collect the featurebits on-demand from all currently active plugins when needed.

cdecker and others added 8 commits February 10, 2020 15:44

features: Add featurebits_or helper to combine two featurebitstrings

b5210d8

We will be doing this when collecting featurebits from the plugins, so make this a reusable function.

plugin: Add function to collect featurebits that plugins registered

8448958

plugin: Include featurebits registered by plugins in invoices

f3ee58c

doc: Document the featurebits functionality for plugins

fcc0d9a

plugin: Allow custom features only if the plugin is not dynamic

10c4c0a

This is in order to avoid having to update featurebits as plugins get activated and deactivated.

Minor fixups on PR ElementsProject#3477

282c5f2

Feedback from @niftynei and me; nothing major, but avoids another round-trip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker force-pushed the plugin-features branch from f9dd252 to 282c5f2 Compare February 10, 2020 14:44

rustyrussell merged commit 3058073 into ElementsProject:master Feb 11, 2020

rustyrussell added the Optech Make Me Famous! label Feb 11, 2020

Conversation

cdecker commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Open Questions

Uh oh!

darosior left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdecker commented Jan 31, 2020

Uh oh!

cdecker commented Jan 31, 2020

Uh oh!

cdecker commented Jan 31, 2020

Uh oh!

ZmnSCPxj commented Feb 3, 2020

Uh oh!

cdecker commented Feb 6, 2020

Uh oh!

cdecker commented Feb 6, 2020

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

niftynei Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

rustyrussell Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

cdecker Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustyrussell commented Feb 10, 2020

Uh oh!

cdecker commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cdecker commented Jan 31, 2020 •

edited

Loading

darosior left a comment •

edited

Loading

cdecker commented Feb 10, 2020 •

edited

Loading