diff --git a/channeld/channeld.c b/channeld/channeld.c index a1a12e104750..4308b8c508f1 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1429,11 +1429,13 @@ static void start_commit_timer(struct peer *peer) send_commit_if_not_stfu, peer); } -/* If old_secret is NULL, we don't care, otherwise it is filled in. */ -static void get_per_commitment_point(u64 index, struct pubkey *point, - struct secret *old_secret) +/* Fetch the requested point. The secret is no longer returned, use + * revoke_commitment. It is leagal to call this on any commitment + * (including distant future). + */ +static void get_per_commitment_point(u64 index, struct pubkey *point) { - struct secret *s; + struct secret *unused; const u8 *msg; msg = hsm_req(tmpctx, @@ -1441,63 +1443,49 @@ static void get_per_commitment_point(u64 index, struct pubkey *point, if (!fromwire_hsmd_get_per_commitment_point_reply(tmpctx, msg, point, - &s)) + &unused)) status_failed(STATUS_FAIL_HSM_IO, "Bad per_commitment_point reply %s", tal_hex(tmpctx, msg)); +} - if (old_secret) { - if (!s) - status_failed(STATUS_FAIL_HSM_IO, - "No secret in per_commitment_point_reply %" - PRIu64, - index); - *old_secret = *s; - } +/* Revoke the specified commitment, the old secret is returned and + * next commitment point are returned. This call is idempotent, it is + * fine to re-revoke a previously revoked commitment. It is an error + * to revoke a commitment beyond the next revocable commitment. + */ +static void revoke_commitment(u64 index, struct secret *old_secret, struct pubkey *point) +{ + const u8 *msg; + + msg = hsm_req(tmpctx, + take(towire_hsmd_revoke_commitment_tx(tmpctx, index))); + + if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, old_secret, point)) + status_failed(STATUS_FAIL_HSM_IO, + "Reading revoke_commitment_tx reply: %s", + tal_hex(tmpctx, msg)); } /* revoke_index == current index - 1 (usually; not for retransmission) */ static u8 *make_revocation_msg(const struct peer *peer, u64 revoke_index, struct pubkey *point) { - const u8 *msg; struct secret old_commit_secret; /* Now that the master has persisted the new commitment advance the HSMD - * and fetch the revocation secret for the old one. */ - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) { - /* Prior to HSM_VERSION 5 we call get_per_commitment_point to - * get the old_secret and next point. - */ - get_per_commitment_point(revoke_index+2, point, &old_commit_secret); - } else { - /* After HSM_VERSION 5 we explicitly revoke the commitment in case - * the original revoke didn't complete. The hsmd_revoke_commitment_tx - * call is idempotent ... - */ - msg = towire_hsmd_revoke_commitment_tx(tmpctx, revoke_index); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_commit_secret, point)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); - } + * and fetch the revocation secret for the old one. + * + * After HSM_VERSION 5 we explicitly revoke the commitment in case + * the original revoke didn't complete. The hsmd_revoke_commitment_tx + * call is idempotent ... + */ + revoke_commitment(revoke_index, &old_commit_secret, point); return towire_revoke_and_ack(peer, &peer->channel_id, &old_commit_secret, point); } -static u8 *make_revocation_msg_from_secret(const struct peer *peer, - u64 revoke_index, - struct pubkey *point, - const struct secret *old_commit_secret, - const struct pubkey *next_point) -{ - *point = *next_point; - return towire_revoke_and_ack(peer, &peer->channel_id, - old_commit_secret, next_point); -} - /* Convert changed htlcs into parts which lightningd expects. */ static void marshall_htlc_info(const tal_t *ctx, const struct htlc **changed_htlcs, @@ -1564,8 +1552,6 @@ static void send_revocation(struct peer *peer, struct added_htlc *added; const u8 *msg; const u8 *msg_for_master; - struct secret old_secret2; - struct pubkey next_point2; /* Marshall it now before channel_sending_revoke_and_ack changes htlcs */ /* FIXME: Make infrastructure handle state post-revoke_and_ack! */ @@ -1608,24 +1594,8 @@ static void send_revocation(struct peer *peer, /* Now that the master has persisted the new commitment advance the HSMD * and fetch the revocation secret for the old one. */ - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) { - /* Prior to HSM_VERSION 5 we use the old_secret - * received earlier from validate_commitment_tx. */ - old_secret2 = *old_secret; - next_point2 = *next_point; - } else { - msg = towire_hsmd_revoke_commitment_tx(tmpctx, peer->next_index[LOCAL] - 2); - msg = hsm_req(tmpctx, take(msg)); - if (!fromwire_hsmd_revoke_commitment_tx_reply(msg, &old_secret2, &next_point2)) - status_failed(STATUS_FAIL_HSM_IO, - "Reading revoke_commitment_tx reply: %s", - tal_hex(tmpctx, msg)); - } - - /* Revoke previous commit, get new point. */ - msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-2, - &peer->next_local_per_commit, - &old_secret2, &next_point2); + msg = make_revocation_msg(peer, peer->next_index[LOCAL]-2, + &peer->next_local_per_commit); /* Now we can finally send revoke_and_ack to peer */ peer_write(peer->pps, take(msg)); @@ -2061,7 +2031,7 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, "error"); } - /* Validate the counterparty's signatures, returns prior per_commitment_secret. */ + /* As of HSM_VERSION 5 returned old_secret is always NULL (revoke returns it instead) */ htlcs = collect_htlcs(NULL, htlc_map); msg2 = towire_hsmd_validate_commitment_tx(NULL, txs[0], @@ -2123,10 +2093,8 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, tal_steal(commitsigs, result); } - // If the HSM doesn't support WIRE_HSMD_REVOKE_COMMITMENT_TX we'd better - // have the old_secret at this point. - if (!hsm_is_capable(peer->hsm_capabilities, WIRE_HSMD_REVOKE_COMMITMENT_TX)) - assert(old_secret); + /* After HSM_VERSION 5 old_secret is always NULL */ + assert(!old_secret); send_revocation(peer, &commit_sig, htlc_sigs, changed_htlcs, txs[0], old_secret, &next_point, commitsigs); @@ -2778,7 +2746,7 @@ static struct commitsig *interactive_send_commitments(struct peer *peer, /* Funding counts as 0th commit so we do inflight_index + 1 */ if (fromwire_peektype(msg) == WIRE_COMMITMENT_SIGNED) { get_per_commitment_point(next_index_local - 1, - &my_current_per_commitment_point, NULL); + &my_current_per_commitment_point); result = handle_peer_commit_sig(peer, msg, inflight_index + 1, @@ -4672,10 +4640,7 @@ static void check_current_dataloss_fields(struct peer *peer, memset(&old_commit_secret, 0, sizeof(old_commit_secret)); else { struct pubkey unused; - /* This gets previous revocation number, since asking for - * commitment point N gives secret for N-2 */ - get_per_commitment_point(next_revocation_number+1, - &unused, &old_commit_secret); + revoke_commitment(next_revocation_number - 1, &old_commit_secret, &unused); } if (!secret_eq_consttime(&old_commit_secret, @@ -4810,7 +4775,7 @@ static void peer_reconnect(struct peer *peer, /* Our current per-commitment point is the commitment point in the last * received signed commitment */ get_per_commitment_point(peer->next_index[LOCAL] - 1, - &my_current_per_commitment_point, NULL); + &my_current_per_commitment_point); send_tlvs = NULL; @@ -5419,7 +5384,7 @@ static void handle_funding_depth(struct peer *peer, const u8 *msg) /* Need to retrieve the first point again, even if we * moved on, as channel_ready explicitly includes the * first one. */ - get_per_commitment_point(1, &point, NULL); + get_per_commitment_point(1, &point); msg = towire_channel_ready(NULL, &peer->channel_id, &point, tlvs); @@ -6006,7 +5971,7 @@ static void init_channel(struct peer *peer) assert(peer->next_index[REMOTE] > 0); get_per_commitment_point(peer->next_index[LOCAL], - &peer->next_local_per_commit, NULL); + &peer->next_local_per_commit); peer->channel = new_full_channel(peer, &peer->channel_id, &funding, diff --git a/common/hsm_version.h b/common/hsm_version.h index 7d3f8fb80da2..8464c1a78045 100644 --- a/common/hsm_version.h +++ b/common/hsm_version.h @@ -23,7 +23,8 @@ * v5 with hsmd_revoke_commitment_tx: 5742538f87ef5d5bf55b66dc19e52c8683cfeb1b887d3e64ba530ba9a4d8e638 * v5 with sign_any_cannouncement: 5fdb9068c43a21887dc03f7dce410d2e3eeff6277f0d49b4fc56595a798fd4a4 * v5 drop init v2: 5024454532fe5a78bb7558000cb344190888b9915360d3d56ddca22eaba9b872 + * v6 no secret from get_per_commitment_point: 5024454532fe5a78bb7558000cb344190888b9915360d3d56ddca22eaba9b872 */ #define HSM_MIN_VERSION 5 -#define HSM_MAX_VERSION 5 +#define HSM_MAX_VERSION 6 #endif /* LIGHTNING_COMMON_HSM_VERSION_H */ diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 41150da75984..d732d93e3ba5 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -436,7 +436,7 @@ static struct io_plan *init_hsm(struct io_conn *conn, struct secret *hsm_encryption_key; struct bip32_key_version bip32_key_version; u32 minversion, maxversion; - const u32 our_minversion = 4, our_maxversion = 5; + const u32 our_minversion = 4, our_maxversion = 6; /* This must be lightningd. */ assert(is_lightningd(c)); @@ -490,8 +490,8 @@ static struct io_plan *init_hsm(struct io_conn *conn, discard_key(take(hsm_encryption_key)); /* Define the minimum common max version for the hsmd one */ - u64 mutual_version = maxversion < our_maxversion ? maxversion : our_maxversion; - return req_reply(conn, c, hsmd_init(hsm_secret, mutual_version, + hsmd_mutual_version = maxversion < our_maxversion ? maxversion : our_maxversion; + return req_reply(conn, c, hsmd_init(hsm_secret, hsmd_mutual_version, bip32_key_version)); } diff --git a/hsmd/hsmd_wire.csv b/hsmd/hsmd_wire.csv index cd75835622c3..6ad2ae0add0a 100644 --- a/hsmd/hsmd_wire.csv +++ b/hsmd/hsmd_wire.csv @@ -294,10 +294,12 @@ msgdata,hsmd_sign_splice_tx,input_index,u32, msgtype,hsmd_sign_tx_reply,112 msgdata,hsmd_sign_tx_reply,sig,bitcoin_signature, -# Openingd/channeld/onchaind asks for Nth per_commitment_point, if > 2, gets N-2 secret. +# Openingd/channeld/onchaind asks for Nth per_commitment_point +# Prior to HSM_VERSION 6 we will return an old_commitment_secret msgtype,hsmd_get_per_commitment_point,18 msgdata,hsmd_get_per_commitment_point,n,u64, +# IMPORTANT - Beginning HSM_VERSION 6 we never return an old_commitment_secret msgtype,hsmd_get_per_commitment_point_reply,118 msgdata,hsmd_get_per_commitment_point_reply,per_commitment_point,pubkey, msgdata,hsmd_get_per_commitment_point_reply,old_commitment_secret,?secret, diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 3dfcf183876a..ac79380dd45c 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -16,6 +16,9 @@ #include #include +/* The negotiated protocol version ends up in here. */ +u64 hsmd_mutual_version; + /* If they specify --dev-force-privkey it ends up in here. */ struct privkey *dev_force_privkey; /* If they specify --dev-force-bip32-seed it ends up in here. */ @@ -1185,7 +1188,7 @@ static u8 *handle_get_per_commitment_point(struct hsmd_client *c, const u8 *msg_ return hsmd_status_bad_request_fmt( c, msg_in, "bad per_commit_point %" PRIu64, n); - if (n >= 2) { + if (hsmd_mutual_version < 6 && n >= 2) { old_secret = tal(tmpctx, struct secret); if (!per_commit_secret(&shaseed, old_secret, n - 2)) { return hsmd_status_bad_request_fmt( diff --git a/hsmd/libhsmd.h b/hsmd/libhsmd.h index 756c6c2f526a..1c7e6698ead1 100644 --- a/hsmd/libhsmd.h +++ b/hsmd/libhsmd.h @@ -89,6 +89,9 @@ void hsmd_status_failed(enum status_failreason code, bool hsmd_check_client_capabilities(struct hsmd_client *client, enum hsmd_wire t); +/* The negotiated protocol version ends up in here. */ +extern u64 hsmd_mutual_version; + /* If they specify --dev-force-privkey it ends up in here. */ extern struct privkey *dev_force_privkey; /* If they specify --dev-force-bip32-seed it ends up in here. */