f Jeff refactor

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -260,6 +260,27 @@ enum UpdateStatus {
 	DisabledStaged,
 }
 
+/// An enum indicating whether the local or remote side offered a given HTLC.
+enum HTLCInitiator {
+	LocalOffered,
+	RemoteOffered,
+}
+
+/// Used when calculating whether we or the remote can afford an additional HTLC.
+struct HTLCCandidate {
+	amount_msat: u64,
+	origin: HTLCInitiator,
+}
+
+impl HTLCCandidate {
+	fn new(amount_msat: u64, origin: HTLCInitiator) -> Self {
+		Self {
+			amount_msat,
+			origin,
+		}
+	}
+}
+
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
@@ -1718,18 +1739,25 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// number of pending HTLCs that are on track to be in our next commitment tx, plus an additional
 	// HTLC if `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs
 	// are excluded.
-	fn next_local_commit_tx_fee_msat(&self, new_htlc_amount: u64, locally_offered: bool, fee_spike_buffer_htlc: Option<()>) -> u64 {
+	fn next_local_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(self.is_outbound());
 
 		let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
 		let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
 
 		let mut addl_htlcs = 0;
 		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
-		if locally_offered && new_htlc_amount / 1000 >= real_dust_limit_timeout_sat {
-			addl_htlcs += 1;
-		} else if !locally_offered && new_htlc_amount / 1000 >= real_dust_limit_success_sat {
-			addl_htlcs += 1;
+		match htlc.origin {
+			HTLCInitiator::LocalOffered => {
+				if htlc.amount_msat / 1000 >= real_dust_limit_timeout_sat {
+					addl_htlcs += 1;
+				}
+			},
+			HTLCInitiator::RemoteOffered => {
+				if htlc.amount_msat / 1000 >= real_dust_limit_success_sat {
+					addl_htlcs += 1;
+				}
+			}
 		}
 
 		let mut included_htlcs = 0;
@@ -1783,11 +1811,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			let commitment_tx_info = CommitmentTxInfoCached {
 				fee,
 				total_pending_htlcs,
-				next_holder_htlc_id: if locally_offered { self.next_holder_htlc_id + 1 } else {
-					self.next_holder_htlc_id
+				next_holder_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_holder_htlc_id + 1,
+					HTLCInitiator::RemoteOffered => self.next_holder_htlc_id,
 				},
-				next_counterparty_htlc_id: if locally_offered { self.next_counterparty_htlc_id } else {
-					self.next_counterparty_htlc_id + 1
+				next_counterparty_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_counterparty_htlc_id,
+					HTLCInitiator::RemoteOffered => self.next_counterparty_htlc_id + 1,
 				},
 				feerate: self.feerate_per_kw,
 			};
@@ -1800,18 +1830,25 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// pending HTLCs that are on track to be in their next commitment tx, plus an additional HTLC if
 	// `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs are
 	// excluded.
-	fn next_remote_commit_tx_fee_msat(&self, new_htlc_amount: u64, locally_offered: bool, fee_spike_buffer_htlc: Option<()>) -> u64 {
+	fn next_remote_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 {
 		assert!(!self.is_outbound());
 
 		let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
 		let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
 
 		let mut addl_htlcs = 0;
 		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
-		if locally_offered && new_htlc_amount / 1000 >= real_dust_limit_success_sat {
-			addl_htlcs += 1;
-		} else if !locally_offered && new_htlc_amount / 1000 >= real_dust_limit_timeout_sat {
-			addl_htlcs += 1;
+		match htlc.origin {
+			HTLCInitiator::LocalOffered => {
+				if htlc.amount_msat / 1000 >= real_dust_limit_success_sat {
+					addl_htlcs += 1;
+				}
+			},
+			HTLCInitiator::RemoteOffered => {
+				if htlc.amount_msat / 1000 >= real_dust_limit_timeout_sat {
+					addl_htlcs += 1;
+				}
+			}
 		}
 
 		// When calculating the set of HTLCs which will be included in their next commitment_signed, all
@@ -1851,11 +1888,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			let commitment_tx_info = CommitmentTxInfoCached {
 				fee,
 				total_pending_htlcs,
-				next_holder_htlc_id: if locally_offered { self.next_holder_htlc_id + 1 } else {
-					self.next_holder_htlc_id
+				next_holder_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_holder_htlc_id + 1,
+					HTLCInitiator::RemoteOffered => self.next_holder_htlc_id,
 				},
-				next_counterparty_htlc_id: if locally_offered { self.next_counterparty_htlc_id } else {
-					self.next_counterparty_htlc_id + 1
+				next_counterparty_htlc_id: match htlc.origin {
+					HTLCInitiator::LocalOffered => self.next_counterparty_htlc_id,
+					HTLCInitiator::RemoteOffered => self.next_counterparty_htlc_id + 1,
 				},
 				feerate: self.feerate_per_kw,
 			};
@@ -1929,7 +1968,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		// Check that the remote can afford to pay for this HTLC on-chain at the current
 		// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
 		let remote_commit_tx_fee_msat = if self.is_outbound() { 0 } else {
-			self.next_remote_commit_tx_fee_msat(msg.amount_msat, false, None) // Don't include the extra fee spike buffer HTLC in calculations
+			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+			self.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
 		};
 		if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
 			return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
@@ -1950,7 +1990,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			// the extra htlc when calculating the next remote commitment transaction fee as we should
 			// still be able to afford adding this HTLC plus one more future HTLC, regardless of being
 			// sensitive to fee spikes.
-			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(msg.amount_msat, false, Some(()));
+			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
 			if pending_remote_value_msat - msg.amount_msat - chan_reserve_msat < remote_fee_cost_incl_stuck_buffer_msat {
 				// Note that if the pending_forward_status is not updated here, then it's because we're already failing
 				// the HTLC, i.e. its status is already set to failing.
@@ -1959,7 +2000,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		} else {
 			// Check that they won't violate our local required channel reserve by adding this HTLC.
-			let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(msg.amount_msat, false, None);
+			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+			let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(htlc_candidate, None);
 			if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
 				return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
 			}
@@ -3863,7 +3905,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			// Check that we won't violate the remote channel reserve by adding this HTLC.
 			let counterparty_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
 			let holder_selected_chan_reserve_msat = Channel::<ChanSigner>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
-			let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(amount_msat, true, None);
+			let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
+			let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 			if counterparty_balance_msat < holder_selected_chan_reserve_msat + counterparty_commit_tx_fee_msat {
 				return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned()));
 			}
@@ -3876,7 +3919,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		// `2 *` and extra HTLC are for the fee spike buffer.
 		let commit_tx_fee_msat = if self.is_outbound() {
-			2 * self.next_local_commit_tx_fee_msat(amount_msat, true, Some(()))
+			let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
+			2 * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(()))
 		} else { 0 };
 		if pending_value_to_self_msat - amount_msat < commit_tx_fee_msat {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", pending_value_to_self_msat, commit_tx_fee_msat)));
@@ -4648,7 +4692,7 @@ mod tests {
 	use bitcoin::hashes::hex::FromHex;
 	use hex;
 	use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash};
-	use ln::channel::{Channel,ChannelKeys,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,TxCreationKeys};
+	use ln::channel::{Channel,ChannelKeys,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
 	use ln::channel::MAX_FUNDING_SATOSHIS;
 	use ln::features::InitFeatures;
 	use ln::msgs::{OptionalField, DataLossProtect, DecodeError};
@@ -4791,15 +4835,17 @@ mod tests {
 
 		// Make sure when Node A calculates their local commitment transaction, none of the HTLCs pass
 		// the dust limit check.
-		let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_amount_msat, true, None);
+		let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
+		let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
 		let local_commit_fee_0_htlcs = node_a_chan.commit_tx_fee_msat(0);
 		assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);
 
 		// Finally, make sure that when Node A calculates the remote's commitment transaction fees, all
 		// of the HTLCs are seen to be above the dust limit.
 		node_a_chan.channel_transaction_parameters.is_outbound_from_holder = false;
 		let remote_commit_fee_3_htlcs = node_a_chan.commit_tx_fee_msat(3);
-		let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_amount_msat, true, None);
+		let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
+		let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
 	}
 
@@ -4825,24 +4871,28 @@ mod tests {
 		// If HTLC_SUCCESS_TX_WEIGHT and HTLC_TIMEOUT_TX_WEIGHT were swapped: then this HTLC would be
 		// counted as dust when it shouldn't be.
 		let htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis + 1) * 1000;
-		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_amt_above_timeout, true, None);
+		let htlc_candidate = HTLCCandidate::new(htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
+		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
 
 		// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
 		let dust_htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis - 1) * 1000;
-		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(dust_htlc_amt_below_success, false, None);
+		let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_below_success, HTLCInitiator::RemoteOffered);
+		let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
 
 		chan.channel_transaction_parameters.is_outbound_from_holder = false;
 
 		// If swapped: this HTLC would be counted as non-dust when it shouldn't be.
 		let dust_htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis + 1) * 1000;
-		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(dust_htlc_amt_above_timeout, true, None);
+		let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
+		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
 
 		// If swapped: this HTLC would be counted as dust when it shouldn't be.
 		let htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis - 1) * 1000;
-		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_amt_below_success, false, None);
+		let htlc_candidate = HTLCCandidate::new(htlc_amt_below_success, HTLCInitiator::RemoteOffered);
+		let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
 	}