Return Result<Signature> instead of modifying args in ChannelKeys

This cleans up sign_local_commitment somewhat by returning a
Result<Signaure, ()> over the local commitment transaction instead
of modifying the struct which was passed in.

This is the first step in making LocalCommitmentTransaction a
completely pub struct, using it just to communicate enough
information to the user to allow them to construct a signaure
instead of having it contain a bunch of logic.

This should make it much easier to implement a custom ChannelKeys
by disconnecting the local commitment transaction signing from our
own datastructures.

Author: TheBlueMatt

lightning/src/chain/keysinterface.rs

@@ -216,21 +216,20 @@ 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
+	/// Create a signature for a local commitment transaction. This will only ever be called with
+	/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
+	/// that it will not be called multiple times.
 	///
 	/// 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<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
 
-	/// 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.
+	/// Same as sign_local_commitment, but exists only for tests to get access to local commitment
+	/// transactions which will be broadcasted later, after the channel has moved on to a newer
+	/// state. Thus, needs its own method as sign_local_commitment may enforce that we only ever
+	/// get called once.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
+	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!
@@ -363,21 +362,21 @@ impl ChannelKeys for InMemoryChannelKeys {
 		Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
 		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
 		let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
 		let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
 
-		local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
+		Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
 		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
 		let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
 		let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
 
-		local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
+		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>) {

lightning/src/ln/chan_utils.rs

@@ -555,7 +555,7 @@ impl LocalCommitmentTransaction {
 	}
 
 	/// Check if LocalCommitmentTransaction has already been signed by us
-	pub fn has_local_sig(&self) -> bool {
+	pub(crate) 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 {
 			assert!(!self.tx.input[0].witness[1].is_empty());
@@ -569,20 +569,23 @@ impl LocalCommitmentTransaction {
 		}
 	}
 
-	/// Add local signature for LocalCommitmentTransaction, do nothing if signature is already
-	/// present
+	/// Gets a local signature for the contained commitment transaction.
 	///
 	/// 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<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; }
+	pub fn get_local_sig<T: secp256k1::Signing>(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) -> Signature {
 		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);
+		secp_ctx.sign(&sighash, funding_key)
+	}
+
+
+	pub(crate) fn add_local_sig(&mut self, funding_redeemscript: &Script, our_sig: Signature) {
+		if self.has_local_sig() { return; }
 
 		if self.tx.input[0].witness[1].is_empty() {
 			self.tx.input[0].witness[1] = our_sig.serialize_der().to_vec();
@@ -598,7 +601,7 @@ impl LocalCommitmentTransaction {
 	/// 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 {
+	pub(crate) fn with_valid_witness(&self) -> &Transaction {
 		assert!(self.has_local_sig());
 		&self.tx
 	}

lightning/src/ln/channel.rs

@@ -4521,7 +4521,8 @@ mod tests {
 				assert_eq!(unsigned_tx.1.len(), per_htlc.len());
 
 				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(), keys.clone(), chan.feerate_per_kw, per_htlc);
-				chan_keys.sign_local_commitment(&mut localtx, &chan.secp_ctx);
+				let local_sig = chan_keys.sign_local_commitment(&localtx, &chan.secp_ctx).unwrap();
+				localtx.add_local_sig(&redeemscript, local_sig);
 
 				assert_eq!(serialize(localtx.with_valid_witness())[..],
 						hex::decode($tx_hex).unwrap()[..]);

lightning/src/ln/channelmonitor.rs

@@ -442,7 +442,9 @@ pub(crate) enum InputMaterial {
 		preimage: Option<PaymentPreimage>,
 		amount: u64,
 	},
-	Funding {}
+	Funding {
+		funding_redeemscript: Script,
+	}
 }
 
 impl Writeable for InputMaterial  {
@@ -469,8 +471,9 @@ impl Writeable for InputMaterial  {
 				preimage.write(writer)?;
 				writer.write_all(&byte_utils::be64_to_array(*amount))?;
 			},
-			&InputMaterial::Funding {} => {
+			&InputMaterial::Funding { ref funding_redeemscript } => {
 				writer.write_all(&[3; 1])?;
+				funding_redeemscript.write(writer)?;
 			}
 		}
 		Ok(())
@@ -517,7 +520,9 @@ impl Readable for InputMaterial {
 				}
 			},
 			3 => {
-				InputMaterial::Funding {}
+				InputMaterial::Funding {
+					funding_redeemscript: Readable::read(reader)?,
+				}
 			}
 			_ => return Err(DecodeError::InvalidValue),
 		};
@@ -1771,7 +1776,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	pub fn get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
 		log_trace!(self, "Getting signed latest local commitment transaction!");
 		self.local_tx_signed = true;
-		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
+		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
@@ -1795,7 +1800,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	#[cfg(test)]
 	pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
 		log_trace!(self, "Getting signed copy of latest local commitment transaction!");
-		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx() {
+		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(&self.funding_redeemscript) {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
@@ -1873,10 +1878,10 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		}
 		let should_broadcast = self.would_broadcast_at_height(height);
 		if should_broadcast {
-			claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.funding_info.0.txid.clone(), vout: self.funding_info.0.index as u32 }, witness_data: InputMaterial::Funding {}});
+			claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.funding_info.0.txid.clone(), vout: self.funding_info.0.index as u32 }, witness_data: InputMaterial::Funding { funding_redeemscript: self.funding_redeemscript.clone() }});
 		}
 		if should_broadcast {
-			if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
+			if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) {
 				let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, &self.current_local_commitment_tx);
 				if !new_outputs.is_empty() {
 					watch_outputs.push((self.current_local_commitment_tx.txid.clone(), new_outputs));

lightning/src/ln/onchaintx.rs

@@ -531,8 +531,8 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 						}
 						return None;
 					},
-					&InputMaterial::Funding {} => {
-						let signed_tx = self.get_fully_signed_local_tx().unwrap();
+					&InputMaterial::Funding { ref funding_redeemscript } => {
+						let signed_tx = self.get_fully_signed_local_tx(funding_redeemscript).unwrap();
 						// Timer set to $NEVER given we can't bump tx without anchor outputs
 						log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid);
 						return Some((None, self.local_commitment.as_ref().unwrap().feerate_per_kw, signed_tx));
@@ -780,19 +780,25 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 	// have empty local commitment transaction if a ChannelMonitor is asked to force-close just after Channel::get_outbound_funding_created,
 	// before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing
 	// to monitor before.
-	pub(super) fn get_fully_signed_local_tx(&mut self) -> Option<Transaction> {
+	pub(super) fn get_fully_signed_local_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
 		if let Some(ref mut local_commitment) = self.local_commitment {
-			self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx);
+			match self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx) {
+				Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig),
+				Err(_) => return None,
+			}
 			return Some(local_commitment.with_valid_witness().clone());
 		}
 		None
 	}
 
 	#[cfg(test)]
-	pub(super) fn get_fully_signed_copy_local_tx(&mut self) -> Option<Transaction> {
+	pub(super) fn get_fully_signed_copy_local_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
 		if let Some(ref mut local_commitment) = self.local_commitment {
 			let mut local_commitment = local_commitment.clone();
-			self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.secp_ctx);
+			match self.key_storage.sign_local_commitment(&local_commitment, &self.secp_ctx) {
+				Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig),
+				Err(_) => return None,
+			}
 			return Some(local_commitment.with_valid_witness().clone());
 		}
 		None

lightning/src/util/enforcing_trait_impls.rs

@@ -79,13 +79,13 @@ 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, secp_ctx: &Secp256k1<T>) {
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
 		self.inner.sign_local_commitment(local_commitment_tx, secp_ctx)
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
-		self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
+		self.inner.unsafe_sign_local_commitment(local_commitment_tx, 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>) {