Track HTLCDescriptor in HolderHTLCOutput

The `ChannelMonitor` and `OnchainTxHandler` have historically been tied
together, often tracking some of the same state twice. As we introduce
support for splices in the `ChannelMonitor`, we'd like to avoid leaking
some of those details to the `OnchainTxHandler`. Ultimately, the
`OnchainTxHandler` should stand on its own and support claiming funds
from multiple `ChannelMonitor`s, allowing us to save on fees by batching
aggregatable claims across multiple in-flight closing channels.

This commit tracks the `HTLCDescriptor` for the specific `FundingScope`
the `HolderHTLCOutput` claim originated from. Previously, when claiming
`HolderHTLCOutput`s, the `OnchainTxHandler` had to check which holder
commitment the HTLC belonged to in order to produce an `HTLCDescriptor`
for signing. We still maintain the legacy logic for existing claims
which have not had an `HTLCDescriptor` written yet.

Along the way, we refactor the claim process for such outputs to
decouple them from the `OnchainTxHandler`. As a result, the
`OnchainTxHandler` is only used to obtain references to the signer and
secp256k1 context.

Once splices are supported, we may run into cases where we are
attempting to claim an output from a specific `FundingScope`, while also
having an additional pending `FundingScope` for a splice. If the pending
splice confirms over the output claim, we need to cancel the claim and
re-offer it with the set of relevant parameters in the new
`FundingScope`.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -3562,29 +3562,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				} => {
 					let channel_id = self.channel_id;
 					let counterparty_node_id = self.counterparty_node_id;
-					let mut htlc_descriptors = Vec::with_capacity(htlcs.len());
-					for htlc in htlcs {
-						htlc_descriptors.push(HTLCDescriptor {
-							channel_derivation_parameters: ChannelDerivationParameters {
-								keys_id: self.channel_keys_id,
-								value_satoshis: self.funding.channel_parameters.channel_value_satoshis,
-								transaction_parameters: self.funding.channel_parameters.clone(),
-							},
-							commitment_txid: htlc.commitment_txid,
-							per_commitment_number: htlc.per_commitment_number,
-							per_commitment_point: htlc.per_commitment_point,
-							feerate_per_kw: 0,
-							htlc: htlc.htlc,
-							preimage: htlc.preimage,
-							counterparty_sig: htlc.counterparty_sig,
-						});
-					}
 					ret.push(Event::BumpTransaction(BumpTransactionEvent::HTLCResolution {
 						channel_id,
 						counterparty_node_id,
 						claim_id,
 						target_feerate_sat_per_1000_weight,
-						htlc_descriptors,
+						htlc_descriptors: htlcs,
 						tx_lock_time,
 					}));
 				}
@@ -3991,25 +3974,39 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		));
 
 		let txid = tx.txid();
-		for htlc in holder_tx.htlcs() {
+		for (idx, htlc) in holder_tx.htlcs().iter().enumerate() {
 			if let Some(transaction_output_index) = htlc.transaction_output_index {
-				let (htlc_output, counterparty_spendable_height) = if htlc.offered {
-					let htlc_output = HolderHTLCOutput::build_offered(
-						htlc.amount_msat, htlc.cltv_expiry, self.channel_type_features().clone()
-					);
-					(htlc_output, conf_height)
-				} else {
-					let payment_preimage = if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) {
-						preimage.clone()
+				let counterparty_sig =
+					if let Some(counterparty_sig) = holder_tx.counterparty_htlc_sigs.get(idx) {
+						*counterparty_sig
 					} else {
-						// We can't build an HTLC-Success transaction without the preimage
+						debug_assert!(false, "All non-dust HTLCs must have a corresponding counterparty signature");
 						continue;
 					};
-					let htlc_output = HolderHTLCOutput::build_accepted(
-						payment_preimage, htlc.amount_msat, self.channel_type_features().clone()
-					);
-					(htlc_output, htlc.cltv_expiry)
+
+				let (preimage, counterparty_spendable_height) = if htlc.offered {
+					(None, conf_height)
+				} else if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) {
+					(Some(*preimage), htlc.cltv_expiry)
+				} else {
+					// We can't build an HTLC-Success transaction without the preimage
+					continue;
 				};
+
+				let htlc_output = HolderHTLCOutput::build(HTLCDescriptor {
+					channel_derivation_parameters: ChannelDerivationParameters {
+						value_satoshis: self.funding.channel_parameters.channel_value_satoshis,
+						keys_id: self.channel_keys_id,
+						transaction_parameters: self.funding.channel_parameters.clone(),
+					},
+					commitment_txid: tx.txid(),
+					per_commitment_number: tx.commitment_number(),
+					per_commitment_point: tx.per_commitment_point(),
+					feerate_per_kw: tx.feerate_per_kw(),
+					htlc: htlc.clone(),
+					preimage,
+					counterparty_sig,
+				});
 				let htlc_package = PackageTemplate::build_package(
 					txid, transaction_output_index,
 					PackageSolvingData::HolderHTLCOutput(htlc_output),
@@ -4153,24 +4150,57 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		&mut self, logger: &WithChannelMonitor<L>
 	) -> Vec<Transaction> 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.redeem_script);
+		let commitment_tx = {
+			let sig = self.onchain_tx_handler.signer.unsafe_sign_holder_commitment(
+				&self.funding.channel_parameters, &self.funding.current_holder_commitment.tx,
+				&self.onchain_tx_handler.secp_ctx,
+			).expect("sign holder commitment");
+			self.funding.current_holder_commitment.tx.add_holder_sig(&self.funding.redeem_script, sig)
+		};
 		let txid = commitment_tx.compute_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.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
 			return holder_transactions;
 		}
-		for htlc in self.funding.current_holder_commitment.tx.htlcs() {
+		for (idx, htlc) in self.funding.current_holder_commitment.tx.htlcs().iter().enumerate() {
 			if let Some(vout) = htlc.transaction_output_index {
-				let preimage = if !htlc.offered {
-					if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(preimage.clone()) } else {
-						// We can't build an HTLC-Success transaction without the preimage
+				let commitment = &self.funding.current_holder_commitment;
+				let trusted_tx = commitment.tx.trust();
+				let counterparty_sig =
+					if let Some(counterparty_sig) = commitment.tx.counterparty_htlc_sigs.get(idx) {
+						*counterparty_sig
+					} else {
+						debug_assert!(false, "All non-dust HTLCs must have a corresponding counterparty signature");
 						continue;
-					}
-				} else { None };
-				if let Some(htlc_tx) = self.onchain_tx_handler.get_maybe_signed_htlc_tx(
-					&::bitcoin::OutPoint { txid, vout }, &preimage
+					};
+
+				let preimage = if htlc.offered {
+					None
+				} else if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) {
+					Some(*preimage)
+				} else {
+					// We can't build an HTLC-Success transaction without the preimage
+					continue;
+				};
+
+				let htlc_output = HolderHTLCOutput::build(HTLCDescriptor {
+					channel_derivation_parameters: ChannelDerivationParameters {
+						value_satoshis: self.funding.channel_parameters.channel_value_satoshis,
+						keys_id: self.channel_keys_id,
+						transaction_parameters: self.funding.channel_parameters.clone(),
+					},
+					commitment_txid: trusted_tx.txid(),
+					per_commitment_number: trusted_tx.commitment_number(),
+					per_commitment_point: trusted_tx.per_commitment_point(),
+					feerate_per_kw: trusted_tx.feerate_per_kw(),
+					htlc: htlc.clone(),
+					preimage,
+					counterparty_sig,
+				});
+				if let Some(htlc_tx) = htlc_output.get_maybe_signed_htlc_tx(
+					&mut self.onchain_tx_handler, &::bitcoin::OutPoint { txid, vout },
 				) {
 					if htlc_tx.is_fully_signed() {
 						holder_transactions.push(htlc_tx.0);

lightning/src/chain/onchaintx.rs

@@ -20,14 +20,12 @@ use bitcoin::script::{Script, ScriptBuf};
 use bitcoin::hashes::{Hash, HashEngine};
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hash_types::{Txid, BlockHash};
-use bitcoin::secp256k1::PublicKey;
 use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
 use bitcoin::secp256k1;
 
 use crate::chain::chaininterface::{ConfirmationTarget, compute_feerate_sat_per_1000_weight};
-use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, EntropySource, SignerProvider, ecdsa::EcdsaChannelSigner};
+use crate::sign::{EntropySource, HTLCDescriptor, SignerProvider, ecdsa::EcdsaChannelSigner};
 use crate::ln::msgs::DecodeError;
-use crate::types::payment::PaymentPreimage;
 use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInCommitment, HolderCommitmentTransaction};
 use crate::chain::ClaimId;
 use crate::chain::chaininterface::{FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator};
@@ -173,17 +171,6 @@ impl Writeable for Option<Vec<Option<(usize, Signature)>>> {
 	}
 }
 
-/// The claim commonly referred to as the pre-signed second-stage HTLC transaction.
-#[derive(Clone, PartialEq, Eq)]
-pub(crate) struct ExternalHTLCClaim {
-	pub(crate) commitment_txid: Txid,
-	pub(crate) per_commitment_number: u64,
-	pub(crate) htlc: HTLCOutputInCommitment,
-	pub(crate) preimage: Option<PaymentPreimage>,
-	pub(crate) counterparty_sig: Signature,
-	pub(crate) per_commitment_point: PublicKey,
-}
-
 // Represents the different types of claims for which events are yielded externally to satisfy said
 // claims.
 #[derive(Clone, PartialEq, Eq)]
@@ -202,7 +189,7 @@ pub(crate) enum ClaimEvent {
 	/// resolved by broadcasting a transaction with sufficient fee to claim them.
 	BumpHTLC {
 		target_feerate_sat_per_1000_weight: u32,
-		htlcs: Vec<ExternalHTLCClaim>,
+		htlcs: Vec<HTLCDescriptor>,
 		tx_lock_time: LockTime,
 	},
 }
@@ -234,7 +221,7 @@ pub(crate) enum FeerateStrategy {
 #[derive(Clone)]
 pub struct OnchainTxHandler<ChannelSigner: EcdsaChannelSigner> {
 	channel_value_satoshis: u64,
-	channel_keys_id: [u8; 32],
+	channel_keys_id: [u8; 32], // Deprecated as of 0.2.
 	destination_script: ScriptBuf, // Deprecated as of 0.2.
 	holder_commitment: HolderCommitmentTransaction,
 	prev_holder_commitment: Option<HolderCommitmentTransaction>,
@@ -610,19 +597,16 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 				let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate(
 					fee_estimator, conf_target, feerate_strategy,
 				);
-				if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) {
-					return Some((
-						new_timer,
-						target_feerate_sat_per_1000_weight as u64,
-						OnchainClaim::Event(ClaimEvent::BumpHTLC {
-							target_feerate_sat_per_1000_weight,
-							htlcs,
-							tx_lock_time: LockTime::from_consensus(cached_request.package_locktime(cur_height)),
-						}),
-					));
-				} else {
-					return None;
-				}
+				let htlcs = cached_request.construct_malleable_package_with_external_funding(self)?;
+				return Some((
+					new_timer,
+					target_feerate_sat_per_1000_weight as u64,
+					OnchainClaim::Event(ClaimEvent::BumpHTLC {
+						target_feerate_sat_per_1000_weight,
+						htlcs,
+						tx_lock_time: LockTime::from_consensus(cached_request.package_locktime(cur_height)),
+					}),
+				));
 			}
 
 			let predicted_weight = cached_request.package_weight(destination_script);
@@ -1218,87 +1202,14 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 		self.prev_holder_commitment = Some(replace(&mut self.holder_commitment, tx));
 	}
 
-	#[cfg(any(test, feature="_test_utils", feature="unsafe_revoked_tx_signing"))]
-	pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction {
-		let sig = self.signer.unsafe_sign_holder_commitment(&self.channel_transaction_parameters, &self.holder_commitment, &self.secp_ctx).expect("sign holder commitment");
-		self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
-	}
-
-	pub(crate) fn get_maybe_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>) -> Option<MaybeSignedTransaction> {
-		let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| {
-			let trusted_tx = holder_commitment.trust();
-			if trusted_tx.txid() != outp.txid {
-				return None;
-			}
-			let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate()
-				.find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout)
-				.unwrap();
-			let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx];
-			let mut htlc_tx = trusted_tx.build_unsigned_htlc_tx(
-				&self.channel_transaction_parameters.as_holder_broadcastable(), htlc_idx, preimage,
-			);
-
-			let htlc_descriptor = HTLCDescriptor {
-				channel_derivation_parameters: ChannelDerivationParameters {
-					value_satoshis: self.channel_value_satoshis,
-					keys_id: self.channel_keys_id,
-					transaction_parameters: self.channel_transaction_parameters.clone(),
-				},
-				commitment_txid: trusted_tx.txid(),
-				per_commitment_number: trusted_tx.commitment_number(),
-				per_commitment_point: trusted_tx.per_commitment_point(),
-				feerate_per_kw: trusted_tx.feerate_per_kw(),
-				htlc: htlc.clone(),
-				preimage: preimage.clone(),
-				counterparty_sig: counterparty_htlc_sig.clone(),
-			};
-			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(MaybeSignedTransaction(htlc_tx))
-		};
-
-		// Check if the HTLC spends from the current holder commitment first, or the previous.
-		get_signed_htlc_tx(&self.holder_commitment)
-			.or_else(|| self.prev_holder_commitment.as_ref().and_then(|prev_holder_commitment| get_signed_htlc_tx(prev_holder_commitment)))
-	}
-
-	pub(crate) fn generate_external_htlc_claim(
-		&self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>
-	) -> Option<ExternalHTLCClaim> {
-		let find_htlc = |holder_commitment: &HolderCommitmentTransaction| -> Option<ExternalHTLCClaim> {
-			let trusted_tx = holder_commitment.trust();
-			if outp.txid != trusted_tx.txid() {
-				return None;
-			}
-			trusted_tx.htlcs().iter().enumerate()
-				.find(|(_, htlc)| if let Some(output_index) = htlc.transaction_output_index {
-					output_index == outp.vout
-				} else {
-					false
-				})
-				.map(|(htlc_idx, htlc)| {
-					let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx];
-					ExternalHTLCClaim {
-						commitment_txid: trusted_tx.txid(),
-						per_commitment_number: trusted_tx.commitment_number(),
-						htlc: htlc.clone(),
-						preimage: *preimage,
-						counterparty_sig: counterparty_htlc_sig,
-						per_commitment_point: trusted_tx.per_commitment_point(),
-					}
-				})
-		};
-		// Check if the HTLC spends from the current holder commitment or the previous one otherwise.
-		find_htlc(&self.holder_commitment)
-			.or_else(|| self.prev_holder_commitment.as_ref().map(|c| find_htlc(c)).flatten())
-	}
-
 	pub(crate) fn channel_type_features(&self) -> &ChannelTypeFeatures {
 		&self.channel_transaction_parameters.channel_type_features
 	}
+
+	// Deprecated as of 0.2, only use in cases where it was not previously available.
+	pub(crate) fn channel_keys_id(&self) -> [u8; 32] {
+		self.channel_keys_id
+	}
 }
 
 #[cfg(test)]
@@ -1319,7 +1230,7 @@ mod tests {
 	};
 	use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint};
 	use crate::ln::functional_test_utils::create_dummy_block;
-	use crate::sign::InMemorySigner;
+	use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, InMemorySigner};
 	use crate::types::payment::{PaymentHash, PaymentPreimage};
 	use crate::util::test_utils::{TestBroadcaster, TestFeeEstimator, TestLogger};
 
@@ -1424,17 +1335,27 @@ mod tests {
 		let logger = TestLogger::new();
 
 		// Request claiming of each HTLC on the holder's commitment, with current block height 1.
-		let holder_commit_txid = tx_handler.current_holder_commitment_tx().trust().txid();
+		let holder_commit = tx_handler.current_holder_commitment_tx();
+		let holder_commit_txid = holder_commit.trust().txid();
 		let mut requests = Vec::new();
-		for (htlc, _) in htlcs {
+		for (idx, htlc) in holder_commit.htlcs().iter().enumerate() {
 			requests.push(PackageTemplate::build_package(
 				holder_commit_txid,
 				htlc.transaction_output_index.unwrap(),
-				PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_offered(
-					htlc.amount_msat,
-					htlc.cltv_expiry,
-					ChannelTypeFeatures::only_static_remote_key(),
-				)),
+				PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(HTLCDescriptor {
+					channel_derivation_parameters: ChannelDerivationParameters {
+						value_satoshis: tx_handler.channel_value_satoshis,
+						keys_id: tx_handler.channel_keys_id,
+						transaction_parameters: tx_handler.channel_transaction_parameters.clone(),
+					},
+					commitment_txid: holder_commit_txid,
+					per_commitment_number: holder_commit.commitment_number(),
+					per_commitment_point: holder_commit.per_commitment_point(),
+					feerate_per_kw: holder_commit.feerate_per_kw(),
+					htlc: htlc.clone(),
+					preimage: None,
+					counterparty_sig: holder_commit.counterparty_htlc_sigs[idx].clone(),
+				})),
 				0,
 			));
 		}