From aae4e7c0cab7f30e38b06a2314eea505389eefe1 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 13 Oct 2023 14:09:37 -0700 Subject: [PATCH] Don't sign holder HTLCs along with holder commitments `sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`. --- lightning/src/chain/onchaintx.rs | 4 +- lightning/src/ln/channel.rs | 37 ++++++++++----- lightning/src/ln/monitor_tests.rs | 2 +- lightning/src/sign/mod.rs | 44 ++++++++--------- lightning/src/util/test_channel_signer.rs | 58 ++++++++++++----------- 5 files changed, 77 insertions(+), 68 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5d44ea640..95824f910 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1101,13 +1101,13 @@ impl OnchainTxHandler // before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing // to monitor before. pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let (sig, _) = self.signer.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment"); + let sig = self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment"); self.holder_commitment.add_holder_sig(funding_redeemscript, sig) } #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let (sig, _) = self.signer.unsafe_sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); + let sig = self.signer.unsafe_sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); self.holder_commitment.add_holder_sig(funding_redeemscript, sig) } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a61a8de82..7b14573a1 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -8184,6 +8184,7 @@ mod tests { use bitcoin::hashes::hex::FromHex; use bitcoin::hash_types::Txid; use bitcoin::secp256k1::Message; + use crate::events::bump_transaction::{ChannelDerivationParameters, HTLCDescriptor}; use crate::sign::EcdsaChannelSigner; use crate::ln::PaymentPreimage; use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys}; @@ -8309,7 +8310,7 @@ mod tests { &chan.context.holder_signer.as_ref().pubkeys().funding_pubkey, chan.context.counterparty_funding_pubkey() ); - let (holder_sig, htlc_sigs) = signer.sign_holder_commitment_and_htlcs(&holder_commitment_tx, &secp_ctx).unwrap(); + let holder_sig = signer.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap(); assert_eq!(Signature::from_der(&hex::decode($sig_hex).unwrap()[..]).unwrap(), holder_sig, "holder_sig"); let funding_redeemscript = chan.context.get_funding_redeemscript(); @@ -8317,14 +8318,14 @@ mod tests { assert_eq!(serialize(&tx)[..], hex::decode($tx_hex).unwrap()[..], "tx"); // ((htlc, counterparty_sig), (index, holder_sig)) - let mut htlc_sig_iter = holder_commitment_tx.htlcs().iter().zip(&holder_commitment_tx.counterparty_htlc_sigs).zip(htlc_sigs.iter().enumerate()); + let mut htlc_counterparty_sig_iter = holder_commitment_tx.counterparty_htlc_sigs.iter(); $({ log_trace!(logger, "verifying htlc {}", $htlc_idx); let remote_signature = Signature::from_der(&hex::decode($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); let ref htlc = htlcs[$htlc_idx]; - let htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, + let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, chan.context.get_counterparty_selected_contest_delay().unwrap(), &htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, $opt_anchors, &keys); @@ -8344,20 +8345,32 @@ mod tests { assert!(preimage.is_some()); } - let htlc_sig = htlc_sig_iter.next().unwrap(); + let htlc_counterparty_sig = htlc_counterparty_sig_iter.next().unwrap(); + let htlc_holder_sig = signer.sign_holder_htlc_transaction(&htlc_tx, 0, &HTLCDescriptor { + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: chan.context.channel_value_satoshis, + keys_id: chan.context.channel_keys_id, + transaction_parameters: chan.context.channel_transaction_parameters.clone(), + }, + commitment_txid: trusted_tx.txid(), + per_commitment_number: trusted_tx.commitment_number(), + per_commitment_point: trusted_tx.per_commitment_point(), + feerate_per_kw: trusted_tx.feerate_per_kw(), + htlc: htlc.clone(), + preimage: preimage.clone(), + counterparty_sig: *htlc_counterparty_sig, + }, &secp_ctx).unwrap(); let num_anchors = if $opt_anchors.supports_anchors_zero_fee_htlc_tx() { 2 } else { 0 }; - assert_eq!((htlc_sig.0).0.transaction_output_index, Some($htlc_idx + num_anchors), "output index"); + assert_eq!(htlc.transaction_output_index, Some($htlc_idx + num_anchors), "output index"); let signature = Signature::from_der(&hex::decode($htlc_sig_hex).unwrap()[..]).unwrap(); - assert_eq!(signature, *(htlc_sig.1).1, "htlc sig"); - let index = (htlc_sig.1).0; - let channel_parameters = chan.context.channel_transaction_parameters.as_holder_broadcastable(); + assert_eq!(signature, htlc_holder_sig, "htlc sig"); let trusted_tx = holder_commitment_tx.trust(); - log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage)))); - assert_eq!(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage))[..], - hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx"); + htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness($htlc_idx, htlc_counterparty_sig, &htlc_holder_sig, &preimage); + log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&htlc_tx))); + assert_eq!(serialize(&htlc_tx)[..], hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx"); })* - assert!(htlc_sig_iter.next().is_none()); + assert!(htlc_counterparty_sig_iter.next().is_none()); } } } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 5be498e91..37fdc647a 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1680,7 +1680,7 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { // secret to the counterparty. However, because we always immediately take the revocation // secret from the keys_manager, we would panic at broadcast as we're trying to sign a // transaction which, from the point of view of our keys_manager, is revoked. - chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let mut user_config = test_default_channel_config(); if anchors { diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index d99925858..6abf4bd25 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -487,31 +487,26 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// This is required in order for the signer to make sure that the state has moved /// forward and it is safe to sign the next counterparty commitment. fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey) -> Result<(), ()>; - /// Creates a signature for a holder's commitment transaction and its claiming HTLC transactions. + /// Creates a signature for a holder's commitment transaction. /// /// This will be called /// - with a non-revoked `commitment_tx`. /// - with the latest `commitment_tx` when we initiate a force-close. - /// - with the previous `commitment_tx`, just to get claiming HTLC - /// signatures, if we are reacting to a [`ChannelMonitor`] - /// [replica](https://github.com/lightningdevkit/rust-lightning/blob/main/GLOSSARY.md#monitor-replicas) - /// that decided to broadcast before it had been updated to the latest `commitment_tx`. /// /// This may be called multiple times for the same transaction. /// /// An external signer implementation should check that the commitment has not been revoked. - /// - /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor + // // TODO: Document the things someone using this interface should enforce before signing. - fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, - secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; - /// Same as [`sign_holder_commitment_and_htlcs`], but exists only for tests to get access to - /// holder commitment transactions which will be broadcasted later, after the channel has moved - /// on to a newer state. Thus, needs its own method as [`sign_holder_commitment_and_htlcs`] may - /// enforce that we only ever get called once. + fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, + secp_ctx: &Secp256k1) -> Result; + /// Same as [`sign_holder_commitment`], but exists only for tests to get access to holder + /// commitment transactions which will be broadcasted later, after the channel has moved on to a + /// newer state. Thus, needs its own method as [`sign_holder_commitment`] may enforce that we + /// only ever get called once. #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, - secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, + secp_ctx: &Secp256k1) -> Result; /// Create a signature for the given input in a transaction spending an HTLC transaction output /// or a commitment transaction `to_local` output when our counterparty broadcasts an old state. /// @@ -554,7 +549,12 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// `htlc_tx`, which spends the commitment transaction at index `input`. The signature returned /// must be be computed using [`EcdsaSighashType::All`]. /// + /// Note that this may be called for HTLCs in the penultimate commitment transaction if a + /// [`ChannelMonitor`] [replica](https://github.com/lightningdevkit/rust-lightning/blob/main/GLOSSARY.md#monitor-replicas) + /// broadcasts it before receiving the update for the latest commitment transaction. + /// /// [`EcdsaSighashType::All`]: bitcoin::blockdata::transaction::EcdsaSighashType::All + /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor fn sign_holder_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, secp_ctx: &Secp256k1 ) -> Result; @@ -1118,27 +1118,21 @@ impl EcdsaChannelSigner for InMemorySigner { Ok(()) } - fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &counterparty_keys.funding_pubkey); let trusted_tx = commitment_tx.trust(); - let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx); - let channel_parameters = self.get_channel_parameters().expect(MISSING_PARAMS_ERR); - let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?; - Ok((sig, htlc_sigs)) + Ok(trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx)) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &counterparty_keys.funding_pubkey); let trusted_tx = commitment_tx.trust(); - let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx); - let channel_parameters = self.get_channel_parameters().expect(MISSING_PARAMS_ERR); - let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?; - Ok((sig, htlc_sigs)) + Ok(trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx)) } fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index c57c5a9d6..99240873f 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -155,11 +155,8 @@ impl EcdsaChannelSigner for TestChannelSigner { Ok(()) } - fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx); - let commitment_txid = trusted_tx.txid(); - let holder_csv = self.inner.counterparty_selected_contest_delay().unwrap(); - let state = self.state.lock().unwrap(); let commitment_number = trusted_tx.commitment_number(); if state.last_holder_revoked_commitment - 1 != commitment_number && state.last_holder_revoked_commitment - 2 != commitment_number { @@ -168,33 +165,12 @@ impl EcdsaChannelSigner for TestChannelSigner { state.last_holder_revoked_commitment, commitment_number, self.inner.commitment_seed[0]) } } - - for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) { - assert!(this_htlc.transaction_output_index.is_some()); - let keys = trusted_tx.keys(); - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, trusted_tx.feerate_per_kw(), holder_csv, &this_htlc, self.channel_type_features(), &keys.broadcaster_delayed_payment_key, &keys.revocation_key); - - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&this_htlc, self.channel_type_features(), &keys); - - let sighash_type = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - EcdsaSighashType::SinglePlusAnyoneCanPay - } else { - EcdsaSighashType::All - }; - let sighash = hash_to_message!( - &sighash::SighashCache::new(&htlc_tx).segwit_signature_hash( - 0, &htlc_redeemscript, this_htlc.amount_msat / 1000, sighash_type, - ).unwrap()[..] - ); - secp_ctx.verify_ecdsa(&sighash, sig, &keys.countersignatory_htlc_key).unwrap(); - } - - Ok(self.inner.sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) + Ok(self.inner.sign_holder_commitment(commitment_tx, secp_ctx).unwrap()) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { - Ok(self.inner.unsafe_sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) + fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { + Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap()) } fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1) -> Result { @@ -209,8 +185,34 @@ impl EcdsaChannelSigner for TestChannelSigner { &self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, secp_ctx: &Secp256k1 ) -> Result { + let state = self.state.lock().unwrap(); + if state.last_holder_revoked_commitment - 1 != htlc_descriptor.per_commitment_number && + state.last_holder_revoked_commitment - 2 != htlc_descriptor.per_commitment_number + { + if !self.disable_revocation_policy_check { + panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", + state.last_holder_revoked_commitment, htlc_descriptor.per_commitment_number, self.inner.commitment_seed[0]) + } + } assert_eq!(htlc_tx.input[input], htlc_descriptor.unsigned_tx_input()); assert_eq!(htlc_tx.output[input], htlc_descriptor.tx_output(secp_ctx)); + { + let witness_script = htlc_descriptor.witness_script(secp_ctx); + let sighash_type = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + EcdsaSighashType::SinglePlusAnyoneCanPay + } else { + EcdsaSighashType::All + }; + let sighash = &sighash::SighashCache::new(&*htlc_tx).segwit_signature_hash( + input, &witness_script, htlc_descriptor.htlc.amount_msat / 1000, sighash_type + ).unwrap(); + let countersignatory_htlc_key = chan_utils::derive_public_key( + &secp_ctx, &htlc_descriptor.per_commitment_point, &self.inner.counterparty_pubkeys().unwrap().htlc_basepoint + ); + secp_ctx.verify_ecdsa( + &hash_to_message!(&sighash), &htlc_descriptor.counterparty_sig, &countersignatory_htlc_key + ).unwrap(); + } Ok(self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx).unwrap()) } -- 2.39.5