lightningd: make "is peer connected" a tristate.

First, connectd tells us the peer has connected, and we call the connected hook, and if it says it's fine, we are actually connected and we fire off notifications. Of course, we could be disconnected while in the connected hook, and that would mean we tell people about a connection which is no longer current. Make this clear with a tristate: if we're not marked disconnected by the time the hooks finish, we're good. It also gives us a cleaner "connect" command return when we connected but disconnected before processing. Signed-off-by: Rusty Russell <[email protected]>
simongerb · Jul 19, 2022 · eff5349 · eff5349
1 parent 571f0fa
commit eff5349
Show file tree

Hide file tree

Showing 10 changed files with 103 additions and 41 deletions.
diff --git a/common/jsonrpc_errors.h b/common/jsonrpc_errors.h
@@ -64,6 +64,7 @@ static const errcode_t FUNDING_STATE_INVALID = 312;
 /* `connect` errors */
 static const errcode_t CONNECT_NO_KNOWN_ADDRESS = 400;
 static const errcode_t CONNECT_ALL_ADDRESSES_FAILED = 401;
+static const errcode_t CONNECT_DISCONNECTED_DURING = 402;
 
 /* bitcoin-cli plugin errors */
 #define BCLI_ERROR                      400

diff --git a/doc/lightning-connect.7.md b/doc/lightning-connect.7.md
@@ -76,6 +76,10 @@ will contain details about the failures:
 
     { "code" : 401, "message" : "..." }
 
+If the peer disconnected while we were connecting:
+
+    { "code" : 402, "message" : "..." }
+
 If the given parameters are wrong:
 
     { "code" : -32602, "message" : "..." }

diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c
@@ -217,7 +217,7 @@ static struct command_result *json_connect(struct command *cmd,
 
 	/* If we know about peer, see if it's already connected. */
 	peer = peer_by_id(cmd->ld, &id_addr.id);
-	if (peer && peer->is_connected) {
+	if (peer && peer->connected == PEER_CONNECTED) {
 		log_debug(cmd->ld->log, "Already connected via %s",
 			  type_to_string(tmpctx, struct wireaddr_internal,
 					 &peer->addr));
@@ -228,7 +228,7 @@ static struct command_result *json_connect(struct command *cmd,
 
  	try_connect(cmd, cmd->ld, &id_addr.id, 0, addr);
 
-	/* Leave this here for peer_connected or connect_failed. */
+	/* Leave this here for peer_connected, connect_failed or peer_disconnect_done. */
 	new_connect(cmd->ld, &id_addr.id, cmd);
 	return command_still_pending(cmd);
 }
@@ -341,35 +341,53 @@ void try_reconnect(const tal_t *ctx,
 		    addrhint);
 }
 
-static void connect_failed(struct lightningd *ld, const u8 *msg)
+/* We were trying to connect, but they disconnected. */
+static void connect_failed(struct lightningd *ld,
+			   const struct node_id *id,
+			   errcode_t errcode,
+			   const char *errmsg,
+			   u32 seconds_to_delay,
+			   const struct wireaddr_internal *addrhint)
 {
-	struct node_id id;
-	errcode_t errcode;
-	char *errmsg;
-	struct connect *c;
-	u32 seconds_to_delay;
-	struct wireaddr_internal *addrhint;
 	struct peer *peer;
-
-	if (!fromwire_connectd_connect_failed(tmpctx, msg, &id, &errcode, &errmsg,
-						&seconds_to_delay, &addrhint))
-		fatal("Connect gave bad CONNECTD_CONNECT_FAILED message %s",
-		      tal_hex(msg, msg));
+	struct connect *c;
 
 	/* We can have multiple connect commands: fail them all */
-	while ((c = find_connect(ld, &id)) != NULL) {
+	while ((c = find_connect(ld, id)) != NULL) {
 		/* They delete themselves from list */
 		was_pending(command_fail(c->cmd, errcode, "%s", errmsg));
 	}
 
 	/* If we have an active channel, then reconnect. */
-	peer = peer_by_id(ld, &id);
+	peer = peer_by_id(ld, id);
 	if (peer) {
 		if (peer_any_active_channel(peer, NULL))
 			try_reconnect(peer, peer, seconds_to_delay, addrhint);
 	}
 }
 
+void connect_failed_disconnect(struct lightningd *ld, const struct node_id *id)
+{
+	connect_failed(ld, id, CONNECT_DISCONNECTED_DURING,
+		       "disconnected during connection", 1, NULL);
+}
+
+static void handle_connect_failed(struct lightningd *ld, const u8 *msg)
+{
+	struct node_id id;
+	errcode_t errcode;
+	char *errmsg;
+	u32 seconds_to_delay;
+	struct wireaddr_internal *addrhint;
+
+	if (!fromwire_connectd_connect_failed(tmpctx, msg, &id, &errcode, &errmsg,
+						&seconds_to_delay, &addrhint))
+		fatal("Connect gave bad CONNECTD_CONNECT_FAILED message %s",
+		      tal_hex(msg, msg));
+
+	connect_failed(ld, &id, errcode, errmsg, seconds_to_delay, addrhint);
+}
+
 void connect_succeeded(struct lightningd *ld, const struct peer *peer,
 		       bool incoming,
 		       const struct wireaddr_internal *addr)
@@ -479,7 +497,7 @@ static unsigned connectd_msg(struct subd *connectd, const u8 *msg, const int *fd
 		break;
 
 	case WIRE_CONNECTD_CONNECT_FAILED:
-		connect_failed(connectd->ld, msg);
+		handle_connect_failed(connectd->ld, msg);
 		break;
 
 	case WIRE_CONNECTD_GOT_ONIONMSG_TO_US:
@@ -635,15 +653,16 @@ void maybe_disconnect_peer(struct lightningd *ld, struct peer *peer)
 		if (channel_is_connected(channel))
 			return;
 
-	/* If shutting down, connectd no longer exists */
+	/* If shutting down, connectd no longer exists.
+	 * FIXME: Call peer_disconnect_done(), but nobody cares. */
 	if (!ld->connectd) {
-		peer->is_connected = false;
+		peer->connected = PEER_DISCONNECTED;
 		return;
 	}
 
 	/* If connectd was the one who told us to cleanup peer, don't
 	 * tell it to discard again: it might have reconnected! */
-	if (peer->is_connected)
+	if (peer->connected == PEER_CONNECTED)
 		subd_send_msg(ld->connectd,
 			      take(towire_connectd_discard_peer(NULL, &peer->id)));
 }
@@ -693,11 +712,11 @@ static struct command_result *json_sendcustommsg(struct command *cmd,
 				    type_to_string(cmd, struct node_id, dest));
 	}
 
-	if (!peer->is_connected) {
+	if (peer->connected != PEER_CONNECTED)
 		return command_fail(cmd, JSONRPC2_INVALID_REQUEST,
-				    "Peer is not connected: %s",
-				    type_to_string(cmd, struct node_id, dest));
-	}
+				    "Peer is %s",
+				    peer->connected == PEER_DISCONNECTED
+				    ? "not connected" : "still connecting");
 
 	subd_send_msg(cmd->ld->connectd,
 		      take(towire_connectd_custommsg_out(cmd, dest, msg)));

diff --git a/lightningd/connect_control.h b/lightningd/connect_control.h
@@ -20,6 +20,7 @@ void try_reconnect(const tal_t *ctx,
 void connect_succeeded(struct lightningd *ld, const struct peer *peer,
 		       bool incoming,
 		       const struct wireaddr_internal *addr);
+void connect_failed_disconnect(struct lightningd *ld, const struct node_id *id);
 
 /* Disconnect a peer (if no subds want to talk any more) */
 void maybe_disconnect_peer(struct lightningd *ld, struct peer *peer);

diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c
@@ -3128,9 +3128,11 @@ static struct command_result *json_queryrates(struct command *cmd,
 		return command_fail(cmd, FUNDING_UNKNOWN_PEER, "Unknown peer");
 	}
 
-	if (!peer->is_connected)
+	if (peer->connected != PEER_CONNECTED)
 		return command_fail(cmd, FUNDING_PEER_NOT_CONNECTED,
-				    "Peer not connected");
+				    "Peer %s",
+				    peer->connected == PEER_DISCONNECTED
+				    ? "not connected" : "still connecting");
 
 	/* FIXME: This is wrong: we should always create a new channel? */
 	channel = peer_any_unsaved_channel(peer, NULL);

diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c
@@ -963,9 +963,11 @@ static struct command_result *json_fundchannel_complete(struct command *cmd,
 		return command_fail(cmd, FUNDING_UNKNOWN_PEER, "Unknown peer");
 	}
 
-	if (!peer->is_connected)
+	if (peer->connected != PEER_CONNECTED)
 		return command_fail(cmd, FUNDING_PEER_NOT_CONNECTED,
-				    "Peer not connected");
+				    "Peer %s",
+				    peer->connected == PEER_DISCONNECTED
+				    ? "not connected" : "still connecting");
 
 	if (!peer->uncommitted_channel
 	    || !peer->uncommitted_channel->fc
@@ -1136,9 +1138,11 @@ static struct command_result *json_fundchannel_start(struct command *cmd,
 		return command_fail(cmd, FUNDING_UNKNOWN_PEER, "Unknown peer");
 	}
 
-	if (!peer->is_connected)
+	if (peer->connected != PEER_CONNECTED)
 		return command_fail(cmd, FUNDING_PEER_NOT_CONNECTED,
-				    "Peer not connected");
+				    "Peer %s",
+				    peer->connected == PEER_DISCONNECTED
+				    ? "not connected" : "still connecting");
 
 	temporary_channel_id(&tmp_channel_id);
 

diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c
@@ -99,7 +99,7 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid,
 	peer->their_features = NULL;
 	list_head_init(&peer->channels);
 	peer->direction = node_id_idx(&peer->ld->id, &peer->id);
-	peer->is_connected = false;
+	peer->connected = PEER_DISCONNECTED;
 #if DEVELOPER
 	peer->ignore_htlcs = false;
 #endif
@@ -1020,15 +1020,27 @@ static void peer_connected_hook_final(struct peer_connected_hook_payload *payloa
 	 * subd). */
 	tal_steal(tmpctx, payload);
 
-	/* Notify anyone who cares */
-	notify_connect(ld, &peer->id, payload->incoming, &addr);
+	/* If we disconnected in the meantime, forget about it.
+	 * (disconnect will have failed any connect commands). */
+	if (peer->connected == PEER_DISCONNECTED)
+		return;
 
 	/* Check for specific errors of a hook */
 	if (payload->error) {
 		error = payload->error;
 		goto send_error;
 	}
 
+	/* Now we finally consider ourselves connected! */
+	assert(peer->connected == PEER_CONNECTING);
+	peer->connected = PEER_CONNECTED;
+
+	/* Succeed any connect() commands */
+	connect_succeeded(ld, peer, payload->incoming, &payload->addr);
+
+	/* Notify anyone who cares */
+	notify_connect(ld, &peer->id, payload->incoming, &addr);
+
 	list_for_each(&peer->channels, channel, list) {
 #if DEVELOPER
 		if (dev_disconnect_permanent(ld)) {
@@ -1196,7 +1208,11 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
 	if (!peer)
 		peer = new_peer(ld, 0, &id, &hook_payload->addr,
 				hook_payload->incoming);
-	peer->is_connected = true;
+
+	/* We mark peer in "connecting" state until hooks have passed. */
+	assert(peer->connected == PEER_DISCONNECTED);
+	peer->connected = PEER_CONNECTING;
+
 	/* Update peer address and direction */
 	peer->addr = hook_payload->addr;
 	peer->connected_incoming = hook_payload->incoming;
@@ -1208,8 +1224,6 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
 	tal_steal(peer, hook_payload);
 	hook_payload->peer = peer;
 
-	connect_succeeded(ld, peer, hook_payload->incoming, &hook_payload->addr);
-
 	/* Log and update remote_addr for Nat/IP discovery. */
 	if (hook_payload->remote_addr) {
 		log_peer_debug(ld->log, &id, "Peer says it sees our address as: %s",
@@ -1429,11 +1443,14 @@ void peer_disconnect_done(struct lightningd *ld, const u8 *msg)
 	p = peer_by_id(ld, &id);
 	if (p) {
 		log_peer_debug(ld->log, &id, "peer_disconnect_done");
-		p->is_connected = false;
+		p->connected = PEER_DISCONNECTED;
 
 		peer_channels_cleanup_on_disconnect(p);
 	}
 
+	/* If you were trying to connect, it failed. */
+	connect_failed_disconnect(ld, &id);
+
 	/* Fire off plugin notifications */
 	notify_disconnect(ld, &id);
 
@@ -1700,12 +1717,12 @@ static void json_add_peer(struct lightningd *ld,
 	json_object_start(response, NULL);
 	json_add_node_id(response, "id", &p->id);
 
-	json_add_bool(response, "connected", p->is_connected);
+	json_add_bool(response, "connected", p->connected == PEER_CONNECTED);
 
 	/* If it's not connected, features are unreliable: we don't
 	 * store them in the database, and they would only reflect
 	 * their features *last* time they connected. */
-	if (p->is_connected) {
+	if (p->connected == PEER_CONNECTED) {
 		json_array_start(response, "netaddr");
 		json_add_string(response, NULL,
 				type_to_string(tmpctx,
@@ -1982,7 +1999,7 @@ static struct command_result *json_disconnect(struct command *cmd,
 	if (!peer) {
 		return command_fail(cmd, LIGHTNINGD, "Unknown peer");
 	}
-	if (!peer->is_connected) {
+	if (peer->connected == PEER_DISCONNECTED) {
 		return command_fail(cmd, LIGHTNINGD, "Peer not connected");
 	}
 

diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h
@@ -31,7 +31,14 @@ struct peer {
 	struct list_head channels;
 
 	/* Are we connected? */
-	bool is_connected;
+	enum {
+		/* Connectd said we're connecting, we called hooks... */
+		PEER_CONNECTING,
+		/* Hooks succeeded, we're connected. */
+		PEER_CONNECTED,
+		/* Start state, also connectd told us we're disconnected */
+		PEER_DISCONNECTED,
+	} connected;
 
 	/* Our (only) uncommitted channel, still opening. */
 	struct uncommitted_channel *uncommitted_channel;
@@ -70,6 +77,7 @@ struct peer *peer_from_json(struct lightningd *ld,
 			    const char *buffer,
 			    const jsmntok_t *peeridtok);
 
+/* connectd tells us what peer is doing */
 void peer_connected(struct lightningd *ld, const u8 *msg);
 void peer_disconnect_done(struct lightningd *ld, const u8 *msg);
 void peer_active(struct lightningd *ld, const u8 *msg, int peer_fd);

diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c
@@ -147,6 +147,9 @@ struct command_result *command_success(struct command *cmd UNNEEDED,
 				       struct json_stream *response)
 
 { fprintf(stderr, "command_success called!\n"); abort(); }
+/* Generated stub for connect_failed_disconnect */
+void connect_failed_disconnect(struct lightningd *ld UNNEEDED, const struct node_id *id UNNEEDED)
+{ fprintf(stderr, "connect_failed_disconnect called!\n"); abort(); }
 /* Generated stub for connect_succeeded */
 void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED,
 		       bool incoming UNNEEDED,

diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c
@@ -106,6 +106,9 @@ struct command_result *command_success(struct command *cmd UNNEEDED,
 				       struct json_stream *response)
 
 { fprintf(stderr, "command_success called!\n"); abort(); }
+/* Generated stub for connect_failed_disconnect */
+void connect_failed_disconnect(struct lightningd *ld UNNEEDED, const struct node_id *id UNNEEDED)
+{ fprintf(stderr, "connect_failed_disconnect called!\n"); abort(); }
 /* Generated stub for connect_succeeded */
 void connect_succeeded(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED,
 		       bool incoming UNNEEDED,