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

plugin: Add notifications from lightningd to plugins (Plugin Saga, 3rd reprise) #2188

Merged
merged 10 commits into from Dec 30, 2018

Conversation

@cdecker
Copy link
Member

cdecker commented Dec 18, 2018

We're nearing the end of the endless plugin saga, with this new feature. It allows plugins to subscribe to a number of notifications/events that will then be streamed from lightningd to the plugin when something happens.

Notice that, while this provides the infrastructure, I've only implemented two very simple notification topics (connect and disconnect). We can (and will) add more as we come up with new use-cases.

@cdecker cdecker added this to the v0.6.3 milestone Dec 18, 2018

@cdecker cdecker self-assigned this Dec 18, 2018

@cdecker cdecker requested a review from rustyrussell Dec 18, 2018

@renepickhardt
Copy link
Collaborator

renepickhardt left a comment

As far as I see you forgot to bump the version number of the pylightning file to 0.0.7 that would be required (followed by a push to pylightning after merging) other than that from what I see this looks good to me and I am excited to test this out with other notifications than connect or disconnect

@@ -24,5 +24,15 @@ def init(options, configuration, plugin):
plugin.log("Plugin helloworld.py initialized")


@plugin.subscribe("connect")

This comment has been minimized.

@renepickhardt

renepickhardt Dec 18, 2018

Collaborator

this is an awesome API!

This comment has been minimized.

@cdecker

cdecker Dec 18, 2018

Member

Thanks, Flask was a huge inspiration.

return false;
}

void plugins_notify(struct plugins *plugins,

This comment has been minimized.

@renepickhardt

renepickhardt Dec 18, 2018

Collaborator

LGTM ( I was searching if subscriptions to the same events would work if several plugins subscribed to the same event ) and from how I understand the code it seems to me that it works. I wasn't able to execute this code though

This comment has been minimized.

@cdecker

cdecker Dec 18, 2018

Member

That is indeed supported, since unlike jsonrpc calls and hooks there is no problem with notification semantics.

return false;
}

void notify_connect(struct lightningd *ld, struct pubkey *nodeid,

This comment has been minimized.

@renepickhardt

renepickhardt Dec 18, 2018

Collaborator

so just for the gist I could create a method void notify_incoming_htlc(struct lightningd *ld, struct pubkey *nodeid, struct channel *chan, struct htlc *h) (not sure if struct channel and struct htlc exist) and create a json string and send it via plugin_notify to the plugin. I would obviously have to call that function at the place where incoming htlc's are being handled. But other than that (and of course adding the definition of the method to notification.h) I would not have to extend other parts of c-lightning or am I missing something?

This comment has been minimized.

@cdecker

cdecker Dec 18, 2018

Member

Yes, the idea is that you'd create a notify_xyz function that gathers all the information and formats it into a JSON message and then passes that to plugins_notify which will take care of dispatching notifications to all subscribers.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 18, 2018

I will push the new version of the pylightning library once we have the @plugin.hook functionality in there. I don't like churning through versions too quickly, especially when this is functionality that is as of now unreleased.

Should get a 0.0.7 out by the end of the week though.

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

ZmnSCPxj commented Dec 19, 2018

Possible other notification:

  • sendpay-failure - autopilots might be interested in this and consider creating direct channels that skip failing channels or nodes.
  • sendpay-success - similarly autopilots might be interested, in that success implies we can lower priority to make channels to nodes at and near destination of payment.
@rustyrussell
Copy link
Contributor

rustyrussell left a comment

This is masterful code.

I was going to suggest we drop the static subscription-in-manifest idea for a dynamic RPC, but that actually makes it harder for plugins (at the moment they can treat STDIN as the source of all commands and notifications, and only read their rpc connection when they've sent some command).

But you should probably block subscriptions for a plugin until after it has returned from init (won't happen with these notifications, but may in future; my pay plugin actually makes rpc call before replying to init).

return false;
}
topic = tal_strndup(
plugin, plugin->buffer + s->start, s->end - s->start);

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor

Note: after rebase onto master, you'll be able to use json_strdup(plugin, plugin->buffer, s);

if (!notifications_have_topic(topic)) {
plugin_kill(
plugin,
"topic %s is not a know notification topic", topic);

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor

s/know/known/

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor

Slight simplification, I would skip the 's->type != JSMN_STRING' check above, and rely on it here (in which case, please put ' around the %s).

{
*tal_arr_expand(&plugin->js_arr) = req->stream;
*tal_arr_expand(&plugin->js_arr) = tal_steal(plugin->js_arr, stream);

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor

Make this two lines: we got bitten by this evaluation order indeterminism before.

*/
static void plugin_request_queue(struct plugin *plugin,
struct plugin_request *req)
static void plugin_send(struct plugin *plugin, struct json_stream *stream TAKES)

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor

TAKES isn't quite right here: that means it takes ownership iff caller calls take(). Perhaps call it something more obviously "takes-ownership"-is, like "plugin_attach_stream()"?

if (plugin_subscriptions_contains(p, n->method))
plugin_send(p, json_stream_dup(p, n->stream));
}
tal_free(n);

This comment has been minimized.

@rustyrussell

rustyrussell Dec 20, 2018

Contributor
if (taken(n))
    tal_free(n);

We could of course optimize this; when we optimize to a per-topic queue of plugins, we'd not have to copy the last one if it's taken().

@@ -801,7 +846,8 @@ static void plugin_manifest_cb(const struct plugin_request *req,
}

if (!plugin_opts_add(plugin, buffer, resulttok)
|| !plugin_rpcmethods_add(plugin, buffer, resulttok))
|| !plugin_rpcmethods_add(plugin, buffer, resulttok)
|| !plugin_subscriptions_add(plugin, buffer, resulttok))
plugin_kill(plugin, "Failed to register options or methods");

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

There's one other thing here now: subscriptions

@@ -1057,3 +1057,27 @@ void json_add_opt_plugins(struct json_stream *response,
json_add_string(response, "plugin", p->cmd);
}
}

/**
* Determine whethe a plugin is subscribed to a given topic/method.

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

:s/whethe/if/

@@ -2,6 +2,8 @@
#include <ccan/array_size/array_size.h>

const char *notification_topics[] = {
"connect",

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

i think it'd be a bit easier to tell what these refer to if we include that it's a node action, something like 'connect-node' ?

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

or 'node_connected'?

This comment has been minimized.

@cdecker

cdecker Dec 22, 2018

Member

I bikeshedded a couple of variants for this, with broader topics, and naming variants, but in the end I just picked the simplest and shortest variant I could think of, i.e., just the actions that are performed. Happy to discuss the naming here further, but then I'd split the addition of this topic out and just get the infrastructure merged, so I can continue working on top of this 😉

This comment has been minimized.

@niftynei

niftynei Dec 27, 2018

Collaborator

sounds good. 👍

fwiw, i like how the 'actor_action' pattern (ie 'node_connected') gives a bit more insight at a glance as to what this refers to, but being as i'm more familiar with the codebase now, connect is pretty easy to map back to RPC commands, which is also nice :)

{
struct jsonrpc_notification *n =
jsonrpc_notification_start(NULL, notification_topics[0]);
json_add_pubkey(n->stream, "id", nodeid);

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

i'd propose changing this to peer_id instead of just id

This comment has been minimized.

@rustyrussell

rustyrussell Dec 25, 2018

Contributor

No, "id" is standard for the RPC layer, unless it's confusing. In this case, it's not.

This comment has been minimized.

@niftynei

niftynei Dec 27, 2018

Collaborator

Ack. I totally misread some python code earlier; I redact this comment.

void notify_disconnect(struct lightningd *ld, struct pubkey *nodeid)
{
struct jsonrpc_notification *n =
jsonrpc_notification_start(NULL, notification_topics[1]);

This comment has been minimized.

@niftynei

niftynei Dec 21, 2018

Collaborator

is there a way to not use hardcoded array indices for these? seems like this will be easy to break without meaning to if the notification_topics ever get reordered.

This comment has been minimized.

@cdecker

cdecker Dec 22, 2018

Member

There is a facility to generate names from enums, maybe we want to use that instead? I had an enum that represented the indices into this array, but it got really repetitive, so I dropped it for now.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Dec 23, 2018

Collaborator

We use AUTODATA to generate an array of conmands, cannot we use same to generate array of notifications?

This comment has been minimized.

@cdecker

cdecker Dec 29, 2018

Member

We could do that, and I think I'm doing something similar in the hooks PR. We can revisit this later I think. The hooks PR also has some nice things like non-dynamic dispatch of serializers and callback, which might be interesting here as an optimization (skipping serialization if nobody has subscribed).

@cdecker cdecker force-pushed the plugin-7 branch from 6299d4d to e8a9aa5 Dec 22, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 22, 2018

Rebased and addressed the feedback in fixups. @rustyrussell and @niftynei thanks for the reviews, if you're happy with the fixups I'll squash and merge :-)

P.S.: I know it looks like a lot of fixups, but they are all single-lines and address a single comment each to make reviewing easier 😉

@cdecker cdecker force-pushed the plugin-7 branch 3 times, most recently from db3b21d to 91c9ce7 Dec 22, 2018

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 25, 2018

I do want "peer_id" changed back to "id" though, since that's the nomenclature used everywhere else. Rest is fine. Using an enum for notifications is a win, but that's a simple neatening which can be done later.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 25, 2018

Ack d9839bc

@conscott
Copy link
Collaborator

conscott left a comment

Right now the docstring for plugin commands includes newline junk in the description text - like

hello [params]
    This is the documentation string for the hello-function.\n\nIt gets reported as the description when registering the function\nas a method with `lightningd`.

You could just add a line here to convert docstring newlines to spaces (if desired)

doc = re.sub('\n+', ' ', doc)
@conscott

This comment has been minimized.

Copy link
Collaborator

conscott commented Dec 27, 2018

Tested ACK 91c9ce7

Just left a minor nit above.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 29, 2018

Rebased, dropped the id -> peer_id fixups, added a docstring squash fixup as suggested by @rustyrussell 😉

@cdecker cdecker force-pushed the plugin-7 branch from e6162c2 to 7de1399 Dec 29, 2018

@rustyrussell
Copy link
Contributor

rustyrussell left a comment

Ack 7de1399

@conscott

This comment has been minimized.

Copy link
Collaborator

conscott commented Dec 30, 2018

Re-ACK 7de1399

cdecker added some commits Dec 11, 2018

jsonrpc: Create a struct for notifications that we send
Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Add subscriptions when processing the plugin manifest
Signed-off-by: Christian Decker <decker.christian@gmail.com>
json: Add function to duplicate a json_stream
Will be used in the next commit to fan out notifications to multiple
subscribing plugins. We can't just use `tal_dup` from outside since
the definition is hidden outside the compilation unit.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Make plugin_send a more generic function
This used to be request-specific, but we now want to send
notifications and requests. As a drive-by we also clarify the
ownership of the json_stream instance that is being sent.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Dispatch notifications to subscribed plugins
Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker added some commits Dec 13, 2018

plugin: Add connect and disconnect notifications
Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Document JSON-RPC passthrough and notifications
Signed-off-by: Christian Decker <decker.christian@gmail.com>
pylightning: Add notification subscription handlers
Just like we added the RPC methods, the notification handlers can also
be registered using a function decorator, and we auto-subscribe when
asked for a manifest.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
pytest: Add a test for the event subscription and notification
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker force-pushed the plugin-7 branch from 7de1399 to 5a8c2c6 Dec 30, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 30, 2018

Rebased and squashed on top of master. Reapplying @rustyrussell's ACK (@bitcoin-bot) is still having issues recognizing rebases and squashes)

ACK 5a8c2c6

@cdecker cdecker merged commit 4f16044 into master Dec 30, 2018

3 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment