Move commitment tx sig validation from FundedChannel to ChannelSigner

Author: tankyleo

lightning/src/ln/channel.rs

@@ -1830,7 +1830,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
 			context.counterparty_funding_pubkey()
 		);
 
-		if context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()).is_err() {
+		if context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new(), &context.secp_ctx).is_err() {
 			return Err(ChannelError::close("Failed to validate our commitment".to_owned()));
 		}
 
@@ -5245,23 +5245,28 @@ impl<SP: Deref> FundedChannel<SP> where
 			return Err(ChannelError::close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned()));
 		}
 
-		let funding_script = self.context.get_funding_redeemscript();
-
 		let keys = self.context.build_holder_transaction_keys(self.holder_commitment_point.current_point());
 
 		let commitment_stats = self.context.build_commitment_transaction(self.holder_commitment_point.transaction_number(), &keys, true, false, logger);
+
+		if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs {
+			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
+		}
+
+		let holder_commitment_tx = HolderCommitmentTransaction::new(
+			commitment_stats.tx.clone(),
+			msg.signature,
+			msg.htlc_signatures.clone(),
+			&self.context.get_holder_pubkeys().funding_pubkey,
+			self.context.counterparty_funding_pubkey()
+		);
+
+		self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages, &self.context.secp_ctx)
+			.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
+
 		let commitment_txid = {
 			let trusted_tx = commitment_stats.tx.trust();
 			let bitcoin_tx = trusted_tx.built_transaction();
-			let sighash = bitcoin_tx.get_sighash_all(&funding_script, self.context.channel_value_satoshis);
-
-			log_trace!(logger, "Checking commitment tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}",
-				log_bytes!(msg.signature.serialize_compact()[..]),
-				log_bytes!(self.context.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction),
-				log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.context.channel_id());
-			if let Err(_) = self.context.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &self.context.counterparty_funding_pubkey()) {
-				return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned()));
-			}
 			bitcoin_tx.txid
 		};
 		let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
@@ -5296,10 +5301,6 @@ impl<SP: Deref> FundedChannel<SP> where
 			}
 		}
 
-		if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs {
-			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
-		}
-
 		// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
 		// `htlcs_and_sigs` vec and in the `holder_commitment_tx` itself, both of which were passed
 		// in the `ChannelMonitorUpdate`. In 0.0.115, support for having a separate set of
@@ -5346,17 +5347,6 @@ impl<SP: Deref> FundedChannel<SP> where
 			debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
 		}
 
-		let holder_commitment_tx = HolderCommitmentTransaction::new(
-			commitment_stats.tx,
-			msg.signature,
-			msg.htlc_signatures.clone(),
-			&self.context.get_holder_pubkeys().funding_pubkey,
-			self.context.counterparty_funding_pubkey()
-		);
-
-		self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages)
-			.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
-
 		// Update state now that we've passed all the can-fail calls...
 		let mut need_commitment = false;
 		if let &mut Some((_, ref mut update_state)) = &mut self.context.pending_update_fee {

lightning/src/sign/mod.rs

@@ -697,8 +697,24 @@ pub trait ChannelSigner {
 	/// and pause future signing operations until this validation completes.
 	fn validate_holder_commitment(
 		&self, holder_tx: &HolderCommitmentTransaction,
-		outbound_htlc_preimages: Vec<PaymentPreimage>,
-	) -> Result<(), ()>;
+		_outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> Result<(), ()> {
+		let channel_value_satoshis = self.get_channel_value_satoshis();
+		let params = self.get_channel_parameters().unwrap();
+		let holder_pubkey = params.holder_pubkeys.funding_pubkey;
+		let counterparty_pubkey =
+			params.counterparty_parameters.as_ref().unwrap().pubkeys.funding_pubkey;
+		let funding_redeemscript = make_funding_redeemscript(&holder_pubkey, &counterparty_pubkey);
+		let trusted_tx = holder_tx.trust();
+		let bitcoin_tx = trusted_tx.built_transaction();
+		let sighash = bitcoin_tx.get_sighash_all(&funding_redeemscript, channel_value_satoshis);
+		if let Err(_) =
+			secp_ctx.verify_ecdsa(&sighash, &holder_tx.counterparty_sig, &counterparty_pubkey)
+		{
+			return Err(());
+		}
+		Ok(())
+	}
 
 	/// Validate the counterparty's revocation.
 	///
@@ -736,6 +752,9 @@ pub trait ChannelSigner {
 	/// Returns the parameters of this signer
 	fn get_channel_parameters(&self) -> Option<&ChannelTransactionParameters>;
 
+	/// Return the channel value
+	fn get_channel_value_satoshis(&self) -> u64;
+
 	/// Returns the script pubkey that should be placed in the `to_remote` output of commitment
 	/// transactions.
 	///
@@ -1602,13 +1621,6 @@ impl ChannelSigner for InMemorySigner {
 		Ok(chan_utils::build_commitment_secret(&self.commitment_seed, idx))
 	}
 
-	fn validate_holder_commitment(
-		&self, _holder_tx: &HolderCommitmentTransaction,
-		_outbound_htlc_preimages: Vec<PaymentPreimage>,
-	) -> Result<(), ()> {
-		Ok(())
-	}
-
 	fn validate_counterparty_revocation(&self, _idx: u64, _secret: &SecretKey) -> Result<(), ()> {
 		Ok(())
 	}
@@ -1851,6 +1863,10 @@ impl ChannelSigner for InMemorySigner {
 			sign_with_aux_rand(secp_ctx, &hash_to_message!(&sighash[..]), &self.funding_key, &self);
 		Ok(chan_utils::build_anchor_input_witness(funding_pubkey, &sig))
 	}
+
+	fn get_channel_value_satoshis(&self) -> u64 {
+		self.channel_value_satoshis
+	}
 }
 
 const MISSING_PARAMS_ERR: &'static str =

lightning/src/util/test_channel_signer.rs

@@ -191,12 +191,12 @@ impl ChannelSigner for TestChannelSigner {
 		self.inner.release_commitment_secret(idx)
 	}
 
-	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>) -> Result<(), ()> {
+	fn validate_holder_commitment(&self, holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(), ()> {
 		let mut state = self.state.lock().unwrap();
 		let idx = holder_tx.commitment_number();
 		assert!(idx == state.last_holder_commitment || idx == state.last_holder_commitment - 1, "expecting to validate the current or next holder commitment - trying {}, current {}", idx, state.last_holder_commitment);
 		state.last_holder_commitment = idx;
-		Ok(())
+		self.inner.validate_holder_commitment(holder_tx, outbound_htlc_preimages, secp_ctx)
 	}
 
 	fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> {
@@ -331,6 +331,10 @@ impl ChannelSigner for TestChannelSigner {
 	) -> Result<Witness, ()> {
 		self.inner.spend_holder_anchor_output(anchor_tx, input_idx, secp_ctx)
 	}
+
+	fn get_channel_value_satoshis(&self) -> u64 {
+		self.inner.get_channel_value_satoshis()
+	}
 }
 
 impl EcdsaChannelSigner for TestChannelSigner {