lightningd/peer_control: don't create peer struct until we've connected.

We currently create a peer struct, then complete handshake to find out
who it is.  This means we have a half-formed peer, and worse: if it's
a reconnect we get two peers the same.

Add an explicit 'struct connection' for the handshake phase, and
construct a 'struct peer' once that's done.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd/Makefile

@@ -66,6 +66,7 @@ LIGHTNINGD_SRC :=				\
 	lightningd/htlc_end.c			\
 	lightningd/hsm_control.c		\
 	lightningd/lightningd.c			\
+	lightningd/new_connection.c		\
 	lightningd/pay.c			\
 	lightningd/peer_control.c		\
 	lightningd/subd.c
@@ -82,6 +83,7 @@ LIGHTNINGD_HEADERS_NOGEN =			\
 	lightningd/hsm_control.h		\
 	lightningd/htlc_end.h			\
 	lightningd/lightningd.h			\
+	lightningd/new_connection.h		\
 	lightningd/pay.h			\
 	lightningd/peer_control.h		\
 	lightningd/peer_state.h			\

lightningd/gossip_control.c

@@ -112,17 +112,6 @@ static void peer_ready(struct subd *gossip, const u8 *msg)
 		  type_to_string(msg, struct pubkey, peer->id),
 		  unique_id);
 
-	if (peer->connect_cmd) {
-		struct json_result *response;
-		response = new_json_result(peer->connect_cmd);
-
-		json_object_start(response, NULL);
-		json_add_pubkey(response, "id", peer->id);
-		json_object_end(response);
-		command_success(peer->connect_cmd, response);
-		peer->connect_cmd = NULL;
-	}
-
 	peer_set_condition(peer, GOSSIPING);
 }
 

lightningd/new_connection.c

@@ -0,0 +1,179 @@
+#include <daemon/jsonrpc.h>
+#include <daemon/log.h>
+#include <errno.h>
+#include <lightningd/cryptomsg.h>
+#include <lightningd/handshake/gen_handshake_wire.h>
+#include <lightningd/hsm/gen_hsm_wire.h>
+#include <lightningd/lightningd.h>
+#include <lightningd/new_connection.h>
+#include <lightningd/peer_control.h>
+#include <lightningd/subd.h>
+
+/* Before we have identified the peer, we just have a connection object. */
+struct connection {
+	/* Lightning daemon, for when we're handed through callbacks. */
+	struct lightningd *ld;
+
+	/* Unique identifier for handshaked. */
+	u64 unique_id;
+
+	/* Socket */
+	int fd;
+
+	/* Json command which made us connect (if any) */
+	struct command *cmd;
+
+	/* If we are initiating, we known their id.  Otherwise NULL. */
+	struct pubkey *known_id;
+};
+
+static void connection_destroy(struct connection *c)
+{
+	/* FIXME: better diagnostics. */
+	if (c->cmd)
+		command_fail(c->cmd, "Failed to connect to peer");
+}
+
+struct connection *new_connection(const tal_t *ctx,
+				  struct lightningd *ld,
+				  struct command *cmd,
+				  const struct pubkey *known_id)
+{
+	static u64 id_counter;
+	struct connection *c = tal(ctx, struct connection);
+
+	c->ld = ld;
+	c->unique_id = id_counter++;
+	c->cmd = cmd;
+	if (known_id)
+		c->known_id = tal_dup(c, struct pubkey, known_id);
+	else
+		c->known_id = NULL;
+	c->fd = -1;
+	tal_add_destructor(c, connection_destroy);
+
+	return c;
+}
+
+static bool handshake_succeeded(struct subd *handshaked,
+				const u8 *msg, const int *fds,
+				struct connection *c)
+{
+	struct crypto_state cs;
+	struct pubkey *id;
+
+	assert(tal_count(fds) == 1);
+
+	/* FIXME: Look for peer duplicates! */
+
+	if (!c->known_id) {
+		id = tal(msg, struct pubkey);
+		if (!fromwire_handshake_responder_reply(msg, NULL, id, &cs))
+			goto err;
+		log_info_struct(handshaked->log, "Peer in from %s",
+				struct pubkey, id);
+	} else {
+		id = c->known_id;
+		if (!fromwire_handshake_initiator_reply(msg, NULL, &cs))
+			goto err;
+		log_info_struct(handshaked->log, "Peer out to %s",
+				struct pubkey, id);
+	}
+
+	if (c->cmd) {
+		struct json_result *response;
+		response = new_json_result(c->cmd);
+
+		json_object_start(response, NULL);
+		json_add_pubkey(response, "id", id);
+		json_object_end(response);
+		command_success(c->cmd, response);
+		c->cmd = NULL;
+	}
+
+	add_peer(handshaked->ld, c->unique_id, fds[0], id, &cs);
+	/* Now shut handshaked down (frees c as well) */
+	return false;
+
+err:
+	log_broken(handshaked->log, "Malformed resp: %s", tal_hex(c, msg));
+	close(fds[0]);
+	return false;
+}
+
+static bool got_handshake_hsmfd(struct subd *hsm, const u8 *msg,
+				const int *fds,
+				struct connection *c)
+{
+	struct lightningd *ld = hsm->ld;
+	const u8 *req;
+	struct subd *handshaked;
+
+	assert(tal_count(fds) == 1);
+	if (!fromwire_hsmctl_hsmfd_ecdh_fd_reply(msg, NULL))
+		fatal("Malformed hsmfd response: %s", tal_hex(msg, msg));
+
+	/* Give handshake daemon the hsm fd. */
+	handshaked = new_subd(ld, ld,
+			      "lightningd_handshake", NULL,
+			      handshake_wire_type_name,
+			      NULL, NULL,
+			      fds[0], c->fd, -1);
+	if (!handshaked) {
+		log_unusual(ld->log, "Could not subdaemon handshake: %s",
+			    strerror(errno));
+		goto error;
+	}
+
+	/* If handshake daemon fails, we just drop connection. */
+	tal_steal(handshaked, c);
+
+	/* We no longer own fd (closed; handshaked has copy). */
+	c->fd = -1;
+	if (c->known_id) {
+		req = towire_handshake_initiator(c, &ld->dstate.id,
+						 c->known_id);
+	} else {
+		req = towire_handshake_responder(c, &ld->dstate.id);
+	}
+
+	/* Now hand peer request to the handshake daemon: hands it
+	 * back on success */
+	subd_req(c, handshaked, take(req), -1, 1, handshake_succeeded, c);
+	return true;
+
+error:
+	close(fds[0]);
+	return true;
+}
+
+/* Same path for connecting in vs connecting out. */
+static struct io_plan *hsm_then_handshake(struct io_conn *conn,
+					  struct lightningd *ld,
+					  struct connection *c)
+{
+
+	/* Get HSM fd for this peer. */
+	subd_req(c, ld->hsm,
+		 take(towire_hsmctl_hsmfd_ecdh(ld, c->unique_id)),
+		 -1, 1, got_handshake_hsmfd, c);
+
+	c->fd = io_conn_fd(conn);
+
+	/* We don't need conn, we'll pass fd to handshaked. */
+	return io_close_taken_fd(conn);
+}
+
+struct io_plan *connection_out(struct io_conn *conn,
+			       struct lightningd_state *dstate,
+			       struct connection *c)
+{
+	return hsm_then_handshake(conn, ld_from_dstate(dstate), c);
+}
+
+struct io_plan *connection_in(struct io_conn *conn, struct lightningd *ld)
+{
+	struct connection *c = new_connection(ld, ld, NULL, NULL);
+
+	return hsm_then_handshake(conn, ld, c);
+}

lightningd/new_connection.h

@@ -0,0 +1,23 @@
+#ifndef LIGHTNING_LIGHTNINGD_NEW_CONNECTION_H
+#define LIGHTNING_LIGHTNINGD_NEW_CONNECTION_H
+#include "config.h"
+#include <stdbool.h>
+
+struct command;
+struct io_conn;
+struct lightningd;
+struct lightningd_state;
+struct pubkey;
+
+struct connection *new_connection(const tal_t *ctx,
+				  struct lightningd *ld,
+				  struct command *cmd,
+				  const struct pubkey *known_id);
+
+struct io_plan *connection_out(struct io_conn *conn,
+			       struct lightningd_state *dstate,
+			       struct connection *c);
+
+struct io_plan *connection_in(struct io_conn *conn, struct lightningd *ld);
+
+#endif /* LIGHTNING_LIGHTNINGD_NEW_CONNECTION_H */

lightningd/peer_control.c

@@ -21,9 +21,9 @@
 #include <lightningd/funding_tx.h>
 #include <lightningd/gen_peer_state_names.h>
 #include <lightningd/gossip/gen_gossip_wire.h>
-#include <lightningd/handshake/gen_handshake_wire.h>
 #include <lightningd/hsm/gen_hsm_wire.h>
 #include <lightningd/key_derive.h>
+#include <lightningd/new_connection.h>
 #include <lightningd/opening/gen_opening_wire.h>
 #include <lightningd/pay.h>
 #include <lightningd/sphinx.h>
@@ -39,9 +39,6 @@ static void destroy_peer(struct peer *peer)
 	list_del_from(&peer->ld->peers, &peer->list);
 	if (peer->fd >= 0)
 		close(peer->fd);
-	if (peer->connect_cmd)
-		command_fail(peer->connect_cmd, "Failed in state %s",
-			     peer_state_name(peer->state));
 }
 
 static void peer_reconnect(struct peer *peer)
@@ -78,44 +75,57 @@ void peer_set_condition(struct peer *peer, enum peer_state state)
 	peer->state = state;
 }
 
-static struct peer *new_peer(struct lightningd *ld,
+void add_peer(struct lightningd *ld, u64 unique_id,
-			     struct io_conn *conn,
+	      int fd, const struct pubkey *id,
-			     struct command *cmd)
+	      const struct crypto_state *cs)
 {
-	static u64 id_counter;
 	struct peer *peer = tal(ld, struct peer);
-	const char *netname;
+	const char *netname, *idname;
+	u8 *msg;
 
 	peer->ld = ld;
-	peer->unique_id = id_counter++;
+	peer->unique_id = unique_id;
 	peer->owner = NULL;
 	peer->scid = NULL;
-	peer->id = NULL;
+	peer->id = tal_dup(peer, struct pubkey, id);
-	peer->fd = io_conn_fd(conn);
+	peer->fd = fd;
-	peer->connect_cmd = cmd;
 	peer->funding_txid = NULL;
 	peer->seed = NULL;
 	peer->balance = NULL;
-	peer->state = HANDSHAKING;
+	peer->state = INITIALIZING;
+
+	idname = type_to_string(peer, struct pubkey, id);
 
 	/* Max 128k per peer. */
 	peer->log_book = new_log_book(peer, 128*1024,
 				      get_log_level(ld->dstate.log_book));
-	peer->log = new_log(peer, peer->log_book,
+	peer->log = new_log(peer, peer->log_book, "peer %s:", idname);
-			    "peer %"PRIu64":", peer->unique_id);
 
 	/* FIXME: Don't assume protocol here! */
 	if (!netaddr_from_fd(peer->fd, SOCK_STREAM, IPPROTO_TCP,
 			     &peer->netaddr)) {
 		log_unusual(ld->log, "Failed to get netaddr for outgoing: %s",
 			    strerror(errno));
-		return tal_free(peer);
+		tal_free(peer);
+		return;
 	}
-	netname = netaddr_name(peer, &peer->netaddr);
+	netname = netaddr_name(idname, &peer->netaddr);
-	tal_free(netname);
+	log_info(peer->log, "Connected from %s", netname);
+	tal_free(idname);
 	list_add_tail(&ld->peers, &peer->list);
 	tal_add_destructor(peer, destroy_peer);
-	return peer;
+
+	/* Let gossip handle it from here. */
+	peer->owner = peer->ld->gossip;
+	tal_steal(peer->owner, peer);
+	peer_set_condition(peer, GOSSIPING);
+
+	msg = towire_gossipctl_new_peer(peer, peer->unique_id, cs);
+	subd_send_msg(peer->ld->gossip, take(msg));
+	subd_send_fd(peer->ld->gossip, peer->fd);
+
+	/* Peer struct longer owns fd. */
+	peer->fd = -1;
 }
 
 struct peer *peer_by_unique_id(struct lightningd *ld, u64 unique_id)
@@ -138,52 +148,6 @@ struct peer *peer_by_id(struct lightningd *ld, const struct pubkey *id)
 	return NULL;
 }
 
-static bool handshake_succeeded(struct subd *hs, const u8 *msg, const int *fds,
-				struct peer *peer)
-{
-	struct crypto_state cs;
-
-	assert(tal_count(fds) == 1);
-	peer->fd = fds[0];
-	if (!peer->id) {
-		struct pubkey id;
-
-		if (!fromwire_handshake_responder_reply(msg, NULL, &id, &cs))
-			goto err;
-		peer->id = tal_dup(peer, struct pubkey, &id);
-		log_info_struct(hs->log, "Peer in from %s",
-				struct pubkey, peer->id);
-	} else {
-		if (!fromwire_handshake_initiator_reply(msg, NULL, &cs))
-			goto err;
-		log_info_struct(hs->log, "Peer out to %s",
-				struct pubkey, peer->id);
-	}
-
-	/* FIXME: Look for peer duplicates! */
-
-	peer->owner = peer->ld->gossip;
-	tal_steal(peer->owner, peer);
-	peer_set_condition(peer, INITIALIZING);
-
-	/* Tell gossip to handle it now. */
-	msg = towire_gossipctl_new_peer(peer, peer->unique_id, &cs);
-	subd_send_msg(peer->ld->gossip, take(msg));
-	subd_send_fd(peer->ld->gossip, peer->fd);
-
-	/* Peer struct longer owns fd. */
-	peer->fd = -1;
-
-	/* Tell handshaked to exit. */
-	return false;
-
-err:
-	log_broken(hs->log, "Malformed resp: %s", tal_hex(peer, msg));
-	close(peer->fd);
-	tal_free(peer);
-	return false;
-}
-
 /* When a per-peer subdaemon exits, see if we need to do anything. */
 static void peer_owner_finished(struct subd *subd, int status)
 {
@@ -195,69 +159,6 @@ static void peer_owner_finished(struct subd *subd, int status)
 	peer_fail(subd->peer, "Owning subdaemon %s died", subd->name);
 }
 
-static bool peer_got_handshake_hsmfd(struct subd *hsm, const u8 *msg,
-				     const int *fds,
-				     struct peer *peer)
-{
-	const u8 *req;
-
-	assert(tal_count(fds) == 1);
-	if (!fromwire_hsmctl_hsmfd_ecdh_fd_reply(msg, NULL)) {
-		peer_fail(peer, "Malformed hsmfd response: %s",
-			  tal_hex(peer, msg));
-		goto error;
-	}
-
-	/* Give handshake daemon the hsm fd. */
-	peer->owner = new_subd(peer->ld, peer->ld,
-			       "lightningd_handshake", peer,
-			       handshake_wire_type_name,
-			       NULL, peer_owner_finished,
-			       fds[0], peer->fd, -1);
-	if (!peer->owner) {
-		peer_fail(peer, "Could not subdaemon handshake: %s",
-			  strerror(errno));
-		goto error;
-	}
-
-	/* Peer struct longer owns fd. */
-	peer->fd = -1;
-
-	if (peer->id) {
-		req = towire_handshake_initiator(peer, &peer->ld->dstate.id,
-						 peer->id);
-	} else {
-		req = towire_handshake_responder(peer, &peer->ld->dstate.id);
-	}
-	peer_set_condition(peer, HANDSHAKING);
-
-	/* Now hand peer request to the handshake daemon: hands it
-	 * back on success */
-	subd_req(peer, peer->owner, take(req), -1, 1, handshake_succeeded, peer);
-	return true;
-
-error:
-	close(fds[0]);
-	return true;
-}
-
-/* FIXME: timeout handshake if taking too long? */
-static struct io_plan *peer_in(struct io_conn *conn, struct lightningd *ld)
-{
-	struct peer *peer = new_peer(ld, conn, NULL);
-
-	if (!peer)
-		return io_close(conn);
-
-	/* Get HSM fd for this peer. */
-	subd_req(peer, ld->hsm,
-		 take(towire_hsmctl_hsmfd_ecdh(ld, peer->unique_id)),
-		 -1, 1, peer_got_handshake_hsmfd, peer);
-
-	/* We don't need conn, we'll pass fd to handshaked. */
-	return io_close_taken_fd(conn);
-}
-
 static int make_listen_fd(struct lightningd *ld,
 			  int domain, void *addr, socklen_t len)
 {
@@ -333,7 +234,7 @@ void setup_listeners(struct lightningd *ld)
 			assert(ld->dstate.portnum == ntohs(addr.sin_port));
 			log_debug(ld->log, "Creating IPv6 listener on port %u",
 				  ld->dstate.portnum);
-			io_new_listener(ld, fd1, peer_in, ld);
+			io_new_listener(ld, fd1, connection_in, ld);
 		}
 	}
 
@@ -350,7 +251,7 @@ void setup_listeners(struct lightningd *ld)
 			assert(ld->dstate.portnum == ntohs(addr.sin_port));
 			log_debug(ld->log, "Creating IPv4 listener on port %u",
 				  ld->dstate.portnum);
-			io_new_listener(ld, fd2, peer_in, ld);
+			io_new_listener(ld, fd2, connection_in, ld);
 		}
 	}
 
@@ -359,77 +260,47 @@ void setup_listeners(struct lightningd *ld)
 		      ld->dstate.portnum);
 }
 
-struct json_connecting {
-	/* This owns us, so we're freed after command_fail or command_success */
-	struct command *cmd;
-	const char *name, *port;
-	struct pubkey id;
-};
-
-/* FIXME: timeout handshake if taking too long? */
-static struct io_plan *peer_out(struct io_conn *conn,
-				struct lightningd_state *dstate,
-				struct json_connecting *jc)
-{
-	struct lightningd *ld = ld_from_dstate(jc->cmd->dstate);
-	struct peer *peer = new_peer(ld, conn, jc->cmd);
-
-	if (!peer)
-		return io_close(conn);
-
-	/* We already know ID we're trying to reach. */
-	peer->id = tal_dup(peer, struct pubkey, &jc->id);
-
-	/* Get HSM fd for this peer. */
-	subd_req(peer, ld->hsm,
-		 take(towire_hsmctl_hsmfd_ecdh(ld, peer->unique_id)),
-		 -1, 1, peer_got_handshake_hsmfd, peer);
-
-	/* We don't need conn, we'll pass fd to handshaked. */
-	return io_close_taken_fd(conn);
-}
-
 static void connect_failed(struct lightningd_state *dstate,
-			   struct json_connecting *connect)
+			   struct connection *c)
 {
-	/* FIXME: Better diagnostics! */
+	tal_free(c);
-	command_fail(connect->cmd, "Failed to connect to peer %s:%s",
-		     connect->name, connect->port);
 }
 
 static void json_connect(struct command *cmd,
 			 const char *buffer, const jsmntok_t *params)
 {
-	struct json_connecting *connect;
+	struct lightningd *ld = ld_from_dstate(cmd->dstate);
-	jsmntok_t *host, *port, *idtok;
+	struct connection *c;
+	jsmntok_t *host, *porttok, *idtok;
 	const tal_t *tmpctx = tal_tmpctx(cmd);
+	struct pubkey id;
+	char *name, *port;
 
 	if (!json_get_params(buffer, params,
 			     "host", &host,
-			     "port", &port,
+			     "port", &porttok,
 			     "id", &idtok,
 			     NULL)) {
 		command_fail(cmd, "Need host, port and id to connect");
 		return;
 	}
 
-	connect = tal(cmd, struct json_connecting);
-	connect->cmd = cmd;
-	connect->name = tal_strndup(connect, buffer + host->start,
-				    host->end - host->start);
-	connect->port = tal_strndup(connect, buffer + port->start,
-				    port->end - port->start);
-
 	if (!pubkey_from_hexstr(buffer + idtok->start,
-				idtok->end - idtok->start, &connect->id)) {
+				idtok->end - idtok->start, &id)) {
 		command_fail(cmd, "id %.*s not valid",
 			     idtok->end - idtok->start,
 			     buffer + idtok->start);
 		return;
 	}
 
-	if (!dns_resolve_and_connect(cmd->dstate, connect->name, connect->port,
+	c = new_connection(cmd, ld, cmd, &id);
-				     peer_out, connect_failed, connect)) {
+	name = tal_strndup(tmpctx,
+			   buffer + host->start, host->end - host->start);
+	port = tal_strndup(tmpctx,
+			   buffer + porttok->start,
+			   porttok->end - porttok->start);
+	if (!dns_resolve_and_connect(cmd->dstate, name, port,
+				     connection_out, connect_failed, c)) {
 		command_fail(cmd, "DNS failed");
 		return;
 	}

lightningd/peer_control.h

@@ -45,9 +45,6 @@ struct peer {
 	/* Where we connected to, or it connected from. */
 	struct netaddr netaddr;
 
-	/* Json command which made us connect (if any) */
-	struct command *connect_cmd;
-
 	/* Our channel config. */
 	struct channel_config our_config;
 
@@ -109,6 +106,9 @@ struct peer *peer_from_json(struct lightningd *ld,
 void peer_accept_open(struct peer *peer,
 		      const struct crypto_state *cs, const u8 *msg);
 
+void add_peer(struct lightningd *ld, u64 unique_id,
+	      int fd, const struct pubkey *id,
+	      const struct crypto_state *cs);
 /* Peer has failed. */
 PRINTF_FMT(2,3) void peer_fail(struct peer *peer, const char *fmt, ...);
 

lightningd/peer_state.h

@@ -4,7 +4,6 @@
 
 enum peer_state {
 	/* Not important: we can forget about peers in these states. */
-	HANDSHAKING,
 	INITIALIZING,
 	GOSSIPING,
 

tests/test_lightningd.py

@@ -193,8 +193,6 @@ class LightningDTests(BaseLightningDTests):
         assert p2['state'] == 'GOSSIPING'
 
         # It should have gone through these steps
-        print(p1['log'])
-        assert 'state: HANDSHAKING -> INITIALIZING' in p1['log']
         assert 'state: INITIALIZING -> GOSSIPING' in p1['log']
 
         # Both should still be owned by gossip