From 3c7e9dba5e612471e6706831b4f23a1c728d0e7b Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Tue, 1 Apr 2025 16:01:46 +0000
Subject: [PATCH 1/4] Add `ChannelContext::build_commitment_stats`

It can be useful to get the stats on a potential commitment transaction
without actually building it. Therefore, this commit splits the stats
calculations from the actual build of a commitment transaction.

This introduces an extra loop over the pending htlcs when actually
building a commitment transaction, but current network behavior
produces very few concurrent htlcs on channels. Furthermore, each
iteration of the loop in the stats calculation is very cheap.

The motivating use case for `build_commitment_stats` is to calculate the
balances of the channel parties in order to validate the
`funding_contribution_satoshis` field of `splice_init` and `splice_ack`
messages without building a full commitment transaction.
---
 lightning/src/ln/channel.rs | 417 ++++++++++++++++++++++--------------
 1 file changed, 255 insertions(+), 162 deletions(-)

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d5cccc0bc1f..096b73668ca 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -226,6 +226,37 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
 	}
 }
 
+impl fmt::Display for InboundHTLCState {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		match self {
+			InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"),
+			InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"),
+			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"),
+			InboundHTLCState::Committed => write!(f, "Committed"),
+			InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"),
+		}
+	}
+}
+
+impl InboundHTLCState {
+	fn included_in_commitment(&self, generated_by_local: bool) -> bool {
+		match self {
+			InboundHTLCState::RemoteAnnounced(_) => !generated_by_local,
+			InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local,
+			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true,
+			InboundHTLCState::Committed => true,
+			InboundHTLCState::LocalRemoved(_) => !generated_by_local,
+		}
+	}
+
+	fn preimage(&self) -> Option<PaymentPreimage> {
+		match self {
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => Some(*preimage),
+			_ => None,
+		}
+	}
+}
+
 struct InboundHTLCOutput {
 	htlc_id: u64,
 	amount_msat: u64,
@@ -234,6 +265,24 @@ struct InboundHTLCOutput {
 	state: InboundHTLCState,
 }
 
+impl InboundHTLCOutput {
+	fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool {
+		let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() {
+			0
+		} else {
+			let htlc_tx_weight = if !local {
+				// this is an offered htlc
+				htlc_timeout_tx_weight(features)
+			} else {
+				htlc_success_tx_weight(features)
+			};
+			// As required by the spec, round down
+			feerate_per_kw as u64 * htlc_tx_weight / 1000
+		};
+		self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat
+	}
+}
+
 #[cfg_attr(test, derive(Clone, Debug, PartialEq))]
 enum OutboundHTLCState {
 	/// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we
@@ -290,6 +339,41 @@ impl From<&OutboundHTLCState> for OutboundHTLCStateDetails {
 	}
 }
 
+impl fmt::Display for OutboundHTLCState {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		match self {
+			OutboundHTLCState::LocalAnnounced(_) => write!(f, "LocalAnnounced"),
+			OutboundHTLCState::Committed => write!(f, "Committed"),
+			OutboundHTLCState::RemoteRemoved(_) => write!(f, "RemoteRemoved"),
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => write!(f, "AwaitingRemoteRevokeToRemove"),
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => write!(f, "AwaitingRemovedRemoteRevoke"),
+		}
+	}
+}
+
+impl OutboundHTLCState {
+	fn included_in_commitment(&self, generated_by_local: bool) -> bool {
+		match self {
+			OutboundHTLCState::LocalAnnounced(_) => generated_by_local,
+			OutboundHTLCState::Committed => true,
+			OutboundHTLCState::RemoteRemoved(_) => generated_by_local,
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => generated_by_local,
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => false,
+		}
+	}
+
+	fn preimage(&self) -> Option<PaymentPreimage> {
+		match self {
+			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage))
+			| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage))
+			| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => {
+				preimage.as_ref().copied()
+			},
+			_ => None,
+		}
+	}
+}
+
 #[derive(Clone)]
 #[cfg_attr(test, derive(Debug, PartialEq))]
 enum OutboundHTLCOutcome {
@@ -329,6 +413,24 @@ struct OutboundHTLCOutput {
 	send_timestamp: Option<Duration>,
 }
 
+impl OutboundHTLCOutput {
+	fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool {
+		let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() {
+			0
+		} else {
+			let htlc_tx_weight = if local {
+				// this is an offered htlc
+				htlc_timeout_tx_weight(features)
+			} else {
+				htlc_success_tx_weight(features)
+			};
+			// As required by the spec, round down
+			feerate_per_kw as u64 * htlc_tx_weight / 1000
+		};
+		self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat
+	}
+}
+
 /// See AwaitingRemoteRevoke ChannelState for more info
 #[cfg_attr(test, derive(Clone, Debug, PartialEq))]
 enum HTLCUpdateAwaitingACK {
@@ -883,6 +985,7 @@ struct HTLCStats {
 
 /// A struct gathering data on a commitment, either local or remote.
 struct CommitmentData<'a> {
+	tx: CommitmentTransaction,
 	stats: CommitmentStats,
 	htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
 	outbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
@@ -891,10 +994,12 @@ struct CommitmentData<'a> {
 
 /// A struct gathering stats on a commitment transaction, either local or remote.
 struct CommitmentStats {
-	tx: CommitmentTransaction, // the transaction info
+	feerate_per_kw: u32, // the feerate of the commitment transaction
 	total_fee_sat: u64, // the total fee included in the transaction
-	local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits
-	remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits
+	total_anchors_sat: u64, // the sum of the anchors' amounts
+	broadcaster_dust_limit_sat: u64, // the broadcaster's dust limit
+	local_balance_before_fee_anchors_msat: u64, // local balance before fees and anchors *not* considering dust limits
+	remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits
 }
 
 /// Used when calculating whether we or the remote can afford an additional HTLC.
@@ -2124,7 +2229,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
 		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.stats.tx;
+		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());
@@ -2164,7 +2269,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide
 		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.stats.tx;
+		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();
 
@@ -3646,7 +3751,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(),
 			true, false, logger);
 		let commitment_txid = {
-			let trusted_tx = commitment_data.stats.tx.trust();
+			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());
 
@@ -3669,7 +3774,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		if update_fee {
 			debug_assert!(!funding.is_outbound());
 			let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000;
-			if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
+			if commitment_data.stats.remote_balance_before_fee_anchors_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat {
 				return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned()));
 			}
 		}
@@ -3691,16 +3796,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}
 		}
 
-		if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() {
-			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len())));
+		if msg.htlc_signatures.len() != commitment_data.tx.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.htlcs().len())));
 		}
 
-		let holder_keys = commitment_data.stats.tx.trust().keys();
-		let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.stats.tx.htlcs().len());
-		let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.stats.tx.htlcs().len());
+		let holder_keys = commitment_data.tx.trust().keys();
+		let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.htlcs().len());
+		let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.htlcs().len());
 		for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() {
 			if let Some(_) = htlc.transaction_output_index {
-				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(),
+				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);
 
@@ -3727,7 +3832,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		}
 
 		let holder_commitment_tx = HolderCommitmentTransaction::new(
-			commitment_data.stats.tx,
+			commitment_data.tx,
 			msg.signature,
 			msg.htlc_signatures.clone(),
 			&funding.get_holder_pubkeys().funding_pubkey,
@@ -3744,28 +3849,12 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		})
 	}
 
-	/// Transaction nomenclature is somewhat confusing here as there are many different cases - a
-	/// transaction is referred to as "a's transaction" implying that a will be able to broadcast
-	/// the transaction. Thus, b will generally be sending a signature over such a transaction to
-	/// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As
-	/// such, a transaction is generally the result of b increasing the amount paid to a (or adding
-	/// an HTLC to a).
-	/// @local is used only to convert relevant internal structures which refer to remote vs local
-	/// to decide value of outputs and direction of HTLCs.
-	/// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC
-	/// state may indicate that one peer has informed the other that they'd like to add an HTLC but
-	/// have not yet committed it. Such HTLCs will only be included in transactions which are being
-	/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
-	/// which peer generated this transaction and "to whom" this transaction flows.
+	/// Builds stats on a potential commitment transaction build, without actually building the
+	/// commitment transaction. See `build_commitment_transaction` for further docs.
 	#[inline]
-	fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData
-		where L::Target: Logger
-	{
-		let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
-		let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
-		let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
-
-		let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
+	fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool) -> CommitmentStats {
+		let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
+		let mut non_dust_htlc_count = 0;
 		let mut remote_htlc_total_msat = 0;
 		let mut local_htlc_total_msat = 0;
 		let mut value_to_self_msat_offset = 0;
@@ -3783,122 +3872,34 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}
 		}
 
-		log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...",
-			commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number),
-			get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()),
-			&self.channel_id,
-			if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
-
-		macro_rules! get_htlc_in_commitment {
-			($htlc: expr, $offered: expr) => {
-				HTLCOutputInCommitment {
-					offered: $offered,
-					amount_msat: $htlc.amount_msat,
-					cltv_expiry: $htlc.cltv_expiry,
-					payment_hash: $htlc.payment_hash,
-					transaction_output_index: None
-				}
-			}
-		}
-
-		macro_rules! add_htlc_output {
-			($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
-				if $outbound == local { // "offered HTLC output"
-					let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
-					let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
-						0
-					} else {
-						feerate_per_kw as u64 * htlc_timeout_tx_weight(funding.get_channel_type()) / 1000
-					};
-					if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
-						log_trace!(logger, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
-						included_non_dust_htlcs.push((htlc_in_tx, $source));
-					} else {
-						log_trace!(logger, "   ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
-						included_dust_htlcs.push((htlc_in_tx, $source));
-					}
-				} else {
-					let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
-					let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
-						0
-					} else {
-						feerate_per_kw as u64 * htlc_success_tx_weight(funding.get_channel_type()) / 1000
-					};
-					if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
-						log_trace!(logger, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
-						included_non_dust_htlcs.push((htlc_in_tx, $source));
-					} else {
-						log_trace!(logger, "   ...including {} {} dust HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
-						included_dust_htlcs.push((htlc_in_tx, $source));
-					}
+		for htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.state.included_in_commitment(generated_by_local) {
+				if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) {
+					non_dust_htlc_count += 1;
 				}
-			}
-		}
-
-		let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
-
-		for ref htlc in self.pending_inbound_htlcs.iter() {
-			let (include, state_name) = match htlc.state {
-				InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
-				InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!generated_by_local, "AwaitingRemoteRevokeToAnnounce"),
-				InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => (true, "AwaitingAnnouncedRemoteRevoke"),
-				InboundHTLCState::Committed => (true, "Committed"),
-				InboundHTLCState::LocalRemoved(_) => (!generated_by_local, "LocalRemoved"),
-			};
-
-			if include {
-				add_htlc_output!(htlc, false, None, state_name);
 				remote_htlc_total_msat += htlc.amount_msat;
 			} else {
-				log_trace!(logger, "   ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
-				match &htlc.state {
-					&InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => {
-						inbound_htlc_preimages.push(preimage);
-						value_to_self_msat_offset += htlc.amount_msat as i64;
-					},
-					_ => {},
+				if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state {
+					value_to_self_msat_offset += htlc.amount_msat as i64;
 				}
 			}
-		}
-
-
-		let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
-
-		for ref htlc in self.pending_outbound_htlcs.iter() {
-			let (include, state_name) = match htlc.state {
-				OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
-				OutboundHTLCState::Committed => (true, "Committed"),
-				OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"),
-				OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
-				OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"),
-			};
-
-			let preimage_opt = match htlc.state {
-				OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) => p,
-				OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) => p,
-				OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p,
-				_ => None,
-			};
-
-			if let Some(preimage) = preimage_opt {
-				outbound_htlc_preimages.push(preimage);
-			}
+		};
 
-			if include {
-				add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
+		for htlc in self.pending_outbound_htlcs.iter() {
+			if htlc.state.included_in_commitment(generated_by_local) {
+				if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) {
+					non_dust_htlc_count += 1;
+				}
 				local_htlc_total_msat += htlc.amount_msat;
 			} else {
-				log_trace!(logger, "   ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
-				match htlc.state {
-					OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) |
+				if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) |
 					OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) |
-					OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => {
-						value_to_self_msat_offset -= htlc.amount_msat as i64;
-					},
-					_ => {},
+					OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_))
+				= htlc.state {
+					value_to_self_msat_offset -= htlc.amount_msat as i64;
 				}
 			}
-		}
+		};
 
 		// # Panics
 		//
@@ -3927,31 +3928,129 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
 		}
 
+		let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, non_dust_htlc_count, &funding.channel_transaction_parameters.channel_type_features);
+		let total_anchors_sat = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
+
+		CommitmentStats {
+			feerate_per_kw,
+			total_fee_sat,
+			total_anchors_sat,
+			broadcaster_dust_limit_sat,
+			local_balance_before_fee_anchors_msat: value_to_self_msat,
+			remote_balance_before_fee_anchors_msat: value_to_remote_msat,
+		}
+	}
+
+	/// Transaction nomenclature is somewhat confusing here as there are many different cases - a
+	/// transaction is referred to as "a's transaction" implying that a will be able to broadcast
+	/// the transaction. Thus, b will generally be sending a signature over such a transaction to
+	/// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As
+	/// such, a transaction is generally the result of b increasing the amount paid to a (or adding
+	/// an HTLC to a).
+	/// @local is used only to convert relevant internal structures which refer to remote vs local
+	/// to decide value of outputs and direction of HTLCs.
+	/// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC
+	/// state may indicate that one peer has informed the other that they'd like to add an HTLC but
+	/// have not yet committed it. Such HTLCs will only be included in transactions which are being
+	/// generated by the peer which proposed adding the HTLCs, and thus we need to understand both
+	/// which peer generated this transaction and "to whom" this transaction flows.
+	#[inline]
+	fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData
+		where L::Target: Logger
+	{
+		let stats = self.build_commitment_stats(funding, local, generated_by_local);
+		let CommitmentStats {
+			feerate_per_kw,
+			total_fee_sat,
+			total_anchors_sat,
+			broadcaster_dust_limit_sat,
+			local_balance_before_fee_anchors_msat,
+			remote_balance_before_fee_anchors_msat
+		} = stats;
+
+		let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
+		let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
+		let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
+
+		log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...",
+			commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number),
+			get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()),
+			self.channel_id,
+			if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
+
+		macro_rules! get_htlc_in_commitment {
+			($htlc: expr, $offered: expr) => {
+				HTLCOutputInCommitment {
+					offered: $offered,
+					amount_msat: $htlc.amount_msat,
+					cltv_expiry: $htlc.cltv_expiry,
+					payment_hash: $htlc.payment_hash,
+					transaction_output_index: None
+				}
+			}
+		}
+
+		macro_rules! add_htlc_output {
+			($htlc: expr, $outbound: expr, $source: expr) => {
+				let htlc_in_tx = get_htlc_in_commitment!($htlc, $outbound == local);
+				if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) {
+					log_trace!(logger, "   ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat);
+					included_dust_htlcs.push((htlc_in_tx, $source));
+				} else {
+					log_trace!(logger, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat);
+					included_non_dust_htlcs.push((htlc_in_tx, $source));
+				}
+			}
+		}
+
+		let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
+		let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
+
+		for htlc in self.pending_inbound_htlcs.iter() {
+			if htlc.state.included_in_commitment(generated_by_local) {
+				add_htlc_output!(htlc, false, None);
+			} else {
+				log_trace!(logger, "   ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state);
+				if let Some(preimage) = htlc.state.preimage() {
+					inbound_htlc_preimages.push(preimage);
+				}
+			}
+		};
+
+		for htlc in self.pending_outbound_htlcs.iter() {
+			if let Some(preimage) = htlc.state.preimage() {
+				outbound_htlc_preimages.push(preimage);
+			}
+			if htlc.state.included_in_commitment(generated_by_local) {
+				add_htlc_output!(htlc, true, Some(&htlc.source));
+			} else {
+				log_trace!(logger, "   ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state);
+			}
+		};
+
 		// We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater
-		// than or equal to the sum of `total_fee_sat` and `anchors_val`.
+		// than or equal to the sum of `total_fee_sat` and `total_anchors_sat`.
 		//
 		// This is because when the remote party sends an `update_fee` message, we build the new
 		// commitment transaction *before* checking whether the remote party's balance is enough to
 		// cover the total fee and the anchors.
 
-		let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), funding.get_channel_type());
-		let anchors_val = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
 		let (value_to_self, value_to_remote) = if funding.is_outbound() {
-			((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000)
+			((local_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat), remote_balance_before_fee_anchors_msat / 1000)
 		} else {
-			(value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat))
+			(local_balance_before_fee_anchors_msat / 1000, (remote_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat))
 		};
 
 		let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote };
 		let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self };
 
-		if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis {
+		if to_broadcaster_value_sat >= broadcaster_dust_limit_sat {
 			log_trace!(logger, "   ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat);
 		} else {
 			to_broadcaster_value_sat = 0;
 		}
 
-		if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis {
+		if to_countersignatory_value_sat >= broadcaster_dust_limit_sat {
 			log_trace!(logger, "   ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat);
 		} else {
 			to_countersignatory_value_sat = 0;
@@ -3974,14 +4073,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
 		htlcs_included.append(&mut included_dust_htlcs);
 
-		let stats = CommitmentStats {
-			tx,
-			total_fee_sat,
-			local_balance_before_fee_msat: value_to_self_msat,
-			remote_balance_before_fee_msat: value_to_remote_msat,
-		};
-
 		CommitmentData {
+			tx,
 			stats,
 			htlcs_included,
 			inbound_htlc_preimages,
@@ -4773,7 +4866,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		let commitment_data = self.build_commitment_transaction(funding,
 			self.cur_counterparty_commitment_transaction_number,
 			&self.counterparty_cur_commitment_point.unwrap(), false, false, logger);
-		let counterparty_initial_commitment_tx = commitment_data.stats.tx;
+		let counterparty_initial_commitment_tx = commitment_data.tx;
 		match self.holder_signer {
 			// TODO (taproot|arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ref ecdsa) => {
@@ -6557,8 +6650,8 @@ impl<SP: Deref> FundedChannel<SP> where
 			&self.funding, self.holder_commitment_point.transaction_number(),
 			&self.holder_commitment_point.current_point(), true, true, logger,
 		);
-		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000;
-		let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
+		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000;
+		let holder_balance_msat = commitment_data.stats.local_balance_before_fee_anchors_msat - htlc_stats.outbound_holding_cell_msat;
 		if holder_balance_msat < buffer_fee_msat  + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
 			//TODO: auto-close after a number of failures?
 			log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
@@ -6872,7 +6965,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			let commitment_data = self.context.build_commitment_transaction(&self.funding,
 				self.context.cur_counterparty_commitment_transaction_number + 1,
 				&self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
-			let counterparty_initial_commitment_tx = commitment_data.stats.tx;
+			let counterparty_initial_commitment_tx = commitment_data.tx;
 			self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx)
 		} else { None };
 		// Provide a `channel_ready` message if we need to, but only if we're _not_ still pending
@@ -8988,7 +9081,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			funding, self.context.cur_counterparty_commitment_transaction_number,
 			&self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger,
 		);
-		let counterparty_commitment_tx = commitment_data.stats.tx;
+		let counterparty_commitment_tx = commitment_data.tx;
 
 		#[cfg(any(test, fuzzing))]
 		{
@@ -9039,7 +9132,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			funding, self.context.cur_counterparty_commitment_transaction_number,
 			&self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger,
 		);
-		let counterparty_commitment_tx = commitment_data.stats.tx;
+		let counterparty_commitment_tx = commitment_data.tx;
 
 		match &self.context.holder_signer {
 			ChannelSignerType::Ecdsa(ecdsa) => {
@@ -9532,7 +9625,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 		let commitment_data = self.context.build_commitment_transaction(&self.funding,
 			self.context.cur_counterparty_commitment_transaction_number,
 			&self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger);
-		let counterparty_initial_commitment_tx = commitment_data.stats.tx;
+		let counterparty_initial_commitment_tx = commitment_data.tx;
 		let signature = match &self.context.holder_signer {
 			// TODO (taproot|arik): move match into calling method for Taproot
 			ChannelSignerType::Ecdsa(ecdsa) => {
@@ -12262,7 +12355,7 @@ mod tests {
 			} ) => { {
 				let commitment_data = chan.context.build_commitment_transaction(&chan.funding,
 					0xffffffffffff - 42, &per_commitment_point, true, false, &logger);
-				let commitment_tx = commitment_data.stats.tx;
+				let commitment_tx = commitment_data.tx;
 				let trusted_tx = commitment_tx.trust();
 				let unsigned_tx = trusted_tx.built_transaction();
 				let redeemscript = chan.funding.get_funding_redeemscript();

From 48fd1db91da686eb14901d23b49f8faa10cbc2a9 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Mon, 24 Mar 2025 23:12:55 +0000
Subject: [PATCH 2/4] Rename `CommitmentTransaction::htlcs` to `nondust_htlcs`

---
 lightning/src/chain/channelmonitor.rs | 36 +++++++++++++--------------
 lightning/src/chain/onchaintx.rs      |  4 +--
 lightning/src/chain/package.rs        |  2 +-
 lightning/src/ln/chan_utils.rs        | 30 +++++++++++-----------
 lightning/src/ln/channel.rs           | 22 ++++++++--------
 lightning/src/sign/mod.rs             |  4 +--
 6 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index f709183f2d1..a74cb7d7685 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -317,7 +317,7 @@ impl HolderCommitment {
 		let delayed_payment_key = &tx_keys.broadcaster_delayed_payment_key;
 		let per_commitment_point = &tx_keys.per_commitment_point;
 
-		let mut nondust_htlcs = self.tx.htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter());
+		let mut nondust_htlcs = self.tx.nondust_htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter());
 		let mut sources = self.nondust_htlc_sources.iter();
 
 		// Use an iterator to write `htlc_outputs` to avoid allocations.
@@ -937,7 +937,7 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment
 		// HTLC sources, separately. All offered, non-dust HTLCs must have a source available.
 
 		let mut missing_nondust_source = false;
-		let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.htlcs().len());
+		let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len());
 		let dust_htlcs = holder_signed_tx.htlc_outputs.into_iter().filter_map(|(htlc, _, source)| {
 			// Filter our non-dust HTLCs, while at the same time pushing their sources into
 			// `nondust_htlc_sources`.
@@ -967,16 +967,16 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment
 
 impl HolderCommitment {
 	fn has_htlcs(&self) -> bool {
-		self.tx.htlcs().len() > 0 || self.dust_htlcs.len() > 0
+		self.tx.nondust_htlcs().len() > 0 || self.dust_htlcs.len() > 0
 	}
 
 	fn htlcs(&self) -> impl Iterator<Item = &HTLCOutputInCommitment> {
-		self.tx.htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc))
+		self.tx.nondust_htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc))
 	}
 
 	fn htlcs_with_sources(&self) -> impl Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> {
 		let mut sources = self.nondust_htlc_sources.iter();
-		let nondust_htlcs = self.tx.htlcs().iter().map(move |htlc| {
+		let nondust_htlcs = self.tx.nondust_htlcs().iter().map(move |htlc| {
 			let mut source = None;
 			if htlc.offered && htlc.transaction_output_index.is_some() {
 				source = sources.next();
@@ -3098,8 +3098,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			// If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the
 			// `holder_commitment_tx`. In the future, we'll no longer provide the redundant data
 			// and just pass in source data via `nondust_htlc_sources`.
-			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().htlcs().len());
-			for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().htlcs().iter()) {
+			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().nondust_htlcs().len());
+			for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().nondust_htlcs().iter()) {
 				debug_assert_eq!(a, b);
 			}
 			debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.counterparty_htlc_sigs.len());
@@ -3109,7 +3109,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 			// Backfill the non-dust HTLC sources.
 			debug_assert!(nondust_htlc_sources.is_empty());
-			nondust_htlc_sources.reserve_exact(holder_commitment_tx.htlcs().len());
+			nondust_htlc_sources.reserve_exact(holder_commitment_tx.nondust_htlcs().len());
 			let dust_htlcs = htlc_outputs.into_iter().filter_map(|(htlc, _, source)| {
 				// Filter our non-dust HTLCs, while at the same time pushing their sources into
 				// `nondust_htlc_sources`.
@@ -3129,7 +3129,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			// `nondust_htlc_sources` and the `holder_commitment_tx`
 			{
 				let mut prev = -1;
-				for htlc in holder_commitment_tx.trust().htlcs().iter() {
+				for htlc in holder_commitment_tx.trust().nondust_htlcs().iter() {
 					assert!(htlc.transaction_output_index.unwrap() as i32 > prev);
 					prev = htlc.transaction_output_index.unwrap() as i32;
 				}
@@ -3137,10 +3137,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 			debug_assert!(htlc_outputs.iter().all(|(htlc, _, _)| htlc.transaction_output_index.is_none()));
 			debug_assert!(htlc_outputs.iter().all(|(_, sig_opt, _)| sig_opt.is_none()));
-			debug_assert_eq!(holder_commitment_tx.trust().htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len());
+			debug_assert_eq!(holder_commitment_tx.trust().nondust_htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len());
 
 			let mut sources = nondust_htlc_sources.iter();
-			for htlc in holder_commitment_tx.trust().htlcs().iter() {
+			for htlc in holder_commitment_tx.trust().nondust_htlcs().iter() {
 				if htlc.offered {
 					let source = sources.next().expect("Non-dust HTLC sources didn't match commitment tx");
 					assert!(source.possibly_matches_output(htlc));
@@ -3955,9 +3955,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		&self, holder_tx: &HolderCommitmentTransaction,
 	) -> Vec<HTLCDescriptor> {
 		let tx = holder_tx.trust();
-		let mut htlcs = Vec::with_capacity(holder_tx.htlcs().len());
-		debug_assert_eq!(holder_tx.htlcs().len(), holder_tx.counterparty_htlc_sigs.len());
-		for (htlc, counterparty_sig) in holder_tx.htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) {
+		let mut htlcs = Vec::with_capacity(holder_tx.nondust_htlcs().len());
+		debug_assert_eq!(holder_tx.nondust_htlcs().len(), holder_tx.counterparty_htlc_sigs.len());
+		for (htlc, counterparty_sig) in holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) {
 			assert!(htlc.transaction_output_index.is_some(), "Expected transaction output index for non-dust HTLC");
 
 			let preimage = if htlc.offered {
@@ -4026,9 +4026,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	// Returns holder HTLC outputs to watch and react to in case of spending.
 	fn get_broadcasted_holder_watch_outputs(&self, holder_tx: &HolderCommitmentTransaction) -> Vec<(u32, TxOut)> {
-		let mut watch_outputs = Vec::with_capacity(holder_tx.htlcs().len());
+		let mut watch_outputs = Vec::with_capacity(holder_tx.nondust_htlcs().len());
 		let tx = holder_tx.trust();
-		for htlc in holder_tx.htlcs() {
+		for htlc in holder_tx.nondust_htlcs() {
 			if let Some(transaction_output_index) = htlc.transaction_output_index {
 				watch_outputs.push((
 					transaction_output_index,
@@ -4121,7 +4121,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			let txid = self.funding.current_holder_commitment.tx.trust().txid();
 			log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid);
 			let mut outpoint = BitcoinOutPoint { txid, vout: 0 };
-			for htlc in self.funding.current_holder_commitment.tx.htlcs() {
+			for htlc in self.funding.current_holder_commitment.tx.nondust_htlcs() {
 				if let Some(vout) = htlc.transaction_output_index {
 					outpoint.vout = vout;
 					self.onchain_tx_handler.abandon_claim(&outpoint);
@@ -4135,7 +4135,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			if txid != *confirmed_commitment_txid {
 				log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid);
 				let mut outpoint = BitcoinOutPoint { txid, vout: 0 };
-				for htlc in prev_holder_commitment.tx.htlcs() {
+				for htlc in prev_holder_commitment.tx.nondust_htlcs() {
 					if let Some(vout) = htlc.transaction_output_index {
 						outpoint.vout = vout;
 						self.onchain_tx_handler.abandon_claim(&outpoint);
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index 645296d6411..92a48034b1d 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -688,7 +688,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 								OnchainClaim::Event(ClaimEvent::BumpCommitment {
 									package_target_feerate_sat_per_1000_weight,
 									commitment_tx: tx,
-									pending_nondust_htlcs: holder_commitment.htlcs().to_vec(),
+									pending_nondust_htlcs: holder_commitment.nondust_htlcs().to_vec(),
 									commitment_tx_fee_satoshis: fee_sat,
 									anchor_output_idx: idx,
 									channel_parameters: channel_parameters.clone(),
@@ -1339,7 +1339,7 @@ mod tests {
 		let holder_commit = tx_handler.current_holder_commitment_tx();
 		let holder_commit_txid = holder_commit.trust().txid();
 		let mut requests = Vec::new();
-		for (htlc, counterparty_sig) in holder_commit.htlcs().iter().zip(holder_commit.counterparty_htlc_sigs.iter()) {
+		for (htlc, counterparty_sig) in holder_commit.nondust_htlcs().iter().zip(holder_commit.counterparty_htlc_sigs.iter()) {
 			requests.push(PackageTemplate::build_package(
 				holder_commit_txid,
 				htlc.transaction_output_index.unwrap(),
diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs
index 14a884ca6f6..cee72e081a6 100644
--- a/lightning/src/chain/package.rs
+++ b/lightning/src/chain/package.rs
@@ -472,7 +472,7 @@ impl HolderHTLCOutput {
 			}
 
 			let (htlc, counterparty_sig) =
-				trusted_tx.htlcs().iter().zip(holder_commitment.counterparty_htlc_sigs.iter())
+				trusted_tx.nondust_htlcs().iter().zip(holder_commitment.counterparty_htlc_sigs.iter())
 					.find(|(htlc, _)| htlc.transaction_output_index.unwrap() == outp.vout)
 					.unwrap();
 
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 8d59304d566..c978680a620 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -1430,7 +1430,7 @@ pub struct CommitmentTransaction {
 	feerate_per_kw: u32,
 	// The set of non-dust HTLCs included in the commitment. They must be sorted in increasing
 	// output index order.
-	htlcs: Vec<HTLCOutputInCommitment>,
+	nondust_htlcs: Vec<HTLCOutputInCommitment>,
 	// Note that on upgrades, some features of existing outputs may be missed.
 	channel_type_features: ChannelTypeFeatures,
 	// A cache of the parties' pubkeys required to construct the transaction, see doc for trust()
@@ -1446,7 +1446,7 @@ impl PartialEq for CommitmentTransaction {
 			self.to_broadcaster_value_sat == o.to_broadcaster_value_sat &&
 			self.to_countersignatory_value_sat == o.to_countersignatory_value_sat &&
 			self.feerate_per_kw == o.feerate_per_kw &&
-			self.htlcs == o.htlcs &&
+			self.nondust_htlcs == o.nondust_htlcs &&
 			self.channel_type_features == o.channel_type_features &&
 			self.keys == o.keys;
 		if eq {
@@ -1468,7 +1468,7 @@ impl Writeable for CommitmentTransaction {
 			(6, self.feerate_per_kw, required),
 			(8, self.keys, required),
 			(10, self.built, required),
-			(12, self.htlcs, required_vec),
+			(12, self.nondust_htlcs, required_vec),
 			(14, legacy_deserialization_prevention_marker, option),
 			(15, self.channel_type_features, required),
 		});
@@ -1486,7 +1486,7 @@ impl Readable for CommitmentTransaction {
 			(6, feerate_per_kw, required),
 			(8, keys, required),
 			(10, built, required),
-			(12, htlcs, required_vec),
+			(12, nondust_htlcs, required_vec),
 			(14, _legacy_deserialization_prevention_marker, (option, explicit_type: ())),
 			(15, channel_type_features, option),
 		});
@@ -1503,7 +1503,7 @@ impl Readable for CommitmentTransaction {
 			feerate_per_kw: feerate_per_kw.0.unwrap(),
 			keys: keys.0.unwrap(),
 			built: built.0.unwrap(),
-			htlcs,
+			nondust_htlcs,
 			channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key())
 		})
 	}
@@ -1526,7 +1526,7 @@ impl CommitmentTransaction {
 		let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
 
 		// Sort outputs and populate output indices while keeping track of the auxiliary data
-		let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters);
+		let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters);
 
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters);
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1537,7 +1537,7 @@ impl CommitmentTransaction {
 			to_countersignatory_value_sat,
 			to_broadcaster_delay: Some(channel_parameters.contest_delay()),
 			feerate_per_kw,
-			htlcs,
+			nondust_htlcs,
 			channel_type_features: channel_parameters.channel_type_features().clone(),
 			keys,
 			built: BuiltCommitmentTransaction {
@@ -1558,7 +1558,7 @@ impl CommitmentTransaction {
 	fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction {
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);
 
-		let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
+		let mut htlcs_with_aux = self.nondust_htlcs.iter().map(|h| (h.clone(), ())).collect();
 		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters);
 
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1653,7 +1653,7 @@ impl CommitmentTransaction {
 			}
 		}
 
-		let mut htlcs = Vec::with_capacity(htlcs_with_aux.len());
+		let mut nondust_htlcs = Vec::with_capacity(htlcs_with_aux.len());
 		for (htlc, _) in htlcs_with_aux {
 			let script = get_htlc_redeemscript(htlc, channel_type, keys);
 			let txout = TxOut {
@@ -1683,11 +1683,11 @@ impl CommitmentTransaction {
 		for (idx, out) in txouts.drain(..).enumerate() {
 			if let Some(htlc) = out.1 {
 				htlc.transaction_output_index = Some(idx as u32);
-				htlcs.push(htlc.clone());
+				nondust_htlcs.push(htlc.clone());
 			}
 			outputs.push(out.0);
 		}
-		(outputs, htlcs)
+		(outputs, nondust_htlcs)
 	}
 
 	fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec<TxIn>) {
@@ -1746,8 +1746,8 @@ impl CommitmentTransaction {
 	///
 	/// This is not exported to bindings users as we cannot currently convert Vec references to/from C, though we should
 	/// expose a less effecient version which creates a Vec of references in the future.
-	pub fn htlcs(&self) -> &Vec<HTLCOutputInCommitment> {
-		&self.htlcs
+	pub fn nondust_htlcs(&self) -> &Vec<HTLCOutputInCommitment> {
+		&self.nondust_htlcs
 	}
 
 	/// Trust our pre-built transaction and derived transaction creation public keys.
@@ -1831,10 +1831,10 @@ impl<'a> TrustedCommitmentTransaction<'a> {
 		let inner = self.inner;
 		let keys = &inner.keys;
 		let txid = inner.built.txid;
-		let mut ret = Vec::with_capacity(inner.htlcs.len());
+		let mut ret = Vec::with_capacity(inner.nondust_htlcs.len());
 		let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key);
 
-		for this_htlc in inner.htlcs.iter() {
+		for this_htlc in inner.nondust_htlcs.iter() {
 			assert!(this_htlc.transaction_output_index.is_some());
 			let htlc_tx = build_htlc_transaction(&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
 
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 096b73668ca..5cb540d0000 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3796,13 +3796,13 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}
 		}
 
-		if msg.htlc_signatures.len() != commitment_data.tx.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.htlcs().len())));
+		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.htlcs().len());
-		let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.htlcs().len());
+		let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len());
+		let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.tx.nondust_htlcs().len());
 		for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() {
 			if let Some(_) = htlc.transaction_output_index {
 				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(),
@@ -6650,7 +6650,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			&self.funding, self.holder_commitment_point.transaction_number(),
 			&self.holder_commitment_point.current_point(), true, true, logger,
 		);
-		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000;
+		let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.nondust_htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000;
 		let holder_balance_msat = commitment_data.stats.local_balance_before_fee_anchors_msat - htlc_stats.outbound_holding_cell_msat;
 		if holder_balance_msat < buffer_fee_msat  + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
 			//TODO: auto-close after a number of failures?
@@ -9094,7 +9094,7 @@ impl<SP: Deref> FundedChannel<SP> where
 						&& info.next_holder_htlc_id == self.context.next_holder_htlc_id
 						&& info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id
 						&& info.feerate == self.context.feerate_per_kw {
-							let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.funding.get_channel_type()) * 1000;
+							let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.nondust_htlcs().len(), self.funding.get_channel_type()) * 1000;
 							assert_eq!(actual_fee, info.fee);
 						}
 				}
@@ -9156,8 +9156,8 @@ impl<SP: Deref> FundedChannel<SP> where
 						log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id());
 
 					let counterparty_keys = trusted_tx.keys();
-					debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len());
-					for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) {
+					debug_assert_eq!(htlc_signatures.len(), trusted_tx.nondust_htlcs().len());
+					for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.nondust_htlcs()) {
 						log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
 							encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), funding.get_holder_selected_contest_delay(), htlc, funding.get_channel_type(), &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
 							encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &counterparty_keys)),
@@ -12370,10 +12370,10 @@ mod tests {
 				counterparty_htlc_sigs.clear(); // Don't warn about excess mut for no-HTLC calls
 				$({
 					let remote_signature = Signature::from_der(&<Vec<u8>>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
-					per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature)));
+					per_htlc.push((commitment_tx.nondust_htlcs()[$htlc_idx].clone(), Some(remote_signature)));
 					counterparty_htlc_sigs.push(remote_signature);
 				})*
-				assert_eq!(commitment_tx.htlcs().len(), per_htlc.len());
+				assert_eq!(commitment_tx.nondust_htlcs().len(), per_htlc.len());
 
 				let holder_commitment_tx = HolderCommitmentTransaction::new(
 					commitment_tx.clone(),
@@ -12396,7 +12396,7 @@ mod tests {
 					log_trace!(logger, "verifying htlc {}", $htlc_idx);
 					let remote_signature = Signature::from_der(&<Vec<u8>>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
 
-					let ref htlc = commitment_tx.htlcs()[$htlc_idx];
+					let ref htlc = commitment_tx.nondust_htlcs()[$htlc_idx];
 					let keys = commitment_tx.trust().keys();
 					let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
 						chan.funding.get_counterparty_selected_contest_delay().unwrap(),
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index df79df6bab8..eb3d57e6dec 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -1364,8 +1364,8 @@ impl EcdsaChannelSigner for InMemorySigner {
 		);
 		let commitment_txid = built_tx.txid;
 
-		let mut htlc_sigs = Vec::with_capacity(commitment_tx.htlcs().len());
-		for htlc in commitment_tx.htlcs() {
+		let mut htlc_sigs = Vec::with_capacity(commitment_tx.nondust_htlcs().len());
+		for htlc in commitment_tx.nondust_htlcs() {
 			let holder_selected_contest_delay = channel_parameters.holder_selected_contest_delay;
 			let chan_type = &channel_parameters.channel_type_features;
 			let htlc_tx = chan_utils::build_htlc_transaction(

From 28dc94a65dbe44366519c028afdf681a931cb9a0 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Tue, 1 Apr 2025 15:59:59 +0000
Subject: [PATCH 3/4] Remove unnecessary clones of the HTLC-HTLCSource table

---
 lightning/src/ln/channel.rs | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 5cb540d0000..41298ce0c3f 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3764,7 +3764,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			}
 			bitcoin_tx.txid
 		};
-		let htlcs_cloned: Vec<_> = commitment_data.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
 
 		// If our counterparty updated the channel fee in this commitment transaction, check that
 		// they can actually afford the new fee now.
@@ -3802,8 +3801,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 
 		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(htlcs_cloned.len() - commitment_data.tx.nondust_htlcs().len());
-		for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() {
+		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() {
 			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(),
@@ -3820,13 +3819,13 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				}
 				if htlc.offered {
 					if let Some(source) = source_opt.take() {
-						nondust_htlc_sources.push(source);
+						nondust_htlc_sources.push(source.clone());
 					} else {
 						panic!("Missing outbound HTLC source");
 					}
 				}
 			} else {
-				dust_htlcs.push((htlc, None, source_opt.take()));
+				dust_htlcs.push((htlc, None, source_opt.take().cloned()));
 			}
 			debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
 		}
@@ -9032,10 +9031,10 @@ impl<SP: Deref> FundedChannel<SP> where
 
 		let mut updates = Vec::with_capacity(self.pending_funding.len() + 1);
 		for funding in core::iter::once(&self.funding).chain(self.pending_funding.iter()) {
-			let (mut htlcs_ref, counterparty_commitment_tx) =
+			let (htlcs_ref, counterparty_commitment_tx) =
 				self.build_commitment_no_state_update(funding, logger);
 			let htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)> =
-				htlcs_ref.drain(..).map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect();
+				htlcs_ref.into_iter().map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect();
 
 			if self.pending_funding.is_empty() {
 				// Soon, we will switch this to `LatestCounterpartyCommitmentTX`,

From 671c4b0a0462c33401c3a9051b60300162408003 Mon Sep 17 00:00:00 2001
From: Leo Nash <hello@leonash.net>
Date: Thu, 3 Apr 2025 19:17:02 +0000
Subject: [PATCH 4/4] Require all HTLC success states to hold their
 corresponding preimage

This allows us to DRY the code that calculates the
`value_to_self_msat_offset` in `ChannelContext::build_commitment_stats`.

HTLC success states have held their corresponding preimage since
0.0.105, and the release notes of 0.1 already require users running
0.0.123 and earlier to resolve their HTLCs before upgrading to 0.1. So
this change is fully compatible with existing upgrade paths to
the yet-to-be-shipped 0.2 release.
---
 lightning/src/ln/channel.rs | 129 +++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 61 deletions(-)

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 41298ce0c3f..4b60b83aaf1 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -367,7 +367,7 @@ impl OutboundHTLCState {
 			OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage))
 			| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage))
 			| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => {
-				preimage.as_ref().copied()
+				Some(*preimage)
 			},
 			_ => None,
 		}
@@ -377,20 +377,12 @@ impl OutboundHTLCState {
 #[derive(Clone)]
 #[cfg_attr(test, derive(Debug, PartialEq))]
 enum OutboundHTLCOutcome {
-	/// LDK version 0.0.105+ will always fill in the preimage here.
-	Success(Option<PaymentPreimage>),
+	/// We started always filling in the preimages here in 0.0.105, and the requirement
+	/// that the preimages always be filled in was added in 0.2.
+	Success(PaymentPreimage),
 	Failure(HTLCFailReason),
 }
 
-impl From<Option<HTLCFailReason>> for OutboundHTLCOutcome {
-	fn from(o: Option<HTLCFailReason>) -> Self {
-		match o {
-			None => OutboundHTLCOutcome::Success(None),
-			Some(r) => OutboundHTLCOutcome::Failure(r)
-		}
-	}
-}
-
 impl<'a> Into<Option<&'a HTLCFailReason>> for &'a OutboundHTLCOutcome {
 	fn into(self) -> Option<&'a HTLCFailReason> {
 		match self {
@@ -3878,7 +3870,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				}
 				remote_htlc_total_msat += htlc.amount_msat;
 			} else {
-				if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_preimage)) = htlc.state {
+				if htlc.state.preimage().is_some() {
 					value_to_self_msat_offset += htlc.amount_msat as i64;
 				}
 			}
@@ -3891,10 +3883,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				}
 				local_htlc_total_msat += htlc.amount_msat;
 			} else {
-				if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) |
-					OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) |
-					OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_))
-				= htlc.state {
+				if htlc.state.preimage().is_some() {
 					value_to_self_msat_offset -= htlc.amount_msat as i64;
 				}
 			}
@@ -5801,20 +5790,15 @@ impl<SP: Deref> FundedChannel<SP> where
 
 	/// Marks an outbound HTLC which we have received update_fail/fulfill/malformed
 	#[inline]
-	fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<PaymentPreimage>, fail_reason: Option<HTLCFailReason>) -> Result<&OutboundHTLCOutput, ChannelError> {
-		assert!(!(check_preimage.is_some() && fail_reason.is_some()), "cannot fail while we have a preimage");
+	fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, outcome: OutboundHTLCOutcome) -> Result<&OutboundHTLCOutput, ChannelError> {
 		for htlc in self.context.pending_outbound_htlcs.iter_mut() {
 			if htlc.htlc_id == htlc_id {
-				let outcome = match check_preimage {
-					None => fail_reason.into(),
-					Some(payment_preimage) => {
-						let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
-						if payment_hash != htlc.payment_hash {
-							return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id)));
-						}
-						OutboundHTLCOutcome::Success(Some(payment_preimage))
+				if let OutboundHTLCOutcome::Success(ref payment_preimage) = outcome {
+					let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
+					if payment_hash != htlc.payment_hash {
+						return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id)));
 					}
-				};
+				}
 				match htlc.state {
 					OutboundHTLCState::LocalAnnounced(_) =>
 						return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))),
@@ -5841,7 +5825,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			return Err(ChannelError::close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned()));
 		}
 
-		self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat))
+		self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Success(msg.payment_preimage)).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat))
 	}
 
 	pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> {
@@ -5855,7 +5839,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			return Err(ChannelError::close("Peer sent update_fail_htlc when we needed a channel_reestablish".to_owned()));
 		}
 
-		self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?;
+		self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Failure(fail_reason))?;
 		Ok(())
 	}
 
@@ -5870,7 +5854,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			return Err(ChannelError::close("Peer sent update_fail_malformed_htlc when we needed a channel_reestablish".to_owned()));
 		}
 
-		self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?;
+		self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Failure(fail_reason))?;
 		Ok(())
 	}
 
@@ -6016,10 +6000,10 @@ impl<SP: Deref> FundedChannel<SP> where
 			if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) = &mut htlc.state {
 				log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
 					&htlc.payment_hash, &self.context.channel_id);
-				// Grab the preimage, if it exists, instead of cloning
-				let mut reason = OutboundHTLCOutcome::Success(None);
+				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 				mem::swap(outcome, &mut reason);
-				if let OutboundHTLCOutcome::Success(Some(preimage)) = reason {
+				if let OutboundHTLCOutcome::Success(preimage) = reason {
 					// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
 					// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
 					// have a `Success(None)` reason. In this case we could forget some HTLC
@@ -6437,8 +6421,8 @@ impl<SP: Deref> FundedChannel<SP> where
 				}
 				if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 					log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
-					// Grab the preimage, if it exists, instead of cloning
-					let mut reason = OutboundHTLCOutcome::Success(None);
+					// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+					let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 					mem::swap(outcome, &mut reason);
 					htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 					require_commitment = true;
@@ -9013,8 +8997,8 @@ impl<SP: Deref> FundedChannel<SP> where
 		for htlc in self.context.pending_outbound_htlcs.iter_mut() {
 			if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
 				log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
-				// Grab the preimage, if it exists, instead of cloning
-				let mut reason = OutboundHTLCOutcome::Success(None);
+				// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
+				let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]));
 				mem::swap(outcome, &mut reason);
 				htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
 			}
@@ -10639,7 +10623,9 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 			}
 		}
 
-		let mut preimages: Vec<&Option<PaymentPreimage>> = vec![];
+		// The elements of this vector will always be `Some` starting in 0.2,
+		// but we still serialize the option to maintain backwards compatibility
+		let mut preimages: Vec<Option<&PaymentPreimage>> = vec![];
 		let mut pending_outbound_skimmed_fees: Vec<Option<u64>> = Vec::new();
 		let mut pending_outbound_blinding_points: Vec<Option<PublicKey>> = Vec::new();
 
@@ -10666,7 +10652,7 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 				&OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => {
 					3u8.write(writer)?;
 					if let OutboundHTLCOutcome::Success(preimage) = outcome {
-						preimages.push(preimage);
+						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
 					reason.write(writer)?;
@@ -10674,7 +10660,7 @@ impl<SP: Deref> Writeable for FundedChannel<SP> where SP::Target: SignerProvider
 				&OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => {
 					4u8.write(writer)?;
 					if let OutboundHTLCOutcome::Success(preimage) = outcome {
-						preimages.push(preimage);
+						preimages.push(Some(preimage));
 					}
 					let reason: Option<&HTLCFailReason> = outcome.into();
 					reason.write(writer)?;
@@ -11013,15 +10999,30 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 					1 => OutboundHTLCState::Committed,
 					2 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::RemoteRemoved(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::RemoteRemoved(outcome)
 					},
 					3 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::AwaitingRemoteRevokeToRemove(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::AwaitingRemoteRevokeToRemove(outcome)
 					},
 					4 => {
 						let option: Option<HTLCFailReason> = Readable::read(reader)?;
-						OutboundHTLCState::AwaitingRemovedRemoteRevoke(option.into())
+						let outcome = match option {
+							Some(r) => OutboundHTLCOutcome::Failure(r),
+							// Initialize this variant with a dummy preimage, the actual preimage will be filled in further down
+							None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])),
+						};
+						OutboundHTLCState::AwaitingRemovedRemoteRevoke(outcome)
 					},
 					_ => return Err(DecodeError::InvalidValue),
 				},
@@ -11168,7 +11169,9 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 		// only, so we default to that if none was written.
 		let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
 		let mut channel_creation_height = 0u32;
-		let mut preimages_opt: Option<Vec<Option<PaymentPreimage>>> = None;
+		// Starting in 0.2, all the elements in this vector will be `Some`, but they are still
+		// serialized as options to maintain backwards compatibility
+		let mut preimages: Vec<Option<PaymentPreimage>> = Vec::new();
 
 		// If we read an old Channel, for simplicity we just treat it as "we never sent an
 		// AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine.
@@ -11222,7 +11225,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 			(10, monitor_pending_update_adds, option), // Added in 0.0.122
 			(11, monitor_pending_finalized_fulfills, optional_vec),
 			(13, channel_creation_height, required),
-			(15, preimages_opt, optional_vec),
+			(15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2
 			(17, announcement_sigs_state, required),
 			(19, latest_inbound_scid_alias, option),
 			(21, outbound_scid_alias, required),
@@ -11250,23 +11253,27 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel
 
 		let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
 
-		if let Some(preimages) = preimages_opt {
-			let mut iter = preimages.into_iter();
-			for htlc in pending_outbound_htlcs.iter_mut() {
-				match &htlc.state {
-					OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => {
-						htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
-					}
-					OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => {
-						htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
-					}
-					_ => {}
+		let mut iter = preimages.into_iter();
+		for htlc in pending_outbound_htlcs.iter_mut() {
+			match &mut htlc.state {
+				OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(ref mut preimage)) => {
+					// This variant was initialized like this further above
+					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));
+					// Flatten and unwrap the preimage; they are always set starting in 0.2.
+					*preimage = iter.next().flatten().ok_or(DecodeError::InvalidValue)?;
 				}
+				OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(ref mut preimage)) => {
+					// This variant was initialized like this further above
+					debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32]));
+					// Flatten and unwrap the preimage; they are always set starting in 0.2.
+					*preimage = iter.next().flatten().ok_or(DecodeError::InvalidValue)?;
+				}
+				_ => {}
 			}
-			// We expect all preimages to be consumed above
-			if iter.next().is_some() {
-				return Err(DecodeError::InvalidValue);
-			}
+		}
+		// We expect all preimages to be consumed above
+		if iter.next().is_some() {
+			return Err(DecodeError::InvalidValue);
 		}
 
 		let chan_features = channel_type.unwrap();