connectd: do dev_disconnect logic.

As connectd handles more packets itself, or diverts them to/from gossipd, it's the only place we can implement the dev_disconnect logic. Signed-off-by: Rusty Russell <[email protected]>
gudnuf · Jan 20, 2022 · 7a51411 · 7a51411
1 parent 9c0bb44
commit 7a51411
Show file tree

Hide file tree

Showing 17 changed files with 71 additions and 70 deletions.
diff --git a/channeld/Makefile b/channeld/Makefile
@@ -44,7 +44,6 @@ CHANNELD_COMMON_OBJS :=				\
 	common/daemon.o				\
 	common/daemon_conn.o			\
 	common/derive_basepoints.o		\
-	common/dev_disconnect.o			\
 	common/ecdh_hsmd.o			\
 	common/features.o			\
 	common/fee_states.o			\

diff --git a/channeld/channeld.c b/channeld/channeld.c
@@ -19,7 +19,6 @@
 #include <channeld/full_channel.h>
 #include <channeld/watchtower.h>
 #include <common/billboard.h>
-#include <common/dev_disconnect.h>
 #include <common/ecdh_hsmd.h>
 #include <common/gossip_store.h>
 #include <common/key_derive.h>

diff --git a/closingd/Makefile b/closingd/Makefile
@@ -30,7 +30,6 @@ CLOSINGD_COMMON_OBJS :=				\
 	common/daemon.o				\
 	common/daemon_conn.o			\
 	common/derive_basepoints.o		\
-	common/dev_disconnect.o			\
 	common/gossip_rcvd_filter.o		\
 	common/gossip_store.o			\
 	common/htlc_wire.o			\

diff --git a/common/dev_disconnect.c b/common/dev_disconnect.c
@@ -51,15 +51,16 @@ void dev_disconnect_init(int fd)
 	dev_disconnect_fd = fd;
 }
 
-enum dev_disconnect dev_disconnect(int pkt_type)
+enum dev_disconnect dev_disconnect(const struct node_id *id, int pkt_type)
 {
 	if (dev_disconnect_fd == -1)
 		return DEV_DISCONNECT_NORMAL;
 
 	if (!dev_disconnect_count)
 		next_dev_disconnect();
 
-	if (!streq(peer_wire_name(pkt_type), dev_disconnect_line+1))
+	if (!dev_disconnect_line[0]
+	    || !streq(peer_wire_name(pkt_type), dev_disconnect_line+1))
 		return DEV_DISCONNECT_NORMAL;
 
 	if (--dev_disconnect_count != 0) {
@@ -70,7 +71,8 @@ enum dev_disconnect dev_disconnect(int pkt_type)
 		err(1, "lseek failure");
 	}
 
-	status_debug("dev_disconnect: %s", dev_disconnect_line);
+	status_peer_debug(id, "dev_disconnect: %s (%s)", dev_disconnect_line,
+			  peer_wire_name(pkt_type));
 	return dev_disconnect_line[0];
 }
 

diff --git a/common/dev_disconnect.h b/common/dev_disconnect.h
@@ -4,6 +4,8 @@
 #include <stdbool.h>
 
 #if DEVELOPER
+struct node_id;
+
 enum dev_disconnect {
 	/* Do nothing. */
 	DEV_DISCONNECT_NORMAL = '=',
@@ -18,7 +20,7 @@ enum dev_disconnect {
 };
 
 /* Force a close fd before or after a certain packet type */
-enum dev_disconnect dev_disconnect(int pkt_type);
+enum dev_disconnect dev_disconnect(const struct node_id *id, int pkt_type);
 
 /* Make next write on fd fail as if they'd disconnected. */
 void dev_sabotage_fd(int fd, bool close_fd);

diff --git a/common/peer_io.c b/common/peer_io.c
@@ -1,7 +1,6 @@
 #include "config.h"
 #include <ccan/read_write_all/read_write_all.h>
 #include <common/cryptomsg.h>
-#include <common/dev_disconnect.h>
 #include <common/peer_failed.h>
 #include <common/peer_io.h>
 #include <common/per_peer_state.h>
@@ -17,40 +16,10 @@
 
 void peer_write(struct per_peer_state *pps, const void *msg TAKES)
 {
-#if DEVELOPER
-	bool post_sabotage = false, post_close;
-	int type = fromwire_peektype(msg);
-#endif
-
 	status_peer_io(LOG_IO_OUT, NULL, msg);
 
-#if DEVELOPER
-	switch (dev_disconnect(type)) {
-	case DEV_DISCONNECT_BEFORE:
-		dev_sabotage_fd(pps->peer_fd, true);
-		peer_failed_connection_lost();
-	case DEV_DISCONNECT_AFTER:
-		post_sabotage = true;
-		post_close = true;
-		break;
-	case DEV_DISCONNECT_BLACKHOLE:
-		dev_blackhole_fd(pps->peer_fd);
-		break;
-	case DEV_DISCONNECT_NORMAL:
-		break;
-	case DEV_DISCONNECT_DISABLE_AFTER:
-		post_sabotage = true;
-		post_close = false;
-		break;
-	}
-#endif
 	if (!wire_sync_write(pps->peer_fd, msg))
 		peer_failed_connection_lost();
-
-#if DEVELOPER
-	if (post_sabotage)
-		dev_sabotage_fd(pps->peer_fd, post_close);
-#endif
 }
 
 /* We're happy for the kernel to batch update and gossip messages, but a

diff --git a/common/subdaemon.c b/common/subdaemon.c
@@ -1,5 +1,4 @@
 #include "config.h"
-#include <common/dev_disconnect.h>
 #include <common/status.h>
 #include <common/subdaemon.h>
 #include <common/version.h>
@@ -33,14 +32,5 @@ void subdaemon_setup(int argc, char *argv[])
 
 	daemon_maybe_debug(argv);
 
-#if DEVELOPER
-	for (int i = 1; i < argc; i++) {
-		if (strstarts(argv[i], "--dev-disconnect=")) {
-			dev_disconnect_init(atoi(argv[i]
-						 + strlen("--dev-disconnect=")));
-		}
-	}
-#endif
-
 	daemon_setup(argv[0], status_backtrace_print, status_backtrace_exit);
 }
diff --git a/connectd/connectd.c b/connectd/connectd.c
@@ -18,6 +18,7 @@
 #include <common/bech32.h>
 #include <common/bech32_util.h>
 #include <common/daemon_conn.h>
+#include <common/dev_disconnect.h>
 #include <common/ecdh_hsmd.h>
 #include <common/jsonrpc_errors.h>
 #include <common/memleak.h>
@@ -1598,6 +1599,7 @@ static void connect_init(struct daemon *daemon, const u8 *msg)
 	enum addr_listen_announce *proposed_listen_announce;
 	struct wireaddr *announcable;
 	char *tor_password;
+	bool dev_disconnect;
 
 	/* Fields which require allocation are allocated off daemon */
 	if (!fromwire_connectd_init(
@@ -1613,7 +1615,8 @@ static void connect_init(struct daemon *daemon, const u8 *msg)
 		&daemon->use_v3_autotor,
 		&daemon->timeout_secs,
 		&daemon->websocket_helper,
-		&daemon->websocket_port)) {
+		&daemon->websocket_port,
+		&dev_disconnect)) {
 		/* This is a helper which prints the type expected and the actual
 		 * message, then exits (it should never be called!). */
 		master_badmsg(WIRE_CONNECTD_INIT, msg);
@@ -1657,6 +1660,10 @@ static void connect_init(struct daemon *daemon, const u8 *msg)
 			 take(towire_connectd_init_reply(NULL,
 							   binding,
 							   announcable)));
+#if DEVELOPER
+	if (dev_disconnect)
+		dev_disconnect_init(5);
+#endif
 }
 
 /*~ lightningd tells us to go! */

diff --git a/connectd/connectd_wire.csv b/connectd/connectd_wire.csv
@@ -21,6 +21,8 @@ msgdata,connectd_init,use_v3_autotor,bool,
 msgdata,connectd_init,timeout_secs,u32,
 msgdata,connectd_init,websocket_helper,wirestring,
 msgdata,connectd_init,websocket_port,u16,
+# If this is set, then fd 5 is dev_disconnect_fd.
+msgdata,connectd_init,dev_disconnect,bool,
 
 # Connectd->master, here are the addresses I bound, can announce.
 msgtype,connectd_init_reply,2100

diff --git a/connectd/multiplex.c b/connectd/multiplex.c
@@ -4,13 +4,15 @@
 #include <assert.h>
 #include <ccan/io/io.h>
 #include <common/cryptomsg.h>
+#include <common/dev_disconnect.h>
 #include <common/per_peer_state.h>
 #include <common/status.h>
 #include <common/utils.h>
 #include <connectd/multiplex.h>
 #include <errno.h>
 #include <sys/socket.h>
 #include <sys/types.h>
+#include <wire/wire.h>
 #include <wire/wire_io.h>
 
 void queue_peer_msg(struct peer *peer, const u8 *msg TAKES)
@@ -32,12 +34,48 @@ static struct io_plan *after_final_msg(struct io_conn *peer_conn,
 	return io_close(peer_conn);
 }
 
+#if DEVELOPER
+static struct io_plan *write_to_peer(struct io_conn *peer_conn,
+				     struct peer *peer);
+
+static struct io_plan *dev_leave_hanging(struct io_conn *peer_conn,
+					 struct peer *peer)
+{
+	/* We don't tell the peer we're disconnecting, but from now on
+	 * our writes go nowhere, and there's nothing to read. */
+	dev_sabotage_fd(io_conn_fd(peer_conn), false);
+	return write_to_peer(peer_conn, peer);
+}
+#endif /* DEVELOPER */
+
 static struct io_plan *encrypt_and_send(struct peer *peer,
 					const u8 *msg TAKES,
 					struct io_plan *(*next)
 					(struct io_conn *peer_conn,
 					 struct peer *peer))
 {
+#if DEVELOPER
+	int type = fromwire_peektype(msg);
+
+	switch (dev_disconnect(&peer->id, type)) {
+	case DEV_DISCONNECT_BEFORE:
+		if (taken(msg))
+			tal_free(msg);
+		return io_close(peer->to_peer);
+	case DEV_DISCONNECT_AFTER:
+		next = (void *)io_close_cb;
+		break;
+	case DEV_DISCONNECT_BLACKHOLE:
+		dev_blackhole_fd(io_conn_fd(peer->to_peer));
+		break;
+	case DEV_DISCONNECT_NORMAL:
+		break;
+	case DEV_DISCONNECT_DISABLE_AFTER:
+		next = dev_leave_hanging;
+		break;
+	}
+#endif
+
 	/* We free this and the encrypted version in next write_to_peer */
 	peer->sent_to_peer = cryptomsg_encrypt_msg(peer, &peer->cs, msg);
 	return io_write(peer->to_peer,

diff --git a/connectd/peer_exchange_initmsg.c b/connectd/peer_exchange_initmsg.c
@@ -208,7 +208,7 @@ struct io_plan *peer_exchange_initmsg(struct io_conn *conn,
 
 	next = read_init;
 #if DEVELOPER
-	switch (dev_disconnect(WIRE_INIT)) {
+	switch (dev_disconnect(&peer->id, WIRE_INIT)) {
 	case DEV_DISCONNECT_BEFORE:
 		dev_sabotage_fd(io_conn_fd(conn), true);
 		break;

diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c
@@ -97,10 +97,6 @@ static bool is_lightningd(const struct client *client)
 	return client == dbid_zero_clients[0];
 }
 
-/* FIXME: This is used by debug.c.  Doesn't apply to us, but lets us link. */
-extern void dev_disconnect_init(int fd);
-void dev_disconnect_init(int fd UNUSED) { }
-
 /* Pre-declare this, due to mutual recursion */
 static struct io_plan *handle_client(struct io_conn *conn, struct client *c);
 

diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c
@@ -360,7 +360,13 @@ int connectd_init(struct lightningd *ld)
 
 	ld->connectd = new_global_subd(ld, "lightning_connectd",
 				       connectd_wire_name, connectd_msg,
-				       take(&hsmfd), take(&fds[1]), NULL);
+				       take(&hsmfd), take(&fds[1]),
+#if DEVELOPER
+				       /* Not take(): we share it */
+				       ld->dev_disconnect_fd >= 0 ?
+				       &ld->dev_disconnect_fd : NULL,
+#endif
+				       NULL);
 	if (!ld->connectd)
 		err(1, "Could not subdaemon connectd");
 
@@ -385,7 +391,8 @@ int connectd_init(struct lightningd *ld)
 	    ld->config.use_v3_autotor,
 	    ld->config.connection_timeout_secs,
 	    websocket_helper_path,
-	    ld->websocket_port);
+	    ld->websocket_port,
+	    IFDEV(ld->dev_disconnect_fd >= 0, false));
 
 	subd_req(ld->connectd, ld->connectd, take(msg), -1, 0,
 		 connect_init_done, NULL);

diff --git a/lightningd/subd.c b/lightningd/subd.c
@@ -188,7 +188,7 @@ static void close_taken_fds(va_list *ap)
 /* We use sockets, not pipes, because fds are bidir. */
 static int subd(const char *path, const char *name,
 		const char *debug_subdaemon,
-		int *msgfd, int dev_disconnect_fd,
+		int *msgfd,
 		bool io_logging,
 		va_list *ap)
 {
@@ -212,7 +212,7 @@ static int subd(const char *path, const char *name,
 
 	if (childpid == 0) {
 		size_t num_args;
-		char *args[] = { NULL, NULL, NULL, NULL, NULL };
+		char *args[] = { NULL, NULL, NULL, NULL };
 		int **fds = tal_arr(tmpctx, int *, 3);
 		int stdoutfd = STDOUT_FILENO, stderrfd = STDERR_FILENO;
 
@@ -230,10 +230,6 @@ static int subd(const char *path, const char *name,
 			tal_arr_expand(&fds, fd);
 		}
 
-		/* If we have a dev_disconnect_fd, add it after. */
-		if (dev_disconnect_fd != -1)
-			tal_arr_expand(&fds, &dev_disconnect_fd);
-
 		/* Finally, the fd to report exec errors on */
 		tal_arr_expand(&fds, &execfail[1]);
 
@@ -248,8 +244,6 @@ static int subd(const char *path, const char *name,
 		if (io_logging)
 			args[num_args++] = "--log-io";
 #if DEVELOPER
-		if (dev_disconnect_fd != -1)
-			args[num_args++] = tal_fmt(NULL, "--dev-disconnect=%i", dev_disconnect_fd);
 		if (debug_subdaemon && strends(name, debug_subdaemon))
 			args[num_args++] = "--debugger";
 #endif
@@ -700,7 +694,6 @@ static struct subd *new_subd(struct lightningd *ld,
 	struct subd *sd = tal(ld, struct subd);
 	int msg_fd;
 	const char *debug_subd = NULL;
-	int disconnect_fd = -1;
 	const char *shortname;
 
 	assert(name != NULL);
@@ -720,13 +713,12 @@ static struct subd *new_subd(struct lightningd *ld,
 
 #if DEVELOPER
 	debug_subd = ld->dev_debug_subprocess;
-	disconnect_fd = ld->dev_disconnect_fd;
 #endif /* DEVELOPER */
 
 	const char *path = subdaemon_path(tmpctx, ld, name);
 
 	sd->pid = subd(path, name, debug_subd,
-		       &msg_fd, disconnect_fd,
+		       &msg_fd,
 		       /* We only turn on subdaemon io logging if we're going
 			* to print it: too stressful otherwise! */
 		       log_print_level(sd->log) < LOG_DBG,

diff --git a/onchaind/Makefile b/onchaind/Makefile
@@ -41,7 +41,6 @@ ONCHAIND_COMMON_OBJS :=				\
 	common/daemon.o				\
 	common/daemon_conn.o			\
 	common/derive_basepoints.o		\
-	common/dev_disconnect.o			\
 	common/status_wiregen.o			\
 	common/htlc_tx.o			\
 	common/htlc_wire.o			\

diff --git a/openingd/Makefile b/openingd/Makefile
@@ -45,7 +45,6 @@ OPENINGD_COMMON_OBJS :=				\
 	common/daemon.o				\
 	common/daemon_conn.o			\
 	common/derive_basepoints.o		\
-	common/dev_disconnect.o			\
 	common/features.o			\
 	common/fee_states.o			\
 	common/gossip_rcvd_filter.o		\

diff --git a/tests/test_gossip.py b/tests/test_gossip.py
@@ -602,7 +602,8 @@ def test_gossip_no_empty_announcements(node_factory, bitcoind):
     # l3 sends CHANNEL_ANNOUNCEMENT to l2, but not CHANNEL_UDPATE.
     l1, l2, l3, l4 = node_factory.line_graph(4, opts=[{'log-level': 'io'},
                                                       {'log-level': 'io'},
-                                                      {'disconnect': ['+WIRE_CHANNEL_ANNOUNCEMENT'],
+                                                      # Writes to l4 first, then l2
+                                                      {'disconnect': ['+WIRE_CHANNEL_ANNOUNCEMENT*2'],
                                                        'may_reconnect': True},
                                                       {'may_reconnect': True}],
                                              fundchannel=False)