From 0f35441a29ed20a21f37ee813b993be48a22ef09 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Apr 2016 16:31:43 +0930 Subject: [PATCH] protocol: move `ack` out of header into specific packets. This reflects the BOLT #1/#2 protocol change, as suggeted by Pierre. Signed-off-by: Rusty Russell --- daemon/cryptopkt.c | 93 +++++++++++++++++++++++++--------------------- daemon/cryptopkt.h | 7 ++++ daemon/packets.c | 3 ++ daemon/peer.c | 11 +++++- lightning.pb-c.c | 58 ++++++++++++++++++++++++----- lightning.pb-c.h | 19 ++++++++-- lightning.proto | 6 +++ 7 files changed, 141 insertions(+), 56 deletions(-) diff --git a/daemon/cryptopkt.c b/daemon/cryptopkt.c index 401f8ccf8bfc..e039dd50ebc9 100644 --- a/daemon/cryptopkt.c +++ b/daemon/cryptopkt.c @@ -22,15 +22,10 @@ #define MAX_PKT_LEN (1024 * 1024) /* BOLT#1: - The header consists of the following fields in order: - - * `acknowledge`: an 8-byte little-endian field indicating the number of non-`authenticate` messages received and processed so far. - * `length`: a 4-byte little-endian field indicating the size of the unencrypted body. + `length` is a 4-byte little-endian field indicating the size of the unencrypted body. */ -/* Do NOT take sizeof() this, since there's extra padding at the end! */ struct crypto_pkt { - le64 acknowledge; le32 length; u8 auth_tag[crypto_aead_chacha20poly1305_ABYTES]; @@ -38,10 +33,6 @@ struct crypto_pkt { u8 data[]; }; -/* Use this instead of sizeof(struct crypto_pkt) */ -#define CRYPTO_HDR_LEN_NOTAG (8 + 4) -#define CRYPTO_HDR_LEN (CRYPTO_HDR_LEN_NOTAG + crypto_aead_chacha20poly1305_ABYTES) - /* Temporary structure for negotiation (peer->io_data->neg) */ struct key_negotiate { /* Our session secret key. */ @@ -113,7 +104,7 @@ struct io_data { /* Stuff we need to keep around to talk to peer. */ struct dir_state in, out; - /* Header we're currently reading. */ + /* Length we're currently reading. */ struct crypto_pkt hdr_in; /* Callback once packet decrypted. */ @@ -221,7 +212,7 @@ static Pkt *decrypt_pkt(struct peer *peer, struct crypto_pkt *cpkt, return ret; } -static struct crypto_pkt *encrypt_pkt(struct peer *peer, const Pkt *pkt, u64 ack, +static struct crypto_pkt *encrypt_pkt(struct peer *peer, const Pkt *pkt, size_t *totlen) { struct crypto_pkt *cpkt; @@ -229,14 +220,13 @@ static struct crypto_pkt *encrypt_pkt(struct peer *peer, const Pkt *pkt, u64 ack struct io_data *iod = peer->io_data; len = pkt__get_packed_size(pkt); - *totlen = CRYPTO_HDR_LEN + len + crypto_aead_chacha20poly1305_ABYTES; + *totlen = sizeof(*cpkt) + len + crypto_aead_chacha20poly1305_ABYTES; cpkt = (struct crypto_pkt *)tal_arr(peer, char, *totlen); - cpkt->acknowledge = cpu_to_le64(ack); cpkt->length = cpu_to_le32(len); /* Encrypt header. */ - encrypt_in_place(cpkt, CRYPTO_HDR_LEN_NOTAG, + encrypt_in_place(cpkt, sizeof(cpkt->length), &iod->out.nonce, &iod->out.enckey); /* Encrypt body. */ @@ -246,11 +236,29 @@ static struct crypto_pkt *encrypt_pkt(struct peer *peer, const Pkt *pkt, u64 ack return cpkt; } -static struct io_plan *decrypt_body(struct io_conn *conn, struct peer *peer) +void peer_process_acks(struct peer *peer, uint64_t acknum) { struct io_data *iod = peer->io_data; struct ack *ack; + while ((ack = list_top(&iod->acks, struct ack, list)) != NULL) { + if (acknum < ack->pktnum) + break; + ack->ack_cb(peer, ack->ack_arg); + list_del_from(&iod->acks, &ack->list); + tal_free(ack); + } +} + +uint64_t peer_outgoing_ack(const struct peer *peer) +{ + return peer->io_data->in.count; +} + +static struct io_plan *decrypt_body(struct io_conn *conn, struct peer *peer) +{ + struct io_data *iod = peer->io_data; + /* We have full packet. */ peer->inpkt = decrypt_pkt(peer, iod->in.cpkt, le32_to_cpu(iod->hdr_in.length)); @@ -261,21 +269,11 @@ static struct io_plan *decrypt_body(struct io_conn *conn, struct peer *peer) if (peer->inpkt->pkt_case != PKT__PKT_AUTH) iod->in.count++; - log_debug(peer->log, "Received packet ACK=%"PRIu64" LEN=%u, type=%s", - le64_to_cpu(iod->hdr_in.acknowledge), + log_debug(peer->log, "Received packet LEN=%u, type=%s", le32_to_cpu(iod->hdr_in.length), peer->inpkt->pkt_case == PKT__PKT_AUTH ? "PKT_AUTH" : input_name(peer->inpkt->pkt_case)); - /* Do callbacks for any packets it acknowledged receiving. */ - while ((ack = list_top(&iod->acks, struct ack, list)) != NULL) { - if (le64_to_cpu(iod->hdr_in.acknowledge) < ack->pktnum) - break; - ack->ack_cb(peer, ack->ack_arg); - list_del_from(&iod->acks, &ack->list); - tal_free(ack); - } - return iod->cb(conn, peer); } @@ -284,8 +282,8 @@ static struct io_plan *decrypt_header(struct io_conn *conn, struct peer *peer) struct io_data *iod = peer->io_data; size_t body_len; - /* We have header: Check it. */ - if (!decrypt_in_place(&iod->hdr_in, CRYPTO_HDR_LEN_NOTAG, + /* We have length: Check it. */ + if (!decrypt_in_place(&iod->hdr_in.length, sizeof(iod->hdr_in.length), &iod->in.nonce, &iod->in.enckey)) { log_unusual(peer->log, "Header decryption failed"); return io_close(conn); @@ -305,9 +303,9 @@ static struct io_plan *decrypt_header(struct io_conn *conn, struct peer *peer) body_len = le32_to_cpu(iod->hdr_in.length) + crypto_aead_chacha20poly1305_ABYTES; - iod->in.cpkt = (struct crypto_pkt *)tal_arr(peer, char, - CRYPTO_HDR_LEN + body_len); - memcpy(iod->in.cpkt, &iod->hdr_in, CRYPTO_HDR_LEN); + iod->in.cpkt = (struct crypto_pkt *) + tal_arr(peer, char, sizeof(iod->hdr_in) + body_len); + *iod->in.cpkt = iod->hdr_in; return io_read(conn, iod->in.cpkt->data, body_len, decrypt_body, peer); } @@ -320,7 +318,7 @@ struct io_plan *peer_read_packet(struct io_conn *conn, struct io_data *iod = peer->io_data; iod->cb = cb; - return io_read(conn, &iod->hdr_in, CRYPTO_HDR_LEN, + return io_read(conn, &iod->hdr_in, sizeof(iod->hdr_in), decrypt_header, peer); } @@ -340,7 +338,7 @@ struct io_plan *peer_write_packet_(struct io_conn *conn, * via io_write */ tal_free(iod->out.cpkt); - iod->out.cpkt = encrypt_pkt(peer, pkt, peer->io_data->in.count, &totlen); + iod->out.cpkt = encrypt_pkt(peer, pkt, &totlen); /* Set up ack callback if any. */ if (ack_cb) { @@ -417,24 +415,26 @@ static struct io_plan *check_proof(struct io_conn *conn, struct peer *peer) return io_close(conn); } - tal_free(auth); - /* Auth messages don't add to count. */ assert(peer->io_data->in.count == 0); /* BOLT #1: - * The receiver MUST NOT examine the `acknowledge` value until - * after the authentication fields have been successfully - * validated. The `acknowledge` field MUST BE set to the - * number of non-authenticate messages received and processed. + * + * The receiver MUST NOT examine the `ack` value until after + * the authentication fields have been successfully validated. + * The `ack` field MUST BE set to the number of + * non-authenticate messages received and processed if + * non-zero. */ /* FIXME: Handle reconnects. */ - if (le64_to_cpu(peer->io_data->hdr_in.acknowledge) != 0) { + if (auth->ack != 0) { log_unusual(peer->log, "FIXME: non-zero acknowledge %"PRIu64, - le64_to_cpu(peer->io_data->hdr_in.acknowledge)); + auth->ack); return io_close(conn); } + tal_free(auth); + /* All complete, return to caller. */ cb = neg->cb; peer->io_data->neg = tal_free(neg); @@ -524,6 +524,8 @@ static struct io_plan *discard_extra(struct io_conn *conn, struct peer *peer) len -= sizeof(neg->their_sessionpubkey); discard = tal_arr(neg, char, len); + log_unusual(peer->log, "Ignoring %zu extra handshake bytes", + len); return io_read(conn, discard, len, keys_exchanged, peer); } @@ -592,7 +594,12 @@ struct io_plan *peer_crypto_setup(struct io_conn *conn, struct peer *peer, secp256k1_pubkey sessionkey; struct key_negotiate *neg; - BUILD_ASSERT(CRYPTO_HDR_LEN == offsetof(struct crypto_pkt, data)); + /* BOLT #1: + * + * The 4-byte length for each message is encrypted separately + * (resulting in a 20 byte header when the authentication tag + * is appended) */ + BUILD_ASSERT(sizeof(struct crypto_pkt) == 20); peer->io_data = tal(peer, struct io_data); list_head_init(&peer->io_data->acks); diff --git a/daemon/cryptopkt.h b/daemon/cryptopkt.h index 82a231c1daf4..0f4989cbb207 100644 --- a/daemon/cryptopkt.h +++ b/daemon/cryptopkt.h @@ -32,4 +32,11 @@ struct io_plan *peer_write_packet_(struct io_conn *conn, (ack_cb), (ack_arg), \ struct peer *), \ (ack_arg), (next)) + +/* Acknowledgements are contained in some messages: caller must call this */ +void peer_process_acks(struct peer *peer, uint64_t ack); + +/* Ack counter for outgoing packets. */ +uint64_t peer_outgoing_ack(const struct peer *peer); + #endif /* LIGHTNING_DAEMON_CRYPTOPKT_H */ diff --git a/daemon/packets.c b/daemon/packets.c index 3acc51ecc1b1..230d4076db98 100644 --- a/daemon/packets.c +++ b/daemon/packets.c @@ -3,6 +3,7 @@ #include "close_tx.h" #include "commit_tx.h" #include "controlled_time.h" +#include "cryptopkt.h" #include "find_p2sh_out.h" #include "lightningd.h" #include "log.h" @@ -333,6 +334,7 @@ void queue_pkt_commit(struct peer *peer) /* Now send message */ update_commit__init(u); u->sig = signature_to_proto(u, &ci->sig->sig); + u->ack = peer_outgoing_ack(peer); queue_pkt(peer, PKT__PKT_UPDATE_COMMIT, u); } @@ -362,6 +364,7 @@ void queue_pkt_revocation(struct peer *peer) u->next_revocation_hash = sha256_to_proto(u, &peer->us.next_revocation_hash); + u->ack = peer_outgoing_ack(peer); queue_pkt(peer, PKT__PKT_UPDATE_REVOCATION, u); } diff --git a/daemon/peer.c b/daemon/peer.c index e4abebef32f5..b96584fa992a 100644 --- a/daemon/peer.c +++ b/daemon/peer.c @@ -280,8 +280,17 @@ static struct io_plan *pkt_in(struct io_conn *conn, struct peer *peer) idata.pkt = tal_steal(ctx, peer->inpkt); /* We ignore packets if they tell us to. */ - if (peer->cond != PEER_CLOSED) + if (peer->cond != PEER_CLOSED) { + /* These two packets contain acknowledgements. */ + if (idata.pkt->pkt_case == PKT__PKT_UPDATE_COMMIT) + peer_process_acks(peer, + idata.pkt->update_commit->ack); + else if (idata.pkt->pkt_case == PKT__PKT_UPDATE_REVOCATION) + peer_process_acks(peer, + idata.pkt->update_revocation->ack); + state_event(peer, peer->inpkt->pkt_case, &idata); + } /* Free peer->inpkt unless stolen above. */ tal_free(ctx); diff --git a/lightning.pb-c.c b/lightning.pb-c.c index 316e63332803..54f6b4ccff18 100644 --- a/lightning.pb-c.c +++ b/lightning.pb-c.c @@ -1258,7 +1258,8 @@ const ProtobufCMessageDescriptor funding__descriptor = (ProtobufCMessageInit) funding__init, NULL,NULL,NULL /* reserved[123] */ }; -static const ProtobufCFieldDescriptor authenticate__field_descriptors[2] = +static const uint64_t authenticate__ack__default_value = 0ull; +static const ProtobufCFieldDescriptor authenticate__field_descriptors[3] = { { "node_id", @@ -1284,15 +1285,28 @@ static const ProtobufCFieldDescriptor authenticate__field_descriptors[2] = 0, /* flags */ 0,NULL,NULL /* reserved1,reserved2, etc */ }, + { + "ack", + 3, + PROTOBUF_C_LABEL_OPTIONAL, + PROTOBUF_C_TYPE_UINT64, + offsetof(Authenticate, has_ack), + offsetof(Authenticate, ack), + NULL, + &authenticate__ack__default_value, + 0, /* flags */ + 0,NULL,NULL /* reserved1,reserved2, etc */ + }, }; static const unsigned authenticate__field_indices_by_name[] = { + 2, /* field[2] = ack */ 0, /* field[0] = node_id */ 1, /* field[1] = session_sig */ }; static const ProtobufCIntRange authenticate__number_ranges[1 + 1] = { { 1, 0 }, - { 0, 2 } + { 0, 3 } }; const ProtobufCMessageDescriptor authenticate__descriptor = { @@ -1302,7 +1316,7 @@ const ProtobufCMessageDescriptor authenticate__descriptor = "Authenticate", "", sizeof(Authenticate), - 2, + 3, authenticate__field_descriptors, authenticate__field_indices_by_name, 1, authenticate__number_ranges, @@ -1888,7 +1902,7 @@ const ProtobufCMessageDescriptor update_fail_htlc__descriptor = (ProtobufCMessageInit) update_fail_htlc__init, NULL,NULL,NULL /* reserved[123] */ }; -static const ProtobufCFieldDescriptor update_commit__field_descriptors[1] = +static const ProtobufCFieldDescriptor update_commit__field_descriptors[2] = { { "sig", @@ -1902,14 +1916,27 @@ static const ProtobufCFieldDescriptor update_commit__field_descriptors[1] = 0, /* flags */ 0,NULL,NULL /* reserved1,reserved2, etc */ }, + { + "ack", + 2, + PROTOBUF_C_LABEL_REQUIRED, + PROTOBUF_C_TYPE_UINT64, + 0, /* quantifier_offset */ + offsetof(UpdateCommit, ack), + NULL, + NULL, + 0, /* flags */ + 0,NULL,NULL /* reserved1,reserved2, etc */ + }, }; static const unsigned update_commit__field_indices_by_name[] = { + 1, /* field[1] = ack */ 0, /* field[0] = sig */ }; static const ProtobufCIntRange update_commit__number_ranges[1 + 1] = { { 1, 0 }, - { 0, 1 } + { 0, 2 } }; const ProtobufCMessageDescriptor update_commit__descriptor = { @@ -1919,14 +1946,14 @@ const ProtobufCMessageDescriptor update_commit__descriptor = "UpdateCommit", "", sizeof(UpdateCommit), - 1, + 2, update_commit__field_descriptors, update_commit__field_indices_by_name, 1, update_commit__number_ranges, (ProtobufCMessageInit) update_commit__init, NULL,NULL,NULL /* reserved[123] */ }; -static const ProtobufCFieldDescriptor update_revocation__field_descriptors[2] = +static const ProtobufCFieldDescriptor update_revocation__field_descriptors[3] = { { "revocation_preimage", @@ -1952,15 +1979,28 @@ static const ProtobufCFieldDescriptor update_revocation__field_descriptors[2] = 0, /* flags */ 0,NULL,NULL /* reserved1,reserved2, etc */ }, + { + "ack", + 3, + PROTOBUF_C_LABEL_REQUIRED, + PROTOBUF_C_TYPE_UINT64, + 0, /* quantifier_offset */ + offsetof(UpdateRevocation, ack), + NULL, + NULL, + 0, /* flags */ + 0,NULL,NULL /* reserved1,reserved2, etc */ + }, }; static const unsigned update_revocation__field_indices_by_name[] = { + 2, /* field[2] = ack */ 1, /* field[1] = next_revocation_hash */ 0, /* field[0] = revocation_preimage */ }; static const ProtobufCIntRange update_revocation__number_ranges[1 + 1] = { { 1, 0 }, - { 0, 2 } + { 0, 3 } }; const ProtobufCMessageDescriptor update_revocation__descriptor = { @@ -1970,7 +2010,7 @@ const ProtobufCMessageDescriptor update_revocation__descriptor = "UpdateRevocation", "", sizeof(UpdateRevocation), - 2, + 3, update_revocation__field_descriptors, update_revocation__field_indices_by_name, 1, update_revocation__number_ranges, diff --git a/lightning.pb-c.h b/lightning.pb-c.h index b0b9da2cd922..58d8b9d3f77d 100644 --- a/lightning.pb-c.h +++ b/lightning.pb-c.h @@ -159,10 +159,15 @@ struct _Authenticate * Signature of your session key. * */ Signature *session_sig; + /* + * How many (non-authenticate) packets we've already received + */ + protobuf_c_boolean has_ack; + uint64_t ack; }; #define AUTHENTICATE__INIT \ { PROTOBUF_C_MESSAGE_INIT (&authenticate__descriptor) \ - , NULL, NULL } + , NULL, NULL, 0,0ull } /* @@ -372,10 +377,14 @@ struct _UpdateCommit * Signature for your new commitment tx. */ Signature *sig; + /* + * How many (non-authenticate) packets we've already received + */ + uint64_t ack; }; #define UPDATE_COMMIT__INIT \ { PROTOBUF_C_MESSAGE_INIT (&update_commit__descriptor) \ - , NULL } + , NULL, 0 } /* @@ -392,10 +401,14 @@ struct _UpdateRevocation * Revocation hash for my next commit transaction */ Sha256Hash *next_revocation_hash; + /* + * How many (non-authenticate) packets we've already received + */ + uint64_t ack; }; #define UPDATE_REVOCATION__INIT \ { PROTOBUF_C_MESSAGE_INIT (&update_revocation__descriptor) \ - , NULL, NULL } + , NULL, NULL, 0 } /* diff --git a/lightning.proto b/lightning.proto index a904338b8f4c..99c63cae0bcd 100644 --- a/lightning.proto +++ b/lightning.proto @@ -57,6 +57,8 @@ message authenticate { required bitcoin_pubkey node_id = 1; // Signature of your session key. */ required signature session_sig = 2; + // How many (non-authenticate) packets we've already received + optional uint64 ack = 3 [ default = 0 ]; }; // Set channel params. @@ -155,6 +157,8 @@ message update_fail_htlc { message update_commit { // Signature for your new commitment tx. required signature sig = 1; + // How many (non-authenticate) packets we've already received + required uint64 ack = 2; } // Complete the update. @@ -163,6 +167,8 @@ message update_revocation { required sha256_hash revocation_preimage = 1; // Revocation hash for my next commit transaction required sha256_hash next_revocation_hash = 2; + // How many (non-authenticate) packets we've already received + required uint64 ack = 3; } // Start clearing out the channel HTLCs so we can close it