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

Add a `sendcustommsg` RPC call and a `custommsg` plugin hook for experimental protocol extensions #3315

Merged
merged 15 commits into from Jan 28, 2020

Conversation

@cdecker
Copy link
Member

cdecker commented Dec 4, 2019

This pull request implements the custommsg facility that allows users to
implement custom protocol extensions on top of what c-lightning offers. The
implementation aims to maximize the flexibility given to the users while at
the same time minimizing the dangers of sending custom messages (see the
limitations section).

Limitations

  • Custom messages are not allowed to have the same type as an internally
    handled message type. This is necessary to ensure that the custom messages
    injected at one end do not throw off the internal state handling code at
    the other end. This means that it's ok to implement things that are in the
    spec (e.g., experimental extensions that are not yet fully specified), but
    once c-lightning implements them internally they may no longer be used.
  • Custom message types are limited to odd-numbered types. This is necessary
    since even-typed messages are required to drop the connection if they are
    unknown. If we were to defer that decision to a plugin we would need to
    lock the owning daemon, and lightningd, until the plugin has returned the
    verdict on whether this is known or unknown. Odd-typed messages on the
    other hand can be dropped silently meaning we can easily continue
    processing while the plugin deliberates on whether or not it knows how to
    handle the message.
  • Custom messages can only be sent if a peer is owned by openingd, i.e.,
    before opening a channel and currently just gossiping, or by channeld,
    i.e., we currently have a channel open and are not closing it. The only
    other daemon that talks to a peer directly is closingd which doesn't
    handle unexpected messages well (hence it also doesn't handle gossip),
    however in most cases that state is very transitory, so it is unlikely to
    be necessary that we need to send custommsgs while owned by closingd.

Examples

The limitations mentioned above aside, this PR opens a wide variety of
possible extensions and opportunity for experimental implementations of
protocols. This is just a minimal list of things that came to my mind while
working on this:

  • @renepickhardt's idea of sharing a bit more information about capacities in
    the friend-of-a-friend network in order to collaborate on finding
    rebalancings.
  • Add a new broadcast for on-chain data, e.g., also transport block headers
    on top of the encryptedoverlay network to provide hints to peers that there
    is a new block or that they might be lagging behind.
  • Add a new broadcast for liquidity announcements or exchange rate offers for
    cross-chain atomic swaps.
  • More advanced gossip sync mechanisms such as set-reconciliation. These
    might negotiate the set differences in custommsgs and then send
    non-custommsgs for the actual exchange of information.
  • Many more that others will come up with...

Testing

The test_custommsg test aims to cover all the limitations and also test the
happy path, in either of the two supported subdaemons. Currently it does not
test when a peer is owned by closingd, however that is identical to not
being owned at all.

Open Questions

  • We currently don't do anything with the result from the hook, we could
    as well make it a notification. However we might later implement the
    hook-chaining and we'd likely want to exit the chain as soon as a
    plugin has handled the message instead of continuing to send it to the
    other plugins. There the result could tell us whether it is handled or
    not.
  • Should we make this experimental only for now?
  • Should we make this generally available (non-developer)?

Changelog-None

@cdecker cdecker requested review from rustyrussell and niftynei Dec 4, 2019
@cdecker cdecker force-pushed the cdecker:custommsg branch 6 times, most recently from 5640447 to 30c089f Dec 4, 2019
@cdecker cdecker marked this pull request as ready for review Dec 9, 2019
@cdecker cdecker force-pushed the cdecker:custommsg branch from 3641287 to 1193d26 Dec 9, 2019
Copy link
Collaborator

darosior left a comment

Looks good, but answering the open question I'd argue for sendcustommsg to be non-dev as it does not seem to be a shootgun for a regular user as could be other dev-only commands (such as dev-forget-channel). It also allows a plugin using this command to not require a compilation with enable-developer. But removing some restrictions could be developer-only 😁 ..

Custom message types are limited to odd-numbered types.

"utility",
json_sendcustommsg,
"Send a custom message to the peer with the given {node_id}",
.verbose = "sendcustommsg node_id hexcustommsg",

This comment has been minimized.

Copy link
@darosior

darosior Dec 9, 2019

Collaborator

nit: dev-sendcustommsg node_id hexcustommsg

tests/test_misc.py Outdated Show resolved Hide resolved
@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Dec 9, 2019

Looks good, but answering the open question I'd argue for sendcustommsg to be non-dev as it does not seem to be a shootgun for a regular user as could be other dev-only commands (such as dev-forget-channel). It also allows a plugin using this command to not require a compilation with enable-developer. But removing some restrictions could be developer-only ..

Custom message types are limited to odd-numbered types.

Good point, however I get the feeling we might end up confusing people, and they might think this is to send some sort of chat message to an arbitrary node. It's more of a "don't show a normal mortal what they aren't supposed to use" kind of deal :-)

@cdecker cdecker added this to the next milestone Dec 11, 2019
@cdecker cdecker force-pushed the cdecker:custommsg branch from cd00e82 to 51978d4 Dec 12, 2019
@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

ZmnSCPxj commented Dec 17, 2019

Good point, however I get the feeling we might end up confusing people, and they might think this is to send some sort of chat message to an arbitrary node. It's more of a "don't show a normal mortal what they aren't supposed to use" kind of deal :-)

Maybe rename to sendcustomprotocolmsg and customprotocolmsg to deter such?

@cdecker cdecker force-pushed the cdecker:custommsg branch 2 times, most recently from 7beec1a to 0fc7895 Jan 3, 2020
@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Jan 6, 2020

Good point, however I get the feeling we might end up confusing people, and they might think this is to send some sort of chat message to an arbitrary node. It's more of a "don't show a normal mortal what they aren't supposed to use" kind of deal :-)

Maybe rename to sendcustomprotocolmsg and customprotocolmsg to deter such?

Let's keep it a developer-only option for now, and see if my fears of causing confusion is justified. If it is we can always rename while making it non-dev-only :-)

Copy link
Contributor

rustyrussell left a comment

Ack b73414b

Trivial requests for enhancements which could go in separate PR.

Am in two minds about dev-. Should we drop it?

$(BOLT_GEN) -s --page header $@ common_wire_type < $< > $@

wire/gen_common_wire.c: wire/common_wire_csv $(WIRE_BOLT_DEPS) wire/Makefile
$(BOLT_GEN) -s --page impl ${@:.c=.h} common_wire_type < $< > $@

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jan 28, 2020

Contributor

TODO: enhance devtools/decodemsg to decode internal messages. That is not only useful, if it's constructed as a giant switch() statement, it also ensures we don't have any duplicate message numbers anywhere (otherwise it won't compile).

Meanwhile, we should probably have such a switch statement somewhere in the source (even if it's just manually maintained) to force this check?

}
return false;
}

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jan 28, 2020

Contributor

can we use thse functions instead of the hacky "name contains INVALID" checks now?

@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Jan 28, 2020

Am in two minds about dev-. Should we drop it?

Let's keep it for now and see how crazy people go with custommsg, and we can promote it in a later release.

cdecker added 5 commits Nov 28, 2019
These messages may be exchanged between the master and any daemon. For now
these are just the daemons that a peer may be attached to at any time since
the first example of this is the custommsg infrastructure.
This is currently in opening_control since that's the only part that has
access to the uncommitted_channel internals. Otherwise it's independent from
the specific daemon.
This command injects a custom message into the encrypted transport stream to
the peer, allowing users to build custom protocols on top of c-lightning
without requiring any changes to c-lightning itself.
We cannot let users use `sendcustommsg` to inject messages that are handled
internally since it could result in our internal state tracking being borked.
cdecker added 10 commits Dec 4, 2019
Most of the work is done in `lightningd`, here we just need to queue the
message itself.
This solves a couple of issues with the need to synchronously drop the
connection in case we were required to understand what the peer was talking
about while still allowing users to experiment, just not kill connections.
This is mainly meant as a marker so that we can later remove the code if we
decide to make the handling of custommsgs a non-developer option. It marks the
place that we would otherwise handle what in dev-mode is a custommsg.
This completes the custommsg epic, finally we are back where we began all that
time ago (about 4 hours really...): in a plugin that implements some custom
logic.
It's a dev-* command for now, but better document it so people can use it
rather than having them guess how it's supposed to work.
@cdecker cdecker force-pushed the cdecker:custommsg branch from b73414b to 09c72dd Jan 28, 2020
@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Jan 28, 2020

Had to rebase on top of master and I'll address the nits in a cleanup PR. Merging to keep the conflict surface low :-)

ACK 09c72dd

@cdecker cdecker merged commit 602b81f into ElementsProject:master Jan 28, 2020
4 checks passed
4 checks passed
bitcoin-bot/acks Acks by cdecker
bitcoin-bot/changelog This PR has opted out of having a changelog entry
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.