Make ChannelSigner solely responsible for validating commitment sigs

As part of the custom transactions project, we want to make channel
generic over any funding, htlc, and revokeable scripts used on-chain.

Hence, this commit removes the validation of holder commitment
signatures from channel, as it requires building the funding, htlc, and
revokeable scripts to create the expected sighash.

This signature validation is now the sole responsibility of
`ChannelSigner::validate_holder_commitment`.

This commit updates `InMemorySigner` to honor this new API contract.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -21,10 +21,9 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::sha256d::Hash as Sha256d;
 use bitcoin::hashes::Hash;
 
-use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE;
-use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1};
-use bitcoin::secp256k1::{PublicKey, SecretKey};
-use bitcoin::{secp256k1, sighash};
+use bitcoin::secp256k1::{
+	self, constants::PUBLIC_KEY_SIZE, ecdsa::Signature, PublicKey, Secp256k1, SecretKey,
+};
 
 use crate::chain::chaininterface::{
 	fee_for_weight, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator,
@@ -2512,29 +2511,6 @@ where
 
 	fn received_msg(&self) -> &'static str;
 
-	#[rustfmt::skip]
-	fn check_counterparty_commitment_signature<L: Deref>(
-		&self, sig: &Signature, holder_commitment_point: &HolderCommitmentPoint, logger: &L
-	) -> Result<CommitmentTransaction, ChannelError> where L::Target: Logger {
-		let funding_script = self.funding().get_funding_redeemscript();
-
-		let commitment_data = self.context().build_commitment_transaction(self.funding(),
-			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
-			true, false, logger);
-		let initial_commitment_tx = commitment_data.tx;
-		let trusted_tx = initial_commitment_tx.trust();
-		let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
-		let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis());
-		// They sign the holder commitment transaction...
-		log_trace!(logger, "Checking {} tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} for channel {}.",
-			self.received_msg(), log_bytes!(sig.serialize_compact()[..]), log_bytes!(self.funding().counterparty_funding_pubkey().serialize()),
-			encode::serialize_hex(&initial_commitment_bitcoin_tx.transaction), log_bytes!(sighash[..]),
-			encode::serialize_hex(&funding_script), &self.context().channel_id());
-		secp_check!(self.context().secp_ctx.verify_ecdsa(&sighash, sig, self.funding().counterparty_funding_pubkey()), format!("Invalid {} signature from peer", self.received_msg()));
-
-		Ok(initial_commitment_tx)
-	}
-
 	#[rustfmt::skip]
 	fn initial_commitment_signed<L: Deref>(
 		&mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &mut HolderCommitmentPoint,
@@ -2543,32 +2519,11 @@ where
 	where
 		L::Target: Logger
 	{
-		let initial_commitment_tx = match self.check_counterparty_commitment_signature(&counterparty_signature, holder_commitment_point, logger) {
-			Ok(res) => res,
-			Err(ChannelError::Close(e)) => {
-				// TODO(dual_funding): Update for V2 established channels.
-				if !self.funding().is_outbound() {
-					self.funding_mut().channel_transaction_parameters.funding_outpoint = None;
-				}
-				return Err(ChannelError::Close(e));
-			},
-			Err(e) => {
-				// The only error we know how to handle is ChannelError::Close, so we fall over here
-				// to make sure we don't continue with an inconsistent state.
-				panic!("unexpected error type from check_counterparty_commitment_signature {:?}", e);
-			}
-		};
 		let context = self.context();
-		let commitment_data = context.build_commitment_transaction(self.funding(),
-			context.cur_counterparty_commitment_transaction_number,
-			&context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
-		let counterparty_initial_commitment_tx = commitment_data.tx;
-		let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
-		let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
-
-		log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
-			&context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));
 
+		let initial_commitment_tx = context.build_commitment_transaction(self.funding(),
+			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
+			true, false, logger).tx;
 		let holder_commitment_tx = HolderCommitmentTransaction::new(
 			initial_commitment_tx,
 			counterparty_signature,
@@ -2577,10 +2532,29 @@ where
 			&self.funding().counterparty_funding_pubkey()
 		);
 
-		if context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()).is_err() {
+		log_trace!(logger, "Validating {} commitment in channel {}", self.received_msg(), context.channel_id());
+		if context.holder_signer.as_ref().validate_holder_commitment(
+			&self.funding().channel_transaction_parameters,
+			&holder_commitment_tx,
+			Vec::new(),
+			&context.secp_ctx,
+		).is_err() {
+			if !self.funding().is_outbound() {
+				self.funding_mut().channel_transaction_parameters.funding_outpoint = None;
+			}
 			return Err(ChannelError::close("Failed to validate our commitment".to_owned()));
 		}
 
+		let commitment_data = context.build_commitment_transaction(self.funding(),
+			context.cur_counterparty_commitment_transaction_number,
+			&context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
+		let counterparty_initial_commitment_tx = commitment_data.tx;
+		let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
+		let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
+
+		log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
+			&context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));
+
 		// Now that we're past error-generating stuff, update our local state:
 
 		let is_v2_established = self.is_v2_established();
@@ -4207,25 +4181,30 @@ where
 	where
 		L::Target: Logger,
 	{
-		let funding_script = funding.get_funding_redeemscript();
-
 		let commitment_data = self.build_commitment_transaction(funding,
 			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
 			true, false, logger);
-		let commitment_txid = {
-			let trusted_tx = commitment_data.tx.trust();
-			let bitcoin_tx = trusted_tx.built_transaction();
-			let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_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!(funding.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction),
-				log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.channel_id());
-			if let Err(_) = self.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &funding.counterparty_funding_pubkey()) {
-				return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned()));
-			}
-			bitcoin_tx.txid
-		};
+
+		if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() {
+			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len())));
+		}
+
+		let holder_commitment_tx = HolderCommitmentTransaction::new(
+			commitment_data.tx,
+			msg.signature,
+			msg.htlc_signatures.clone(),
+			&funding.get_holder_pubkeys().funding_pubkey,
+			funding.counterparty_funding_pubkey()
+		);
+
+		log_trace!(logger, "Validating commitment_signed commitment in channel {}", self.channel_id());
+		self.holder_signer.as_ref().validate_holder_commitment(
+			&funding.channel_transaction_parameters,
+			&holder_commitment_tx,
+			commitment_data.outbound_htlc_preimages,
+			&self.secp_ctx,
+		)
+			.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
 
 		// If our counterparty updated the channel fee in this commitment transaction, check that
 		// they can actually afford the new fee now.
@@ -4257,28 +4236,10 @@ where
 			}
 		}
 
-		if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() {
-			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len())));
-		}
-
-		let holder_keys = commitment_data.tx.trust().keys();
-		let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len());
-		let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - commitment_data.tx.nondust_htlcs().len());
-		for (idx, (htlc, mut source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() {
+		let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len());
+		let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - holder_commitment_tx.nondust_htlcs().len());
+		for (htlc, mut source_opt) in commitment_data.htlcs_included.into_iter() {
 			if let Some(_) = htlc.transaction_output_index {
-				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(),
-					funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, funding.get_channel_type(),
-					&holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key);
-
-				let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &holder_keys);
-				let htlc_sighashtype = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All };
-				let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]);
-				log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.",
-					log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(holder_keys.countersignatory_htlc_key.to_public_key().serialize()),
-					encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.channel_id());
-				if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &holder_keys.countersignatory_htlc_key.to_public_key()) {
-					return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned()));
-				}
 				if htlc.offered {
 					if let Some(source) = source_opt.take() {
 						nondust_htlc_sources.push(source.clone());
@@ -4292,17 +4253,6 @@ where
 			debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
 		}
 
-		let holder_commitment_tx = HolderCommitmentTransaction::new(
-			commitment_data.tx,
-			msg.signature,
-			msg.htlc_signatures.clone(),
-			&funding.get_holder_pubkeys().funding_pubkey,
-			funding.counterparty_funding_pubkey()
-		);
-
-		self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_data.outbound_htlc_preimages)
-			.map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?;
-
 		Ok(LatestHolderCommitmentTXInfo {
 			commitment_tx: holder_commitment_tx,
 			htlc_outputs: dust_htlcs,

lightning/src/ln/functional_tests.rs

@@ -8892,7 +8892,7 @@ pub fn test_duplicate_funding_err_in_funding() {
 	funding_created_msg.funding_output_index += 10;
 	nodes[1].node.handle_funding_created(node_c_id, &funding_created_msg);
 	get_err_msg(&nodes[1], &node_c_id);
-	let err = "Invalid funding_created signature from peer".to_owned();
+	let err = "Failed to validate our commitment".to_owned();
 	let reason = ClosureReason::ProcessingError { err };
 	let expected_closing = ExpectedCloseEvent::from_id_reason(real_channel_id, false, reason);
 	check_closed_events(&nodes[1], &[expected_closing]);

lightning/src/sign/mod.rs

@@ -763,8 +763,9 @@ pub trait ChannelSigner {
 	/// closed. If you wish to make this operation asynchronous, you should instead return `Ok(())`
 	/// and pause future signing operations until this validation completes.
 	fn validate_holder_commitment(
-		&self, holder_tx: &HolderCommitmentTransaction,
-		outbound_htlc_preimages: Vec<PaymentPreimage>,
+		&self, channel_parameters: &ChannelTransactionParameters,
+		holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>,
+		secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> Result<(), ()>;
 
 	/// Validate the counterparty's revocation.
@@ -1362,9 +1363,68 @@ impl ChannelSigner for InMemorySigner {
 	}
 
 	fn validate_holder_commitment(
-		&self, _holder_tx: &HolderCommitmentTransaction,
-		_outbound_htlc_preimages: Vec<PaymentPreimage>,
+		&self, channel_parameters: &ChannelTransactionParameters,
+		holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec<PaymentPreimage>,
+		secp_ctx: &secp256k1::Secp256k1<secp256k1::All>,
 	) -> Result<(), ()> {
+		let counterparty_funding_pubkey =
+			&channel_parameters.counterparty_pubkeys().as_ref().unwrap().funding_pubkey;
+		let funding_script = channel_parameters.make_funding_redeemscript();
+		let channel_value_satoshis = channel_parameters.channel_value_satoshis;
+		let trusted_tx = holder_tx.trust();
+		let bitcoin_tx = trusted_tx.built_transaction();
+		let sighash = bitcoin_tx.get_sighash_all(&funding_script, channel_value_satoshis);
+
+		secp_ctx
+			.verify_ecdsa(&sighash, &holder_tx.counterparty_sig, counterparty_funding_pubkey)
+			.map_err(|_| ())?;
+
+		let holder_keys = trusted_tx.keys();
+
+		if holder_tx.nondust_htlcs().len() != holder_tx.counterparty_htlc_sigs.len() {
+			return Err(());
+		}
+		for (htlc, sig) in
+			holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter())
+		{
+			let htlc_tx = chan_utils::build_htlc_transaction(
+				&bitcoin_tx.txid,
+				holder_tx.feerate_per_kw(),
+				channel_parameters.as_holder_broadcastable().contest_delay(),
+				&htlc,
+				&channel_parameters.channel_type_features,
+				&holder_keys.broadcaster_delayed_payment_key,
+				&holder_keys.revocation_key,
+			);
+			let htlc_redeemscript = chan_utils::get_htlc_redeemscript(
+				&htlc,
+				&channel_parameters.channel_type_features,
+				&holder_keys,
+			);
+			let htlc_sighashtype =
+				if channel_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() {
+					EcdsaSighashType::SinglePlusAnyoneCanPay
+				} else {
+					EcdsaSighashType::All
+				};
+			let htlc_sighash = hash_to_message!(
+				&sighash::SighashCache::new(&htlc_tx)
+					.p2wsh_signature_hash(
+						0,
+						&htlc_redeemscript,
+						htlc.to_bitcoin_amount(),
+						htlc_sighashtype
+					)
+					.unwrap()[..]
+			);
+			secp_ctx
+				.verify_ecdsa(
+					&htlc_sighash,
+					&sig,
+					&holder_keys.countersignatory_htlc_key.to_public_key(),
+				)
+				.map_err(|_| ())?;
+		}
 		Ok(())
 	}
 

lightning/src/util/dyn_signer.rs

@@ -172,8 +172,10 @@ delegate!(DynSigner, ChannelSigner,
 	) -> Result<PublicKey, ()>,
 	fn release_commitment_secret(, idx: u64) -> Result<[u8; 32], ()>,
 	fn validate_holder_commitment(,
+		channel_parameters: &ChannelTransactionParameters,
 		holder_tx: &HolderCommitmentTransaction,
-		preimages: Vec<PaymentPreimage>
+		preimages: Vec<PaymentPreimage>,
+		secp_ctx: &Secp256k1<All>
 	) -> Result<(), ()>,
 	fn pubkeys(,
 		splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>

lightning/src/util/test_channel_signer.rs

@@ -191,8 +191,9 @@ impl ChannelSigner for TestChannelSigner {
 	}
 
 	fn validate_holder_commitment(
-		&self, holder_tx: &HolderCommitmentTransaction,
-		_outbound_htlc_preimages: Vec<PaymentPreimage>,
+		&self, channel_parameters: &ChannelTransactionParameters,
+		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();
@@ -205,7 +206,12 @@ impl ChannelSigner for TestChannelSigner {
 			);
 		}
 		state.last_holder_commitment = idx;
-		Ok(())
+		self.inner.validate_holder_commitment(
+			channel_parameters,
+			holder_tx,
+			outbound_htlc_preimages,
+			secp_ctx,
+		)
 	}
 
 	fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> {