Remove signing local commitment transaction from ChannelMonitor

Author: Antoine Riard

lightning/src/chain/keysinterface.rs

@@ -2,7 +2,7 @@
 //! spendable on-chain outputs which the user owns and is responsible for using just as any other
 //! on-chain output which is theirs.
 
-use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut};
+use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut, SigHashType};
 use bitcoin::blockdata::script::{Script, Builder};
 use bitcoin::blockdata::opcodes;
 use bitcoin::network::constants::Network;
@@ -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 Transaction, 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,35 @@ impl ChannelKeys for InMemoryChannelKeys {
 		Ok((commitment_sig, htlc_sigs))
 	}
 
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut Transaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
+		if local_commitment_tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); }
+		let has_local_sig = if local_commitment_tx.input[0].witness.len() == 4 {
+			assert!(!local_commitment_tx.input[0].witness[1].is_empty());
+			assert!(!local_commitment_tx.input[0].witness[2].is_empty());
+			true
+		} else {
+			assert_eq!(local_commitment_tx.input[0].witness.len(), 3);
+			assert!(local_commitment_tx.input[0].witness[0].is_empty());
+			assert!(local_commitment_tx.input[0].witness[1].is_empty() || local_commitment_tx.input[0].witness[2].is_empty());
+			false
+		};
+		if has_local_sig { return; }
+
+		let sighash = hash_to_message!(&bip143::SighashComponents::new(&local_commitment_tx)
+			.sighash_all(&local_commitment_tx.input[0], funding_redeemscript, channel_value_satoshis)[..]);
+		let our_sig = secp_ctx.sign(&sighash, &self.funding_key);
+
+		if local_commitment_tx.input[0].witness[1].is_empty() {
+			local_commitment_tx.input[0].witness[1] = our_sig.serialize_der().to_vec();
+			local_commitment_tx.input[0].witness[1].push(SigHashType::All as u8);
+		} else {
+			local_commitment_tx.input[0].witness[2] = our_sig.serialize_der().to_vec();
+			local_commitment_tx.input[0].witness[2].push(SigHashType::All as u8);
+		}
+
+		local_commitment_tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec());
+	}
+
 	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

@@ -553,6 +553,10 @@ impl LocalCommitmentTransaction {
 		self.tx.txid()
 	}
 
+	pub fn tx(&mut self) -> &mut Transaction {
+		&mut self.tx
+	}
+
 	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,23 +571,6 @@ impl LocalCommitmentTransaction {
 		}
 	}
 
-	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)
-			.sighash_all(&self.tx.input[0], funding_redeemscript, channel_value_satoshis)[..]);
-		let our_sig = secp_ctx.sign(&sighash, funding_key);
-
-		if self.tx.input[0].witness[1].is_empty() {
-			self.tx.input[0].witness[1] = our_sig.serialize_der().to_vec();
-			self.tx.input[0].witness[1].push(SigHashType::All as u8);
-		} else {
-			self.tx.input[0].witness[2] = our_sig.serialize_der().to_vec();
-			self.tx.input[0].witness[2].push(SigHashType::All as u8);
-		}
-
-		self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec());
-	}
-
 	pub fn without_valid_witness(&self) -> &Transaction { &self.tx }
 	pub fn with_valid_witness(&self) -> &Transaction {
 		assert!(self.has_local_sig());

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(localtx.tx(), &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

@@ -1825,7 +1825,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.funding_key, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx);
+			self.onchain_detection.keys.sign_local_commitment(local_tx.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()];
@@ -1907,7 +1907,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.funding_key, self.funding_redeemscript.as_ref().unwrap(), self.channel_value_satoshis.unwrap(), &self.secp_ctx);
+				self.onchain_detection.keys.sign_local_commitment(cur_local_tx.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

@@ -6,6 +6,8 @@ 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 Transaction, 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())
 	}