feat: peer_connected hook chainable

doc/PLUGINS.md

@@ -777,7 +777,7 @@ no plugin is registered on the hook.
 ### `peer_connected`
 
 This hook is called whenever a peer has connected and successfully completed
-the cryptographic handshake. The parameters have the following structure if there is a channel with the peer:
+the cryptographic handshake. The parameters have the following structure:
 
 ```json
 {
@@ -789,7 +789,7 @@ the cryptographic handshake. The parameters have the following structure if ther
 }
 ```
 
-The hook is sparse on purpose, since the plugin can use the JSON-RPC
+The hook is sparse on information, since the plugin can use the JSON-RPC
 `listpeers` command to get additional details should they be required. The
 `addr` field shows the address that we are connected to ourselves, not the
 gossiped list of known addresses. In particular this means that the port for
@@ -801,6 +801,9 @@ the string `disconnect` or `continue`.  If `disconnect` and
 there's a member `error_message`, that member is sent to the peer
 before disconnection.
 
+Note that `peer_connected` is a chained hook. The first plugin that decides to
+`disconnect` with or without an `error_message` will lead to the subsequent
+plugins not being called anymore.
 
 ### `commitment_revocation`
 
@@ -936,7 +939,7 @@ This hook is called whenever a valid payment for an unpaid invoice has arrived.
 }
 ```
 
-The hook is sparse on purpose, since the plugin can use the JSON-RPC
+The hook is deliberately sparse, since the plugin can use the JSON-RPC
 `listinvoices` command to get additional details about this invoice.
 It can return a `failure_message` field as defined for final
 nodes in [BOLT 4][bolt4-failure-messages], a `result` field with the string
@@ -987,7 +990,12 @@ e.g.
 }
 ```
 
-Note that `close_to` must be a valid address for the current chain; an invalid address will cause the node to exit with an error.
+Note that `close_to` must be a valid address for the current chain,
+an invalid address will cause the node to exit with an error.
+
+Note that `openchannel` is a chained hook. Therefore `close_to` will only be
+evaluated for the first plugin that sets it. If more than one plugin tries to
+set a `close_to` address an error will be logged.
 
 
 ### `htlc_accepted`

lightningd/dual_open_control.c

@@ -387,7 +387,7 @@ openchannel2_hook_cb(struct openchannel2_payload *payload STEALS)
 
 	if (payload->err_msg) {
 		log_debug(dualopend->ld->log,
-			  "openchannel2_hook rejects and says '%s'",
+			  "openchannel2 hook rejects and says '%s'",
 			  payload->err_msg);
 		msg = towire_dualopend_fail(NULL, payload->err_msg);
 		return subd_send_msg(dualopend, take(msg));

lightningd/opening_control.c

@@ -707,7 +707,7 @@ openchannel_hook_deserialize(struct openchannel_hook_payload *payload,
 		if (t_errmsg)
 			payload->errmsg = json_strdup(payload, buffer, t_errmsg);
 		log_debug(openingd->ld->log,
-			  "openchannel_hook rejects and says '%s'",
+			  "openchannel hook rejects and says '%s'",
 			  payload->errmsg);
 		if (t_closeto)
 			fatal("Plugin rejected openchannel but also set close_to");
@@ -724,7 +724,7 @@ openchannel_hook_deserialize(struct openchannel_hook_payload *payload,
 		/* First plugin can set close_to. Log others. */
 		if (payload->our_upfront_shutdown_script != NULL) {
 			log_broken(openingd->ld->log,
-				   "openchannel_hook close_to address was"
+				   "openchannel hook close_to address was"
 				   " already set by other plugin. Ignoring!");
 			return true;
 		}

lightningd/peer_control.c

@@ -437,14 +437,6 @@ void channel_errmsg(struct channel *channel,
 				       err_for_them ? "sent" : "received", desc);
 }
 
-struct peer_connected_hook_payload {
-	struct lightningd *ld;
-	struct channel *channel;
-	struct wireaddr_internal addr;
-	struct peer *peer;
-	struct per_peer_state *pps;
-};
-
 static void json_add_htlcs(struct lightningd *ld,
 			   struct json_stream *response,
 			   const struct channel *channel)
@@ -932,6 +924,15 @@ static void json_add_channel(struct lightningd *ld,
 	json_object_end(response);
 }
 
+struct peer_connected_hook_payload {
+	struct lightningd *ld;
+	struct channel *channel;
+	struct wireaddr_internal addr;
+	struct peer *peer;
+	struct per_peer_state *pps;
+	u8 *error;
+};
+
 static void
 peer_connected_serialize(struct peer_connected_hook_payload *payload,
 			 struct json_stream *stream)
@@ -946,10 +947,7 @@ peer_connected_serialize(struct peer_connected_hook_payload *payload,
 	json_object_end(stream); /* .peer */
 }
 
-static void
-peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
-		       const char *buffer,
-		       const jsmntok_t *toks)
+static void peer_connected_hook_final(struct peer_connected_hook_payload *payload STEALS)
 {
 	struct lightningd *ld = payload->ld;
 	struct channel *channel = payload->channel;
@@ -962,30 +960,10 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
 	 * subd). */
 	tal_steal(tmpctx, payload);
 
-	/* If we had a hook, interpret result. */
-	if (buffer) {
-		const jsmntok_t *resulttok;
-
-		resulttok = json_get_member(buffer, toks, "result");
-		if (!resulttok) {
-			fatal("Plugin returned an invalid response to the connected "
-			      "hook: %s", buffer);
-		}
-
-		if (json_tok_streq(buffer, resulttok, "disconnect")) {
-			const jsmntok_t *m = json_get_member(buffer, toks,
-							     "error_message");
-			if (m) {
-				error = towire_errorfmt(tmpctx, NULL,
-							"%.*s",
-							m->end - m->start,
-							buffer + m->start);
-				goto send_error;
-			}
-			return;
-		} else if (!json_tok_streq(buffer, resulttok, "continue"))
-			fatal("Plugin returned an invalid response to the connected "
-			      "hook: %s", buffer);
+	/* Check for specific errors of a hook */
+	if (payload->error) {
+		error = payload->error;
+		goto send_error;
 	}
 
 	if (channel) {
@@ -1000,8 +978,7 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
 
 #if DEVELOPER
 		if (dev_disconnect_permanent(ld)) {
-			channel_fail_permanent(channel,
-					       REASON_LOCAL,
+			channel_fail_permanent(channel, REASON_LOCAL,
 					       "dev_disconnect permfail");
 			error = channel->error;
 			goto send_error;
@@ -1030,11 +1007,8 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
 		case DUALOPEND_AWAITING_LOCKIN:
 #if EXPERIMENTAL_FEATURES
 			assert(!channel->owner);
-
 			channel->peer->addr = addr;
-			peer_restart_dualopend(peer, payload->pps,
-				               channel, NULL);
-
+			peer_restart_dualopend(peer, payload->pps, channel, NULL);
 			return;
 #else
 			abort();
@@ -1043,18 +1017,14 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
 		case CHANNELD_NORMAL:
 		case CHANNELD_SHUTTING_DOWN:
 			assert(!channel->owner);
-
 			channel->peer->addr = addr;
-			peer_start_channeld(channel, payload->pps,
-					    NULL, true);
+			peer_start_channeld(channel, payload->pps, NULL, true);
 			return;
 
 		case CLOSINGD_SIGEXCHANGE:
 			assert(!channel->owner);
-
 			channel->peer->addr = addr;
-			peer_start_closingd(channel, payload->pps,
-					    true, NULL);
+			peer_start_closingd(channel, payload->pps, true, NULL);
 			return;
 		}
 		abort();
@@ -1074,23 +1044,67 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS,
 		 * dualopend. we only get here if there's an error  */
 		if (channel) {
 			assert(!channel->owner);
-
 			assert(channel->state == DUALOPEND_OPEN_INIT
 			       || channel->state == DUALOPEND_AWAITING_LOCKIN);
 			channel->peer->addr = addr;
-			peer_restart_dualopend(peer, payload->pps,
-				               channel, error);
+			peer_restart_dualopend(peer, payload->pps, channel, error);
 		} else
 			peer_start_dualopend(peer, payload->pps, error);
 	} else
 #endif /* EXPERIMENTAL_FEATURES */
 		peer_start_openingd(peer, payload->pps, error);
 }
 
-REGISTER_SINGLE_PLUGIN_HOOK(peer_connected,
-			    peer_connected_hook_cb,
-			    peer_connected_serialize,
-			    struct peer_connected_hook_payload *);
+static bool
+peer_connected_hook_deserialize(struct peer_connected_hook_payload *payload,
+				const char *buffer,
+				const jsmntok_t *toks)
+{
+	struct lightningd *ld = payload->ld;
+
+	/* already rejected by prior plugin hook in the chain */
+	if (payload->error != NULL)
+		return true;
+
+	if (!toks || !buffer)
+		return true;
+
+	/* If we had a hook, interpret result. */
+	const jsmntok_t *t_res = json_get_member(buffer, toks, "result");
+	const jsmntok_t *t_err = json_get_member(buffer, toks, "error_message");
+
+	/* fail */
+	if (!t_res)
+		fatal("Plugin returned an invalid response to the "
+		      "peer_connected hook: %s", buffer);
+
+	/* reject */
+	if (json_tok_streq(buffer, t_res, "disconnect")) {
+		payload->error = (u8*)"";
+		if (t_err) {
+			payload->error = towire_errorfmt(tmpctx, NULL, "%.*s",
+						         t_err->end - t_err->start,
+						         buffer + t_err->start);
+		}
+		log_debug(ld->log, "peer_connected hook rejects and says '%s'",
+			  payload->error);
+		/* At this point we suppress other plugins in the chain and
+		 * directly move to final */
+		peer_connected_hook_final(payload);
+		return false;
+	} else if (!json_tok_streq(buffer, t_res, "continue"))
+		fatal("Plugin returned an invalid response to the "
+		      "peer_connected hook: %s", buffer);
+
+	/* call next hook */
+	return true;
+}
+
+REGISTER_PLUGIN_HOOK(peer_connected,
+		     peer_connected_hook_deserialize,
+		     peer_connected_hook_final,
+		     peer_connected_serialize,
+		     struct peer_connected_hook_payload *);
 
 /* Connectd tells us a peer has connected: it never hands us duplicates, since
  * it holds them until we say peer_died. */
@@ -1104,6 +1118,7 @@ void peer_connected(struct lightningd *ld, const u8 *msg,
 
 	hook_payload = tal(NULL, struct peer_connected_hook_payload);
 	hook_payload->ld = ld;
+	hook_payload->error = NULL;
 	if (!fromwire_connectd_peer_connected(hook_payload, msg,
 					     &id, &hook_payload->addr,
 					     &hook_payload->pps,

tests/plugins/peer_connected_logger_a.py

@@ -0,0 +1,17 @@
+#!/usr/bin/env python3
+"""Simple plugin to log the connected_hook.
+
+"""
+
+from pyln.client import Plugin
+
+plugin = Plugin()
+
+
+@plugin.hook('peer_connected')
+def on_connected(peer, plugin, **kwargs):
+    print("peer_connected_logger_a {}".format(peer['id']))
+    return {'result': 'continue'}
+
+
+plugin.run()

tests/plugins/peer_connected_logger_b.py

@@ -0,0 +1,17 @@
+#!/usr/bin/env python3
+"""Simple plugin to log the connected_hook.
+
+"""
+
+from pyln.client import Plugin
+
+plugin = Plugin()
+
+
+@plugin.hook('peer_connected')
+def on_connected(peer, plugin, **kwargs):
+    print("peer_connected_logger_b {}".format(peer['id']))
+    return {'result': 'continue'}
+
+
+plugin.run()

tests/test_plugin.py

@@ -392,31 +392,47 @@ def test_pay_plugin(node_factory):
     assert only_one(l1.rpc.help('pay')['help'])['command'] == msg
 
 
-def test_plugin_connected_hook(node_factory):
-    """ l1 uses the reject plugin to reject connections.
+def test_plugin_connected_hook_chaining(node_factory):
+    """ l1 uses the logger_a, reject and logger_b plugin.
 
     l1 is configured to accept connections from l2, but not from l3.
+    we check that logger_a is always called and logger_b only for l2.
     """
-    opts = [{'plugin': os.path.join(os.getcwd(), 'tests/plugins/reject.py')}, {}, {}]
+    opts = [{'plugin': [
+        os.path.join(os.getcwd(), 'tests/plugins/peer_connected_logger_a.py'),
+        os.path.join(os.getcwd(), 'tests/plugins/reject.py'),
+        os.path.join(os.getcwd(), 'tests/plugins/peer_connected_logger_b.py'),
+    ]}, {}, {}]
+
     l1, l2, l3 = node_factory.get_nodes(3, opts=opts)
+    l2id = l2.info['id']
+    l3id = l3.info['id']
     l1.rpc.reject(l3.info['id'])
 
     l2.connect(l1)
-    l1.daemon.wait_for_log(r"{} is allowed".format(l2.info['id']))
-    assert len(l1.rpc.listpeers(l2.info['id'])['peers']) == 1
+    l1.daemon.wait_for_logs([
+        f"peer_connected_logger_a {l2id}",
+        f"{l2id} is allowed",
+        f"peer_connected_logger_b {l2id}"
+    ])
+    assert len(l1.rpc.listpeers(l2id)['peers']) == 1
 
     l3.connect(l1)
-    l1.daemon.wait_for_log(r"{} is in reject list".format(l3.info['id']))
+    l1.daemon.wait_for_logs([
+        f"peer_connected_logger_a {l3id}",
+        f"{l3id} is in reject list"
+    ])
 
     # FIXME: this error occurs *after* connection, so we connect then drop.
     l3.daemon.wait_for_log(r"chan#1: peer_in WIRE_ERROR")
     l3.daemon.wait_for_log(r"You are in reject list")
 
     def check_disconnect():
-        peers = l1.rpc.listpeers(l3.info['id'])['peers']
+        peers = l1.rpc.listpeers(l3id)['peers']
         return peers == [] or not peers[0]['connected']
 
     wait_for(check_disconnect)
+    assert not l3.daemon.is_in_log(f"peer_connected_logger_b {l3id}")
 
 
 def test_async_rpcmethod(node_factory, executor):
@@ -644,7 +660,7 @@ def test_openchannel_hook_chaining(node_factory, bitcoind):
     l1, l2 = node_factory.line_graph(2, fundchannel=False, opts=opts)
     l1.fundwallet(10**6)
 
-    hook_msg = "openchannel2?_hook rejects and says '"
+    hook_msg = "openchannel2? hook rejects and says '"
     # 100005sat fundchannel should fail fatal() for l2
     # because hook_accepter.py rejects on that amount 'for a reason'
     with pytest.raises(RpcError, match=r'They sent error channel'):