From 16819f345d5eb534ebfbfeb0fa3d1e8c311954c2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 17 Aug 2025 09:39:12 +0930 Subject: [PATCH] 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 Changelog-Deprecated: pyln-client: plugin custom notifications origins and payload (use parameters directly) --- doc/developers-guide/deprecated-features.md | 1 + lightningd/jsonrpc.c | 14 ++- lightningd/jsonrpc.h | 12 ++ lightningd/plugin.c | 6 +- plugins/bkpr/bookkeeper.c | 18 +-- plugins/channel_hint.c | 12 +- tests/plugins/custom_notifications.py | 16 +-- tests/test_plugin.py | 3 +- tests/test_xpay.py | 116 +++++++++++++++++--- 9 files changed, 160 insertions(+), 38 deletions(-) diff --git a/doc/developers-guide/deprecated-features.md b/doc/developers-guide/deprecated-features.md index 204098c7f..3193abd48 100644 --- a/doc/developers-guide/deprecated-features.md +++ b/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. diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 43286a289..7370ad73a 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -1556,7 +1556,7 @@ 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); @@ -1564,6 +1564,13 @@ struct jsonrpc_notification *jsonrpc_notification_start(const tal_t *ctx, const 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. */ diff --git a/lightningd/jsonrpc.h b/lightningd/jsonrpc.h index c1e8172b6..e5a5edfa3 100644 --- a/lightningd/jsonrpc.h +++ b/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. diff --git a/lightningd/plugin.c b/lightningd/plugin.c index e1588f246..ccbdf83a2 100644 --- a/lightningd/plugin.c +++ b/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)); } diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c index abf845ba4..9f2dce9cb 100644 --- a/plugins/bkpr/bookkeeper.c +++ b/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)); diff --git a/plugins/channel_hint.c b/plugins/channel_hint.c index e1c12a8a5..af5457591 100644 --- a/plugins/channel_hint.c +++ b/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), diff --git a/tests/plugins/custom_notifications.py b/tests/plugins/custom_notifications.py index e09942b84..c57a386cd 100755 --- a/tests/plugins/custom_notifications.py +++ b/tests/plugins/custom_notifications.py @@ -6,8 +6,8 @@ plugin = Plugin() @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") diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 427085d03..706bda882 100644 --- a/tests/test_plugin.py +++ b/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} diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 9022e28dd..f9c95a307 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -7,6 +7,7 @@ from utils import ( sync_blockheight, ) +import ast import os import pytest import re @@ -835,6 +836,19 @@ lightning-cli pay lni1qqgv5nalmz08ukj4av074kyk6pepq93pqvvhnlnvurnfanndnxjtcjnmxr 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,12 +898,38 @@ 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() @@ -876,9 +937,38 @@ 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': '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