lightningd: make notifications from plugins just like native ones.

Rather than forcing them to wrap their parameters in a "payload"
sub-object, copy in params directly.  We include the "origin" field
one level up, if they care.

The next patch restores compatibility for the one place we currently use
them, which is the pay plugin.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Deprecated: pyln-client: plugin custom notifications origins and payload (use parameters directly)

Author: rustyrussell

doc/developers-guide/deprecated-features.md

@@ -22,6 +22,7 @@ hidden: false
 | coin_movement.utxo_txid              | Notification Field | v25.09           | v26.09         | Use `utxo` instead of `utxo_txid` & `vout`                                                                                             |
 | 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)              |
 
 Inevitably there are features which need to change: either to be generalized, or removed when they can no longer be supported.
 

lightningd/jsonrpc.c

@@ -1556,14 +1556,21 @@ static struct command_result *param_command(struct command *cmd,
 			    tok->end - tok->start, buffer + tok->start);
 }
 
-struct jsonrpc_notification *jsonrpc_notification_start(const tal_t *ctx, const char *method)
+struct jsonrpc_notification *jsonrpc_notification_start_noparams(const tal_t *ctx, const char *method)
 {
 	struct jsonrpc_notification *n = tal(ctx, struct jsonrpc_notification);
 	n->method = tal_strdup(n, method);
 	n->stream = new_json_stream(n, NULL, NULL);
 	json_object_start(n->stream, NULL);
 	json_add_string(n->stream, "jsonrpc", "2.0");
 	json_add_string(n->stream, "method", method);
+
+	return n;
+}
+
+struct jsonrpc_notification *jsonrpc_notification_start(const tal_t *ctx, const char *method)
+{
+	struct jsonrpc_notification *n = jsonrpc_notification_start_noparams(ctx, method);
 	json_object_start(n->stream, "params");
 
 	return n;
@@ -1572,6 +1579,11 @@ struct jsonrpc_notification *jsonrpc_notification_start(const tal_t *ctx, const
 void jsonrpc_notification_end(struct jsonrpc_notification *n)
 {
 	json_object_end(n->stream); /* closes '.params' */
+	jsonrpc_notification_end_noparams(n);
+}
+
+void jsonrpc_notification_end_noparams(struct jsonrpc_notification *n)
+{
 	json_object_end(n->stream); /* closes '.' */
 
 	/* We guarantee to have \n\n at end of each response. */

lightningd/jsonrpc.h

@@ -226,11 +226,23 @@ bool jsonrpc_command_add(struct jsonrpc *rpc, struct json_command *command,
  */
 struct jsonrpc_notification *jsonrpc_notification_start(const tal_t *ctx, const char *topic);
 
+/**
+ * Begin a JSON-RPC notification with the specified topic.
+ *
+ * Does *not* start a "params" object.
+ */
+struct jsonrpc_notification *jsonrpc_notification_start_noparams(const tal_t *ctx, const char *topic);
+
 /**
  * Counterpart to jsonrpc_notification_start.
  */
 void jsonrpc_notification_end(struct jsonrpc_notification *n);
 
+/**
+ * Counterpart to jsonrpc_notification_start_noparams.
+ */
+void jsonrpc_notification_end_noparams(struct jsonrpc_notification *n);
+
 /**
  * start a JSONRPC request; id_prefix is non-NULL if this was triggered by
  * another JSONRPC request.

lightningd/plugin.c

@@ -624,10 +624,10 @@ static const char *plugin_notification_handle(struct plugin *plugin,
 			    "forwarding to subscribers.",
 			    methname);
 	} else if (notifications_have_topic(plugin->plugins, methname)) {
-		n = jsonrpc_notification_start(NULL, methname);
+		n = jsonrpc_notification_start_noparams(NULL, methname);
 		json_add_string(n->stream, "origin", plugin->shortname);
-		json_add_tok(n->stream, "payload", paramstok, plugin->buffer);
-		jsonrpc_notification_end(n);
+		json_add_tok(n->stream, "params", paramstok, plugin->buffer);
+		jsonrpc_notification_end_noparams(n);
 
 		plugins_notify(plugin->plugins, take(n));
 	}

plugins/bkpr/bookkeeper.c

@@ -1428,7 +1428,7 @@ parse_and_log_chain_move(struct command *cmd,
 
 	if (err)
 		plugin_err(cmd->plugin,
-			   "`coin_movement` payload did"
+			   "`coin_movement` parameters did"
 			   " not scan %s: %.*s",
 			   err, json_tok_full_len(params),
 			   json_tok_full(buf, params));
@@ -1725,14 +1725,14 @@ static struct command_result *json_utxo_deposit(struct command *cmd, const char
 	const char *err;
 
 	err = json_scan(tmpctx, buf, params,
-			"{payload:{utxo_deposit:{"
+			"{utxo_deposit:{"
 			"account:%"
 			",transfer_from:%"
 			",outpoint:%"
 			",amount_msat:%"
 			",timestamp:%"
 			",blockheight:%"
-			"}}}",
+			"}}",
 			JSON_SCAN_TAL(tmpctx, json_strdup, &ev->acct_name),
 			JSON_SCAN_TAL(tmpctx, json_strdup, &ev->origin_acct),
 			JSON_SCAN(json_to_outpoint, &ev->outpoint),
@@ -1742,7 +1742,7 @@ static struct command_result *json_utxo_deposit(struct command *cmd, const char
 
 	if (err)
 		plugin_err(cmd->plugin,
-			   "`%s` payload did not scan %s: %.*s",
+			   "`%s` parameters did not scan %s: %.*s",
 			   move_tag, err, json_tok_full_len(params),
 			   json_tok_full(buf, params));
 
@@ -1800,14 +1800,14 @@ static struct command_result *json_utxo_spend(struct command *cmd, const char *b
 
 	ev->spending_txid = tal(ev, struct bitcoin_txid);
 	err = json_scan(tmpctx, buf, params,
-			"{payload:{utxo_spend:{"
+			"{utxo_spend:{"
 			"account:%"
 			",outpoint:%"
 			",spending_txid:%"
 			",amount_msat:%"
 			",timestamp:%"
 			",blockheight:%"
-			"}}}",
+			"}}",
 			JSON_SCAN_TAL(tmpctx, json_strdup, &acct_name),
 			JSON_SCAN(json_to_outpoint, &ev->outpoint),
 			JSON_SCAN(json_to_txid, ev->spending_txid),
@@ -1817,7 +1817,7 @@ static struct command_result *json_utxo_spend(struct command *cmd, const char *b
 
 	if (err)
 		plugin_err(cmd->plugin,
-			   "`%s` payload did not scan %s: %.*s",
+			   "`%s` parameters did not scan %s: %.*s",
 			   move_tag, err, json_tok_full_len(params),
 			   json_tok_full(buf, params));
 
@@ -1911,7 +1911,7 @@ static struct command_result *json_coin_moved(struct command *cmd,
 
 	if (err)
 		plugin_err(cmd->plugin,
-			   "`coin_movement` payload did not scan %s: %.*s",
+			   "`coin_movement` parameters did not scan %s: %.*s",
 			   err, json_tok_full_len(params),
 			   json_tok_full(buf, params));
 
@@ -1920,7 +1920,7 @@ static struct command_result *json_coin_moved(struct command *cmd,
 			 &tags);
 	if (err)
 		plugin_err(cmd->plugin,
-			   "`coin_movement` payload did not scan %s: %.*s",
+			   "`coin_movement` parameters did not scan %s: %.*s",
 			   err, json_tok_full_len(params),
 			   json_tok_full(buf, params));
 

plugins/channel_hint.c

@@ -184,11 +184,17 @@ struct channel_hint *channel_hint_from_json(const tal_t *ctx,
 					    const jsmntok_t *toks)
 {
 	const char *ret;
-	const jsmntok_t *payload = json_get_member(buffer, toks, "payload"),
-			*jhint =
-			    json_get_member(buffer, payload, "channel_hint");
+	const jsmntok_t *payload , *jhint;
 	struct channel_hint *hint = tal(ctx, struct channel_hint);
 
+	/* Deprecated API uses "payload" */
+	payload = json_get_member(buffer, toks, "payload");
+	/* Modern API includes fields directly */
+	if (!payload) {
+		jhint = json_get_member(buffer, toks, "channel_hint");
+	} else {
+		jhint = json_get_member(buffer, payload, "channel_hint");
+	}
 	ret = json_scan(ctx, buffer, jhint,
 			"{timestamp:%,scid:%,estimated_capacity_msat:%,total_capacity_msat:%,enabled:%}",
 			JSON_SCAN(json_to_u32, &hint->timestamp),

tests/plugins/custom_notifications.py

@@ -6,8 +6,8 @@
 
 
 @plugin.subscribe("custom")
-def on_custom_notification(origin, payload, **kwargs):
-    plugin.log("Got a custom notification {} from plugin {}".format(payload, origin))
+def on_custom_notification(origin, message, **kwargs):
+    plugin.log("Got a custom notification {} from plugin {}".format(message, origin))
 
 
 @plugin.method("emit")
@@ -25,23 +25,23 @@ def faulty_emit(plugin):
 
 
 @plugin.subscribe("pay_success")
-def on_pay_success(origin, payload, **kwargs):
+def on_pay_success(origin, payment_hash, **kwargs):
     plugin.log(
         "Got a pay_success notification from plugin {} for payment_hash {}".format(
             origin,
-            payload['payment_hash']
+            payment_hash
         )
     )
 
 
 @plugin.subscribe("pay_part_start")
-def on_pay_part_start(origin, payload, **kwargs):
-    plugin.log("Got pay_part_start: {}".format(payload))
+def on_pay_part_start(origin, **kwargs):
+    plugin.log("Got pay_part_start: {}".format(kwargs))
 
 
 @plugin.subscribe("pay_part_end")
-def on_pay_part_end(origin, payload, **kwargs):
-    plugin.log("Got pay_part_end: {}".format(payload))
+def on_pay_part_end(origin, **kwargs):
+    plugin.log("Got pay_part_end: {}".format(kwargs))
 
 
 @plugin.subscribe("ididntannouncethis")

tests/test_plugin.py

@@ -2641,7 +2641,7 @@ def test_custom_notification_topics(node_factory):
     )
     l1, l2 = node_factory.line_graph(2, opts=[{'plugin': plugin}, {}])
     l1.rpc.emit()
-    l1.daemon.wait_for_log("Got a custom notification {'message': 'Hello world'} from plugin custom_notifications.py")
+    l1.daemon.wait_for_log("Got a custom notification Hello world from plugin custom_notifications.py")
 
     inv = l2.rpc.invoice(42, "lbl", "desc")['bolt11']
     l1.rpc.pay(inv)
@@ -4374,6 +4374,7 @@ 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):
     plugin = os.path.join(os.getcwd(), 'tests/plugins/all_notifications.py')
     opts = {"plugin": plugin}

tests/test_xpay.py

@@ -7,6 +7,7 @@
     sync_blockheight,
 )
 
+import ast
 import os
 import pytest
 import re
@@ -835,6 +836,19 @@ def test_xpay_twohop_bug(node_factory, bitcoind):
 
 
 def test_attempt_notifications(node_factory):
+    def zero_fields(obj, fieldnames):
+        if isinstance(obj, dict):
+            for k, v in obj.items():
+                if k in fieldnames:
+                    obj[k] = 0
+                else:
+                    zero_fields(v, fieldnames)
+        elif isinstance(obj, list):
+            for item in obj:
+                zero_fields(item, fieldnames)
+        # other types are ignored
+        return obj
+
     plugin_path = os.path.join(os.getcwd(), 'tests/plugins/custom_notifications.py')
     l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True,
                                          opts=[{"plugin": plugin_path}, {}, {}])
@@ -847,13 +861,34 @@ def test_attempt_notifications(node_factory):
     l1.rpc.xpay(inv1['bolt11'])
 
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_start: ")
-    regex = r".*Got pay_part_start: \{'payment_hash': '" + inv1['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1, 'total_payment_msat': 5000000, 'attempt_msat': 5000000, 'hops': \[\{'next_node': '" + l2.info['id'] + r"', 'short_channel_id': '" + scid12 + r"', 'direction': " + str(scid12_dir) + r", 'channel_in_msat': 5000051, 'channel_out_msat': 5000051\}, \{'next_node': '" + l3.info['id'] + r"', 'short_channel_id': '" + scid23 + r"', 'direction': " + str(scid23_dir) + r", 'channel_in_msat': 5000051, 'channel_out_msat': 5000000\}\]\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_start: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ['groupid'])
+    expected = {'payment_hash': inv1['payment_hash'],
+                'groupid': 0,
+                'partid': 1,
+                'total_payment_msat': 5000000,
+                'attempt_msat': 5000000,
+                'hops': [{'next_node': l2.info['id'],
+                          'short_channel_id': scid12,
+                          'direction': scid12_dir,
+                          'channel_in_msat': 5000051,
+                          'channel_out_msat': 5000051},
+                         {'next_node': l3.info['id'],
+                          'short_channel_id': scid23,
+                          'direction': scid23_dir,
+                          'channel_in_msat': 5000051,
+                          'channel_out_msat': 5000000}]}
+    assert data == expected
 
-    # Note, duration always has 9 decimals, EXCEPT that the python code interprets it, so if the last digit is a 0 it will only print 8.
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_end: ")
-    regex = r".*Got pay_part_end: \{'status': 'success', 'duration': [0-9]*\.[0-9]*, 'payment_hash': '" + inv1['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_end: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ('duration', 'groupid'))
+    expected = {'payment_hash': inv1['payment_hash'],
+                'status': 'success',
+                'duration': 0,
+                'groupid': 0,
+                'partid': 1}
+    assert data == expected
 
     inv2 = l3.rpc.invoice(10000000, 'test_attempt_notifications2', 'test_attempt_notifications2')
     l3.rpc.delinvoice('test_attempt_notifications2', "unpaid")
@@ -863,22 +898,77 @@ def test_attempt_notifications(node_factory):
         l1.rpc.xpay(inv2['bolt11'])
 
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_start: ")
-    regex = r".*Got pay_part_start: \{'payment_hash': '" + inv2['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1, 'total_payment_msat': 10000000, 'attempt_msat': 10000000, 'hops': \[\{'next_node': '" + l2.info['id'] + r"', 'short_channel_id': '" + scid12 + r"', 'direction': " + str(scid12_dir) + r", 'channel_in_msat': 10000101, 'channel_out_msat': 10000101\}, \{'next_node': '" + l3.info['id'] + r"', 'short_channel_id': '" + scid23 + r"', 'direction': " + str(scid23_dir) + r", 'channel_in_msat': 10000101, 'channel_out_msat': 10000000\}\]\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_start: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ['groupid'])
+    expected = {'payment_hash': inv2['payment_hash'],
+                'groupid': 0,
+                'partid': 1,
+                'total_payment_msat': 10000000,
+                'attempt_msat': 10000000,
+                'hops': [{'next_node': l2.info['id'],
+                          'short_channel_id': scid12,
+                          'direction': scid12_dir,
+                          'channel_in_msat': 10000101,
+                          'channel_out_msat': 10000101},
+                         {'next_node': l3.info['id'],
+                          'short_channel_id': scid23,
+                          'direction': scid23_dir,
+                          'channel_in_msat': 10000101,
+                          'channel_out_msat': 10000000}]}
+    assert data == expected
 
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_end: ")
-    regex = r".*Got pay_part_end: \{'status': 'failure', 'payment_hash': '" + inv2['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1, 'failed_msg': '400f00000000009896800000006c', 'duration': [0-9]*\.[0-9]*, 'failed_node_id': '" + l3.info['id'] + r"', 'error_code': 16399, 'error_message': 'incorrect_or_unknown_payment_details'\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_end: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ('duration', 'groupid'))
+    expected = {'payment_hash': inv2['payment_hash'],
+                'status': 'failure',
+                'duration': 0,
+                'groupid': 0,
+                'partid': 1,
+                'failed_msg': '400f00000000009896800000006c',
+                'failed_node_id': l3.info['id'],
+                'error_code': 16399,
+                'error_message': 'incorrect_or_unknown_payment_details'}
+    assert data == expected
 
     # Intermediary node failure
     l3.stop()
     with pytest.raises(RpcError, match=r"Failed after 1 attempts"):
         l1.rpc.xpay(inv2['bolt11'])
 
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_start: ")
-    regex = r".*Got pay_part_start: \{'payment_hash': '" + inv2['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1, 'total_payment_msat': 10000000, 'attempt_msat': 10000000, 'hops': \[\{'next_node': '" + l2.info['id'] + r"', 'short_channel_id': '" + scid12 + r"', 'direction': " + str(scid12_dir) + r", 'channel_in_msat': 10000101, 'channel_out_msat': 10000101\}, \{'next_node': '" + l3.info['id'] + r"', 'short_channel_id': '" + scid23 + r"', 'direction': " + str(scid23_dir) + r", 'channel_in_msat': 10000101, 'channel_out_msat': 10000000\}\]\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_start: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ['groupid'])
+    expected = {'payment_hash': inv2['payment_hash'],
+                'groupid': 0,
+                'partid': 1,
+                'total_payment_msat': 10000000,
+                'attempt_msat': 10000000,
+                'hops': [{'next_node': l2.info['id'],
+                          'short_channel_id': scid12,
+                          'direction': scid12_dir,
+                          'channel_in_msat': 10000101,
+                          'channel_out_msat': 10000101},
+                         {'next_node': l3.info['id'],
+                          'short_channel_id': scid23,
+                          'direction': scid23_dir,
+                          'channel_in_msat': 10000101,
+                          'channel_out_msat': 10000000}]}
+    assert data == expected
 
     line = l1.daemon.wait_for_log("plugin-custom_notifications.py: Got pay_part_end: ")
-    regex = r".*Got pay_part_end: \{'status': 'failure', 'payment_hash': '" + inv2['payment_hash'] + r"', 'groupid': [0-9]*, 'partid': 1, 'failed_msg': '1007[a-f0-9]*', 'duration': [0-9]*\.[0-9]*, 'failed_node_id': '" + l2.info['id'] + r"', 'failed_short_channel_id': '" + scid23 + r"', 'failed_direction': " + str(scid23_dir) + r", 'error_code': 4103, 'error_message': 'temporary_channel_failure'\}"
-    assert re.match(regex, line)
+    dict_str = line.split("Got pay_part_end: ", 1)[1]
+    data = zero_fields(ast.literal_eval(dict_str), ('duration', 'groupid', 'failed_msg'))
+    expected = {'payment_hash': inv2['payment_hash'],
+                'status': 'failure',
+                'duration': 0,
+                'groupid': 0,
+                'partid': 1,
+                # This includes the channel update: just zero it out
+                'failed_msg': 0,
+                'failed_direction': 0,
+                'failed_node_id': l2.info['id'],
+                'failed_short_channel_id': scid23,
+                'error_code': 4103,
+                'error_message': 'temporary_channel_failure'}
+    assert data == expected