lightningd/gossip: clean up channel resolve failure handling.

We were getting an assert "!secp256k1_fe_is_zero(&ge->x)", because
an all-zero pubkey is invalid.  We allow marshal/unmarshal of NULL for
now, and clean up the error handling.

1. Use status_failed if master sends a bad message.
2. Similarly, kill the gossip daemon if it gives a bad reply.
3. Use an array for returned pubkeys: 0 or 2.
4. Use type_to_string(trc, struct short_channel_id, &scid) for tracing.

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

lightningd/gossip/gossip.c

@@ -638,26 +638,25 @@ static struct io_plan *resolve_channel_req(struct io_conn *conn,
 {
 	struct short_channel_id scid;
 	struct node_connection *nc;
-	u8 *reply;
-	struct pubkey *nullkey = talz(msg, struct pubkey);
-	if (!fromwire_gossip_resolve_channel_request(msg, NULL, &scid)) {
-		status_trace("Unable to parse resolver request");
-		reply = towire_gossip_resolve_channel_reply(msg, 1, nullkey,
-							    nullkey);
-	} else {
-		status_trace("Attempting to resolce channel %d/%d/%d",
-			     scid.blocknum, scid.txnum, scid.outnum);
+	struct pubkey *keys;
 
-		nc = get_connection_by_scid(daemon->rstate, &scid, 0);
-		if (!nc) {
-			reply = towire_gossip_resolve_channel_reply(
-			    msg, 2, nullkey, nullkey);
-		} else {
-			reply = towire_gossip_resolve_channel_reply(
-			    msg, 0, &nc->src->id, &nc->dst->id);
-		}
+	if (!fromwire_gossip_resolve_channel_request(msg, NULL, &scid))
+		status_failed(WIRE_GOSSIPSTATUS_BAD_REQUEST,
+			      "Unable to parse resolver request");
+
+	status_trace("Attempting to resolve channel %s",
+		     type_to_string(trc, struct short_channel_id, &scid));
+
+	nc = get_connection_by_scid(daemon->rstate, &scid, 0);
+	if (!nc) {
+		keys = NULL;
+	} else {
+		keys = tal_arr(msg, struct pubkey, 2);
+		keys[0] = nc->src->id;
+		keys[1] = nc->dst->id;
 	}
-	daemon_conn_send(&daemon->master, reply);
+	daemon_conn_send(&daemon->master,
+			 take(towire_gossip_resolve_channel_reply(msg, keys)));
 	return daemon_conn_read_next(conn, &daemon->master);
 }
 

lightningd/gossip/gossip_wire.csv

@@ -96,6 +96,5 @@ gossip_resolve_channel_request,9
 gossip_resolve_channel_request,0,channel_id,struct short_channel_id
 
 gossip_resolve_channel_reply,109
-gossip_resolve_channel_reply,0,error,u8
-gossip_resolve_channel_reply,1,node_id_1,struct pubkey
-gossip_resolve_channel_reply,34,node_id_2,struct pubkey
+gossip_resolve_channel_reply,0,num_keys,u16
+gossip_resolve_channel_reply,0,keys,num_keys*struct pubkey

lightningd/peer_control.c

@@ -1039,20 +1039,36 @@ fail:
 static bool channel_resolve_reply(struct subd *gossip, const u8 *msg,
 				  const int *fds, struct htlc_end *hend)
 {
-	struct pubkey node_id_1, node_id_2, peer_id;
-	u8 error;
-	fromwire_gossip_resolve_channel_reply(msg, NULL, &error, &node_id_1,
-					      &node_id_2);
+	struct pubkey *nodes, *peer_id;
+
+	if (!fromwire_gossip_resolve_channel_reply(msg, msg, NULL, &nodes)) {
+		log_broken(gossip->log,
+			   "bad fromwire_gossip_resolve_channel_reply %s",
+			   tal_hex(msg, msg));
+		return false;
+	}
+
+	if (tal_count(nodes) == 0) {
+		fail_htlc(hend->peer, hend->htlc_id,
+			  take(towire_unknown_next_peer(hend)));
+		tal_free(hend);
+		return true;
+	} else if (tal_count(nodes) != 2) {
+		log_broken(gossip->log,
+			   "fromwire_gossip_resolve_channel_reply has %zu nodes",
+			   tal_count(nodes));
+		return false;
+	}
 
 	/* Get the other peer matching the id that is not us */
-	if (pubkey_cmp(&node_id_1, &gossip->ld->dstate.id) == 0) {
-		peer_id = node_id_2;
+	if (pubkey_cmp(&nodes[0], &gossip->ld->dstate.id) == 0) {
+		peer_id = &nodes[1];
 	} else {
-		peer_id = node_id_1;
+		peer_id = &nodes[0];
 	}
 
 	forward_htlc(hend, hend->cltv_expiry, &hend->payment_hash,
-		     hend->amt_to_forward, hend->outgoing_cltv_value, &peer_id,
+		     hend->amt_to_forward, hend->outgoing_cltv_value, peer_id,
 		     hend->next_onion);
 	/* FIXME(cdecker) Cleanup things we stuffed into hend before (maybe?) */
 	return true;

wire/fromwire.c

@@ -99,7 +99,10 @@ void fromwire_pubkey(const u8 **cursor, size_t *max, struct pubkey *pubkey)
 	if (!fromwire(cursor, max, der, sizeof(der)))
 		return;
 
-	if (!pubkey_from_der(der, sizeof(der), pubkey))
+	/* FIXME: Handing dummy keys through here is dumb.
+	 * See towire_gossip_resolve_channel_reply --RR */
+	if (!memeqzero(der, sizeof(der))
+	    && !pubkey_from_der(der, sizeof(der), pubkey))
 		fail_pull(cursor, max);
 }
 

wire/towire.c

@@ -48,9 +48,13 @@ void towire_pubkey(u8 **pptr, const struct pubkey *pubkey)
 	u8 output[PUBKEY_DER_LEN];
 	size_t outputlen = sizeof(output);
 
-	secp256k1_ec_pubkey_serialize(secp256k1_ctx, output, &outputlen,
-				      &pubkey->pubkey,
-				      SECP256K1_EC_COMPRESSED);
+	if (pubkey)
+		secp256k1_ec_pubkey_serialize(secp256k1_ctx, output, &outputlen,
+					      &pubkey->pubkey,
+					      SECP256K1_EC_COMPRESSED);
+	else
+		memset(output, 0, sizeof(output));
+
 	towire(pptr, output, outputlen);
 }