Remove signing htlc transaction from ChannelMonitor

Extend external signer interface to sign HTLC transactions on its
behalf without seckey passing. This move will allow us to remove
key access access from ChannelMonitor hot memory in further work.

HTLC transactions should stay half-signed by remote until
we need to broadcast them for timing-out/claiming HTLCs onchain.

Author: Antoine Riard

lightning/src/chain/keysinterface.rs

@@ -25,6 +25,7 @@ use util::ser::{Writeable, Writer, Readable};
 
 use ln::chan_utils;
 use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys};
+use ln::channelmanager::PaymentPreimage;
 use ln::msgs;
 
 use std::sync::Arc;
@@ -222,6 +223,10 @@ pub trait ChannelKeys : Send+Clone {
 	/// 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>);
 
+	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
+	/// HTLC-Success transaction (ie htlc.offered is false), preimage must be set!
+	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, 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
@@ -378,6 +383,40 @@ impl ChannelKeys for InMemoryChannelKeys {
 		local_commitment_tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec());
 	}
 
+	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1<T>) {
+		if htlc_tx.input.len() != 1 { return; }
+		if htlc_tx.input[0].witness.len() != 0 { return; }
+
+		let htlc_redeemscript = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key);
+
+		if let Ok(our_htlc_key) = chan_utils::derive_private_key(secp_ctx, per_commitment_point, &self.htlc_base_key) {
+			let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
+			let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key;
+			let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
+
+			htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
+
+			if local_tx { // b, then a
+				htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec());
+				htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec());
+			} else {
+				htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec());
+				htlc_tx.input[0].witness.push(their_sig.serialize_der().to_vec());
+			}
+			htlc_tx.input[0].witness[1].push(SigHashType::All as u8);
+			htlc_tx.input[0].witness[2].push(SigHashType::All as u8);
+
+			if htlc.offered {
+				htlc_tx.input[0].witness.push(Vec::new());
+				assert!(preimage.is_none());
+			} else {
+				htlc_tx.input[0].witness.push(preimage.unwrap().0.to_vec());
+			}
+
+			htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());
+		} else { return; }
+	}
+
 	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

@@ -6,15 +6,14 @@ use bitcoin::blockdata::script::{Script,Builder};
 use bitcoin::blockdata::opcodes;
 use bitcoin::blockdata::transaction::{TxIn,TxOut,OutPoint,Transaction, SigHashType};
 use bitcoin::consensus::encode::{self, Decodable, Encodable};
-use bitcoin::util::bip143;
 
 use bitcoin_hashes::{Hash, HashEngine};
 use bitcoin_hashes::sha256::Hash as Sha256;
 use bitcoin_hashes::ripemd160::Hash as Ripemd160;
 use bitcoin_hashes::hash160::Hash as Hash160;
 use bitcoin_hashes::sha256d::Hash as Sha256dHash;
 
-use ln::channelmanager::{PaymentHash, PaymentPreimage};
+use ln::channelmanager::PaymentHash;
 use ln::msgs::DecodeError;
 use util::ser::{Readable, Writeable, Writer, WriterWriteAdaptor};
 use util::byte_utils;
@@ -355,7 +354,7 @@ impl_writeable!(HTLCOutputInCommitment, 1 + 8 + 4 + 32 + 5, {
 });
 
 #[inline]
-pub(super) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
+pub(crate) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
 	let payment_hash160 = Ripemd160::hash(&htlc.payment_hash.0[..]).into_inner();
 	if htlc.offered {
 		Builder::new().push_opcode(opcodes::all::OP_DUP)
@@ -475,43 +474,6 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s
 	}
 }
 
-/// Signs a transaction created by build_htlc_transaction. If the transaction is an
-/// HTLC-Success transaction (ie htlc.offered is false), preimage must be set!
-pub(crate) fn sign_htlc_transaction<T: secp256k1::Signing>(tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, htlc_base_key: &SecretKey, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Script), ()> {
-	if tx.input.len() != 1 { return Err(()); }
-	if tx.input[0].witness.len() != 0 { return Err(()); }
-
-	let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&htlc, a_htlc_key, b_htlc_key, revocation_key);
-
-	let our_htlc_key = derive_private_key(secp_ctx, per_commitment_point, htlc_base_key).map_err(|_| ())?;
-	let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
-	let local_tx = PublicKey::from_secret_key(&secp_ctx, &our_htlc_key) == *a_htlc_key;
-	let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
-
-	tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
-
-	if local_tx { // b, then a
-		tx.input[0].witness.push(their_sig.serialize_der().to_vec());
-		tx.input[0].witness.push(our_sig.serialize_der().to_vec());
-	} else {
-		tx.input[0].witness.push(our_sig.serialize_der().to_vec());
-		tx.input[0].witness.push(their_sig.serialize_der().to_vec());
-	}
-	tx.input[0].witness[1].push(SigHashType::All as u8);
-	tx.input[0].witness[2].push(SigHashType::All as u8);
-
-	if htlc.offered {
-		tx.input[0].witness.push(Vec::new());
-		assert!(preimage.is_none());
-	} else {
-		tx.input[0].witness.push(preimage.unwrap().0.to_vec());
-	}
-
-	tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());
-
-	Ok((our_sig, htlc_redeemscript))
-}
-
 #[derive(Clone)]
 /// 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

lightning/src/ln/channel.rs

@@ -4519,7 +4519,7 @@ mod tests {
 					assert!(preimage.is_some());
 				}
 
-				chan_utils::sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, chan.local_keys.htlc_base_key(), &chan.secp_ctx).unwrap();
+				chan_keys.sign_htlc_transaction(&mut htlc_tx, &remote_signature, &preimage, &htlc, &keys.a_htlc_key, &keys.b_htlc_key, &keys.revocation_key, &keys.per_commitment_point, &chan.secp_ctx);
 				assert_eq!(serialize(&htlc_tx)[..],
 						hex::decode($tx_hex).unwrap()[..]);
 			};

lightning/src/ln/channelmonitor.rs

@@ -1694,23 +1694,15 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					if htlc.offered {
 						log_trace!(self, "Broadcasting HTLC-Timeout transaction against local commitment transactions");
 						let mut htlc_timeout_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key);
-						let (our_sig, htlc_script) = match
-								chan_utils::sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.onchain_detection.keys.htlc_base_key(), &self.secp_ctx) {
-							Ok(res) => res,
-							Err(_) => continue,
-						};
+						self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx);
 
 						log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_timeout_tx.input[0].previous_output.vout, htlc_timeout_tx.input[0].previous_output.txid);
 						res.push(htlc_timeout_tx);
 					} else {
 						if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) {
 							log_trace!(self, "Broadcasting HTLC-Success transaction against local commitment transactions");
 							let mut htlc_success_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key);
-							let (our_sig, htlc_script) = match
-									chan_utils::sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.onchain_detection.keys.htlc_base_key(), &self.secp_ctx) {
-								Ok(res) => res,
-								Err(_) => continue,
-							};
+							self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx);
 
 							log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_success_tx.input[0].previous_output.vout, htlc_success_tx.input[0].previous_output.txid);
 							res.push(htlc_success_tx);

lightning/src/util/enforcing_trait_impls.rs

@@ -1,4 +1,5 @@
 use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys};
+use ln::channelmanager::PaymentPreimage;
 use ln::msgs;
 use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys};
 
@@ -84,6 +85,10 @@ impl ChannelKeys for EnforcingChannelKeys {
 		self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx)
 	}
 
+	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, htlc_tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1<T>) {
+		self.inner.sign_htlc_transaction(htlc_tx, their_sig, preimage, htlc, a_htlc_key, b_htlc_key, revocation_key, per_commitment_point, 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())
 	}