Remove panics for sign_holder_commitment_and_htlcs

Author: rmalonson

lightning/src/chain/channelmonitor.rs

@@ -43,6 +43,7 @@ use crate::chain;
 use crate::chain::{BestBlock, WatchedOutput};
 use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
 use crate::chain::transaction::{OutPoint, TransactionData};
+use crate::sign::errors::SigningError;
 use crate::sign::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, WriteableEcdsaChannelSigner, SignerProvider, EntropySource};
 use crate::chain::onchaintx::{ClaimEvent, OnchainTxHandler};
 use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput};
@@ -1482,21 +1483,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		self.inner.lock().unwrap().counterparty_node_id
 	}
 
-	/// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy
-	/// of the channel state was out-of-date.
-	///
-	/// You may also use this to broadcast the latest local commitment transaction, either because
+	/// You may use this to broadcast the latest local commitment transaction, either because
 	/// a monitor update failed or because we've fallen behind (i.e. we've received proof that our
 	/// counterparty side knows a revocation secret we gave them that they shouldn't know).
 	///
-	/// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty
+	/// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty
 	/// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't
 	/// close channel with their commitment transaction after a substantial amount of time. Best
 	/// may be to contact the other node operator out-of-band to coordinate other options available
 	/// to you.
-	///
-	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
-	pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
+	pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Result<Vec<Transaction>, SigningError>
 	where L::Target: Logger {
 		self.inner.lock().unwrap().get_latest_holder_commitment_txn(logger)
 	}
@@ -1505,7 +1501,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 	/// to bypass HolderCommitmentTransaction state update lockdown after signature and generate
 	/// revoked commitment transaction.
 	#[cfg(any(test, feature = "unsafe_revoked_tx_signing"))]
-	pub fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
+	pub fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Result<Vec<Transaction>, SigningError>
 	where L::Target: Logger {
 		self.inner.lock().unwrap().unsafe_get_latest_holder_commitment_txn(logger)
 	}
@@ -2515,18 +2511,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
-	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
+	fn generate_claimable_outpoints_and_watch_outputs(&mut self) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) {
+		let funding_outp = HolderFundingOutput::build(
+			self.funding_redeemscript.clone(),
+			self.channel_value_satoshis,
+			self.onchain_tx_handler.channel_type_features().clone()
+		);
+		let commitment_package = PackageTemplate::build_package(
+			self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+			PackageSolvingData::HolderFundingOutput(funding_outp),
+			self.best_block.height(), self.best_block.height()
+		);
+		let mut claimable_outpoints = vec![commitment_package];
+		self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
+		// Although we aren't signing the transaction directly here, the transaction will be signed
+		// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
+		// new channel updates.
+		self.holder_tx_signed = true;
+		let mut watch_outputs = Vec::new();
+		// We can't broadcast our HTLC transactions while the commitment transaction is
+		// unconfirmed. We'll delay doing so until we detect the confirmed commitment in
+		// `transactions_confirmed`.
+		if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+			// Because we're broadcasting a commitment transaction, we should construct the package
+			// assuming it gets confirmed in the next block. Sadly, we have code which considers
+			// "not yet confirmed" things as discardable, so we cannot do that here.
+			let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
+				&self.current_holder_commitment_tx, self.best_block.height()
+			);
+			let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
+			let new_outputs = self.get_broadcasted_holder_watch_outputs(
+				&self.current_holder_commitment_tx, &unsigned_commitment_tx
+			);
+			if !new_outputs.is_empty() {
+				watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
+			}
+			claimable_outpoints.append(&mut new_outpoints);
+		}
+		(claimable_outpoints, watch_outputs)
+	}
+
+	pub(crate) fn queue_latest_holder_commitment_txn_for_broadcast<B: Deref, F: Deref, L: Deref>(&mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
 		where B::Target: BroadcasterInterface,
-					L::Target: Logger,
+			F::Target: FeeEstimator,
+			L::Target: Logger,
 	{
-		let commit_txs = self.get_latest_holder_commitment_txn(logger);
-		let mut txs = vec![];
-		for tx in commit_txs.iter() {
-			log_info!(logger, "Broadcasting local {}", log_tx!(tx));
-			txs.push(tx);
-		}
-		broadcaster.broadcast_transactions(&txs);
-		self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
+		let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs();
+		self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
 	}
 
 	pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()>
@@ -2614,26 +2645,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain");
 							continue;
 						}
-						self.broadcast_latest_holder_commitment_txn(broadcaster, logger);
-						// If the channel supports anchor outputs, we'll need to emit an external
-						// event to be consumed such that a child transaction is broadcast with a
-						// high enough feerate for the parent commitment transaction to confirm.
-						if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
-							let funding_output = HolderFundingOutput::build(
-								self.funding_redeemscript.clone(), self.channel_value_satoshis,
-								self.onchain_tx_handler.channel_type_features().clone(),
-							);
-							let best_block_height = self.best_block.height();
-							let commitment_package = PackageTemplate::build_package(
-								self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
-								PackageSolvingData::HolderFundingOutput(funding_output),
-								best_block_height, best_block_height
-							);
-							self.onchain_tx_handler.update_claims_view_from_requests(
-								vec![commitment_package], best_block_height, best_block_height,
-								broadcaster, &bounded_fee_estimator, logger,
-							);
-						}
+						self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &bounded_fee_estimator, logger);
 					} else if !self.holder_tx_signed {
 						log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast");
 						log_error!(logger, "    in channel monitor for channel {}!", &self.funding_info.0.to_channel_id());
@@ -3211,16 +3223,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
-	pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
+	pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Result<Vec<Transaction>, SigningError> where L::Target: Logger {
 		log_debug!(logger, "Getting signed latest holder commitment transaction!");
 		self.holder_tx_signed = true;
-		let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript);
+		let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript)?;
 		let txid = commitment_tx.txid();
 		let mut holder_transactions = vec![commitment_tx];
 		// When anchor outputs are present, the HTLC transactions are only valid once the commitment
 		// transaction confirms.
 		if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
-			return holder_transactions;
+			return Ok(holder_transactions);
 		}
 		for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() {
 			if let Some(vout) = htlc.0.transaction_output_index {
@@ -3245,20 +3257,20 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 		// We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do.
 		// The data will be re-generated and tracked in check_spend_holder_transaction if we get a confirmation.
-		holder_transactions
+		Ok(holder_transactions)
 	}
 
 	#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
 	/// Note that this includes possibly-locktimed-in-the-future transactions!
-	fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
+	fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Result<Vec<Transaction>, SigningError> where L::Target: Logger {
 		log_debug!(logger, "Getting signed copy of latest holder commitment transaction!");
 		let commitment_tx = self.onchain_tx_handler.get_fully_signed_copy_holder_tx(&self.funding_redeemscript);
 		let txid = commitment_tx.txid();
 		let mut holder_transactions = vec![commitment_tx];
 		// When anchor outputs are present, the HTLC transactions are only final once the commitment
 		// transaction confirms due to the CSV 1 encumberance.
 		if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
-			return holder_transactions;
+			return Ok(holder_transactions);
 		}
 		for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() {
 			if let Some(vout) = htlc.0.transaction_output_index {
@@ -3274,7 +3286,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				}
 			}
 		}
-		holder_transactions
+		Ok(holder_transactions)
 	}
 
 	pub fn block_connected<B: Deref, F: Deref, L: Deref>(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L) -> Vec<TransactionOutputs>

lightning/src/chain/onchaintx.rs

@@ -30,6 +30,7 @@ use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInComm
 use crate::chain::ClaimId;
 use crate::chain::chaininterface::{ConfirmationTarget, FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator};
 use crate::chain::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER};
+use crate::sign::errors::SigningError;
 use crate::sign::WriteableEcdsaChannelSigner;
 use crate::chain::package::{PackageSolvingData, PackageTemplate};
 use crate::util::logger::Logger;
@@ -1131,10 +1132,12 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 	// have empty holder commitment transaction if a ChannelMonitor is asked to force-close just after OutboundV1Channel::get_funding_created,
 	// before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing
 	// to monitor before.
-	pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction {
-		let (sig, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment");
+	pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Result<Transaction, SigningError> {
+		let (sig, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs(
+			&self.holder_commitment, &self.secp_ctx
+		)?;
 		self.holder_htlc_sigs = Some(Self::extract_holder_sigs(&self.holder_commitment, htlc_sigs));
-		self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
+		Ok(self.holder_commitment.add_holder_sig(funding_redeemscript, sig))
 	}
 
 	#[cfg(any(test, feature="unsafe_revoked_tx_signing"))]

lightning/src/chain/package.rs

@@ -25,6 +25,7 @@ use crate::ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment};
 use crate::ln::chan_utils;
 use crate::ln::msgs::DecodeError;
 use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT};
+use crate::sign::errors::SigningError;
 use crate::sign::WriteableEcdsaChannelSigner;
 use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler};
 use crate::util::logger::Logger;
@@ -613,7 +614,19 @@ impl PackageSolvingData {
 				return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage);
 			}
 			PackageSolvingData::HolderFundingOutput(ref outp) => {
-				return Some(onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript));
+				match onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript) {
+					Ok(tx) => {
+						return Some(tx);
+					},
+					Err(e) => match e{
+						SigningError::NotAvailable => {
+							return None;
+						},
+						SigningError::PermanentFailure => {
+							panic!("Failed to sign holder transaction!");
+						},
+					},
+				}
 			}
 			_ => { panic!("API Error!"); }
 		}

lightning/src/ln/functional_test_utils.rs

@@ -885,7 +885,7 @@ macro_rules! get_monitor {
 macro_rules! get_local_commitment_txn {
 	($node: expr, $channel_id: expr) => {
 		{
-			$crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger)
+			$crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger).unwrap()
 		}
 	}
 }

lightning/src/sign/errors.rs

@@ -0,0 +1,20 @@
+// This file is Copyright its original authors, visible in version control
+// history.
+//
+// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
+// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Signing error types live here.
+
+#[derive(Clone, Debug)]
+pub enum SigningError {
+    /// The signature is not immediately available from the signer but will be
+    /// provided later when the signer is online.
+    NotAvailable,
+    /// The signer failed permanently and we should attempt to close the
+    /// channel.
+    PermanentFailure,
+}

lightning/src/sign/mod.rs

@@ -56,6 +56,8 @@ use crate::util::atomic_counter::AtomicCounter;
 use crate::util::chacha20::ChaCha20;
 use crate::util::invoice::construct_invoice_preimage;
 
+pub mod errors;
+
 pub(crate) mod type_resolver;
 
 /// Used as initial key material, to be expanded into multiple secret keys (but not to be used
@@ -446,14 +448,14 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
 	// TODO: Document the things someone using this interface should enforce before signing.
 	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
-		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
+		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), errors::SigningError>;
 	/// Same as [`sign_holder_commitment_and_htlcs`], but exists only for tests to get access to
 	/// holder commitment transactions which will be broadcasted later, after the channel has moved
 	/// on to a newer state. Thus, needs its own method as [`sign_holder_commitment_and_htlcs`] may
 	/// enforce that we only ever get called once.
 	#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
 	fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
-		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
+		secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), errors::SigningError>;
 	/// Create a signature for the given input in a transaction spending an HTLC transaction output
 	/// or a commitment transaction `to_local` output when our counterparty broadcasts an old state.
 	///
@@ -1012,24 +1014,26 @@ impl EcdsaChannelSigner for InMemorySigner {
 		Ok(())
 	}
 
-	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
+	fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), errors::SigningError> {
 		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
 		let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey);
 		let trusted_tx = commitment_tx.trust();
 		let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx);
 		let channel_parameters = self.get_channel_parameters();
-		let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?;
+		let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)
+		.map_err(|_| errors::SigningError::PermanentFailure)?;
 		Ok((sig, htlc_sigs))
 	}
 
 	#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
-	fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
+	fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), errors::SigningError> {
 		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
 		let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey);
 		let trusted_tx = commitment_tx.trust();
 		let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx);
 		let channel_parameters = self.get_channel_parameters();
-		let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?;
+		let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)
+		.map_err(|_| errors::SigningError::PermanentFailure)?;
 		Ok((sig, htlc_sigs))
 	}