Batch-sign local HTLC txn with a well-doc'd API, returning sigs

1107ab0 introduced an API to have a
ChannelKeys implementer sign HTLC transactions by calling into the
LocalCommitmentTransaction object, which would then store the tx.

This API was incredibly awkward, both because it required an
external signer trust our own internal interfaces, but also because
it didn't allow for any inspection of what was about to be signed.

Further, it signed the HTLC transactions one-by-one in a somewhat
inefficient way, and there isn't a clear way to resolve this (as
the which-HTLC parameter has to refer to something in between the
HTLC's arbitrary index, and its index in the commitment tx, which
has "holes" for the non-HTLC outputs and skips some HTLCs).

We replace it with a new function in ChannelKeys which allows us
to sign all HTLCs in a given commitment transaction (which allows
for a bit more effeciency on the signers' part, as well as
sidesteps the which-HTLC issue). This may also simplify the signer
implementation as we will always want to sign all HTLCs spending a
given commitment transaction at once anyway.

We also de-mut the LocalCommitmentTransaction passed to the
ChanKeys, instead opting to make LocalCommitmentTransaction const
and avoid storing any new HTLC-related data in it.

Author: TheBlueMatt

lightning/src/chain/keysinterface.rs

@@ -25,7 +25,6 @@ use util::ser::{Writeable, Writer, Readable};
 
 use ln::chan_utils;
 use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction};
-use ln::channelmanager::PaymentPreimage;
 use ln::msgs;
 
 use std::sync::Arc;
@@ -231,10 +230,21 @@ pub trait ChannelKeys : Send+Clone {
 	#[cfg(test)]
 	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
 
-	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
-	/// HTLC-Success transaction, preimage must be set!
-	/// TODO: should be merged with sign_local_commitment as a slice of HTLC transactions to sign
-	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>);
+	/// Create a signature for each HTLC transaction spending a local commitment transaction.
+	///
+	/// Unlike sign_local_commitment, this may be called multiple times with *different*
+	/// local_commitment_tx values. While this will never be called with a revoked
+	/// local_commitment_tx, it is possible that it is called with the second-latest
+	/// local_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
+	/// local_commitment_tx. For those HTLCs which have transaction_output_index set to None
+	/// (implying they were considered dust at the time the commitment transaction was negotiated),
+	/// a corresponding None should be included in the return value. All other positions in the
+	/// return value must contain a signature.
+	fn sign_local_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()>;
+
 	/// Create a signature for a (proposed) closing transaction.
 	///
 	/// Note that, due to rounding, there may be one "missing" satoshi, and either party may have
@@ -379,8 +389,8 @@ impl ChannelKeys for InMemoryChannelKeys {
 		Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
 	}
 
-	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
-		local_commitment_tx.add_htlc_sig(&self.htlc_base_key, htlc_index, preimage, local_csv, secp_ctx);
+	fn sign_local_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()> {
+		local_commitment_tx.get_htlc_sigs(&self.htlc_base_key, local_csv, secp_ctx)
 	}
 
 	fn sign_closing_transaction<T: secp256k1::Signing>(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {

lightning/src/ln/chan_utils.rs

@@ -524,7 +524,7 @@ impl 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, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> LocalCommitmentTransaction {
+	pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_data: Vec<(HTLCOutputInCommitment, Option<Signature>)>) -> 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?"); }
 
@@ -543,8 +543,7 @@ impl LocalCommitmentTransaction {
 		Self { tx,
 			local_keys,
 			feerate_per_kw,
-			// TODO: Avoid the conversion of a Vec created likely just for this:
-			per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(),
+			per_htlc: htlc_data,
 		}
 	}
 
@@ -606,55 +605,68 @@ impl LocalCommitmentTransaction {
 		&self.tx
 	}
 
-	/// Add local signature for a htlc transaction, do nothing if a cached signed transaction is
-	/// already present
-	pub fn add_htlc_sig<T: secp256k1::Signing>(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
+	/// Get a signature for each HTLC which was included in the commitment transaction (ie for
+	/// which HTLCOutputInCommitment::transaction_output_index.is_some()).
+	///
+	/// The returned Vec has one entry for each HTLC, and in the same order. For HTLCs which were
+	/// considered dust and not included, a None entry exists, for all others a signature is
+	/// included.
+	pub fn get_htlc_sigs<T: secp256k1::Signing + secp256k1::Verification>(&self, htlc_base_key: &SecretKey, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()> {
 		let txid = self.txid();
-		for this_htlc in self.per_htlc.iter_mut() {
-			if this_htlc.0.transaction_output_index == Some(htlc_index) {
-				if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index
-				let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
-				if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate
-				let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
-				if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc
-				if htlc_tx.input.len() != 1 { return; }
-				if htlc_tx.input[0].witness.len() != 0 { return; }
-
-				let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
-
-				if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) {
-					let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
-					let our_sig = secp_ctx.sign(&sighash, &our_htlc_key);
+		let mut ret = Vec::with_capacity(self.per_htlc.len());
+		let our_htlc_key = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key).map_err(|_| ())?;
 
-					htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
-
-					htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec());
-					htlc_tx.input[0].witness.push(our_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 this_htlc.0.offered {
-						htlc_tx.input[0].witness.push(Vec::new());
-						assert!(htlc_secret.is_none());
-					} else {
-						htlc_tx.input[0].witness.push(htlc_secret.unwrap().0.to_vec());
-					}
+		for this_htlc in self.per_htlc.iter() {
+			if this_htlc.0.transaction_output_index.is_some() {
+				let htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
+				assert_eq!(htlc_tx.input.len(), 1);
+				assert_eq!(htlc_tx.input[0].witness.len(), 0);
 
-					htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());
+				let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
 
-					this_htlc.2 = Some(htlc_tx);
-				} else { return; }
+				let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]);
+				ret.push(Some(secp_ctx.sign(&sighash, &our_htlc_key)));
+			} else {
+				ret.push(None);
 			}
 		}
+		Ok(ret)
 	}
-	/// Expose raw htlc transaction, guarante witness is complete if non-empty
-	pub fn htlc_with_valid_witness(&self, htlc_index: u32) -> &Option<Transaction> {
-		for this_htlc in self.per_htlc.iter() {
-			if this_htlc.0.transaction_output_index.unwrap() == htlc_index {
-				return &this_htlc.2;
-			}
+
+	/// Gets a signed HTLC transaction given a preimage (for !htlc.offered) and the local HTLC transaction signature.
+	pub(crate) fn get_signed_htlc_tx(&self, htlc_index: usize, signature: &Signature, preimage: &Option<PaymentPreimage>, local_csv: u16) -> Transaction {
+		let txid = self.txid();
+		let this_htlc = &self.per_htlc[htlc_index];
+		assert!(this_htlc.0.transaction_output_index.is_some());
+		// if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction.
+		if !this_htlc.0.offered && preimage.is_none() { unreachable!(); }
+		// Further, we should never be provided the preimage for an HTLC-Timeout transaction.
+		if  this_htlc.0.offered && preimage.is_some() { unreachable!(); }
+
+		let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key);
+		// Channel should have checked that we have a remote signature for this HTLC at
+		// creation, and we should have a sensible htlc transaction:
+		assert!(this_htlc.1.is_some());
+		assert_eq!(htlc_tx.input.len(), 1);
+		assert_eq!(htlc_tx.input[0].witness.len(), 0);
+
+		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
+
+		htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
+
+		htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec());
+		htlc_tx.input[0].witness.push(signature.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 this_htlc.0.offered {
+			htlc_tx.input[0].witness.push(Vec::new());
+		} else {
+			htlc_tx.input[0].witness.push(preimage.unwrap().0.to_vec());
 		}
-		&None
+
+		htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec());
+		htlc_tx
 	}
 }
 impl PartialEq for LocalCommitmentTransaction {
@@ -674,10 +686,9 @@ impl Writeable for LocalCommitmentTransaction {
 		self.local_keys.write(writer)?;
 		self.feerate_per_kw.write(writer)?;
 		writer.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?;
-		for &(ref htlc, ref sig, ref htlc_tx) in self.per_htlc.iter() {
+		for &(ref htlc, ref sig) in self.per_htlc.iter() {
 			htlc.write(writer)?;
 			sig.write(writer)?;
-			htlc_tx.write(writer)?;
 		}
 		Ok(())
 	}
@@ -694,12 +705,11 @@ impl Readable for LocalCommitmentTransaction {
 		let local_keys = Readable::read(reader)?;
 		let feerate_per_kw = Readable::read(reader)?;
 		let htlcs_count: u64 = Readable::read(reader)?;
-		let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option<Signature>, Option<Transaction>)>()));
+		let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option<Signature>)>()));
 		for _ in 0..htlcs_count {
 			let htlc: HTLCOutputInCommitment = Readable::read(reader)?;
 			let sigs = Readable::read(reader)?;
-			let htlc_tx = Readable::read(reader)?;
-			per_htlc.push((htlc, sigs, htlc_tx));
+			per_htlc.push((htlc, sigs));
 		}
 
 		if tx.input.len() != 1 {

lightning/src/ln/channel.rs

@@ -4527,6 +4527,9 @@ mod tests {
 				assert_eq!(serialize(localtx.with_valid_witness())[..],
 						hex::decode($tx_hex).unwrap()[..]);
 
+				let htlc_sigs = chan_keys.sign_local_commitment_htlc_transactions(&localtx, chan.their_to_self_delay, &chan.secp_ctx).unwrap();
+				let mut htlc_sig_iter = localtx.per_htlc.iter().zip(htlc_sigs.iter().enumerate());
+
 				$({
 					let remote_signature = Signature::from_der(&hex::decode($their_htlc_sig_hex).unwrap()[..]).unwrap();
 
@@ -4548,11 +4551,18 @@ mod tests {
 						assert!(preimage.is_some());
 					}
 
-					chan_keys.sign_htlc_transaction(&mut localtx, $htlc_idx, preimage, chan.their_to_self_delay, &chan.secp_ctx);
+					let mut htlc_sig = htlc_sig_iter.next().unwrap();
+					while (htlc_sig.1).1.is_none() { htlc_sig = htlc_sig_iter.next().unwrap(); }
+					assert_eq!((htlc_sig.0).0.transaction_output_index, Some($htlc_idx));
 
-					assert_eq!(serialize(localtx.htlc_with_valid_witness($htlc_idx).as_ref().unwrap())[..],
+					assert_eq!(serialize(&localtx.get_signed_htlc_tx((htlc_sig.1).0, &(htlc_sig.1).1.unwrap(), &preimage, chan.their_to_self_delay))[..],
 							hex::decode($htlc_tx_hex).unwrap()[..]);
 				})*
+				loop {
+					let htlc_sig = htlc_sig_iter.next();
+					if htlc_sig.is_none() { break; }
+					assert!((htlc_sig.unwrap().1).1.is_none());
+				}
 			}
 		}
 

lightning/src/ln/channelmonitor.rs

@@ -1677,8 +1677,18 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 
 		for &(ref htlc, _, _) in local_tx.htlc_outputs.iter() {
 			if let Some(transaction_output_index) = htlc.transaction_output_index {
-				let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { Some(*preimage) } else { None };
-				claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 }, witness_data: InputMaterial::LocalHTLC { preimage, amount: htlc.amount_msat / 1000 }});
+				claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 },
+					witness_data: InputMaterial::LocalHTLC {
+						preimage: if !htlc.offered {
+								if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) {
+									Some(preimage.clone())
+								} else {
+									// We can't build an HTLC-Success transaction without the preimage
+									continue;
+								}
+							} else { None },
+						amount: htlc.amount_msat,
+				}});
 				watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone());
 			}
 		}
@@ -1780,9 +1790,15 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
-				if let Some(htlc_index) = htlc.0.transaction_output_index {
-					let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None };
-					if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) {
+				if let Some(vout) = htlc.0.transaction_output_index {
+					let preimage = if !htlc.0.offered {
+							if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else {
+								// We can't build an HTLC-Success transaction without the preimage
+								continue;
+							}
+						} else { None };
+					if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(
+							&::bitcoin::OutPoint { txid, vout }, &preimage) {
 						res.push(htlc_tx);
 					}
 				}
@@ -1804,9 +1820,15 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
-				if let Some(htlc_index) = htlc.0.transaction_output_index {
-					let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None };
-					if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) {
+				if let Some(vout) = htlc.0.transaction_output_index {
+					let preimage = if !htlc.0.offered {
+							if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else {
+								// We can't build an HTLC-Success transaction without the preimage
+								continue;
+							}
+						} else { None };
+					if let Some(htlc_tx) = self.onchain_tx_handler.unsafe_get_fully_signed_htlc_tx(
+							&::bitcoin::OutPoint { txid, vout }, &preimage) {
 						res.push(htlc_tx);
 					}
 				}