channeld/lightningd/hsmd: strengthen our checks against 0-output txs.

If we ever do this, we'd end up with an unspendable commitment tx anyway.
It might be able to happen if we have htlcs added from the non-fee-paying
party while the fees are increased, though.  But better to close the
channel and get a report about it if that happens.

Signed-off-by: Rusty Russell <[email protected]>

Author: rustyrussell

channeld/commit_tx.c

@@ -242,6 +242,14 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		n++;
 	}
 
+	/* BOLT #2:
+	 *
+	 *  - MUST set `channel_reserve_satoshis` greater than or equal to
+	 *    `dust_limit_satoshis`.
+	 */
+	/* This means there must be at least one output. */
+	assert(n > 0);
+
 	assert(n <= tx->wtx->outputs_allocation_len);
 	tal_resize(htlcmap, n);
 

hsmd/hsmd.c

@@ -152,6 +152,9 @@ static PRINTF_FMT(4,5)
 		master_badmsg(fromwire_peektype(msg_in), msg_in);
 	}
 
+	/*~ Nobody should give us bad requests; it's a sign something is broken */
+	status_broken("%s: %s", type_to_string(tmpctx, struct node_id, &c->id), str);
+
 	/*~ Note the use of NULL as the ctx arg to towire_hsmstatus_: only
 	 * use NULL as the allocation when we're about to immediately free it
 	 * or hand it off with take(), as here.  That makes it clear we don't
@@ -770,6 +773,12 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn,
 					     &funding))
 		return bad_req(conn, c, msg_in);
 
+	/* Basic sanity checks. */
+	if (tx->wtx->num_inputs != 1)
+		return bad_req_fmt(conn, c, msg_in, "tx must have 1 input");
+	if (tx->wtx->num_outputs == 0)
+		return bad_req_fmt(conn, c, msg_in, "tx must have > 0 outputs");
+
 	get_channel_seed(&peer_id, dbid, &channel_seed);
 	derive_basepoints(&channel_seed,
 			  &local_funding_pubkey, NULL, &secrets, NULL);
@@ -823,6 +832,12 @@ static struct io_plan *handle_sign_remote_commitment_tx(struct io_conn *conn,
 						    &funding))
 		bad_req(conn, c, msg_in);
 
+	/* Basic sanity checks. */
+	if (tx->wtx->num_inputs != 1)
+		return bad_req_fmt(conn, c, msg_in, "tx must have 1 input");
+	if (tx->wtx->num_outputs == 0)
+		return bad_req_fmt(conn, c, msg_in, "tx must have > 0 outputs");
+
 	get_channel_seed(&c->id, c->dbid, &channel_seed);
 	derive_basepoints(&channel_seed,
 			  &local_funding_pubkey, NULL, &secrets, NULL);

lightningd/peer_htlcs.c

@@ -1224,6 +1224,21 @@ static bool changed_htlc(struct channel *channel,
 		return update_in_htlc(channel, changed->id, changed->newstate);
 }
 
+/* FIXME: This should be a complete check, not just a sanity check.
+ * Perhaps that means we need a cookie from the HSM? */
+static bool valid_commitment_tx(struct channel *channel,
+				const struct bitcoin_tx *tx)
+{
+	/* We've had past issues where all outputs are trimmed. */
+	if (tx->wtx->num_outputs == 0) {
+		channel_internal_error(channel,
+				       "channel_got_commitsig: zero output tx! %s",
+				       type_to_string(tmpctx, struct bitcoin_tx, tx));
+		return false;
+	}
+	return true;
+}
+
 static bool peer_save_commitsig_received(struct channel *channel, u64 commitnum,
 					 struct bitcoin_tx *tx,
 					 const struct bitcoin_signature *commit_sig)
@@ -1236,6 +1251,10 @@ static bool peer_save_commitsig_received(struct channel *channel, u64 commitnum,
 		return false;
 	}
 
+	/* Basic sanity check */
+	if (!valid_commitment_tx(channel, tx))
+		return false;
+
 	channel->next_index[LOCAL]++;
 
 	/* Update channel->last_sig and channel->last_tx before saving to db */