Track HolderCommitmentTransaction in HolderFundingOutput

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.

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`.

This commit tracks the `HolderCommitmentTransaction` for the specific
`FundingScope` the `HolderFundingOutput` claim originated from. This
allows us to remove the dependency on `OnchainTxHandler` when obtaining
the current `HolderCommitmentTransaction` and ensures any alternative
commitment transaction state due to splicing does not leak into the
`OnchainTxHandler`. As a result, we can now include all the information
required to emit a `BumpTransactionEvent::ChannelClose` within its
intermediate representation `ClaimEvent::BumpCommitment`, as opposed to
relying on the `ChannelMonitor` to provide it.

Along the way, this commit also fixes a bug where
`BumpTransactionEvent::ChannelClose` could be emitted with an unsigned
commitment transaction, resulting in the child transaction not being
broadcastable due to a non-existent ancestor. This isn't a huge deal as
we have retry logic for such claims; once the signer returns a valid
signature, the event is re-emitted properly.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -3278,6 +3278,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	fn generate_claimable_outpoints_and_watch_outputs(&mut self, reason: ClosureReason) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) {
 		let funding_outp = HolderFundingOutput::build(
+			self.funding.current_holder_commitment.tx.clone(),
 			self.funding.redeem_script.clone(),
 			self.funding.channel_parameters.channel_value_satoshis,
 			self.channel_type_features().clone(),
@@ -3530,16 +3531,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		for (claim_id, claim_event) in pending_claim_events {
 			match claim_event {
 				ClaimEvent::BumpCommitment {
-					package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_output_idx,
+					package_target_feerate_sat_per_1000_weight, commitment_tx,
+					commitment_tx_fee_satoshis, pending_nondust_htlcs, anchor_output_idx,
 				} => {
 					let channel_id = self.channel_id;
 					let counterparty_node_id = self.counterparty_node_id;
 					let commitment_txid = commitment_tx.compute_txid();
-					debug_assert_eq!(self.funding.current_holder_commitment.tx.trust().txid(), commitment_txid);
-					let pending_htlcs = self.funding.current_holder_commitment.tx.trust().htlcs().to_vec();
-					let channel_value_satoshis = self.funding.channel_parameters.channel_value_satoshis;
-					let commitment_tx_fee_satoshis = channel_value_satoshis -
-						commitment_tx.output.iter().fold(0u64, |sum, output| sum + output.value.to_sat());
 					ret.push(Event::BumpTransaction(BumpTransactionEvent::ChannelClose {
 						channel_id,
 						counterparty_node_id,
@@ -3550,15 +3547,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						anchor_descriptor: AnchorDescriptor {
 							channel_derivation_parameters: ChannelDerivationParameters {
 								keys_id: self.channel_keys_id,
-								value_satoshis: channel_value_satoshis,
+								value_satoshis: self.funding.channel_parameters.channel_value_satoshis,
 								transaction_parameters: self.funding.channel_parameters.clone(),
 							},
 							outpoint: BitcoinOutPoint {
 								txid: commitment_txid,
 								vout: anchor_output_idx,
 							},
 						},
-						pending_htlcs,
+						pending_htlcs: pending_nondust_htlcs,
 					}));
 				},
 				ClaimEvent::BumpHTLC {

lightning/src/chain/onchaintx.rs

@@ -193,6 +193,8 @@ pub(crate) enum ClaimEvent {
 	BumpCommitment {
 		package_target_feerate_sat_per_1000_weight: u32,
 		commitment_tx: Transaction,
+		commitment_tx_fee_satoshis: u64,
+		pending_nondust_htlcs: Vec<HTLCOutputInCommitment>,
 		anchor_output_idx: u32,
 	},
 	/// Event yielded to signal that the commitment transaction has confirmed and its HTLCs must be
@@ -641,37 +643,55 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 			// which require external funding.
 			let mut inputs = cached_request.inputs();
 			debug_assert_eq!(inputs.len(), 1);
-			let tx = match cached_request.maybe_finalize_untractable_package(self, logger) {
-				Some(tx) => tx,
-				None => return None,
-			};
+
 			if !cached_request.requires_external_funding() {
-				return Some((new_timer, 0, OnchainClaim::Tx(tx)));
+				return cached_request.maybe_finalize_untractable_package(self, logger)
+					.map(|tx| (new_timer, 0, OnchainClaim::Tx(tx)))
 			}
+
 			return inputs.find_map(|input| match input {
 				// Commitment inputs with anchors support are the only untractable inputs supported
 				// thus far that require external funding.
 				PackageSolvingData::HolderFundingOutput(output) => {
-					debug_assert_eq!(tx.0.compute_txid(), self.holder_commitment.trust().txid(),
-						"Holder commitment transaction mismatch");
+					let maybe_signed_commitment_tx = output.get_maybe_signed_commitment_tx(self);
+					let tx = if maybe_signed_commitment_tx.is_fully_signed() {
+						maybe_signed_commitment_tx.0
+					} else {
+						// We couldn't sign the commitment as the signer was unavailable, but we
+						// should still retry it later. We return the unsigned transaction anyway to
+						// register the claim.
+						return Some((new_timer, 0, OnchainClaim::Tx(maybe_signed_commitment_tx)));
+					};
+
+					let holder_commitment = output.commitment_tx.as_ref()
+						.unwrap_or(self.current_holder_commitment_tx());
 
+					let input_amount_sats = if let Some(funding_amount_sats) = output.funding_amount_sats {
+						funding_amount_sats
+					} else {
+						debug_assert!(false, "Funding amount should always exist for anchor-based claims");
+						self.channel_value_satoshis
+					};
+
+					let fee_sat = input_amount_sats - tx.output.iter()
+						.map(|output| output.value.to_sat()).sum::<u64>();
+					let commitment_tx_feerate_sat_per_1000_weight =
+						compute_feerate_sat_per_1000_weight(fee_sat, tx.weight().to_wu());
 					let package_target_feerate_sat_per_1000_weight = cached_request
 						.compute_package_feerate(fee_estimator, conf_target, feerate_strategy);
-					if let Some(input_amount_sat) = output.funding_amount {
-						let fee_sat = input_amount_sat - tx.0.output.iter().map(|output| output.value.to_sat()).sum::<u64>();
-						let commitment_tx_feerate_sat_per_1000_weight =
-							compute_feerate_sat_per_1000_weight(fee_sat, tx.0.weight().to_wu());
-						if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight {
-							log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW",
-								tx.0.compute_txid(), commitment_tx_feerate_sat_per_1000_weight,
-								package_target_feerate_sat_per_1000_weight);
-							return Some((new_timer, 0, OnchainClaim::Tx(tx.clone())));
-						}
+					if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight {
+						log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW",
+							tx.compute_txid(), commitment_tx_feerate_sat_per_1000_weight,
+							package_target_feerate_sat_per_1000_weight);
+						// The commitment transaction already meets the required feerate and doesn't
+						// need a CPFP. We still want to return something other than the event to
+						// register the claim.
+						return Some((new_timer, 0, OnchainClaim::Tx(MaybeSignedTransaction(tx))));
 					}
 
 					// We'll locate an anchor output we can spend within the commitment transaction.
 					let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey;
-					match chan_utils::get_keyed_anchor_output(&tx.0, funding_pubkey) {
+					match chan_utils::get_keyed_anchor_output(&tx, funding_pubkey) {
 						// An anchor output was found, so we should yield a funding event externally.
 						Some((idx, _)) => {
 							// TODO: Use a lower confirmation target when both our and the
@@ -681,7 +701,9 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 								package_target_feerate_sat_per_1000_weight as u64,
 								OnchainClaim::Event(ClaimEvent::BumpCommitment {
 									package_target_feerate_sat_per_1000_weight,
-									commitment_tx: tx.0.clone(),
+									commitment_tx: tx,
+									pending_nondust_htlcs: holder_commitment.htlcs().to_vec(),
+									commitment_tx_fee_satoshis: fee_sat,
 									anchor_output_idx: idx,
 								}),
 							))
@@ -690,7 +712,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 						// attempt to broadcast the transaction with its current fee rate and hope
 						// it confirms. This is essentially the same behavior as a commitment
 						// transaction without anchor outputs.
-						None => Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))),
+						None => Some((new_timer, 0, OnchainClaim::Tx(MaybeSignedTransaction(tx)))),
 					}
 				},
 				_ => {
@@ -1193,17 +1215,6 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 		self.prev_holder_commitment = Some(replace(&mut self.holder_commitment, tx));
 	}
 
-	pub(crate) fn get_unsigned_holder_commitment_tx(&self) -> &Transaction {
-		&self.holder_commitment.trust().built_transaction().transaction
-	}
-
-	pub(crate) fn get_maybe_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> MaybeSignedTransaction {
-		let tx = self.signer.sign_holder_commitment(&self.channel_transaction_parameters, &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());
-		MaybeSignedTransaction(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");
@@ -1410,7 +1421,7 @@ 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.get_unsigned_holder_commitment_tx().compute_txid();
+		let holder_commit_txid = tx_handler.current_holder_commitment_tx().trust().txid();
 		let mut requests = Vec::new();
 		for (htlc, _) in htlcs {
 			requests.push(PackageTemplate::build_package(

lightning/src/chain/package.rs

@@ -20,12 +20,14 @@ use bitcoin::transaction::{TxOut,TxIn, Transaction};
 use bitcoin::transaction::OutPoint as BitcoinOutPoint;
 use bitcoin::script::{Script, ScriptBuf};
 use bitcoin::hash_types::Txid;
-use bitcoin::secp256k1::{SecretKey,PublicKey};
+use bitcoin::secp256k1::{SecretKey, PublicKey};
 use bitcoin::sighash::EcdsaSighashType;
 use bitcoin::transaction::Version;
 
 use crate::types::payment::PaymentPreimage;
-use crate::ln::chan_utils::{self, TxCreationKeys, HTLCOutputInCommitment};
+use crate::ln::chan_utils::{
+	self, HolderCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment,
+};
 use crate::types::features::ChannelTypeFeatures;
 use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint};
 use crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA;
@@ -440,19 +442,41 @@ impl Readable for HolderHTLCOutput {
 #[derive(Clone, PartialEq, Eq)]
 pub(crate) struct HolderFundingOutput {
 	funding_redeemscript: ScriptBuf,
-	pub(crate) funding_amount: Option<u64>,
+	pub(crate) funding_amount_sats: Option<u64>,
 	channel_type_features: ChannelTypeFeatures,
+	pub(crate) commitment_tx: Option<HolderCommitmentTransaction>,
 }
 
 
 impl HolderFundingOutput {
-	pub(crate) fn build(funding_redeemscript: ScriptBuf, funding_amount: u64, channel_type_features: ChannelTypeFeatures) -> Self {
+	pub(crate) fn build(
+		commitment_tx: HolderCommitmentTransaction, funding_redeemscript: ScriptBuf,
+		funding_amount_sats: u64, channel_type_features: ChannelTypeFeatures,
+	) -> Self {
 		HolderFundingOutput {
 			funding_redeemscript,
-			funding_amount: Some(funding_amount),
+			funding_amount_sats: Some(funding_amount_sats),
 			channel_type_features,
+			commitment_tx: Some(commitment_tx),
 		}
 	}
+
+	pub(crate) fn get_maybe_signed_commitment_tx<Signer: EcdsaChannelSigner>(
+		&self, onchain_tx_handler: &mut OnchainTxHandler<Signer>,
+	) -> MaybeSignedTransaction {
+		let channel_parameters = &onchain_tx_handler.channel_transaction_parameters;
+		let commitment_tx = self.commitment_tx.as_ref()
+			.unwrap_or(onchain_tx_handler.current_holder_commitment_tx());
+		let maybe_signed_tx = onchain_tx_handler.signer
+			.sign_holder_commitment(channel_parameters, commitment_tx, &onchain_tx_handler.secp_ctx)
+			.map(|holder_sig| {
+				commitment_tx.add_holder_sig(&self.funding_redeemscript, holder_sig)
+			})
+			.unwrap_or_else(|_| {
+				commitment_tx.trust().built_transaction().transaction.clone()
+			});
+		MaybeSignedTransaction(maybe_signed_tx)
+	}
 }
 
 impl Writeable for HolderFundingOutput {
@@ -462,7 +486,8 @@ impl Writeable for HolderFundingOutput {
 			(0, self.funding_redeemscript, required),
 			(1, self.channel_type_features, required),
 			(2, legacy_deserialization_prevention_marker, option),
-			(3, self.funding_amount, option),
+			(3, self.funding_amount_sats, option),
+			(5, self.commitment_tx, option), // Added in 0.2
 		});
 		Ok(())
 	}
@@ -473,21 +498,24 @@ impl Readable for HolderFundingOutput {
 		let mut funding_redeemscript = RequiredWrapper(None);
 		let mut _legacy_deserialization_prevention_marker: Option<()> = None;
 		let mut channel_type_features = None;
-		let mut funding_amount = None;
+		let mut funding_amount_sats = None;
+		let mut commitment_tx = None;
 
 		read_tlv_fields!(reader, {
 			(0, funding_redeemscript, required),
 			(1, channel_type_features, option),
 			(2, _legacy_deserialization_prevention_marker, option),
-			(3, funding_amount, option),
+			(3, funding_amount_sats, option),
+			(5, commitment_tx, option), // Added in 0.2.
 		});
 
 		verify_channel_type_features(&channel_type_features, None)?;
 
 		Ok(Self {
 			funding_redeemscript: funding_redeemscript.0.unwrap(),
 			channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()),
-			funding_amount
+			funding_amount_sats,
+			commitment_tx,
 		})
 	}
 }
@@ -519,7 +547,7 @@ impl PackageSolvingData {
 			},
 			PackageSolvingData::HolderFundingOutput(ref outp) => {
 				debug_assert!(outp.channel_type_features.supports_anchors_zero_fee_htlc_tx());
-				outp.funding_amount.unwrap()
+				outp.funding_amount_sats.unwrap()
 			}
 		};
 		amt
@@ -716,7 +744,7 @@ impl PackageSolvingData {
 				onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage)
 			}
 			PackageSolvingData::HolderFundingOutput(ref outp) => {
-				Some(onchain_handler.get_maybe_signed_holder_tx(&outp.funding_redeemscript))
+				Some(outp.get_maybe_signed_commitment_tx(onchain_handler))
 			}
 			_ => { panic!("API Error!"); }
 		}
@@ -1406,7 +1434,7 @@ where
 mod tests {
 	use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc, feerate_bump};
 	use crate::chain::Txid;
-	use crate::ln::chan_utils::HTLCOutputInCommitment;
+	use crate::ln::chan_utils::{HolderCommitmentTransaction, HTLCOutputInCommitment};
 	use crate::types::payment::{PaymentPreimage, PaymentHash};
 	use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint};
 
@@ -1508,9 +1536,12 @@ mod tests {
 	}
 
 	macro_rules! dumb_funding_output {
-		() => {
-			PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key()))
-		}
+		() => {{
+			let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut Vec::new());
+			PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(
+				commitment_tx, ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key()
+			))
+		}}
 	}
 
 	#[test]