libplugin: correctly wrap notifications we send in the notification name.

All the core notifications changed over to wrapping the notification
fields in an object with the name of the notification, but notifications
from plugins were missed.

Changelog-Added: Plugins: `channel_hint_update`, `pay_failure` and `pay_success` notifications now have objects of the same name containing the expected fields.
Changelog-Deprecated: Plugins: `channel_hint_update`, `pay_failure` and `pay_success` notification fields outside the same-named object.
Signed-off-by: Rusty Russell <[email protected]>

Author: rustyrussell

doc/developers-guide/deprecated-features.md

@@ -23,6 +23,7 @@ hidden: false
 | coin_movement.txid                   | Notification Field | v25.09           | v26.09         | Use `spending_txid` instead                                                                                                            |
 | channel_state_changed.null_scid         | Notification Field | v25.09        | v26.09         | In channel_state_changed notification, `short_channel_id` will be missing instead of `null`                                            |
 | notification.payload                    | Notification Field | v25.09        | v26.09         | Notifications from plugins used to have fields in `payload` sub-object, now they are not (just like normal notifications)              |
+| pay_notifications.raw_fields            | Field              | v25.09           | v26.09         | `channel_hint_update`, `pay_failure` and `pay_success` notifications now wrap members in an object of the same name                    |
 
 Inevitably there are features which need to change: either to be generalized, or removed when they can no longer be supported.
 

plugins/channel_hint.c

@@ -191,7 +191,7 @@ struct channel_hint *channel_hint_from_json(const tal_t *ctx,
 	payload = json_get_member(buffer, toks, "payload");
 	/* Modern API includes fields directly */
 	if (!payload) {
-		jhint = json_get_member(buffer, toks, "channel_hint");
+		jhint = json_get_member(buffer, toks, "channel_hint_update");
 	} else {
 		jhint = json_get_member(buffer, payload, "channel_hint");
 	}

plugins/libplugin-pay.c

@@ -383,15 +383,32 @@ void payment_start(struct payment *p)
      * We share the channel_hints across payments, and across plugins, in order
      * to maximize the context they have when performing payments.
      */
+static void channel_hint_notify_core(struct plugin *plugin,
+				     struct json_stream *js,
+				     const char *fieldname,
+				     const struct channel_hint *hint)
+{
+	/* The timestamp used to decay the observation over time. */
+	channel_hint_to_json(fieldname, hint, js);
+}
+
 static void channel_hint_notify(struct plugin *plugin,
 				const struct channel_hint *hint)
 {
 	struct json_stream *js =
-	    plugin_notification_start(plugin, "channel_hint_update");
+	    plugin_notification_start_obs(plugin, "channel_hint_update");
 
-	/* The timestamp used to decay the observation over time. */
-	channel_hint_to_json("channel_hint", hint, js);
-	plugin_notification_end(plugin, js);
+	/* Fake up the old "payload" style, *and* the old unwrapped style */
+	if (notification_deprecated_out_ok(plugin, "notification", "payload",
+					   "v25.09", "v26.09")) {
+		json_add_string(js, "origin", "pay");
+		json_object_start(js, "payload");
+		channel_hint_notify_core(plugin, js, "channel_hint", hint);
+		json_object_end(js);
+	}
+
+	channel_hint_notify_core(plugin, js, "channel_hint_update", hint);
+	plugin_notification_end_obs(plugin, js);
 }
 
 static void channel_hints_update(struct payment *p,
@@ -2077,21 +2094,38 @@ static void payment_json_add_attempts(struct json_stream *s,
 	json_array_end(s);
 }
 
-static void payment_notify_failure(struct payment *p, const char *error_message)
+static void payment_failure_notify_core(struct payment *p,
+					struct json_stream *n,
+					const char *error_message)
 {
 	struct payment *root = payment_root(p);
-	struct json_stream *n;
-
-	n = plugin_notification_start(p->plugin, "pay_failure");
 	json_add_sha256(n, "payment_hash", p->payment_hash);
 	if (root->invstring != NULL)
 		json_add_string(n, "bolt11", root->invstring);
 
 	json_object_start(n, "error");
 	json_add_string(n, "message", error_message);
 	json_object_end(n); /* .error */
+}
+
+static void payment_notify_failure(struct payment *p, const char *error_message)
+{
+	struct json_stream *n;
 
-	plugin_notification_end(p->plugin, n);
+	n = plugin_notification_start_obs(p->plugin, "pay_failure");
+	/* Fake up the old "payload" style, *and* the old unwrapped style */
+	if (notification_deprecated_out_ok(p->plugin, "notification", "payload",
+					   "v25.09", "v26.09")) {
+		json_add_string(n, "origin", "pay");
+		json_object_start(n, "payload");
+		payment_failure_notify_core(p, n, error_message);
+		json_object_end(n);
+	}
+
+	json_object_start(n, "pay_failure");
+	payment_failure_notify_core(p, n, error_message);
+	json_object_end(n);
+	plugin_notification_end_obs(p->plugin, n);
 }
 
 /* Code shared by selfpay fast-path: populate JSON output for successful
@@ -2126,11 +2160,24 @@ void json_add_payment_success(struct json_stream *js,
 	json_add_preimage(js, "payment_preimage", preimage);
 	json_add_string(js, "status", "complete");
 
-	n = plugin_notification_start(p->plugin, "pay_success");
+	n = plugin_notification_start_obs(p->plugin, "pay_success");
+	/* Fake up the old "payload" style, *and* the old unwrapped style */
+	if (notification_deprecated_out_ok(p->plugin, "notification", "payload",
+					   "v25.09", "v26.09")) {
+		json_add_string(n, "origin", "pay");
+		json_object_start(n, "payload");
+		json_add_sha256(n, "payment_hash", p->payment_hash);
+		if (root->invstring != NULL)
+			json_add_string(n, "bolt11", root->invstring);
+		json_object_end(n);
+	}
+
+	json_object_start(n, "pay_success");
 	json_add_sha256(n, "payment_hash", p->payment_hash);
 	if (root->invstring != NULL)
 		json_add_string(n, "bolt11", root->invstring);
-	plugin_notification_end(p->plugin, n);
+	json_object_end(n);
+	plugin_notification_end_obs(p->plugin, n);
 }
 
 /* This function is called whenever a payment ends up in a final state, or all

plugins/libplugin.c

@@ -239,6 +239,19 @@ bool command_deprecated_out_ok(struct command *cmd,
 			     NULL, NULL);
 }
 
+bool notification_deprecated_out_ok(struct plugin *plugin,
+				    const char *method,
+				    const char *fieldname,
+				    const char *depr_start,
+				    const char *depr_end)
+{
+	return deprecated_ok(plugin->deprecated_ok,
+			     tal_fmt(tmpctx, "%s.%s", method, fieldname),
+			     depr_start, depr_end,
+			     plugin->beglist,
+			     NULL, NULL);
+}
+
 static void ld_send(struct plugin *plugin, struct json_stream *stream)
 {
 	struct jstream *jstr = tal(plugin, struct jstream);
@@ -1859,8 +1872,8 @@ void plugin_gossmap_logcb(struct plugin *plugin,
 	va_end(ap);
 }
 
-struct json_stream *plugin_notification_start(const tal_t *ctx,
-					      const char *method)
+struct json_stream *plugin_notification_start_obs(const tal_t *ctx,
+						  const char *method)
 {
 	struct json_stream *js = new_json_stream(ctx, NULL, NULL);
 
@@ -1872,11 +1885,26 @@ struct json_stream *plugin_notification_start(const tal_t *ctx,
 	return js;
 }
 
+void plugin_notification_end_obs(struct plugin *plugin,
+				 struct json_stream *stream STEALS)
+{
+	json_object_end(stream);
+	jsonrpc_finish_and_send(plugin, stream);
+}
+
+struct json_stream *plugin_notification_start(const tal_t *ctx,
+					      const char *method)
+{
+	struct json_stream *js = plugin_notification_start_obs(ctx, method);
+	json_object_start(js, method);
+	return js;
+}
+
 void plugin_notification_end(struct plugin *plugin,
 			     struct json_stream *stream STEALS)
 {
 	json_object_end(stream);
-	jsonrpc_finish_and_send(plugin, stream);
+	plugin_notification_end_obs(plugin, stream);
 }
 
 struct json_stream *plugin_notify_start(struct command *cmd, const char *method)

plugins/libplugin.h

@@ -381,6 +381,13 @@ bool command_deprecated_in_named_ok(struct command *cmd,
 				    const char *depr_start,
 				    const char *depr_end);
 
+/* Should we include this field in the notification? */
+bool notification_deprecated_out_ok(struct plugin *plugin,
+				    const char *method,
+				    const char *fieldname,
+				    const char *depr_start,
+				    const char *depr_end);
+
 /* Call this on fatal error. */
 void NORETURN PRINTF_FMT(2,3) plugin_err(struct plugin *p, const char *fmt, ...);
 
@@ -539,6 +546,12 @@ struct json_stream *plugin_notification_start(const tal_t *ctx,
 void plugin_notification_end(struct plugin *plugin,
 			     struct json_stream *stream STEALS);
 
+/* Obsolete versions: do not use for new code! */
+struct json_stream *plugin_notification_start_obs(const tal_t *ctx,
+						  const char *method);
+void plugin_notification_end_obs(struct plugin *plugin,
+				 struct json_stream *stream TAKES);
+
 /* Convenience wrapper for notify "message" */
 void plugin_notify_message(struct command *cmd,
 			   enum log_level level,

plugins/test/run-route-calc.c

@@ -284,6 +284,13 @@ void memleak_add_helper_(const tal_t *p UNNEEDED, void (*cb)(struct htable *memt
 /* Generated stub for memleak_scan_htable */
 void memleak_scan_htable(struct htable *memtable UNNEEDED, const struct htable *ht UNNEEDED)
 { fprintf(stderr, "memleak_scan_htable called!\n"); abort(); }
+/* Generated stub for notification_deprecated_out_ok */
+bool notification_deprecated_out_ok(struct plugin *plugin UNNEEDED,
+				    const char *method UNNEEDED,
+				    const char *fieldname UNNEEDED,
+				    const char *depr_start UNNEEDED,
+				    const char *depr_end UNNEEDED)
+{ fprintf(stderr, "notification_deprecated_out_ok called!\n"); abort(); }
 /* Generated stub for notleak_ */
 void *notleak_(void *ptr UNNEEDED, bool plus_children UNNEEDED)
 { fprintf(stderr, "notleak_ called!\n"); abort(); }
@@ -299,14 +306,14 @@ void plugin_gossmap_logcb(struct plugin *plugin UNNEEDED,
 /* Generated stub for plugin_log */
 void plugin_log(struct plugin *p UNNEEDED, enum log_level l UNNEEDED, const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "plugin_log called!\n"); abort(); }
-/* Generated stub for plugin_notification_end */
-void plugin_notification_end(struct plugin *plugin UNNEEDED,
-			     struct json_stream *stream STEALS UNNEEDED)
-{ fprintf(stderr, "plugin_notification_end called!\n"); abort(); }
-/* Generated stub for plugin_notification_start */
-struct json_stream *plugin_notification_start(const tal_t *ctx UNNEEDED,
-					      const char *method UNNEEDED)
-{ fprintf(stderr, "plugin_notification_start called!\n"); abort(); }
+/* Generated stub for plugin_notification_end_obs */
+void plugin_notification_end_obs(struct plugin *plugin UNNEEDED,
+				 struct json_stream *stream TAKES UNNEEDED)
+{ fprintf(stderr, "plugin_notification_end_obs called!\n"); abort(); }
+/* Generated stub for plugin_notification_start_obs */
+struct json_stream *plugin_notification_start_obs(const tal_t *ctx UNNEEDED,
+						  const char *method UNNEEDED)
+{ fprintf(stderr, "plugin_notification_start_obs called!\n"); abort(); }
 /* Generated stub for plugin_notify_message */
 void plugin_notify_message(struct command *cmd UNNEEDED,
 			   enum log_level level UNNEEDED,

plugins/test/run-route-overlong.c

@@ -281,6 +281,13 @@ void memleak_add_helper_(const tal_t *p UNNEEDED, void (*cb)(struct htable *memt
 /* Generated stub for memleak_scan_htable */
 void memleak_scan_htable(struct htable *memtable UNNEEDED, const struct htable *ht UNNEEDED)
 { fprintf(stderr, "memleak_scan_htable called!\n"); abort(); }
+/* Generated stub for notification_deprecated_out_ok */
+bool notification_deprecated_out_ok(struct plugin *plugin UNNEEDED,
+				    const char *method UNNEEDED,
+				    const char *fieldname UNNEEDED,
+				    const char *depr_start UNNEEDED,
+				    const char *depr_end UNNEEDED)
+{ fprintf(stderr, "notification_deprecated_out_ok called!\n"); abort(); }
 /* Generated stub for notleak_ */
 void *notleak_(void *ptr UNNEEDED, bool plus_children UNNEEDED)
 { fprintf(stderr, "notleak_ called!\n"); abort(); }
@@ -296,14 +303,14 @@ void plugin_gossmap_logcb(struct plugin *plugin UNNEEDED,
 /* Generated stub for plugin_log */
 void plugin_log(struct plugin *p UNNEEDED, enum log_level l UNNEEDED, const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "plugin_log called!\n"); abort(); }
-/* Generated stub for plugin_notification_end */
-void plugin_notification_end(struct plugin *plugin UNNEEDED,
-			     struct json_stream *stream STEALS UNNEEDED)
-{ fprintf(stderr, "plugin_notification_end called!\n"); abort(); }
-/* Generated stub for plugin_notification_start */
-struct json_stream *plugin_notification_start(const tal_t *ctx UNNEEDED,
-					      const char *method UNNEEDED)
-{ fprintf(stderr, "plugin_notification_start called!\n"); abort(); }
+/* Generated stub for plugin_notification_end_obs */
+void plugin_notification_end_obs(struct plugin *plugin UNNEEDED,
+				 struct json_stream *stream TAKES UNNEEDED)
+{ fprintf(stderr, "plugin_notification_end_obs called!\n"); abort(); }
+/* Generated stub for plugin_notification_start_obs */
+struct json_stream *plugin_notification_start_obs(const tal_t *ctx UNNEEDED,
+						  const char *method UNNEEDED)
+{ fprintf(stderr, "plugin_notification_start_obs called!\n"); abort(); }
 /* Generated stub for plugin_notify_message */
 void plugin_notify_message(struct command *cmd UNNEEDED,
 			   enum log_level level UNNEEDED,

tests/plugins/custom_notifications.py

@@ -25,11 +25,11 @@ def faulty_emit(plugin):
 
 
 @plugin.subscribe("pay_success")
-def on_pay_success(origin, payment_hash, **kwargs):
+def on_pay_success(origin, pay_success, **kwargs):
     plugin.log(
         "Got a pay_success notification from plugin {} for payment_hash {}".format(
             origin,
-            payment_hash
+            pay_success['payment_hash']
         )
     )
 

tests/test_plugin.py

@@ -4374,10 +4374,12 @@ def test_peer_storage(node_factory, bitcoind):
     assert not l2.daemon.is_in_log(r'PeerStorageFailed')
 
 
-@pytest.mark.xfail(strict=True)
-def test_pay_plugin_notifications(node_factory, bitcoind, chainparams):
+@pytest.mark.parametrize("deprecated", [False, True])
+def test_pay_plugin_notifications(node_factory, bitcoind, chainparams, deprecated):
     plugin = os.path.join(os.getcwd(), 'tests/plugins/all_notifications.py')
     opts = {"plugin": plugin}
+    if deprecated:
+        opts['allow-deprecated-apis'] = True
 
     l1, l2, l3 = node_factory.line_graph(3, opts=[opts, {}, {}],
                                          wait_for_announce=True)
@@ -4403,15 +4405,17 @@ def zero_timestamps(obj):
     dict_str = line.split("notification channel_hint_update: ", 1)[1]
     data = zero_timestamps(ast.literal_eval(dict_str))
 
-    # pyln-client's plugin.py duplicated payload into same name as update.
     channel_hint_update_core = {'scid': first_scid(l1, l2) + '/1',
                                 'estimated_capacity_msat': 964719000 if chainparams['elements'] else 978718000,
                                 'total_capacity_msat': 1000000000,
                                 'timestamp': 0,
                                 'enabled': True}
     channel_hint_update = {'origin': 'pay',
-                           'payload': {'channel_hint': channel_hint_update_core},
-                           'channel_hint_update': {'channel_hint': channel_hint_update_core}}
+                           'channel_hint_update': channel_hint_update_core}
+    if deprecated:
+        # pyln-client's plugin.py duplicated payload into same name as update.
+        channel_hint_update['payload'] = {'channel_hint': channel_hint_update_core}
+
     assert data == channel_hint_update
 
     # It gets a success notification
@@ -4420,10 +4424,11 @@ def zero_timestamps(obj):
     data = ast.literal_eval(dict_str)
     success_core = {'payment_hash': inv1['payment_hash'],
                     'bolt11': inv1['bolt11']}
-    # Includes deprecated and modern.  pyln-client plugin.py copies fields as necessary.
     success = {'origin': 'pay',
-               'payload': success_core,
                'pay_success': success_core}
+    if deprecated:
+        # pyln-client's plugin.py duplicated payload into same name as update.
+        success['payload'] = success_core
     assert data == success
 
     inv2 = l3.rpc.invoice(10000, "second", "desc")
@@ -4435,10 +4440,11 @@ def zero_timestamps(obj):
     dict_str = line.split("notification pay_failure: ", 1)[1]
     data = ast.literal_eval(dict_str)
     failure_core = {'payment_hash': inv2['payment_hash'], 'bolt11': inv2['bolt11'], 'error': {'message': 'failed: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS (reply from remote)'}}
-    # Includes deprecated and modern.
     failure = {'origin': 'pay',
-               'payload': failure_core,
                'pay_failure': failure_core}
+    if deprecated:
+        # pyln-client's plugin.py duplicated payload into same name as update.
+        failure['payload'] = failure_core
     assert data == failure