Better handling unexpected msgs

channeld/channel.c

@@ -1555,7 +1555,6 @@ static void handle_peer_shutdown(struct peer *peer, const u8 *shutdown)
 static void peer_in(struct peer *peer, const u8 *msg)
 {
 	enum wire_type type = fromwire_peektype(msg);
-	status_trace("peer_in %s", wire_type_name(type));
 
 	/* FIXME: We don't support concurrent channels with same peer. */
 	if (type == WIRE_OPEN_CHANNEL) {
@@ -1767,6 +1766,73 @@ static void resend_commitment(struct peer *peer, const struct changed_htlc *last
 	assert(peer->revocations_received == peer->next_index[REMOTE] - 2);
 }
 
+/* Handle random messages we might get, returning NULL if we handled it. */
+static u8 *read_peer_msg(struct peer *peer)
+{
+	u8 *msg;
+	struct channel_id channel_id;
+
+	msg = sync_crypto_read(peer, &peer->cs, PEER_FD);
+	if (!msg)
+		peer_conn_broken(peer);
+
+	status_trace("peer_in %s", wire_type_name(fromwire_peektype(msg)));
+
+	if (is_gossip_msg(msg)) {
+		/* Forward to gossip daemon */
+		wire_sync_write(GOSSIP_FD, take(msg));
+		return NULL;
+	}
+
+	if (fromwire_peektype(msg) == WIRE_PING) {
+		handle_ping(peer, msg);
+		return tal_free(msg);
+	}
+
+	if (fromwire_peektype(msg) == WIRE_ERROR) {
+		struct channel_id chanid;
+		char *err = sanitize_error(msg, msg, &chanid);
+
+		/* BOLT #1:
+		 *
+		 * The channel is referred to by `channel_id`, unless
+		 * `channel_id` is 0 (i.e. all bytes are 0), in which
+		 * case it refers to all channels.
+		 * ...
+
+		 * The receiving node:
+		 *   - upon receiving `error`:
+		 *    - MUST fail the channel referred to by the error
+		 *       message.
+		 *  - if no existing channel is referred to by the
+		 *    message:
+		 *    - MUST ignore the message.
+		 */
+		if (channel_id_is_all(&chanid)
+		    || structeq(&chanid, &peer->channel_id)) {
+			status_failed(STATUS_FAIL_PEER_BAD,
+				      "Received ERROR %s", err);
+		}
+		return tal_free(msg);
+	}
+
+	/* They're talking about a different channel? */
+	if (extract_channel_id(msg, &channel_id)
+	    && !structeq(&channel_id, &peer->channel_id)) {
+		status_trace("Rejecting %s for unknown channel_id %s",
+			     wire_type_name(fromwire_peektype(msg)),
+			     type_to_string(msg, struct channel_id,
+					    &channel_id));
+		enqueue_peer_msg(peer,
+				 take(towire_errorfmt(msg, &channel_id,
+						      "Multiple channels"
+						      " unsupported")));
+		return tal_free(msg);
+	}
+
+	return msg;
+}
+
 static void peer_reconnect(struct peer *peer)
 {
 	struct channel_id channel_id;
@@ -1797,22 +1863,8 @@ static void peer_reconnect(struct peer *peer)
 		status_failed(STATUS_FAIL_PEER_IO,
 			      "Failed writing reestablish: %s", strerror(errno));
 
-again:
-	msg = sync_crypto_read(peer, &peer->cs, PEER_FD);
-	if (!msg)
-		status_failed(STATUS_FAIL_PEER_IO,
-			      "Failed reading reestablish: %s", strerror(errno));
-
-	if (is_gossip_msg(msg)) {
-		/* Forward to gossip daemon */
-		wire_sync_write(GOSSIP_FD, take(msg));
-		goto again;
-	}
-
-	if (fromwire_peektype(msg) == WIRE_PING) {
-		handle_ping(peer, msg);
-		goto again;
-	}
+	/* Read until they say something interesting */
+	while ((msg = read_peer_msg(peer)) == NULL);
 
 	if (!fromwire_channel_reestablish(msg, NULL, &channel_id,
 					  &next_local_commitment_number,
@@ -2706,11 +2758,9 @@ int main(int argc, char *argv[])
 			gossip_in(peer, msg);
 		} else if (FD_ISSET(PEER_FD, &rfds)) {
 			/* This could take forever, but who cares? */
-			msg = sync_crypto_read(peer, &peer->cs, PEER_FD);
-
-			if (!msg)
-				peer_conn_broken(peer);
-			peer_in(peer, msg);
+			msg = read_peer_msg(peer);
+			if (msg)
+				peer_in(peer, msg);
 		} else
 			msg = NULL;
 		tal_free(msg);

closingd/Makefile

@@ -54,6 +54,7 @@ CLOSINGD_COMMON_OBJS :=				\
 	common/msg_queue.o			\
 	common/peer_failed.o			\
 	common/permute_tx.o			\
+	common/ping.o				\
 	common/status.o				\
 	common/subdaemon.o			\
 	common/type_to_string.o			\

closingd/closing.c

@@ -1,15 +1,18 @@
 #include <bitcoin/script.h>
+#include <ccan/structeq/structeq.h>
 #include <closingd/gen_closing_wire.h>
 #include <common/close_tx.h>
 #include <common/crypto_sync.h>
 #include <common/derive_basepoints.h>
 #include <common/htlc.h>
 #include <common/peer_failed.h>
+#include <common/ping.h>
 #include <common/status.h>
 #include <common/subdaemon.h>
 #include <common/type_to_string.h>
 #include <common/utils.h>
 #include <common/version.h>
+#include <common/wire_error.h>
 #include <errno.h>
 #include <inttypes.h>
 #include <stdio.h>
@@ -80,6 +83,92 @@ static u64 one_towards(u64 target, u64 value)
 	return value;
 }
 
+static void handle_ping(const u8 *msg,
+			struct crypto_state *cs,
+			const struct channel_id *our_channel_id)
+{
+	u8 *pong;
+
+	if (!check_ping_make_pong(msg, msg, &pong))
+		peer_failed(PEER_FD, cs, our_channel_id, "Bad ping");
+
+	status_trace("Got ping, sending %s", pong ?
+		     wire_type_name(fromwire_peektype(pong))
+		     : "nothing");
+
+	if (pong && !sync_crypto_write(cs, PEER_FD, take(pong)))
+		status_failed(STATUS_FAIL_PEER_IO,
+			      "Failed writing pong: %s", strerror(errno));
+}
+
+/* Handle random messages we might get, returning NULL if we handled it. */
+static u8 *read_peer_msg(const tal_t *ctx,
+			 struct crypto_state *cs,
+			 const struct channel_id *our_channel_id)
+{
+	u8 *msg;
+	struct channel_id channel_id;
+
+	msg = sync_crypto_read(ctx, cs, PEER_FD);
+	if (!msg)
+		status_failed(STATUS_FAIL_PEER_IO,
+			      "Failed reading from peer: %s", strerror(errno));
+
+	if (is_gossip_msg(msg)) {
+		/* Forward to gossip daemon */
+		wire_sync_write(GOSSIP_FD, take(msg));
+		return NULL;
+	}
+
+	if (fromwire_peektype(msg) == WIRE_PING) {
+		handle_ping(msg, cs, our_channel_id);
+		return tal_free(msg);
+	}
+
+	if (fromwire_peektype(msg) == WIRE_ERROR) {
+		struct channel_id chanid;
+		char *err = sanitize_error(msg, msg, &chanid);
+
+		/* BOLT #1:
+		 *
+		 * The channel is referred to by `channel_id`, unless
+		 * `channel_id` is 0 (i.e. all bytes are 0), in which
+		 * case it refers to all channels.
+		 * ...
+
+		 * The receiving node:
+		 *   - upon receiving `error`:
+		 *    - MUST fail the channel referred to by the error
+		 *       message.
+		 *  - if no existing channel is referred to by the
+		 *    message:
+		 *    - MUST ignore the message.
+		 */
+		if (channel_id_is_all(&chanid)
+		    || structeq(&chanid, our_channel_id)) {
+			status_failed(STATUS_FAIL_PEER_BAD,
+				      "Received ERROR %s", err);
+		}
+		return tal_free(msg);
+	}
+
+	/* They're talking about a different channel? */
+	if (extract_channel_id(msg, &channel_id)
+	    && !structeq(&channel_id, our_channel_id)) {
+		status_trace("Rejecting %s for unknown channel_id %s",
+			     wire_type_name(fromwire_peektype(msg)),
+			     type_to_string(msg, struct channel_id,
+					    &channel_id));
+		sync_crypto_write(cs, PEER_FD,
+				  take(towire_errorfmt(msg, &channel_id,
+						       "Multiple channels"
+						       " unsupported")));
+		return tal_free(msg);
+	}
+
+	return msg;
+}
+
 static void do_reconnect(struct crypto_state *cs,
 			 const struct channel_id *channel_id,
 			 const u64 next_index[NUM_SIDES],
@@ -108,17 +197,8 @@ static void do_reconnect(struct crypto_state *cs,
 		status_failed(STATUS_FAIL_PEER_IO,
 			      "Failed writing reestablish: %s", strerror(errno));
 
-again:
-	msg = sync_crypto_read(tmpctx, cs, PEER_FD);
-	if (!msg)
-		status_failed(STATUS_FAIL_PEER_IO,
-			      "Failed reading reestablish: %s", strerror(errno));
-
-	if (is_gossip_msg(msg)) {
-		if (!wire_sync_write(GOSSIP_FD, take(msg)))
-			status_failed(STATUS_FAIL_GOSSIP_IO, "Writing gossip");
-		goto again;
-	}
+	/* Wait for them to say something interesting */
+	while ((msg = read_peer_msg(tmpctx, cs, channel_id)) == NULL);
 
 	if (!fromwire_channel_reestablish(msg, NULL, &their_channel_id,
 					  &next_local_commitment_number,
@@ -263,17 +343,8 @@ int main(int argc, char *argv[])
 			break;
 
 	again:
-		msg = sync_crypto_read(tmpctx, &cs, PEER_FD);
-		if (!msg)
-			status_failed(STATUS_FAIL_PEER_IO, "Reading input");
-
-		/* We don't send gossip at this stage, but we can recv it */
-		if (is_gossip_msg(msg)) {
-			if (!wire_sync_write(GOSSIP_FD, take(msg)))
-				status_failed(STATUS_FAIL_GOSSIP_IO,
-					      "Writing gossip");
-			goto again;
-		}
+		/* Wait for them to say something interesting */
+		while ((msg = read_peer_msg(tmpctx, &cs, &channel_id)) == NULL);
 
 		/* BOLT #2:
 		 *

lightningd/log.c

@@ -128,7 +128,8 @@ new_log(const tal_t *ctx, struct log_book *record, const char *fmt, ...)
 	log->lr = record;
 	va_start(ap, fmt);
 	/* log->lr owns this, since its entries keep a pointer to it. */
-	log->prefix = tal_vfmt(log->lr, fmt, ap);
+	/* FIXME: Refcount this! */
+	log->prefix = notleak(tal_vfmt(log->lr, fmt, ap));
 	va_end(ap);
 
 	return log;

lightningd/peer_control.c

@@ -699,12 +699,10 @@ static void copy_to_parent_log(const char *prefix,
 			       const char *str,
 			       struct peer *peer)
 {
-	const char *idstr = type_to_string(peer, struct pubkey, &peer->id);
 	if (continued)
-		log_add(peer->ld->log, "Peer %s: ... %s", idstr, str);
+		log_add(peer->ld->log, "%s ... %s", prefix, str);
 	else
-		log_(peer->ld->log, level, "Peer %s: %s", idstr, str);
-	tal_free(idstr);
+		log_(peer->ld->log, level, "%s %s", prefix, str);
 }
 
 void populate_peer(struct lightningd *ld, struct peer *peer)

lightningd/subd.c

@@ -625,7 +625,16 @@ static struct subd *new_subd(struct lightningd *ld,
 		return tal_free(sd);
 	}
 	sd->ld = ld;
-	sd->log = new_log(sd, ld->log_book, "%s(%u):", name, sd->pid);
+	if (peer) {
+		/* FIXME: Use minimal unique pubkey prefix for logs! */
+		const char *idstr = type_to_string(peer, struct pubkey,
+						   &peer->id);
+		sd->log = new_log(sd, peer->log_book, "%s(%s):", name, idstr);
+		tal_free(idstr);
+	} else {
+		sd->log = new_log(sd, ld->log_book, "%s(%u):", name, sd->pid);
+	}
+
 	sd->name = name;
 	sd->must_not_exit = false;
 	sd->msgname = msgname;