From cb83cfe366aaa07179cac1079694e9ea5c6cc9c6 Mon Sep 17 00:00:00 2001 From: Devrandom Date: Tue, 5 Jan 2021 11:50:54 -0800 Subject: [PATCH] Fold sign_holder_commitment_htlc_transactions into sign_holder_commitment Signing the commitment transaction is almost always followed by signing the attached HTLC transactions, so fold the signing operations into a single method. --- lightning/src/chain/keysinterface.rs | 41 +++++++++------------ lightning/src/ln/channel.rs | 4 +- lightning/src/ln/onchaintx.rs | 31 +++++++++++----- lightning/src/util/enforcing_trait_impls.rs | 26 +++++-------- 4 files changed, 50 insertions(+), 52 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 4a3a937a5..80f734edb 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -233,13 +233,21 @@ pub trait ChannelKeys : Send+Clone + Writeable { // TODO: Document the things someone using this interface should enforce before signing. fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; - /// Create a signature for a holder's commitment transaction. This will only ever be called with - /// the same commitment_tx (or a copy thereof), though there are currently no guarantees - /// that it will not be called multiple times. + /// Create a signatures for a holder's commitment transaction and its claiming HTLC transactions. + /// This will only ever be called with a non-revoked commitment_tx. This will be called with the + /// latest commitment_tx when we initiate a force-close. + /// This will be called with the previous latest, just to get claiming HTLC signatures, if we are + /// reacting to a ChannelMonitor replica that decided to broadcast before it had been updated to + /// the latest. + /// This may be called multiple times for the same transaction. + /// /// An external signer implementation should check that the commitment has not been revoked. + /// + /// May return Err if key derivation fails. Callers, such as ChannelMonitor, will panic in such a case. // // TODO: Document the things someone using this interface should enforce before signing. - fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; + // TODO: Key derivation failure should panic rather than Err + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; /// 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 @@ -248,18 +256,6 @@ pub trait ChannelKeys : Send+Clone + Writeable { #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; - /// Create a signature for each HTLC transaction spending a holder's commitment transaction. - /// - /// Unlike sign_holder_commitment, this may be called multiple times with *different* - /// commitment_tx values. While this will never be called with a revoked - /// commitment_tx, it is possible that it is called with the second-latest - /// commitment_tx (only if we haven't yet revoked it) if some watchtower/secondary - /// ChannelMonitor decided to broadcast before it had been updated to the latest. - /// - /// Either an Err should be returned, or a Vec with one entry for each HTLC which exists in - /// commitment_tx. - fn sign_holder_commitment_htlc_transactions(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result, ()>; - /// Create a signature for the given input in a transaction spending an HTLC or commitment /// transaction output when our counterparty broadcasts an old state. /// @@ -500,11 +496,14 @@ impl ChannelKeys for InMemoryChannelKeys { Ok((commitment_sig, htlc_sigs)) } - fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); let sig = commitment_tx.trust().built_transaction().sign(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, secp_ctx); - Ok(sig) + let channel_parameters = self.get_channel_parameters(); + let trusted_tx = commitment_tx.trust(); + let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx)?; + Ok((sig, htlc_sigs)) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] @@ -514,12 +513,6 @@ impl ChannelKeys for InMemoryChannelKeys { Ok(commitment_tx.trust().built_transaction().sign(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx)) } - fn sign_holder_commitment_htlc_transactions(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result, ()> { - let channel_parameters = self.get_channel_parameters(); - let trusted_tx = commitment_tx.trust(); - trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx) - } - fn sign_justice_transaction(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option, secp_ctx: &Secp256k1) -> Result { let revocation_key = match chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key) { Ok(revocation_key) => revocation_key, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 4ba8b28bf..08faea160 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4742,15 +4742,13 @@ mod tests { &chan.holder_keys.pubkeys().funding_pubkey, chan.counterparty_funding_pubkey() ); - let holder_sig = chan_keys.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap(); + let (holder_sig, htlc_sigs) = chan_keys.sign_holder_commitment_and_htlcs(&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.get_funding_redeemscript(); let tx = holder_commitment_tx.add_holder_sig(&funding_redeemscript, holder_sig); assert_eq!(serialize(&tx)[..], hex::decode($tx_hex).unwrap()[..], "tx"); - let htlc_sigs = chan_keys.sign_holder_commitment_htlc_transactions(&holder_commitment_tx, &secp_ctx).unwrap(); - // ((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()); diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 1b0060dab..80ad434ce 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -490,6 +490,8 @@ impl OnchainTxHandler { /// Lightning security model (i.e being able to redeem/timeout HTLC or penalize coutnerparty onchain) lays on the assumption of claim transactions getting confirmed before timelock expiration /// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent. + /// Panics if there are signing errors, because signing operations in reaction to on-chain events + /// are not expected to fail, and if they do, we may lose funds. fn generate_claim_tx(&mut self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: &F, logger: &L) -> Option<(Option, u32, Transaction)> where F::Target: FeeEstimator, L::Target: Logger, @@ -906,20 +908,29 @@ impl OnchainTxHandler { pub(crate) fn provide_latest_holder_tx(&mut self, tx: HolderCommitmentTransaction) { self.prev_holder_commitment = self.holder_commitment.take(); + self.holder_htlc_sigs = None; self.holder_commitment = Some(tx); } + // Normally holder HTLCs are signed at the same time as the holder commitment tx. However, + // in some configurations, the holder commitment tx has been signed and broadcast by a + // ChannelMonitor replica, so we handle that case here. fn sign_latest_holder_htlcs(&mut self) { - if let Some(ref holder_commitment) = self.holder_commitment { - if let Ok(sigs) = self.key_storage.sign_holder_commitment_htlc_transactions(holder_commitment, &self.secp_ctx) { + if self.holder_htlc_sigs.is_none() { + if let Some(ref holder_commitment) = self.holder_commitment { + let (_sig, sigs) = self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx).expect("sign holder commitment"); self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, sigs)); } } } + // Normally only the latest commitment tx and HTLCs need to be signed. However, in some + // configurations we may have updated our holder commtiment but a replica of the ChannelMonitor + // broadcast the previous one before we sync with it. We handle that case here. fn sign_prev_holder_htlcs(&mut self) { - if let Some(ref holder_commitment) = self.prev_holder_commitment { - if let Ok(sigs) = self.key_storage.sign_holder_commitment_htlc_transactions(holder_commitment, &self.secp_ctx) { + if self.prev_holder_htlc_sigs.is_none() { + if let Some(ref holder_commitment) = self.prev_holder_commitment { + let (_sig, sigs) = self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx).expect("sign previous holder commitment"); self.prev_holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, sigs)); } } @@ -941,8 +952,9 @@ impl OnchainTxHandler { // to monitor before. pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut holder_commitment) = self.holder_commitment { - match self.key_storage.sign_holder_commitment(&holder_commitment, &self.secp_ctx) { - Ok(sig) => { + match self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx) { + Ok((sig, htlc_sigs)) => { + self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) }, Err(_) => return None, @@ -955,8 +967,9 @@ impl OnchainTxHandler { #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut holder_commitment) = self.holder_commitment { - match self.key_storage.sign_holder_commitment(holder_commitment, &self.secp_ctx) { - Ok(sig) => { + match self.key_storage.sign_holder_commitment_and_htlcs(holder_commitment, &self.secp_ctx) { + Ok((sig, htlc_sigs)) => { + self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs)); Some(holder_commitment.add_holder_sig(funding_redeemscript, sig)) }, Err(_) => return None, @@ -982,7 +995,7 @@ impl OnchainTxHandler { } } } - if self.prev_holder_commitment.is_some() { + if htlc_tx.is_none() && self.prev_holder_commitment.is_some() { let commitment_txid = self.prev_holder_commitment.as_ref().unwrap().trust().txid(); if commitment_txid == outp.txid { self.sign_prev_holder_htlcs(); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 92cf178c8..a340cbf42 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -8,7 +8,7 @@ // licenses. use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, HolderCommitmentTransaction, CommitmentTransaction, ChannelTransactionParameters, TrustedCommitmentTransaction}; -use ln::{chan_utils, msgs}; +use ln::{msgs, chan_utils}; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; use std::cmp; @@ -72,20 +72,7 @@ impl ChannelKeys for EnforcingChannelKeys { Ok(self.inner.sign_counterparty_commitment(commitment_tx, secp_ctx).unwrap()) } - fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { - self.verify_holder_commitment_tx(commitment_tx, secp_ctx); - - // TODO: enforce the ChannelKeys contract - error if this commitment was already revoked - // TODO: need the commitment number - Ok(self.inner.sign_holder_commitment(commitment_tx, secp_ctx).unwrap()) - } - - #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - 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_holder_commitment_htlc_transactions(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result, ()> { + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { 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(); @@ -101,7 +88,14 @@ impl ChannelKeys for EnforcingChannelKeys { secp_ctx.verify(&sighash, sig, &keys.countersignatory_htlc_key).unwrap(); } - Ok(self.inner.sign_holder_commitment_htlc_transactions(commitment_tx, secp_ctx).unwrap()) + // TODO: enforce the ChannelKeys contract - error if this commitment was already revoked + // TODO: need the commitment number + Ok(self.inner.sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) + } + + #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] + 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_transaction(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option, secp_ctx: &Secp256k1) -> Result { -- 2.39.5