plugins: allow plugins to disable themselves at startup.

By returning 'disable: <reason>' inside getmanifest or init result.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: plugins: plugins can now disable themselves by returning `disable`, even if marked important.

Author: rustyrussell

doc/PLUGINS.md

@@ -128,6 +128,10 @@ The `dynamic` indicates if the plugin can be managed after `lightningd`
 has been started. Critical plugins that should not be stopped should set it
 to false.
 
+If a `disable` member exists, the plugin will be disabled and the contents
+of this member is the reason why.  This allows plugins to disable themselves
+if they are not supported in this configuration.
+
 The `featurebits` object allows the plugin to register featurebits that should be
 announced in a number of places in [the protocol][bolt9]. They can be used to signal
 support for custom protocol extensions to direct peers, remote nodes and in
@@ -237,10 +241,11 @@ simple JSON object containing the options:
 }
 ```
 
-The plugin must respond to `init` calls, however the response can be
-arbitrary and will currently be discarded by `lightningd`. JSON-RPC
-commands were chosen over notifications in order not to force plugins
-to implement notifications which are not that well supported.
+The plugin must respond to `init` calls.  The response should be a
+valid JSON-RPC response to the `init`, but this is not currently
+enforced.  If the response is an object containing `result` which
+contains `disable` then the plugin will be disabled and the contents
+of this member is the reason why.
 
 The `startup` field allows a plugin to detect if it was started at
 `lightningd` startup (true), or at runtime (false).

lightningd/plugin.c

@@ -1229,6 +1229,17 @@ static const char *plugin_parse_getmanifest_response(const char *buffer,
 			       json_tok_full_len(toks),
 			       json_tok_full(buffer, toks));
 
+	/* Plugin can disable itself: returns why it's disabled. */
+	tok = json_get_member(buffer, resulttok, "disable");
+	if (tok) {
+		log_debug(plugin->log, "disabled itself: %.*s",
+			  json_tok_full_len(tok),
+			  json_tok_full(buffer, tok));
+		/* Don't get upset if this was a built-in! */
+		plugin->important = false;
+		return json_strdup(plugin, buffer, tok);
+	}
+
 	dynamictok = json_get_member(buffer, resulttok, "dynamic");
 	if (dynamictok && !json_to_bool(buffer, dynamictok, &plugin->dynamic)) {
 		return tal_fmt(plugin, "Bad 'dynamic' field ('%.*s')",
@@ -1572,6 +1583,19 @@ static void plugin_config_cb(const char *buffer,
 			     const jsmntok_t *idtok,
 			     struct plugin *plugin)
 {
+	const char *disable;
+
+	/* Plugin can also disable itself at this stage. */
+	if (json_scan(tmpctx, buffer, toks, "{result:{disable:%}}",
+		      JSON_SCAN_TAL(tmpctx, json_strdup, &disable)) == NULL) {
+		log_debug(plugin->log, "disabled itself at init: %s",
+			  disable);
+		/* Don't get upset if this was a built-in! */
+		plugin->important = false;
+		plugin_kill(plugin, disable);
+		return;
+	}
+
 	plugin->plugin_state = INIT_COMPLETE;
 	plugin->timeout_timer = tal_free(plugin->timeout_timer);
 	if (plugin->start_cmd) {

tests/plugins/Makefile

@@ -1,10 +1,15 @@
-PLUGIN_TESTLIBPLUGIN_SRC := tests/plugins/test_libplugin.c
+PLUGIN_TESTLIBPLUGIN_SRC := tests/plugins/test_libplugin.c 
 PLUGIN_TESTLIBPLUGIN_OBJS := $(PLUGIN_TESTLIBPLUGIN_SRC:.c=.o)
 
 tests/plugins/test_libplugin: bitcoin/chainparams.o $(PLUGIN_TESTLIBPLUGIN_OBJS) $(PLUGIN_LIB_OBJS) $(PLUGIN_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS)
 
 $(PLUGIN_TESTLIBPLUGIN_OBJS): $(PLUGIN_LIB_HEADER)
 
+PLUGIN_TESTSELFDISABLE_AFTER_GETMANIFEST_SRC := tests/plugins/test_selfdisable_after_getmanifest.c 
+PLUGIN_TESTSELFDISABLE_AFTER_GETMANIFEST_OBJS := $(PLUGIN_TESTSELFDISABLE_AFTER_GETMANIFEST_SRC:.c=.o)
+
+tests/plugins/test_selfdisable_after_getmanifest: bitcoin/chainparams.o $(PLUGIN_TESTSELFDISABLE_AFTER_GETMANIFEST_OBJS) common/json.o common/json_stream.o common/setup.o common/utils.o $(JSMN_OBJS) $(CCAN_OBJS)
+
 # Make sure these depend on everything.
-ALL_TEST_PROGRAMS += tests/plugins/test_libplugin
-ALL_C_SOURCES += $(PLUGIN_TESTLIBPLUGIN_SRC)
+ALL_TEST_PROGRAMS += tests/plugins/test_libplugin tests/plugins/test_selfdisable_after_getmanifest
+ALL_C_SOURCES += $(PLUGIN_TESTLIBPLUGIN_SRC) $(PLUGIN_TESTSELFDISABLE_AFTER_GETMANIFEST_SRC)

tests/plugins/test_selfdisable_after_getmanifest.c

@@ -0,0 +1,42 @@
+#include <ccan/err/err.h>
+#include <ccan/read_write_all/read_write_all.h>
+#include <ccan/tal/tal.h>
+#include <ccan/tal/str/str.h>
+#include <common/json.h>
+#include <common/setup.h>
+#include <common/utils.h>
+#include <unistd.h>
+
+/* Our normal frameworks don't (yet?) support custom post-manifest responses,
+ * so this is open-coded */
+int main(int argc, char *argv[])
+{
+	char *buf;
+	int r, off;
+	const jsmntok_t *toks, *id;
+
+	common_setup(argv[0]);
+
+	buf = tal_arr(tmpctx, char, 100);
+	off = 0;
+	do {
+		r = read(STDIN_FILENO, buf + off, tal_bytelen(buf) - off);
+		if (r < 0)
+			err(1, "reading stdin");
+		off += r;
+		if (off == tal_bytelen(buf))
+			tal_resize(&buf, off * 2);
+
+		toks = json_parse_simple(tmpctx, buf, off);
+	} while (!toks);
+
+	/* Tell it we're disabled (reusing id). */
+	id = json_get_member(buf, toks, "id");
+	buf = tal_fmt(tmpctx, "{\"jsonrpc\":\"2.0\",\"id\":%.*s,\"result\":{\"disable\":\"Self-disable test after getmanifest\"} }",
+		      json_tok_full_len(id),
+		      json_tok_full(buf, id));
+	write_all(STDOUT_FILENO, buf, strlen(buf));
+
+	common_shutdown();
+	return 0;
+}

tests/test_plugin.py

@@ -2321,3 +2321,24 @@ def n(*args, message, **kwargs):
     notifications = []
     l1.rpc.countdown(10)
     assert notifications == []
+
+
+def test_self_disable(node_factory):
+    """Test that plugin can disable itself without penalty.
+    """
+    plugin_path = os.path.join(
+        os.path.dirname(__file__), 'plugins/test_selfdisable'
+    )
+    l1 = node_factory.get_node(options={'important-plugin': plugin_path})
+
+    # Could happen before it gets set up.
+    l1.daemon.logsearch_start = 0
+    l1.daemon.wait_for_log('test_selfdisable: disabled itself: "Self-disable test after getmanifest"')
+
+    assert plugin_path not in [p['name'] for p in l1.rpc.plugin_list()['plugins']]
+
+    # Also works with dynamic load attempts
+    with pytest.raises(RpcError, match="Self-disable test after getmanifest"):
+        l1.rpc.plugin_start(plugin_path)
+
+    # Now test the disable-in-init-response.