connectd: remove reconnection logic.

We don't have to put aside a peer which is reconnecting and wait for
lightningd to remove the old peer, we can now simply free the old
and add the new.

Fixes: ElementsProject#5240
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

connectd/connectd.c

@@ -211,80 +211,6 @@ static void peer_connected_in(struct daemon *daemon,
 	tal_free(connect);
 }
 
-/*~ For simplicity, lightningd only ever deals with a single connection per
- * peer.  So if we already know about a peer, we tell lightning to disconnect
- * the old one and retry once it does. */
-static struct io_plan *retry_peer_connected(struct io_conn *conn,
-					    struct peer_reconnected *pr)
-{
-	/*~ As you can see, we've had issues with this code before :( */
-	status_peer_debug(&pr->id, "processing now old peer gone");
-
-	/* If this fails (still waiting), pr will be freed, so reparent onto
-	 * tmpctx so it gets freed either way. */
-	tal_steal(tmpctx, pr);
-
-	/*~ Usually the pattern is to return this directly. */
-	return peer_connected(conn, pr->daemon, &pr->id, &pr->addr,
-			      pr->remote_addr,
-			      &pr->cs, take(pr->their_features), pr->incoming);
-}
-
-/*~ A common use for destructors is to remove themselves from a data structure */
-static void destroy_peer_reconnected(struct peer_reconnected *pr)
-{
-	peer_reconnected_htable_del(&pr->daemon->reconnected, pr);
-}
-
-/*~ If we already know about this peer, we tell lightningd and it disconnects
- * the old one.  We wait until it tells us that's happened. */
-static struct io_plan *peer_reconnected(struct io_conn *conn,
-					struct daemon *daemon,
-					const struct node_id *id,
-					const struct wireaddr_internal *addr,
-					const struct wireaddr *remote_addr,
-					const struct crypto_state *cs,
-					const u8 *their_features TAKES,
-					bool incoming)
-{
-	u8 *msg;
-	struct peer_reconnected *pr;
-
-	status_peer_debug(id, "reconnect");
-
-	/* If we have a previous reconnection, we replace it. */
-	pr = peer_reconnected_htable_get(&daemon->reconnected, id);
-	if (pr) {
-		peer_reconnected_htable_del(&daemon->reconnected, pr);
-		tal_free(pr);
-	}
-
-	/* Tell master to kill it: will send peer_disconnect */
-	msg = towire_connectd_reconnected(NULL, id);
-	daemon_conn_send(daemon->master, take(msg));
-
-	/* Save arguments for next time. */
-	pr = tal(daemon, struct peer_reconnected);
-	pr->daemon = daemon;
-	pr->id = *id;
-	pr->cs = *cs;
-	pr->addr = *addr;
-	pr->remote_addr = tal_dup_or_null(pr, struct wireaddr, remote_addr);
-	pr->incoming = incoming;
-	peer_reconnected_htable_add(&daemon->reconnected, pr);
-	tal_add_destructor(pr, destroy_peer_reconnected);
-
-	/*~ Note that tal_dup_talarr() will do handle the take() of features
-	 * (turning it into a simply tal_steal() in those cases). */
-	pr->their_features = tal_dup_talarr(pr, u8, their_features);
-
-	/*~ ccan/io supports waiting on an address: in this case, the key in
-	 * the peer set.  When someone calls `io_wake()` on that address, it
-	 * will call retry_peer_connected above. */
-	return io_wait(conn, peer_htable_get(&daemon->peers, id),
-		       retry_peer_connected, pr);
-}
-
 /*~ When we free a peer, we remove it from the daemon's hashtable */
 void destroy_peer(struct peer *peer)
 {
@@ -353,10 +279,10 @@ struct io_plan *peer_connected(struct io_conn *conn,
 	int subd_fd;
 	bool option_gossip_queries;
 
+	/* We remove any previous connection, on the assumption it's dead */
 	peer = peer_htable_get(&daemon->peers, id);
 	if (peer)
-		return peer_reconnected(conn, daemon, id, addr, remote_addr, cs,
-					their_features, incoming);
+		tal_free(peer);
 
 	/* We promised we'd take it by marking it TAKEN above; prepare to free it. */
 	if (taken(their_features))
@@ -1946,7 +1872,6 @@ static void dev_connect_memleak(struct daemon *daemon, const u8 *msg)
 	/* Now delete daemon and those which it has pointers to. */
 	memleak_remove_region(memtable, daemon, sizeof(daemon));
 	memleak_remove_htable(memtable, &daemon->peers.raw);
-	memleak_remove_htable(memtable, &daemon->reconnected.raw);
 
 	found_leak = dump_memleak(memtable, memleak_status_broken);
 	daemon_conn_send(daemon->master,
@@ -2021,7 +1946,6 @@ static struct io_plan *recv_req(struct io_conn *conn,
 	case WIRE_CONNECTD_PEER_CONNECTED:
 	case WIRE_CONNECTD_PEER_ALREADY_CONNECTED:
 	case WIRE_CONNECTD_PEER_ACTIVE:
-	case WIRE_CONNECTD_RECONNECTED:
 	case WIRE_CONNECTD_CONNECT_FAILED:
 	case WIRE_CONNECTD_DEV_MEMLEAK_REPLY:
 	case WIRE_CONNECTD_PING_REPLY:
@@ -2093,7 +2017,6 @@ int main(int argc, char *argv[])
 	/* Allocate and set up our simple top-level structure. */
 	daemon = tal(NULL, struct daemon);
 	peer_htable_init(&daemon->peers);
-	peer_reconnected_htable_init(&daemon->reconnected);
 	memleak_add_helper(daemon, memleak_daemon_cb);
 	list_head_init(&daemon->connecting);
 	timers_init(&daemon->timers, time_mono());

connectd/connectd.h

@@ -119,37 +119,6 @@ HTABLE_DEFINE_TYPE(struct peer,
 		   peer_eq_node_id,
 		   peer_htable);
 
-/*~ This is an ad-hoc marshalling structure where we store arguments so we
- * can call peer_connected again. */
-struct peer_reconnected {
-	struct daemon *daemon;
-	struct node_id id;
-	struct wireaddr_internal addr;
-	const struct wireaddr *remote_addr;
-	struct crypto_state cs;
-	const u8 *their_features;
-	bool incoming;
-};
-
-static const struct node_id *
-peer_reconnected_keyof(const struct peer_reconnected *pr)
-{
-	return &pr->id;
-}
-
-static bool peer_reconnected_eq_node_id(const struct peer_reconnected *pr,
-					const struct node_id *id)
-{
-	return node_id_eq(&pr->id, id);
-}
-
-/*~ This defines 'struct peer_reconnected_htable'. */
-HTABLE_DEFINE_TYPE(struct peer_reconnected,
-		   peer_reconnected_keyof,
-		   node_id_hash,
-		   peer_reconnected_eq_node_id,
-		   peer_reconnected_htable);
-
 /*~ This is the global state, like `struct lightningd *ld` in lightningd. */
 struct daemon {
 	/* Who am I? */
@@ -166,9 +135,6 @@ struct daemon {
 	 * have disconnected. */
 	struct peer_htable peers;
 
-	/* Peers which have reconnected, waiting for us to kill existing conns */
-	struct peer_reconnected_htable reconnected;
-
 	/* Peers we are trying to reach */
 	struct list_head connecting;
 

connectd/connectd_wire.csv

@@ -45,10 +45,6 @@ msgdata,connectd_activate,listen,bool,
 msgtype,connectd_activate_reply,2125
 msgdata,connectd_activate_reply,failmsg,?wirestring,
 
-# connectd->master: disconnect this peer please (due to reconnect).
-msgtype,connectd_reconnected,2112
-msgdata,connectd_reconnected,id,node_id,
-
 # Master -> connectd: connect to a peer.
 msgtype,connectd_connect_to_peer,2001
 msgdata,connectd_connect_to_peer,id,node_id,

lightningd/connect_control.c

@@ -328,21 +328,6 @@ static void peer_already_connected(struct lightningd *ld, const u8 *msg)
 				  &peer->addr);
 }
 
-static void peer_please_disconnect(struct lightningd *ld, const u8 *msg)
-{
-	struct node_id id;
-	struct peer *peer;
-
-	if (!fromwire_connectd_reconnected(msg, &id))
-		fatal("Bad msg %s from connectd", tal_hex(tmpctx, msg));
-
-	peer = peer_by_id(ld, &id);
-	if (!peer)
-		return;
-
-	peer_channels_cleanup_on_disconnect(peer);
-}
-
 struct custommsg_payload {
 	struct node_id peer_id;
 	u8 *msg;
@@ -424,10 +409,6 @@ static unsigned connectd_msg(struct subd *connectd, const u8 *msg, const int *fd
 	case WIRE_CONNECTD_PING_REPLY:
 		break;
 
-	case WIRE_CONNECTD_RECONNECTED:
-		peer_please_disconnect(connectd->ld, msg);
-		break;
-
 	case WIRE_CONNECTD_PEER_CONNECTED:
 		peer_connected(connectd->ld, msg);
 		break;