From 083b41f0902f228b6dd039190f3c2f11a90dd7b3 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 27 Apr 2021 15:15:09 +0200 Subject: [PATCH] plugin: Add a list of notification topics registered by plugin We will eventually start emitting and dispatching custom notifications from plugins just like we dispatch internal notifications. In order to get reasonable error messages we need to make sure that the topics plugins are asking for were correctly registered. When doing this we don't really care about whether the plugin that registered the notification is still alive or not (it might have died, but subscribers should stay up and running), so we keep a list of all topics attached to the `struct plugins` which gathers global plugin information. --- contrib/pyln-client/pyln/client/plugin.py | 9 ++++++ lightningd/notification.c | 9 +++++- lightningd/notification.h | 2 +- lightningd/plugin.c | 34 ++++++++++++++++++----- lightningd/plugin.h | 4 +++ tests/plugins/custom_notifications.py | 21 ++++++++++++++ tests/test_plugin.py | 10 +++++++ 7 files changed, 80 insertions(+), 9 deletions(-) create mode 100755 tests/plugins/custom_notifications.py diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 5d3095e7e..a730fc449 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -223,6 +223,7 @@ class Plugin(object): } self.options: Dict[str, Dict[str, Any]] = {} + self.notification_topics: List[str] = [] def convert_featurebits( bits: Optional[Union[int, str, bytes]]) -> Optional[str]: @@ -420,6 +421,11 @@ class Plugin(object): self.add_option(name, None, description, opt_type="flag", deprecated=deprecated) + def add_notification_topic(self, topic: str): + """Announce that the plugin will emit notifications for the topic. + """ + self.notification_topics.append(topic) + def get_option(self, name: str) -> str: if name not in self.options: raise ValueError("No option with name {} registered".format(name)) @@ -898,6 +904,9 @@ class Plugin(object): 'subscriptions': list(self.subscriptions.keys()), 'hooks': hooks, 'dynamic': self.dynamic, + 'notifications': [ + {"method": name} for name in self.notification_topics + ], } # Compact the features a bit, not important. diff --git a/lightningd/notification.c b/lightningd/notification.c index 7cb2ebea9..bcd4491d0 100644 --- a/lightningd/notification.c +++ b/lightningd/notification.c @@ -18,12 +18,19 @@ static struct notification *find_notification_by_topic(const char* topic) return NULL; } -bool notifications_have_topic(const char *topic) +bool notifications_have_topic(const struct plugins *plugins, const char *topic) { struct notification *noti = find_notification_by_topic(topic); if (noti) return true; + /* Some plugin at some point announced it'd be emitting + * notifications to this topic. We don't care if it died, just + * that it was a valid topic at some point in time. */ + for (size_t i=0; inotification_topics); i++) + if (streq(plugins->notification_topics[i], topic)) + return true; + return false; } diff --git a/lightningd/notification.h b/lightningd/notification.h index 8ad8e9de0..6736db912 100644 --- a/lightningd/notification.h +++ b/lightningd/notification.h @@ -25,7 +25,7 @@ struct onionreply; struct wally_psbt; -bool notifications_have_topic(const char *topic); +bool notifications_have_topic(const struct plugins *plugins, const char *topic); struct notification { const char *topic; diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 5c68d86cd..ac0700fc5 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -76,6 +76,7 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, p->startup = true; p->json_cmds = tal_arr(p, struct command *, 0); p->blacklist = tal_arr(p, const char *, 0); + p->notification_topics = tal_arr(p, const char *, 0); p->shutdown = false; p->plugin_idx = 0; #if DEVELOPER @@ -103,6 +104,25 @@ void plugins_free(struct plugins *plugins) tal_free(plugins); } +/* Check that all the plugin's subscriptions are actually for known + * notification topics. Emit a warning if that's not the case, but + * don't kill the plugin. */ +static void plugin_check_subscriptions(struct plugins *plugins, + struct plugin *plugin) +{ + if (plugin->subscriptions == NULL) + return; + + for (size_t i = 0; i < tal_count(plugin->subscriptions); i++) { + const char *topic = plugin->subscriptions[i]; + if (!notifications_have_topic(plugins, topic)) + log_unusual( + plugin->log, + "topic '%s' is not a known notification topic", + topic); + } +} + /* Once they've all replied with their manifests, we can order them. */ static void check_plugins_manifests(struct plugins *plugins) { @@ -1136,14 +1156,12 @@ static const char *plugin_subscriptions_add(struct plugin *plugin, json_tok_full_len(s), json_tok_full(buffer, s)); } + + /* We add all subscriptions while parsing the + * manifest, without checking that they exist, since + * later plugins may also emit notifications of custom + * types that we don't know about yet. */ topic = json_strdup(plugin, plugin->buffer, s); - - if (!notifications_have_topic(topic)) { - return tal_fmt( - plugin, - "topic '%s' is not a known notification topic", topic); - } - tal_arr_expand(&plugin->subscriptions, topic); } return NULL; @@ -1338,6 +1356,8 @@ static const char *plugin_parse_getmanifest_response(const char *buffer, if (!err) err = plugin_add_params(plugin); + plugin_check_subscriptions(plugin->plugins, plugin); + plugin->plugin_state = NEEDS_INIT; return err; } diff --git a/lightningd/plugin.h b/lightningd/plugin.h index f7954e568..27e2ee777 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -128,6 +128,10 @@ struct plugins { /* Whether builtin plugins should be overridden as unimportant. */ bool dev_builtin_plugins_unimportant; #endif /* DEVELOPER */ + + /* Notification topics that plugins have registered with us + * and that other plugins may subscribe to. */ + const char **notification_topics; }; /* The value of a plugin option, which can have different types. diff --git a/tests/plugins/custom_notifications.py b/tests/plugins/custom_notifications.py new file mode 100755 index 000000000..9b32aa45e --- /dev/null +++ b/tests/plugins/custom_notifications.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + + +plugin = Plugin() + + +@plugin.subscribe("custom") +def on_custom_notification(val, plugin, **kwargs): + plugin.log("Got a custom notification {}".format(val)) + + +@plugin.method("emit") +def emit(plugin): + """Emit a simple string notification to topic "custom" + """ + plugin.notify("custom", "Hello world") + + +plugin.add_notification_topic("custom") +plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 6ea840e9d..308710c79 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2402,3 +2402,13 @@ def test_self_disable(node_factory): # Also works with dynamic load attempts with pytest.raises(RpcError, match="Disabled via selfdisable option"): l1.rpc.plugin_start(p2, selfdisable=True) + + +@pytest.mark.xfail(strict=True) +def test_custom_notification_topics(node_factory): + plugin = os.path.join( + os.path.dirname(__file__), "plugins", "custom_notifications.py" + ) + l1 = node_factory.get_node(options={'plugin': plugin}) + l1.rpc.emit() + l1.daemon.wait_for_log(r'Got a custom notification Hello world')