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.

Author: Antoine Riard

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,13 @@ pub trait ChannelKeys : Send+Clone {
 	/// making the callee generate it via some util function we expose)!
 	fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
 
+	/// 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
+	/// making the callee generate it via some util function we expose)!
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+
 	/// 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 +349,10 @@ impl ChannelKeys for InMemoryChannelKeys {
 		Ok((commitment_sig, htlc_sigs))
 	}
 
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
+		local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	}
+
 	fn sign_closing_transaction<T: secp256k1::Signing>(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
 		if closing_tx.input.len() != 1 { return Err(()); }
 		if closing_tx.input[0].witness.len() != 0 { return Err(()); }

lightning/src/ln/chan_utils.rs

@@ -516,7 +516,7 @@ pub(crate) fn sign_htlc_transaction<T: secp256k1::Signing>(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,6 +530,7 @@ impl LocalCommitmentTransaction {
 		} }
 	}
 
+	/// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, remote signature and both parties keys
 	pub 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 +550,12 @@ 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 +570,11 @@ impl LocalCommitmentTransaction {
 		}
 	}
 
+	/// Add local signature for LocalCommitmentTransaction, do nothing if signature is already present
+	///
+	/// Funding key is your key related to the one part of 2-2 funding_outpoint lock. Should be provided by your ChannelKeys.
+	/// Funding redeemscript is script locking funding_outpoint. Should be computed based on channel keys provided by your ChannelKeys.
+	/// Channel value is amount locked in funding_outpoint.
 	pub fn add_local_sig<T: secp256k1::Signing>(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
 		if self.has_local_sig() { return; }
 		let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx)
@@ -584,7 +592,9 @@ impl LocalCommitmentTransaction {
 		self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec());
 	}
 
+	/// Get raw transaction without asserting if witness is complete
 	pub fn without_valid_witness(&self) -> &Transaction { &self.tx }
+	/// Get raw transaction with asserting witness is complete
 	pub fn with_valid_witness(&self) -> &Transaction {
 		assert!(self.has_local_sig());
 		&self.tx

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()[..]);

lightning/src/ln/channelmonitor.rs

@@ -1806,7 +1806,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		// 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<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		} 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 {

lightning/src/util/enforcing_trait_impls.rs

@@ -1,11 +1,13 @@
-use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys};
+use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction};
 use ln::msgs;
 use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys};
 
 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 +80,10 @@ 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<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
+		self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx)
+	}
+
 	fn sign_closing_transaction<T: secp256k1::Signing>(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
 		Ok(self.inner.sign_closing_transaction(closing_tx, secp_ctx).unwrap())
 	}