protocol: no signature in update_commit if receiver has no outputs.

So if there are no HTLCs, and the receiver can't spend anyway, don't
sign.  This has the added benefit that no two signed commitment
transactions will ever be identical (the revocation preimage changes).

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

daemon/commit_tx.c

@@ -111,29 +111,32 @@ u8 *commit_output_to_them(const tal_t *ctx,
 	}
 }
 
-static void add_output(struct bitcoin_tx *tx, u8 *script, u64 amount,
+static bool add_output(struct bitcoin_tx *tx, u8 *script, u64 amount,
 		       u64 *total)
 {
 	assert(tx->output_count < tal_count(tx->output));
 	if (is_dust(amount))
-		return;
+		return false;
 	tx->output[tx->output_count].script = script;
 	tx->output[tx->output_count].script_length = tal_count(script);
 	tx->output[tx->output_count].amount = amount;
 	tx->output_count++;
 	(*total) += amount;
+	return true;
 }
 
 struct bitcoin_tx *create_commit_tx(const tal_t *ctx,
 				    struct peer *peer,
 				    const struct sha256 *rhash,
 				    const struct channel_state *cstate,
-				    enum htlc_side side)
+				    enum htlc_side side,
+				    bool *otherside_only)
 {
 	struct bitcoin_tx *tx;
 	uint64_t total = 0;
 	struct htlc_map_iter it;
 	struct htlc *h;
+	bool pays_to[2];
 	int committed_flag = HTLC_FLAG(side,HTLC_F_COMMITTED);
 
 	/* Now create commitment tx: one input, two outputs (plus htlcs) */
@@ -145,21 +148,31 @@ struct bitcoin_tx *create_commit_tx(const tal_t *ctx,
 	tx->input[0].amount = tal_dup(tx->input, u64, &peer->anchor.satoshis);
 
 	tx->output_count = 0;
-	add_output(tx, commit_output_to_us(tx, peer, rhash, side, NULL),
-		   cstate->side[OURS].pay_msat / 1000, &total);
-	add_output(tx, commit_output_to_them(tx, peer, rhash, side, NULL),
-		   cstate->side[THEIRS].pay_msat / 1000, &total);
+	pays_to[LOCAL] = add_output(tx, commit_output_to_us(tx, peer, rhash,
+							    side, NULL),
+				    cstate->side[OURS].pay_msat / 1000,
+				    &total);
+	pays_to[REMOTE] = add_output(tx, commit_output_to_them(tx, peer, rhash,
+							       side, NULL),
+				     cstate->side[THEIRS].pay_msat / 1000,
+				     &total);
+
+	/* If their tx doesn't pay to them, or our tx doesn't pay to us... */
+	*otherside_only = !pays_to[side];
 
 	/* First two outputs done, now for the HTLCs. */
 	for (h = htlc_map_first(&peer->htlcs, &it);
 	     h;
 	     h = htlc_map_next(&peer->htlcs, &it)) {
+		const u8 *wscript;
+
 		if (!htlc_has(h, committed_flag))
 			continue;
-		add_output(tx, scriptpubkey_p2wsh(tx,
-						  wscript_for_htlc(tx, peer, h,
-								   rhash, side)),
-			   h->msatoshis / 1000, &total);
+		wscript = wscript_for_htlc(tx, peer, h, rhash, side);
+		/* If we pay any HTLC, it's txout is not just to other side. */
+		if (add_output(tx, scriptpubkey_p2wsh(tx, wscript),
+			       h->msatoshis / 1000, &total))
+			*otherside_only = false;
 	}
 	assert(total <= peer->anchor.satoshis);
 

daemon/commit_tx.h

@@ -34,5 +34,6 @@ struct bitcoin_tx *create_commit_tx(const tal_t *ctx,
 				    struct peer *peer,
 				    const struct sha256 *rhash,
 				    const struct channel_state *cstate,
-				    enum htlc_side side);
+				    enum htlc_side side,
+				    bool *otherside_only);
 #endif

daemon/packets.c

@@ -185,7 +185,11 @@ void queue_pkt_commit(struct peer *peer, const struct bitcoin_signature *sig)
 
 	/* Now send message */
 	update_commit__init(u);
-	u->sig = signature_to_proto(u, peer->dstate->secpctx, &sig->sig);
+	if (sig)
+		u->sig = signature_to_proto(u, peer->dstate->secpctx,
+					    &sig->sig);
+	else
+		u->sig = NULL;
 
 	queue_pkt(peer, PKT__PKT_UPDATE_COMMIT, u);
 }
@@ -479,6 +483,15 @@ Pkt *accept_pkt_commit(struct peer *peer, const Pkt *pkt,
 {
 	const UpdateCommit *c = pkt->update_commit;
 
+	if (!c->sig && sig)
+		return pkt_err(peer, "Expected signature");
+
+	if (!sig && c->sig)
+		return pkt_err(peer, "Unexpected signature");
+
+	if (!sig && !c->sig)
+		return NULL;
+
 	sig->stype = SIGHASH_ALL;
 	if (!proto_to_signature(peer->dstate->secpctx, c->sig, &sig->sig))
 		return pkt_err(peer, "Malformed signature");

daemon/peer.c

@@ -290,7 +290,7 @@ static void peer_breakdown(struct peer *peer)
 		log_unusual(peer->log, "Peer breakdown: sending close tx");
 		broadcast_tx(peer, bitcoin_close(peer));
 	/* If we have a signed commit tx (maybe not if we just offered
-	 * anchor, or they supplied anchor). */
+	 * anchor, or they supplied anchor, or no outputs to us). */
 	} else if (peer->local.commit->sig) {
 		log_unusual(peer->log, "Peer breakdown: sending commit tx");
 		broadcast_tx(peer, bitcoin_commit(peer));
@@ -769,6 +769,7 @@ static Pkt *handle_pkt_commit(struct peer *peer, const Pkt *pkt)
 	Pkt *err;
 	struct sha256 preimage;
 	struct commit_info *ci;
+	bool to_them_only;
 	/* FIXME: We can actually merge these two... */
 	static const struct state_table commit_changes[] = {
 		{ RCVD_ADD_REVOCATION, RCVD_ADD_ACK_COMMIT },
@@ -784,10 +785,6 @@ static Pkt *handle_pkt_commit(struct peer *peer, const Pkt *pkt)
 	};
 
 	ci = new_commit_info(peer, peer->local.commit->commit_num + 1);
-	ci->sig = tal(ci, struct bitcoin_signature);
-	err = accept_pkt_commit(peer, pkt, ci->sig);
-	if (err)
-		return err;
 
 	/* BOLT #2:
 	 *
@@ -808,21 +805,35 @@ static Pkt *handle_pkt_commit(struct peer *peer, const Pkt *pkt)
 	/* (We already applied them to staging_cstate as we went) */
 	ci->cstate = copy_cstate(ci, peer->local.staging_cstate);
 	ci->tx = create_commit_tx(ci, peer, &ci->revocation_hash,
-				  ci->cstate, LOCAL);
+				  ci->cstate, LOCAL, &to_them_only);
 	bitcoin_txid(ci->tx, &ci->txid);
 
+	/* BOLT #2:
+	 *
+	 * If the commitment transaction has only a single output which pays
+	 * to the other node, `sig` MUST be unset.  Otherwise, a sending node
+	 * MUST apply all remote acked and unacked changes except unacked fee
+	 * changes to the remote commitment before generating `sig`.
+	 */
+	if (!to_them_only)
+		ci->sig = tal(ci, struct bitcoin_signature);
+
+	err = accept_pkt_commit(peer, pkt, ci->sig);
+	if (err)
+		return err;
+
 	/* BOLT #2:
 	 *
 	 * A receiving node MUST apply all local acked and unacked changes
 	 * except unacked fee changes to the local commitment, then it MUST
 	 * check `sig` is valid for that transaction.
 	 */
-	if (!check_tx_sig(peer->dstate->secpctx,
-			  ci->tx, 0,
-			  NULL, 0,
-			  peer->anchor.witnessscript,
-			  &peer->remote.commitkey,
-			  ci->sig))
+	if (ci->sig && !check_tx_sig(peer->dstate->secpctx,
+				     ci->tx, 0,
+				     NULL, 0,
+				     peer->anchor.witnessscript,
+				     &peer->remote.commitkey,
+				     ci->sig))
 		return pkt_err(peer, "Bad signature");
 
 	/* Switch to the new commitment. */
@@ -835,7 +846,7 @@ static Pkt *handle_pkt_commit(struct peer *peer, const Pkt *pkt)
 	/* Now, send the revocation. */
 
 	/* We have their signature on the current one, right? */
-	assert(peer->local.commit->sig);
+	assert(to_them_only || peer->local.commit->sig);
 	assert(peer->local.commit->commit_num > 0);
 
 	if (!htlcs_changestate(peer, revocation_changes,
@@ -1666,13 +1677,6 @@ static void retransmit_pkts(struct peer *peer, s64 ack)
 	 */
 	while (ack < peer->order_counter) {
 		if (peer->remote.commit && ack == peer->remote.commit->order) {
-			if (!peer->remote.commit->sig) {
-				log_broken(peer->log, "No sig for commit order %"
-					   PRIu64, ack);
-				peer_comms_err(peer,
-					       pkt_err(peer, "invalid ack"));
-				return;
-			}
 			/* BOLT #2:
 			 *
 			 * Before retransmitting `update_commit`, the node
@@ -1783,6 +1787,7 @@ static void do_commit(struct peer *peer, struct command *jsoncmd)
 		{ SENT_ADD_REVOCATION, SENT_ADD_ACK_COMMIT},
 		{ SENT_REMOVE_HTLC, SENT_REMOVE_COMMIT}
 	};
+	bool to_us_only;
 
 	/* We can have changes we suggested, or changes they suggested. */
 	if (!peer_uncommitted_changes(peer)) {
@@ -1818,25 +1823,27 @@ static void do_commit(struct peer *peer, struct command *jsoncmd)
 	ci->revocation_hash = peer->remote.next_revocation_hash;
 	/* BOLT #2:
 	 *
-	 * A sending node MUST apply all remote acked and unacked
+	 * ...a sending node MUST apply all remote acked and unacked
 	 * changes except unacked fee changes to the remote commitment
 	 * before generating `sig`. */
 	ci->cstate = copy_cstate(ci, peer->remote.staging_cstate);
 	ci->tx = create_commit_tx(ci, peer, &ci->revocation_hash,
-				  ci->cstate, REMOTE);
+				  ci->cstate, REMOTE, &to_us_only);
 	bitcoin_txid(ci->tx, &ci->txid);
 
-	log_debug(peer->log, "Signing tx for %u/%u msatoshis, %u/%u htlcs (%u non-dust)",
-		  ci->cstate->side[OURS].pay_msat,
-		  ci->cstate->side[THEIRS].pay_msat,
-		  ci->cstate->side[OURS].num_htlcs,
-		  ci->cstate->side[THEIRS].num_htlcs,
-		  ci->cstate->num_nondust);
-	log_add_struct(peer->log, " (txid %s)", struct sha256_double, &ci->txid);
+	if (!to_us_only) {
+		log_debug(peer->log, "Signing tx for %u/%u msatoshis, %u/%u htlcs (%u non-dust)",
+			  ci->cstate->side[OURS].pay_msat,
+			  ci->cstate->side[THEIRS].pay_msat,
+			  ci->cstate->side[OURS].num_htlcs,
+			  ci->cstate->side[THEIRS].num_htlcs,
+			  ci->cstate->num_nondust);
+		log_add_struct(peer->log, " (txid %s)", struct sha256_double, &ci->txid);
 
-	ci->sig = tal(ci, struct bitcoin_signature);
-	ci->sig->stype = SIGHASH_ALL;
-	peer_sign_theircommit(peer, ci->tx, &ci->sig->sig);
+		ci->sig = tal(ci, struct bitcoin_signature);
+		ci->sig->stype = SIGHASH_ALL;
+		peer_sign_theircommit(peer, ci->tx, &ci->sig->sig);
+	}
 
 	/* Switch to the new commitment. */
 	tal_free(peer->remote.commit);
@@ -3523,6 +3530,8 @@ const struct bitcoin_tx *bitcoin_anchor(struct peer *peer)
  * insufficient funds. */
 bool setup_first_commit(struct peer *peer)
 {
+	bool to_them_only, to_us_only;
+
 	assert(!peer->local.commit->tx);
 	assert(!peer->remote.commit->tx);
 
@@ -3549,14 +3558,18 @@ bool setup_first_commit(struct peer *peer)
 						  peer,
 						  &peer->local.commit->revocation_hash,
 						  peer->local.commit->cstate,
-						  LOCAL);
+						  LOCAL, &to_them_only);
 	bitcoin_txid(peer->local.commit->tx, &peer->local.commit->txid);
 
 	peer->remote.commit->tx = create_commit_tx(peer->remote.commit,
 						   peer,
 						   &peer->remote.commit->revocation_hash,
 						   peer->remote.commit->cstate,
-						   REMOTE);
+						   REMOTE, &to_us_only);
+	assert(to_them_only != to_us_only);
+
+	/* If we offer anchor, their commit is to-us only. */
+	assert(to_us_only == (peer->local.offer_anchor == CMD_OPEN_WITH_ANCHOR));
 	bitcoin_txid(peer->remote.commit->tx, &peer->remote.commit->txid);
 
 	peer->local.staging_cstate = copy_cstate(peer, peer->local.commit->cstate);

lightning.pb-c.c

@@ -2270,7 +2270,7 @@ static const ProtobufCFieldDescriptor update_commit__field_descriptors[1] =
   {
     "sig",
     1,
-    PROTOBUF_C_LABEL_REQUIRED,
+    PROTOBUF_C_LABEL_OPTIONAL,
     PROTOBUF_C_TYPE_MESSAGE,
     0,   /* quantifier_offset */
     offsetof(UpdateCommit, sig),

lightning.pb-c.h

@@ -439,7 +439,7 @@ struct  _UpdateCommit
 {
   ProtobufCMessage base;
   /*
-   * Signature for your new commitment tx.
+   * Signature for your new commitment tx (if any outputs are HTLCs or to you)
    */
   Signature *sig;
 };

lightning.proto

@@ -180,8 +180,8 @@ message update_fail_htlc {
 
 // Commit all the staged changes.
 message update_commit {
-  // Signature for your new commitment tx.
-  required signature sig = 1;
+  // Signature for your new commitment tx (if any outputs are HTLCs or to you)
+  optional signature sig = 1;
 }
 
 // Complete the update.

test/test_protocol.c

@@ -648,7 +648,7 @@ static void send_commit(struct peer *peer)
 
 	/* BOLT #2:
 	 *
-	 * A sending node MUST apply all remote acked and unacked
+	 * ...a sending node MUST apply all remote acked and unacked
 	 * changes except unacked fee changes to the remote commitment
 	 * before generating `sig`.
 	 */