From 8a3c9908ce1025620466f598b0ac896042b94bb2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 18 Aug 2016 14:25:13 +0930 Subject: [PATCH] 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 --- daemon/commit_tx.c | 35 +++++++++++++------ daemon/commit_tx.h | 3 +- daemon/packets.c | 15 +++++++- daemon/peer.c | 81 +++++++++++++++++++++++++------------------- lightning.pb-c.c | 2 +- lightning.pb-c.h | 2 +- lightning.proto | 4 +-- test/test_protocol.c | 2 +- 8 files changed, 92 insertions(+), 52 deletions(-) diff --git a/daemon/commit_tx.c b/daemon/commit_tx.c index 76d6c591f..1816f0783 100644 --- a/daemon/commit_tx.c +++ b/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); diff --git a/daemon/commit_tx.h b/daemon/commit_tx.h index 3101ce968..dd0cdb1f2 100644 --- a/daemon/commit_tx.h +++ b/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 diff --git a/daemon/packets.c b/daemon/packets.c index 3bcea2822..a6194142f 100644 --- a/daemon/packets.c +++ b/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"); diff --git a/daemon/peer.c b/daemon/peer.c index b6a553070..6d33c7d02 100644 --- a/daemon/peer.c +++ b/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); diff --git a/lightning.pb-c.c b/lightning.pb-c.c index 29ffedd53..a30658e87 100644 --- a/lightning.pb-c.c +++ b/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), diff --git a/lightning.pb-c.h b/lightning.pb-c.h index dd617a8af..06732c67a 100644 --- a/lightning.pb-c.h +++ b/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; }; diff --git a/lightning.proto b/lightning.proto index 9373dd2a6..4e8d2909c 100644 --- a/lightning.proto +++ b/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. diff --git a/test/test_protocol.c b/test/test_protocol.c index 247de4500..c41cbd62f 100644 --- a/test/test_protocol.c +++ b/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`. */