Fold sign_holder_commitment_htlc_transactions into sign_holder_commitment
authorDevrandom <c1.devrandom@niftybox.net>
Tue, 5 Jan 2021 19:50:54 +0000 (11:50 -0800)
committerDevrandom <c1.devrandom@niftybox.net>
Mon, 18 Jan 2021 18:24:31 +0000 (10:24 -0800)
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
lightning/src/ln/channel.rs
lightning/src/ln/onchaintx.rs
lightning/src/util/enforcing_trait_impls.rs

index 4a3a937a5c4586824ce0b875525b737ca39bdd67..80f734edbde3e0def17dcd23ceb903d564d969e2 100644 (file)
@@ -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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &CommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
 
-       /// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
+       // TODO: Key derivation failure should panic rather than Err
+       fn sign_holder_commitment_and_htlcs<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
 
        /// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
 
-       /// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()>;
-
        /// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
+       fn sign_holder_commitment_and_htlcs<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
                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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()> {
-               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<T: secp256k1::Signing + secp256k1::Verification>(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option<HTLCOutputInCommitment>, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
                let revocation_key = match chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key) {
                        Ok(revocation_key) => revocation_key,
index 4ba8b28bfd5780a49ec0e9260ee4f4da0b81b990..08faea160bb193a0d652a03211d4d2039942f500 100644 (file)
@@ -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());
 
index 1b0060dabc7c59af2eab7e6d01cfbf42085dd102..80ad434ce403abdf1bc61106507dffe6e2fd6ea2 100644 (file)
@@ -490,6 +490,8 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 
        /// 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<F: Deref, L: Deref>(&mut self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: &F, logger: &L) -> Option<(Option<u32>, u32, Transaction)>
                where F::Target: FeeEstimator,
                                        L::Target: Logger,
@@ -906,20 +908,29 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 
        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<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
        // to monitor before.
        pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
                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<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
        #[cfg(any(test, feature="unsafe_revoked_tx_signing"))]
        pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
                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<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
                                }
                        }
                }
-               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();
index 92cf178c8c5d912a61e21a6e0d24f778ece99e7c..a340cbf42a17090e9c0e4c785479fe154ba8709d 100644 (file)
@@ -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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
-               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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
-               Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap())
-       }
-
-       fn sign_holder_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()> {
+       fn sign_holder_commitment_and_htlcs<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
                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<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
+               Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap())
        }
 
        fn sign_justice_transaction<T: secp256k1::Signing + secp256k1::Verification>(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option<HTLCOutputInCommitment>, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {