From 640ff4b4b964a580ea69ec9a5df6f4afdb17bed4 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 13 Mar 2018 09:36:00 +1030
Subject: [PATCH] gossipd: cleanups due to feedback from cdecker.

1. make queue_peer_msg() use both if branches, as both equally likely.
2. Remove redundant *scid = NULL in handle_channel_announcement.
3. Log failing pending channel_updates.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 gossipd/gossip.c  | 15 +++++-------
 gossipd/routing.c | 59 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 44 insertions(+), 30 deletions(-)

diff --git a/gossipd/gossip.c b/gossipd/gossip.c
index 309076043..21b5b1710 100644
--- a/gossipd/gossip.c
+++ b/gossipd/gossip.c
@@ -293,18 +293,15 @@ static void reached_peer(struct daemon *daemon, const struct pubkey *id,
 
 static void queue_peer_msg(struct peer *peer, const u8 *msg TAKES)
 {
-	const u8 *send;
-
 	if (peer->local) {
 		msg_enqueue(&peer->local->peer_out, msg);
-		return;
+	} else {
+		/* Use gossip_index 0 meaning don't update index */
+		const u8 *send = towire_gossip_send_gossip(NULL, 0, msg);
+		if (taken(msg))
+			tal_free(msg);
+		daemon_conn_send(peer->remote, take(send));
 	}
-
-	/* Use gossip_index 0 meaning don't update index */
-	send = towire_gossip_send_gossip(NULL, 0, msg);
-	if (taken(msg))
-		tal_free(msg);
-	daemon_conn_send(peer->remote, take(send));
 }
 
 static void peer_error(struct peer *peer, const char *fmt, ...)
diff --git a/gossipd/routing.c b/gossipd/routing.c
index 887559df5..57c72ff80 100644
--- a/gossipd/routing.c
+++ b/gossipd/routing.c
@@ -627,9 +627,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 		err = towire_errorfmt(rstate, NULL,
 				      "Malformed channel_announcement %s",
 				      tal_hex(pending, pending->announce));
-		tal_free(pending);
-		*scid = NULL;
-		return err;
+		goto malformed;
 	}
 
 	/* Check if we know the channel already (no matter in what
@@ -640,9 +638,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 			     __func__,
 			     type_to_string(trc, struct short_channel_id,
 					    &pending->short_channel_id));
-		tal_free(pending);
-		*scid = NULL;
-		return NULL;
+		goto ignored;
 	}
 
 	/* We don't replace previous ones, since we might validate that and
@@ -652,8 +648,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 			     __func__,
 			     type_to_string(trc, struct short_channel_id,
 					    &pending->short_channel_id));
-		*scid = NULL;
-		return tal_free(pending);
+		goto ignored;
 	}
 
 	/* FIXME: Handle duplicates as per BOLT #7 */
@@ -668,9 +663,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 	if (unsupported_features(features, NULL)) {
 		status_trace("Ignoring channel announcement, unsupported features %s.",
 			     tal_hex(pending, features));
-		tal_free(pending);
-		*scid = NULL;
-		return NULL;
+		goto ignored;
 	}
 
 	/* BOLT #7:
@@ -684,9 +677,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 		    type_to_string(pending, struct short_channel_id,
 				   &pending->short_channel_id),
 		    type_to_string(pending, struct bitcoin_blkid, &chain_hash));
-		tal_free(pending);
-		*scid = NULL;
-		return NULL;
+		goto ignored;
 	}
 
 	err = check_channel_announcement(rstate,
@@ -707,8 +698,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 		 *    correct:
 		 *    - SHOULD fail the connection.
 		 */
-		tal_free(pending);
-		return err;
+		goto malformed;
 	}
 
 	status_trace("Received channel_announcement for channel %s",
@@ -723,8 +713,38 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
 	list_add_tail(&rstate->pending_cannouncement, &pending->list);
 	tal_add_destructor2(pending, destroy_pending_cannouncement, rstate);
 
+	/* Success */
 	*scid = &pending->short_channel_id;
 	return NULL;
+
+malformed:
+	tal_free(pending);
+	*scid = NULL;
+	return err;
+
+ignored:
+	tal_free(pending);
+	*scid = NULL;
+	return NULL;
+}
+
+static void process_pending_channel_update(struct routing_state *rstate,
+					   const struct short_channel_id *scid,
+					   const u8 *cupdate)
+{
+	u8 *err;
+
+	if (!cupdate)
+		return;
+
+	/* FIXME: We don't remember who sent us updates, so can't error them */
+	err = handle_channel_update(rstate, cupdate);
+	if (err) {
+		status_trace("Pending channel_update for %s: %s",
+			     type_to_string(trc, struct short_channel_id, scid),
+			     sanitize_error(trc, err, NULL));
+		tal_free(err);
+	}
 }
 
 bool handle_pending_cannouncement(struct routing_state *rstate,
@@ -808,11 +828,8 @@ bool handle_pending_cannouncement(struct routing_state *rstate,
 		pubkey_eq(&pending->node_id_2, &rstate->local_id);
 
 	/* Did we have an update waiting?  If so, apply now. */
-	/* FIXME: We don't remember who sent us updates, so we can't error them */
-	if (pending->updates[0])
-		tal_free(handle_channel_update(rstate, pending->updates[0]));
-	if (pending->updates[1])
-		tal_free(handle_channel_update(rstate, pending->updates[1]));
+	process_pending_channel_update(rstate, scid, pending->updates[0]);
+	process_pending_channel_update(rstate, scid, pending->updates[1]);
 
 	process_pending_node_announcement(rstate, &pending->node_id_1);
 	process_pending_node_announcement(rstate, &pending->node_id_2);