From 5101d2086c0cf2134ce9d729cfdf69b0a21d564e Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 20 Mar 2020 18:04:01 -0400 Subject: [PATCH] Remove signing local commitment transaction from ChannelMonitor Extend external signer interface to sign local commitment transactions on its behalf without seckey passing. This move will allow us to remove key access from ChannelMonitor hot memory in further work. Local commitment transaction should stay half-signed by remote until we need to broadcast for a channel force-close or a HTLC to timeout onchain. Add an unsafe test-only version of sign_local_commitment to fulfill our test_framework needs. --- lightning/src/chain/keysinterface.rs | 27 ++++++++++++++++++++- lightning/src/ln/chan_utils.rs | 22 ++++++++++++++--- lightning/src/ln/channel.rs | 4 +-- lightning/src/ln/channelmonitor.rs | 4 +-- lightning/src/util/enforcing_trait_impls.rs | 12 ++++++++- 5 files changed, 60 insertions(+), 9 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 2b9018863..5f1a307bf 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -24,7 +24,7 @@ use util::logger::Logger; use util::ser::{Writeable, Writer, Readable}; use ln::chan_utils; -use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys}; +use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction}; use ln::msgs; use std::sync::Arc; @@ -215,6 +215,22 @@ pub trait ChannelKeys : Send+Clone { /// making the callee generate it via some util function we expose)! fn sign_remote_commitment(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + /// Create a signature for a local commitment transaction + /// + /// TODO: Document the things someone using this interface should enforce before signing. + /// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and + /// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method + /// making the callee generate it via some util function we expose)! + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + + /// Create a signature for a local commitment transaction without enforcing one-time signing. + /// + /// Testing revocation logic by our test framework needs to sign multiple local commitment + /// transactions. This unsafe test-only version doesn't enforce one-time signing security + /// requirement. + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + /// Create a signature for a (proposed) closing transaction. /// /// Note that, due to rounding, there may be one "missing" satoshi, and either party may have @@ -342,6 +358,15 @@ impl ChannelKeys for InMemoryChannelKeys { Ok((commitment_sig, htlc_sigs)) } + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { if closing_tx.input.len() != 1 { return Err(()); } if closing_tx.input[0].witness.len() != 0 { return Err(()); } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 561448c8f..0ef269c8c 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -516,7 +516,7 @@ pub(crate) fn sign_htlc_transaction(tx: &mut Transaction, /// We use this to track local commitment transactions and put off signing them until we are ready /// to broadcast. Eventually this will require a signer which is possibly external, but for now we /// just pass in the SecretKeys required. -pub(crate) struct LocalCommitmentTransaction { +pub struct LocalCommitmentTransaction { tx: Transaction } impl LocalCommitmentTransaction { @@ -530,7 +530,9 @@ impl LocalCommitmentTransaction { } } } - pub fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction { + /// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, + /// remote signature and both parties keys + pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey) -> LocalCommitmentTransaction { if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); } if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); } @@ -549,10 +551,13 @@ impl LocalCommitmentTransaction { Self { tx } } + /// Get the txid of the local commitment transaction contained in this + /// LocalCommitmentTransaction pub fn txid(&self) -> Sha256dHash { self.tx.txid() } + /// Check if LocalCommitmentTransaction has already been signed by us pub fn has_local_sig(&self) -> bool { if self.tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); } if self.tx.input[0].witness.len() == 4 { @@ -567,6 +572,15 @@ impl LocalCommitmentTransaction { } } + /// Add local signature for LocalCommitmentTransaction, do nothing if signature is already + /// present + /// + /// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided + /// by your ChannelKeys. + /// Funding redeemscript is script locking funding_outpoint. This is the mutlsig script + /// between your own funding key and your counterparty's. Currently, this is provided in + /// ChannelKeys::sign_local_commitment() calls directly. + /// Channel value is amount locked in funding_outpoint. pub fn add_local_sig(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { if self.has_local_sig() { return; } let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx) @@ -584,7 +598,9 @@ impl LocalCommitmentTransaction { self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec()); } - pub fn without_valid_witness(&self) -> &Transaction { &self.tx } + /// Get raw transaction without asserting if witness is complete + pub(crate) fn without_valid_witness(&self) -> &Transaction { &self.tx } + /// Get raw transaction with panics if witness is incomplete pub fn with_valid_witness(&self) -> &Transaction { assert!(self.has_local_sig()); &self.tx diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 374d74f6c..43aba4201 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4433,7 +4433,7 @@ mod tests { assert_eq!(PublicKey::from_secret_key(&secp_ctx, chan_keys.funding_key()).serialize()[..], hex::decode("023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb").unwrap()[..]); - let keys_provider = Keys { chan_keys }; + let keys_provider = Keys { chan_keys: chan_keys.clone() }; let their_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let mut config = UserConfig::default(); @@ -4490,7 +4490,7 @@ mod tests { secp_ctx.verify(&sighash, &their_signature, chan.their_funding_pubkey()).unwrap(); let mut localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey()); - localtx.add_local_sig(chan.local_keys.funding_key(), &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx); + chan_keys.sign_local_commitment(&mut localtx, &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx); assert_eq!(serialize(localtx.with_valid_witness())[..], hex::decode($tx_hex).unwrap()[..]); diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 2ac113b98..8b116b9d6 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1806,7 +1806,7 @@ impl ChannelMonitor { // tracking state and panic!()ing if we get an update after force-closure/local-tx signing. log_trace!(self, "Getting signed latest local commitment transaction!"); if let &mut Some(ref mut local_tx) = &mut self.current_local_signed_commitment_tx { - local_tx.tx.add_local_sig(&self.onchain_detection.keys.funding_key(), self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); + self.onchain_detection.keys.sign_local_commitment(&mut local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); } if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx { let mut res = vec![local_tx.tx.with_valid_witness().clone()]; @@ -1888,7 +1888,7 @@ impl ChannelMonitor { } else { false }; if let Some(ref mut cur_local_tx) = self.current_local_signed_commitment_tx { if should_broadcast { - cur_local_tx.tx.add_local_sig(&self.onchain_detection.keys.funding_key(), self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx); + self.onchain_detection.keys.sign_local_commitment(&mut cur_local_tx.tx, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &mut self.secp_ctx); } } if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 26c3a0777..bc3a1a0da 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -1,4 +1,4 @@ -use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys}; +use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction}; use ln::msgs; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; @@ -6,6 +6,7 @@ use std::cmp; use std::sync::{Mutex, Arc}; use bitcoin::blockdata::transaction::Transaction; +use bitcoin::blockdata::script::Script; use secp256k1; use secp256k1::key::{SecretKey, PublicKey}; @@ -78,6 +79,15 @@ impl ChannelKeys for EnforcingChannelKeys { Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap()) } + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx) + } + + #[cfg(test)] + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { + self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx); + } + fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { Ok(self.inner.sign_closing_transaction(closing_tx, secp_ctx).unwrap()) } -- 2.39.5