From 0c38653272150524aebb4776ef77eb299220c922 Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Fri, 12 Jan 2024 16:27:47 -0800 Subject: [PATCH 1/5] tests: augment test_onchain_their_unilateral_out to check to_remote witness generation Changelog-Added: added a withdraw all to the end of test_onchain_their_unilateral_out to ensure that the unilateral close info is correct with anchors. Tests https://github.com/Blockstream/greenlight/issues/348 --- tests/test_closing.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_closing.py b/tests/test_closing.py index 3a5c9f1453b9..764cf816531d 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -2438,6 +2438,8 @@ def try_pay(): assert acc['account_resolved'] assert acc['resolved_at_block'] > 0 + # Have l1 send all funds to check that the unilateral close info is correct + l1.rpc.withdraw(l1.rpc.newaddr('bech32')['bech32'], 'all', minconf=0) def test_listfunds_after_their_unilateral(node_factory, bitcoind): """We keep spending info around for their unilateral closes. From ba68ebab2fcbcd1ba7db30ebf15aaf1b0e0ba9c4 Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Thu, 18 Jan 2024 13:19:55 -0800 Subject: [PATCH 2/5] channeld: add hsm_capabilities and add hsm_is_capable to common Changelog-Added: Added hsm_capabilities and hsm_is_capable to channeld. --- channeld/channeld.c | 4 ++++ channeld/channeld_wire.csv | 2 ++ common/Makefile | 1 + common/hsm_capable.c | 13 +++++++++++++ common/hsm_capable.h | 11 +++++++++++ lightningd/Makefile | 1 + lightningd/channel_control.c | 3 +++ lightningd/hsm_control.c | 7 ++----- wallet/test/Makefile | 1 + 9 files changed, 38 insertions(+), 5 deletions(-) create mode 100644 common/hsm_capable.c create mode 100644 common/hsm_capable.h diff --git a/channeld/channeld.c b/channeld/channeld.c index a4ea9219c744..1758f82e4839 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -77,6 +77,9 @@ struct peer { /* Features we support. */ struct feature_set *our_features; + /* What (additional) messages the HSM accepts */ + u32 *hsm_capabilities; + /* Tolerable amounts for feerate (only relevant for fundee). */ u32 feerate_min, feerate_max; @@ -6085,6 +6088,7 @@ static void init_channel(struct peer *peer) if (!fromwire_channeld_init(peer, msg, &chainparams, &peer->our_features, + &peer->hsm_capabilities, &peer->channel_id, &funding, &funding_sats, diff --git a/channeld/channeld_wire.csv b/channeld/channeld_wire.csv index 53590a2e6ee6..fcc76b5ff8a0 100644 --- a/channeld/channeld_wire.csv +++ b/channeld/channeld_wire.csv @@ -15,6 +15,8 @@ msgtype,channeld_init,1000 msgdata,channeld_init,chainparams,chainparams, msgdata,channeld_init,our_features,feature_set, +msgdata,channeld_init,num_hsm_capabilities,u16, +msgdata,channeld_init,hsm_capabilities,u32,num_hsm_capabilities msgdata,channeld_init,channel_id,channel_id, msgdata,channeld_init,funding,bitcoin_outpoint, msgdata,channeld_init,funding_satoshi,amount_sat, diff --git a/common/Makefile b/common/Makefile index 3e6a5e167bc0..4a1afedc36ef 100644 --- a/common/Makefile +++ b/common/Makefile @@ -41,6 +41,7 @@ COMMON_SRC_NOGEN := \ common/gossmap.c \ common/hash_u5.c \ common/hmac.c \ + common/hsm_capable.c \ common/hsm_encryption.c \ common/htlc_state.c \ common/htlc_trim.c \ diff --git a/common/hsm_capable.c b/common/hsm_capable.c new file mode 100644 index 000000000000..3b4e38e8d402 --- /dev/null +++ b/common/hsm_capable.c @@ -0,0 +1,13 @@ +#include "config.h" +#include + +/* Is this capability supported by the HSM? (So far, always a message + * number) */ +bool hsm_is_capable(u32 *capabilities, u32 msgtype) +{ + for (size_t i = 0; i < tal_count(capabilities); i++) { + if (capabilities[i] == msgtype) + return true; + } + return false; +} diff --git a/common/hsm_capable.h b/common/hsm_capable.h new file mode 100644 index 000000000000..a1af0cfac6ea --- /dev/null +++ b/common/hsm_capable.h @@ -0,0 +1,11 @@ +#ifndef LIGHTNING_COMMON_HSM_CAPABLE_H +#define LIGHTNING_COMMON_HSM_CAPABLE_H +#include "config.h" +#include +#include +#include + +/* Is this capability supported by the HSM? (So far, always a message + * number) */ +bool hsm_is_capable(u32 *capabilities, u32 msgtype); +#endif /* LIGHTNING_COMMON_HSM_CAPABLE_H */ diff --git a/lightningd/Makefile b/lightningd/Makefile index db354b199e42..5ff34e724cf3 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -103,6 +103,7 @@ LIGHTNINGD_COMMON_OBJS := \ common/status_wiregen.o \ common/hash_u5.o \ common/hmac.o \ + common/hsm_capable.o \ common/hsm_encryption.o \ common/htlc_state.o \ common/htlc_trim.o \ diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 7018e63457aa..c2c15dbd2c05 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -1626,6 +1626,9 @@ bool peer_start_channeld(struct channel *channel, initmsg = towire_channeld_init(tmpctx, chainparams, ld->our_features, + /* Capabilities arg needs to be a tal array */ + tal_dup_arr(tmpctx, u32, ld->hsm_capabilities, + tal_count(ld->hsm_capabilities), 0), &channel->cid, &channel->funding, channel->funding_sats, diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index 8d1b6c536a7e..3765787c63f6 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -78,11 +79,7 @@ static unsigned int hsm_msg(struct subd *hsmd, * number) */ bool hsm_capable(struct lightningd *ld, u32 msgtype) { - for (size_t i = 0; i < tal_count(ld->hsm_capabilities); i++) { - if (ld->hsm_capabilities[i] == msgtype) - return true; - } - return false; + return hsm_is_capable(ld->hsm_capabilities, msgtype); } struct ext_key *hsm_init(struct lightningd *ld) diff --git a/wallet/test/Makefile b/wallet/test/Makefile index 2f5952967430..f5ab780e0341 100644 --- a/wallet/test/Makefile +++ b/wallet/test/Makefile @@ -14,6 +14,7 @@ WALLET_TEST_COMMON_OBJS := \ common/derive_basepoints.o \ common/features.o \ common/htlc_state.o \ + common/hsm_capable.o \ common/htlc_wire.o \ common/fee_states.o \ common/type_to_string.o \ From f2874f6f1b869847657fe11250031d71f8cd67ae Mon Sep 17 00:00:00 2001 From: Ken Sedgwick Date: Wed, 17 Jan 2024 13:14:24 -0800 Subject: [PATCH 3/5] channeld: defer construction of revocation message until master sync --- channeld/channeld.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 1758f82e4839..08bce83372da 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1934,11 +1934,6 @@ static void send_revocation(struct peer *peer, &failed, &added); - /* Revoke previous commit, get new point. */ - msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-1, - &peer->next_local_per_commit, - old_secret, next_point); - /* From now on we apply changes to the next commitment */ peer->next_index[LOCAL]++; @@ -1969,6 +1964,11 @@ static void send_revocation(struct peer *peer, peer->splice_state->await_commitment_succcess = false; + /* Revoke previous commit, get new point. */ + msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-2, + &peer->next_local_per_commit, + old_secret, next_point); + /* Now we can finally send revoke_and_ack to peer */ peer_write(peer->pps, take(msg)); } From fe779981e3490fea30e5410b0cba410ebba1650f Mon Sep 17 00:00:00 2001 From: Devrandom Date: Mon, 22 Jan 2024 17:35:43 +0100 Subject: [PATCH 4/5] hsmd: separate revoke_commitment_tx ChangeLog-Added: Added hsmd_revoke_commitment_tx to ensure synchronization of local state with remote signers. --- channeld/Makefile | 1 + common/hsm_version.h | 4 ++- hsmd/hsmd.c | 2 ++ hsmd/hsmd_wire.csv | 8 ++++++ hsmd/libhsmd.c | 62 +++++++++++++++++++++++++++++++++++++------- 5 files changed, 67 insertions(+), 10 deletions(-) diff --git a/channeld/Makefile b/channeld/Makefile index 894929b3ed01..8bd27e42566f 100644 --- a/channeld/Makefile +++ b/channeld/Makefile @@ -60,6 +60,7 @@ CHANNELD_COMMON_OBJS := \ common/status_wiregen.o \ common/gossip_store.o \ common/hmac.o \ + common/hsm_capable.o \ common/interactivetx.o \ common/htlc_state.o \ common/htlc_trim.o \ diff --git a/common/hsm_version.h b/common/hsm_version.h index fd9d8066e69e..5ca445ce2962 100644 --- a/common/hsm_version.h +++ b/common/hsm_version.h @@ -19,7 +19,9 @@ * v4 with capabilities called permissions: 7c5bf8ec7cf30302740db85260a9d1ac2c5b0323a2376c28df6b611831f91655 * v4 with renaming of channel_ready to setup_channel: 60b92a0930b631cc77df564cb9235e6cb220f4337a2bb00e5153145e0bf8c80e * v4 with buried outpoint check: f44fae666895cab0347b3de7c245267c71cc7de834827b83e286e86318c08aec + * v4 with forget_channel: d87c6934ea188f92785d38d7cd0b13ed7f76aa7417f3200baf0c7b5aa832fe29 + * v5 with hsmd_revoke_commitment_tx: 5742538f87ef5d5bf55b66dc19e52c8683cfeb1b887d3e64ba530ba9a4d8e638 */ #define HSM_MIN_VERSION 3 -#define HSM_MAX_VERSION 4 +#define HSM_MAX_VERSION 5 #endif /* LIGHTNING_COMMON_HSM_VERSION_H */ diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 29b4b066aeb7..d557799aed52 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -650,6 +650,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c) case WIRE_HSMD_FORGET_CHANNEL: case WIRE_HSMD_SIGN_COMMITMENT_TX: case WIRE_HSMD_VALIDATE_COMMITMENT_TX: + case WIRE_HSMD_REVOKE_COMMITMENT_TX: case WIRE_HSMD_VALIDATE_REVOCATION: case WIRE_HSMD_SIGN_PENALTY_TO_US: case WIRE_HSMD_SIGN_REMOTE_COMMITMENT_TX: @@ -705,6 +706,7 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c) case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_COMMITMENT_TX_REPLY: + case WIRE_HSMD_REVOKE_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_REVOCATION_REPLY: case WIRE_HSMD_SIGN_TX_REPLY: case WIRE_HSMD_SIGN_OPTION_WILL_FUND_OFFER_REPLY: diff --git a/hsmd/hsmd_wire.csv b/hsmd/hsmd_wire.csv index ca97f9c72a23..3b0c9c230e30 100644 --- a/hsmd/hsmd_wire.csv +++ b/hsmd/hsmd_wire.csv @@ -210,6 +210,14 @@ msgtype,hsmd_validate_commitment_tx_reply,135 msgdata,hsmd_validate_commitment_tx_reply,old_commitment_secret,?secret, msgdata,hsmd_validate_commitment_tx_reply,next_per_commitment_point,pubkey, +# Revoke our local commitment, returns the revocation secret and next point +msgtype,hsmd_revoke_commitment_tx,40 +msgdata,hsmd_revoke_commitment_tx,commit_num,u64, + +msgtype,hsmd_revoke_commitment_tx_reply,140 +msgdata,hsmd_revoke_commitment_tx_reply,old_commitment_secret,secret, +msgdata,hsmd_revoke_commitment_tx_reply,next_per_commitment_point,pubkey, + # Vaidate the counterparty's revocation secret msgtype,hsmd_validate_revocation,36 msgdata,hsmd_validate_revocation,revoke_num,u64, diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 9ca742c73212..a7686ea24ed8 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -100,6 +100,7 @@ bool hsmd_check_client_capabilities(struct hsmd_client *client, case WIRE_HSMD_SIGN_REMOTE_COMMITMENT_TX: case WIRE_HSMD_SIGN_REMOTE_HTLC_TX: case WIRE_HSMD_VALIDATE_COMMITMENT_TX: + case WIRE_HSMD_REVOKE_COMMITMENT_TX: case WIRE_HSMD_VALIDATE_REVOCATION: return (client->capabilities & HSM_PERM_SIGN_REMOTE_TX) != 0; @@ -160,6 +161,7 @@ bool hsmd_check_client_capabilities(struct hsmd_client *client, case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_COMMITMENT_TX_REPLY: + case WIRE_HSMD_REVOKE_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_REVOCATION_REPLY: case WIRE_HSMD_SIGN_TX_REPLY: case WIRE_HSMD_SIGN_OPTION_WILL_FUND_OFFER_REPLY: @@ -1709,20 +1711,58 @@ static u8 *handle_validate_commitment_tx(struct hsmd_client *c, const u8 *msg_in return hsmd_status_bad_request_fmt( c, msg_in, "bad per_commit_point %" PRIu64, commit_num + 1); - if (commit_num >= 1) { - old_secret = tal(tmpctx, struct secret); - if (!per_commit_secret(&shaseed, old_secret, commit_num - 1)) { - return hsmd_status_bad_request_fmt( - c, msg_in, "Cannot derive secret %" PRIu64, commit_num - 1); - } - } else { - old_secret = NULL; - } + /* Don't ever return the old_secret here anymore. The node should + * call hsmd_revoke_commitment_tx to transactionally revoke the commitment + * and return the secret ... + */ + old_secret = NULL; return towire_hsmd_validate_commitment_tx_reply( NULL, old_secret, &next_per_commitment_point); } +/* ~This stub implementation is overriden by fully validating signers + * that need to independently revoke the old local commitment tx and + * release it's secret. + * Revoke the old commitment tx by disclosing its secret and also return + * the next commitiment's per-commitment-point. + */ +static u8 *handle_revoke_commitment_tx(struct hsmd_client *c, const u8 *msg_in) +{ + u64 commit_num; + struct secret channel_seed; + struct sha256 shaseed; + struct secret *old_secret; + struct pubkey next_per_commitment_point; + + if (!fromwire_hsmd_revoke_commitment_tx(msg_in, &commit_num)) + return hsmd_status_malformed_request(c, msg_in); + + /* Stub implementation */ + + /* The signatures are not checked in this stub because they + * are already checked by the caller. However, the returned + * old_secret and next_per_commitment_point are used. + */ + + get_channel_seed(&c->id, c->dbid, &channel_seed); + if (!derive_shaseed(&channel_seed, &shaseed)) + return hsmd_status_bad_request(c, msg_in, "bad derive_shaseed"); + + if (!per_commit_point(&shaseed, &next_per_commitment_point, commit_num + 2)) + return hsmd_status_bad_request_fmt( + c, msg_in, "bad per_commit_point %" PRIu64, commit_num + 2); + + old_secret = tal(tmpctx, struct secret); + if (!per_commit_secret(&shaseed, old_secret, commit_num)) { + return hsmd_status_bad_request_fmt( + c, msg_in, "Cannot derive secret %" PRIu64, commit_num); + } + + return towire_hsmd_revoke_commitment_tx_reply( + NULL, old_secret, &next_per_commitment_point); +} + /* This stub implementation is overriden by fully validating signers * that need to independently verify that the latest state is * commited. */ @@ -2010,6 +2050,8 @@ u8 *hsmd_handle_client_message(const tal_t *ctx, struct hsmd_client *client, return handle_sign_commitment_tx(client, msg); case WIRE_HSMD_VALIDATE_COMMITMENT_TX: return handle_validate_commitment_tx(client, msg); + case WIRE_HSMD_REVOKE_COMMITMENT_TX: + return handle_revoke_commitment_tx(client, msg); case WIRE_HSMD_VALIDATE_REVOCATION: return handle_validate_revocation(client, msg); case WIRE_HSMD_SIGN_REMOTE_HTLC_TO_US: @@ -2052,6 +2094,7 @@ u8 *hsmd_handle_client_message(const tal_t *ctx, struct hsmd_client *client, case WIRE_HSMSTATUS_CLIENT_BAD_REQUEST: case WIRE_HSMD_SIGN_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_COMMITMENT_TX_REPLY: + case WIRE_HSMD_REVOKE_COMMITMENT_TX_REPLY: case WIRE_HSMD_VALIDATE_REVOCATION_REPLY: case WIRE_HSMD_SIGN_TX_REPLY: case WIRE_HSMD_SIGN_OPTION_WILL_FUND_OFFER_REPLY: @@ -2088,6 +2131,7 @@ u8 *hsmd_init(struct secret hsm_secret, WIRE_HSMD_SIGN_SPLICE_TX, WIRE_HSMD_CHECK_OUTPOINT, WIRE_HSMD_FORGET_CHANNEL, + WIRE_HSMD_REVOKE_COMMITMENT_TX, }; /*~ Don't swap this. */ From 2894cd486c0864c4a468bb1ab7d02654adbfb3e4 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Mon, 22 Jan 2024 17:35:58 +0100 Subject: [PATCH 5/5] channeld: use revoke_commitment_tx if hsmd supports --- channeld/channeld.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 08bce83372da..40532ca550c2 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -25,6 +25,8 @@ #include #include #include +#include +#include #include #include #include @@ -1923,6 +1925,7 @@ static void send_revocation(struct peer *peer, const struct failed_htlc **failed; struct added_htlc *added; const u8 *msg; + const u8 *msg2; const u8 *msg_for_master; /* Marshall it now before channel_sending_revoke_and_ack changes htlcs */ @@ -1964,10 +1967,28 @@ static void send_revocation(struct peer *peer, peer->splice_state->await_commitment_succcess = false; + /* Now that the master has persisted the new commitment advance the HSMD + * and fetch the revocation secret for the old one. */ + struct secret old_secret2; + struct pubkey next_point2; + 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. */ + memcpy(&old_secret2, old_secret, sizeof(old_secret2)); + memcpy(&next_point2, next_point, sizeof(next_point2)); + } else { + msg2 = towire_hsmd_revoke_commitment_tx(tmpctx, peer->next_index[LOCAL] - 2); + msg2 = hsm_req(tmpctx, take(msg2)); + if (!fromwire_hsmd_revoke_commitment_tx_reply(msg2, &old_secret2, &next_point2)) + status_failed(STATUS_FAIL_HSM_IO, + "Reading revoke_commitment_tx reply: %s", + tal_hex(tmpctx, msg2)); + } + /* Revoke previous commit, get new point. */ msg = make_revocation_msg_from_secret(peer, peer->next_index[LOCAL]-2, &peer->next_local_per_commit, - old_secret, next_point); + &old_secret2, &next_point2); /* Now we can finally send revoke_and_ack to peer */ peer_write(peer->pps, take(msg)); @@ -2273,7 +2294,10 @@ static struct commitsig_info *handle_peer_commit_sig(struct peer *peer, tal_steal(commitsigs, result); } - assert(old_secret); + // 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); send_revocation(peer, &commit_sig, htlc_sigs, changed_htlcs, txs[0], old_secret, &next_point, commitsigs);