plugin: Allow plugins to publish and subscribe to custom notifications #4496
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdecker
added
plugin
pay-plugin
Optech Make Me Famous!
cdecker
force-pushed
the
plugin-notifications
branch
from
71805e1
to
3215d3e
Compare
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.
A plugin might subscribe to a notification topic that is only registered by another plugin later, so push the check to that consistency check phase where we do hook ordering as well.
Changelog-Added: plugin: Plugins may now send custom notifications that other plugins can subscribe to.
They may already have subscribers, and they may crash if presented with a malformed notification.
We want to ensure that plugins register their topics before sending any notification, so we need to remember which plugin registered which topics.
We use it in a couple of places, so let's remember it for easier access.
This should allow us to differentiate the origin of the notification, and further prevent plugins from spoofing native notifications.
We want to have well-behaved notifications that are clearly announced during the initialization, kill plugins that don't behave.
Since plugins will start sending them soon, and they are likely to get it wrong sometimes, be a bit more lenient, warn them in the logs instead and then make sure it doesn't accidentally work anyway.
cdecker
force-pushed
the
plugin-notifications
branch
from
3215d3e
to
9faa907
Compare
rustyrussell
approved these changes
May 3, 2021
Comment on lines
+113
to
+116
| if (plugin->subscriptions == NULL) | ||
| return; | ||
|
|
||
| for (size_t i = 0; i < tal_count(plugin->subscriptions); i++) { |
There was a problem hiding this comment.
Nit: tal_count(NULL) == 0, so this loop doesn't need a precursor check.
Comment on lines
+1292
to
+1293
| for (size_t i = 0; i < notifications->size; i++) { | ||
| obj = json_get_arr(notifications, i); |
There was a problem hiding this comment.
json_for_each_arr(i, obj, notifications) does these two lines, BTW (and slightly more efficiently).
Comment on lines
+1837
to
+1840
| static void payment_notify_failure(struct payment *p, const char *error_message) { | ||
| struct payment *root = payment_root(p); | ||
| struct json_stream *n; | ||
| n = plugin_notification_start(p->plugin, "pay_failure"); |
There was a problem hiding this comment.
Nit: genuinely weird indent?
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far plugins could only subscribe to native notifications that originated in
lightningdand which were limited to a few topics directly built into the daemon. With this PR we enable plugins to announce their intention to publish notifications to custom topics, and for other plugins to subscribe to those topics and receive the notifications.Custom topics are required to be announced as part of the
getmanifestcall, allowing us to verify that subscriptions match the announced topics at startup, and print a warning message should a plugin subscribe to a topic that wasn't announced, and therefore will never receive a notification. Trying to send a notification with an unannounced topic will result in a warning being logged, and the message not being delivered to subscribers.To allow subscribers to identify the sender we wrap the notification payload in an outer object with the sender metadata alongside the payload. So the
sendersending the first message below, will result in the second message being delivered to all subscribers:{ "jsonrpc": "2.0", "method": "mycustomnotification", "params": { "key": "value", "message": "Hello fellow plugin!" } }is delivered as
{ "jsonrpc": "2.0", "method": "mycustomnotification", "params": { "origin": "sender", "payload": { "key": "value", "message": "Hello fellow plugin!" } } }Notification subscribers should be lenient in what they accept, given that multiple plugins may publish to the same topic, potentially with different payload structures. This in turn allows for both fanout and fanin communication patterns.
We also instrument the
payplugin, reporting a minimalpay_successorpay_failurenotification, which includes thepayment_hash, thebolt11string as well as the failure message for failures. The remaining fields may be added if required or can be looked up by queryinglistpays.