Allow holder commitment and HTLC signature requests to fail

As part of the ongoing async signer work, our holder signatures must
also be capable of being obtained asynchronously. We expose a new
`ChannelMonitor::signer_unblocked` method to retry pending onchain
claims by re-signing and rebroadcasting transactions. Unfortunately, we
cannot retry said claims without them being registered first, so if
we're not able to obtain the signature synchronously, we must return the
transaction as unsigned and ensure it is not broadcast.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -1769,6 +1769,25 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		);
 	}
 
+	/// Triggers rebroadcasts of pending claims from a force-closed channel after a transaction
+	/// signature generation failure.
+	pub fn signer_unblocked<B: Deref, F: Deref, L: Deref>(
+		&self, broadcaster: B, fee_estimator: F, logger: &L,
+	)
+	where
+		B::Target: BroadcasterInterface,
+		F::Target: FeeEstimator,
+		L::Target: Logger,
+	{
+		let fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
+		let mut inner = self.inner.lock().unwrap();
+		let logger = WithChannelMonitor::from_impl(logger, &*inner);
+		let current_height = inner.best_block.height;
+		inner.onchain_tx_handler.rebroadcast_pending_claims(
+			current_height, FeerateStrategy::RetryPrevious, &broadcaster, &fee_estimator, &logger,
+		);
+	}
+
 	/// Returns the descriptors for relevant outputs (i.e., those that we can spend) within the
 	/// transaction if they exist and the transaction has at least [`ANTI_REORG_DELAY`]
 	/// confirmations. For [`SpendableOutputDescriptor::DelayedPaymentOutput`] descriptors to be
@@ -1811,6 +1830,12 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 	pub fn set_counterparty_payment_script(&self, script: ScriptBuf) {
 		self.inner.lock().unwrap().counterparty_payment_script = script;
 	}
+
+	#[cfg(test)]
+	pub fn do_signer_call<F: FnMut(&Signer) -> ()>(&self, mut f: F) {
+		let inner = self.inner.lock().unwrap();
+		f(&inner.onchain_tx_handler.signer);
+	}
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
@@ -3508,9 +3533,12 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						continue;
 					}
 				} else { None };
-				if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(
-					&::bitcoin::OutPoint { txid, vout }, &preimage) {
-					holder_transactions.push(htlc_tx);
+				if let Some(htlc_tx) = self.onchain_tx_handler.get_maybe_signed_htlc_tx(
+					&::bitcoin::OutPoint { txid, vout }, &preimage
+				) {
+					if !htlc_tx.input.iter().any(|input| input.witness.is_empty()) {
+						holder_transactions.push(htlc_tx);
+					}
 				}
 			}
 		}

lightning/src/chain/onchaintx.rs

@@ -212,6 +212,8 @@ pub(crate) enum OnchainClaim {
 
 /// Represents the different feerates a pending request can use when generating a claim.
 pub(crate) enum FeerateStrategy {
+	/// We must reuse the most recently used feerate, if any.
+	RetryPrevious,
 	/// We must pick the highest between the most recently used and the current feerate estimate.
 	HighestOfPreviousOrNew,
 	/// We must force a bump of the most recently used feerate, either by using the current feerate
@@ -506,9 +508,13 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 					}
 					match claim {
 						OnchainClaim::Tx(tx) => {
-							let log_start = if bumped_feerate { "Broadcasting RBF-bumped" } else { "Rebroadcasting" };
-							log_info!(logger, "{} onchain {}", log_start, log_tx!(tx));
-							broadcaster.broadcast_transactions(&[&tx]);
+							if tx.input.iter().any(|input| input.witness.is_empty()) {
+								log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", tx.txid());
+							} else {
+								let log_start = if bumped_feerate { "Broadcasting RBF-bumped" } else { "Rebroadcasting" };
+								log_info!(logger, "{} onchain {}", log_start, log_tx!(tx));
+								broadcaster.broadcast_transactions(&[&tx]);
+							}
 						},
 						OnchainClaim::Event(event) => {
 							let log_start = if bumped_feerate { "Yielding fee-bumped" } else { "Replaying" };
@@ -645,8 +651,8 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 						let commitment_tx_feerate_sat_per_1000_weight =
 							compute_feerate_sat_per_1000_weight(fee_sat, tx.weight().to_wu());
 						if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight {
-							log_debug!(logger, "Pre-signed {} already has feerate {} sat/kW above required {} sat/kW",
-								log_tx!(tx), commitment_tx_feerate_sat_per_1000_weight,
+							log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW",
+								tx.txid(), commitment_tx_feerate_sat_per_1000_weight,
 								package_target_feerate_sat_per_1000_weight);
 							return Some((new_timer, 0, OnchainClaim::Tx(tx.clone())));
 						}
@@ -785,8 +791,12 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 				// `OnchainClaim`.
 				let claim_id = match claim {
 					OnchainClaim::Tx(tx) => {
-						log_info!(logger, "Broadcasting onchain {}", log_tx!(tx));
-						broadcaster.broadcast_transactions(&[&tx]);
+						if tx.input.iter().any(|input| input.witness.is_empty()) {
+							log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", tx.txid());
+						} else {
+							log_info!(logger, "Broadcasting onchain {}", log_tx!(tx));
+							broadcaster.broadcast_transactions(&[&tx]);
+						}
 						ClaimId(tx.txid().to_byte_array())
 					},
 					OnchainClaim::Event(claim_event) => {
@@ -978,8 +988,13 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 			) {
 				match bump_claim {
 					OnchainClaim::Tx(bump_tx) => {
-						log_info!(logger, "Broadcasting RBF-bumped onchain {}", log_tx!(bump_tx));
-						broadcaster.broadcast_transactions(&[&bump_tx]);
+						if bump_tx.input.iter().any(|input| input.witness.is_empty()) {
+							log_info!(logger, "Waiting for signature of RBF-bumped unsigned onchain transaction {}",
+								bump_tx.txid());
+						} else {
+							log_info!(logger, "Broadcasting RBF-bumped onchain {}", log_tx!(bump_tx));
+							broadcaster.broadcast_transactions(&[&bump_tx]);
+						}
 					},
 					OnchainClaim::Event(claim_event) => {
 						log_info!(logger, "Yielding RBF-bumped onchain event to spend inputs {:?}", request.outpoints());
@@ -1061,8 +1076,12 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 				request.set_feerate(new_feerate);
 				match bump_claim {
 					OnchainClaim::Tx(bump_tx) => {
-						log_info!(logger, "Broadcasting onchain {}", log_tx!(bump_tx));
-						broadcaster.broadcast_transactions(&[&bump_tx]);
+						if bump_tx.input.iter().any(|input| input.witness.is_empty()) {
+							log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", bump_tx.txid());
+						} else {
+							log_info!(logger, "Broadcasting onchain {}", log_tx!(bump_tx));
+							broadcaster.broadcast_transactions(&[&bump_tx]);
+						}
 					},
 					OnchainClaim::Event(claim_event) => {
 						log_info!(logger, "Yielding onchain event after reorg to spend inputs {:?}", request.outpoints());
@@ -1115,13 +1134,10 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 		&self.holder_commitment.trust().built_transaction().transaction
 	}
 
-	//TODO: getting lastest holder transactions should be infallible and result in us "force-closing the channel", but we may
-	// 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 = self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment");
-		self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
+	pub(crate) fn get_maybe_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction {
+		self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx)
+			.map(|sig| self.holder_commitment.add_holder_sig(funding_redeemscript, sig))
+			.unwrap_or_else(|_| self.get_unsigned_holder_commitment_tx().clone())
 	}
 
 	#[cfg(any(test, feature="unsafe_revoked_tx_signing"))]
@@ -1130,7 +1146,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 		self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
 	}
 
-	pub(crate) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>) -> Option<Transaction> {
+	pub(crate) fn get_maybe_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>) -> Option<Transaction> {
 		let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| {
 			let trusted_tx = holder_commitment.trust();
 			if trusted_tx.txid() != outp.txid {
@@ -1158,10 +1174,11 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 				preimage: preimage.clone(),
 				counterparty_sig: counterparty_htlc_sig.clone(),
 			};
-			let htlc_sig = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx).unwrap();
-			htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness(
-				htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage,
-			);
+			if let Ok(htlc_sig) = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx) {
+				htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness(
+					htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage,
+				);
+			}
 			Some(htlc_tx)
 		};
 

lightning/src/chain/package.rs

@@ -637,10 +637,10 @@ impl PackageSolvingData {
 		match self {
 			PackageSolvingData::HolderHTLCOutput(ref outp) => {
 				debug_assert!(!outp.channel_type_features.supports_anchors_zero_fee_htlc_tx());
-				return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage);
+				onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage)
 			}
 			PackageSolvingData::HolderFundingOutput(ref outp) => {
-				return Some(onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript));
+				Some(onchain_handler.get_maybe_signed_holder_tx(&outp.funding_redeemscript))
 			}
 			_ => { panic!("API Error!"); }
 		}
@@ -996,6 +996,7 @@ impl PackageTemplate {
 		if self.feerate_previous != 0 {
 			let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value());
 			match feerate_strategy {
+				FeerateStrategy::RetryPrevious => previous_feerate,
 				FeerateStrategy::HighestOfPreviousOrNew => cmp::max(previous_feerate, feerate_estimate),
 				FeerateStrategy::ForceBump => if feerate_estimate > previous_feerate {
 					feerate_estimate
@@ -1141,6 +1142,10 @@ where
 	// If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee...
 	let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, fee_estimator, logger) {
 		match feerate_strategy {
+			FeerateStrategy::RetryPrevious => {
+				let previous_fee = previous_feerate * predicted_weight / 1000;
+				(previous_fee, previous_feerate)
+			},
 			FeerateStrategy::HighestOfPreviousOrNew => if new_feerate > previous_feerate {
 				(new_fee, new_feerate)
 			} else {

lightning/src/ln/async_signer_tests.rs

@@ -10,7 +10,12 @@
 //! Tests for asynchronous signing. These tests verify that the channel state machine behaves
 //! properly with a signer implementation that asynchronously derives signatures.
 
-use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider};
+use bitcoin::{Transaction, TxOut, TxIn, Amount};
+use bitcoin::blockdata::locktime::absolute::LockTime;
+
+use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
+use crate::events::bump_transaction::WalletSource;
+use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 use crate::ln::functional_test_utils::*;
 use crate::ln::msgs::ChannelMessageHandler;
 use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
@@ -321,3 +326,107 @@ fn test_async_commitment_signature_for_peer_disconnect() {
 		};
 	}
 }
+
+fn do_test_async_holder_signatures(anchors: bool) {
+	// Ensures that we can obtain holder signatures for commitment and HTLC transactions
+	// asynchronously by allowing their retrieval to fail and retrying via
+	// `ChannelMonitor::signer_maybe_unblocked`.
+	let mut config = test_default_channel_config();
+	if anchors {
+		config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+		config.manually_accept_inbound_channels = true;
+	}
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let coinbase_tx = Transaction {
+		version: 2,
+		lock_time: LockTime::ZERO,
+		input: vec![TxIn { ..Default::default() }],
+		output: vec![
+			TxOut {
+				value: Amount::ONE_BTC.to_sat(),
+				script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
+			},
+		],
+	};
+	if anchors {
+		*nodes[0].fee_estimator.sat_per_kw.lock().unwrap() *= 2;
+		nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value);
+	}
+
+	// Route an HTLC and set the signer as unavailable.
+	let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1);
+	route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+
+	nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false);
+
+	// We'll connect blocks until the sender has to go onchain to time out the HTLC.
+	connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);
+
+	// No transaction should be broadcast since the signer is not available yet.
+	assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty());
+	assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
+
+	// Mark it as available now, we should see the signed commitment transaction.
+	nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true);
+	get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger);
+
+	let commitment_tx = {
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		if anchors {
+			assert_eq!(txn.len(), 1);
+			check_spends!(txn[0], funding_tx);
+			txn.remove(0)
+		} else {
+			assert_eq!(txn.len(), 2);
+			if txn[0].input[0].previous_output.txid == funding_tx.txid() {
+				check_spends!(txn[0], funding_tx);
+				check_spends!(txn[1], txn[0]);
+				txn.remove(0)
+			} else {
+				check_spends!(txn[1], funding_tx);
+				check_spends!(txn[0], txn[1]);
+				txn.remove(1)
+			}
+		}
+	};
+
+	// Mark it as unavailable again to now test the HTLC transaction. We'll mine the commitment such
+	// that the HTLC transaction is retried.
+	nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false);
+	mine_transaction(&nodes[0], &commitment_tx);
+
+	check_added_monitors(&nodes[0], 1);
+	check_closed_broadcast(&nodes[0], 1, true);
+	check_closed_event(&nodes[0], 1, ClosureReason::CommitmentTxConfirmed, false, &[nodes[1].node.get_our_node_id()], 100_000);
+
+	// No HTLC transaction should be broadcast as the signer is not available yet.
+	if anchors {
+		handle_bump_htlc_event(&nodes[0], 1);
+	}
+	assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty());
+
+	// Mark it as available now, we should see the signed HTLC transaction.
+	nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true);
+	get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger);
+
+	if anchors {
+		handle_bump_htlc_event(&nodes[0], 1);
+	}
+	{
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		assert_eq!(txn.len(), 1);
+		check_spends!(txn[0], commitment_tx, coinbase_tx);
+		txn.remove(0)
+	};
+}
+
+#[test]
+fn test_async_holder_signatures() {
+	do_test_async_holder_signatures(false);
+	do_test_async_holder_signatures(true);
+}

lightning/src/ln/channel.rs

@@ -1258,7 +1258,10 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 
 	/// The unique identifier used to re-derive the private key material for the channel through
 	/// [`SignerProvider::derive_channel_signer`].
+	#[cfg(not(test))]
 	channel_keys_id: [u8; 32],
+	#[cfg(test)]
+	pub channel_keys_id: [u8; 32],
 
 	/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
 	/// store it here and only release it to the `ChannelManager` once it asks for it.

lightning/src/ln/functional_test_utils.rs

@@ -487,16 +487,38 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
 	/// `release_commitment_secret` are affected by this setting.
 	#[cfg(test)]
 	pub fn set_channel_signer_available(&self, peer_id: &PublicKey, chan_id: &ChannelId, available: bool) {
+		use crate::sign::ChannelSigner;
+		log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available);
+
 		let per_peer_state = self.node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(peer_id).unwrap().lock().unwrap();
-		let signer = (|| {
-			match chan_lock.channel_by_id.get(chan_id) {
-				Some(phase) => phase.context().get_signer(),
-				None => panic!("Couldn't find a channel with id {}", chan_id),
+
+		let mut channel_keys_id = None;
+		if let Some(chan) = chan_lock.channel_by_id.get(chan_id).map(|phase| phase.context()) {
+			chan.get_signer().as_ecdsa().unwrap().set_available(available);
+			channel_keys_id = Some(chan.channel_keys_id);
+		}
+
+		let mut monitor = None;
+		for (funding_txo, channel_id) in self.chain_monitor.chain_monitor.list_monitors() {
+			if *chan_id == channel_id {
+				monitor = self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok();
 			}
-		})();
-		log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available);
-		signer.as_ecdsa().unwrap().set_available(available);
+		}
+		if let Some(monitor) = monitor {
+			monitor.do_signer_call(|signer| {
+				channel_keys_id = channel_keys_id.or(Some(signer.inner.channel_keys_id()));
+				signer.set_available(available)
+			});
+		}
+
+		if available {
+			self.keys_manager.unavailable_signers.lock().unwrap()
+				.remove(channel_keys_id.as_ref().unwrap());
+		} else {
+			self.keys_manager.unavailable_signers.lock().unwrap()
+				.insert(channel_keys_id.unwrap());
+		}
 	}
 }