Merge pull request #3946 from joostjager/simplify-send-htlc

Simplify send htlc

Author: tnull

lightning/src/ln/channel.rs

@@ -7245,12 +7245,12 @@ where
 							fee_estimator,
 							logger,
 						) {
-							Ok(update_add_msg_opt) => {
-								// `send_htlc` only returns `Ok(None)`, when an update goes into
+							Ok(can_add_htlc) => {
+								// `send_htlc` only returns `Ok(false)`, when an update goes into
 								// the holding cell, but since we're currently freeing it, we should
-								// always expect to see the `update_add` go out.
+								// always expect to see the htlc added.
 								debug_assert!(
-									update_add_msg_opt.is_some(),
+									can_add_htlc,
 									"Must generate new update if we're freeing the holding cell"
 								);
 								update_add_count += 1;
@@ -10581,34 +10581,43 @@ where
 		))
 	}
 
-	// Send stuff to our remote peers:
-
 	/// Queues up an outbound HTLC to send by placing it in the holding cell. You should call
 	/// [`Self::maybe_free_holding_cell_htlcs`] in order to actually generate and send the
 	/// commitment update.
-	#[rustfmt::skip]
 	pub fn queue_add_htlc<F: Deref, L: Deref>(
-		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
-		onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option<u64>,
-		blinding_point: Option<PublicKey>, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32,
+		source: HTLCSource, onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option<u64>,
+		blinding_point: Option<PublicKey>, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
 	) -> Result<(), (LocalHTLCFailureReason, String)>
-	where F::Target: FeeEstimator, L::Target: Logger
+	where
+		F::Target: FeeEstimator,
+		L::Target: Logger,
 	{
-		self
-			.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, true,
-				skimmed_fee_msat, blinding_point, fee_estimator, logger)
-			.map(|msg_opt| assert!(msg_opt.is_none(), "We forced holding cell?"))
-			.map_err(|err| {
-				debug_assert!(err.0.is_temporary(), "Queuing HTLC should return temporary error");
-				err
-			})
+		self.send_htlc(
+			amount_msat,
+			payment_hash,
+			cltv_expiry,
+			source,
+			onion_routing_packet,
+			true,
+			skimmed_fee_msat,
+			blinding_point,
+			fee_estimator,
+			logger,
+		)
+		.map(|can_add_htlc| assert!(!can_add_htlc, "We forced holding cell?"))
+		.map_err(|err| {
+			debug_assert!(err.0.is_temporary(), "Queuing HTLC should return temporary error");
+			err
+		})
 	}
 
 	/// Adds a pending outbound HTLC to this channel, note that you probably want
 	/// [`Self::send_htlc_and_commit`] instead cause you'll want both messages at once.
 	///
-	/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
-	/// the wire:
+	/// This returns a boolean indicating whether we are in a state where we can add HTLCs on the wire.
+	/// Reasons we may not be able to add HTLCs on the wire include:
+	///
 	/// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we
 	///   wouldn't be able to determine what they actually ACK'ed if we have two sets of updates
 	///   awaiting ACK.
@@ -10620,18 +10629,19 @@ where
 	/// on this [`FundedChannel`] if `force_holding_cell` is false.
 	///
 	/// `Err`'s will always be temporary channel failures.
-	#[rustfmt::skip]
 	fn send_htlc<F: Deref, L: Deref>(
-		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
-		onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool,
+		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32,
+		source: HTLCSource, onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool,
 		skimmed_fee_msat: Option<u64>, blinding_point: Option<PublicKey>,
-		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
-	) -> Result<Option<msgs::UpdateAddHTLC>, (LocalHTLCFailureReason, String)>
-	where F::Target: FeeEstimator, L::Target: Logger
+		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+	) -> Result<bool, (LocalHTLCFailureReason, String)>
+	where
+		F::Target: FeeEstimator,
+		L::Target: Logger,
 	{
-		if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) ||
-			self.context.channel_state.is_local_shutdown_sent() ||
-			self.context.channel_state.is_remote_shutdown_sent()
+		if !matches!(self.context.channel_state, ChannelState::ChannelReady(_))
+			|| self.context.channel_state.is_local_shutdown_sent()
+			|| self.context.channel_state.is_remote_shutdown_sent()
 		{
 			return Err((LocalHTLCFailureReason::ChannelNotReady,
 				"Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
@@ -10643,13 +10653,23 @@ where
 
 		let available_balances = self.get_available_balances(fee_estimator);
 		if amount_msat < available_balances.next_outbound_htlc_minimum_msat {
-			return Err((LocalHTLCFailureReason::HTLCMinimum, format!("Cannot send less than our next-HTLC minimum - {} msat",
-				available_balances.next_outbound_htlc_minimum_msat)));
+			return Err((
+				LocalHTLCFailureReason::HTLCMinimum,
+				format!(
+					"Cannot send less than our next-HTLC minimum - {} msat",
+					available_balances.next_outbound_htlc_minimum_msat
+				),
+			));
 		}
 
 		if amount_msat > available_balances.next_outbound_htlc_limit_msat {
-			return Err((LocalHTLCFailureReason::HTLCMaximum, format!("Cannot send more than our next-HTLC maximum - {} msat",
-				available_balances.next_outbound_htlc_limit_msat)));
+			return Err((
+				LocalHTLCFailureReason::HTLCMaximum,
+				format!(
+					"Cannot send more than our next-HTLC maximum - {} msat",
+					available_balances.next_outbound_htlc_limit_msat
+				),
+			));
 		}
 
 		if self.context.channel_state.is_peer_disconnected() {
@@ -10659,16 +10679,26 @@ where
 			// disconnected during the time the previous hop was doing the commitment dance we may
 			// end up getting here after the forwarding delay. In any case, returning an
 			// IgnoreError will get ChannelManager to do the right thing and fail backwards now.
-			return Err((LocalHTLCFailureReason::PeerOffline,
-				"Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
+			return Err((
+				LocalHTLCFailureReason::PeerOffline,
+				"Cannot send an HTLC while disconnected from channel counterparty".to_owned(),
+			));
 		}
 
 		let need_holding_cell = !self.context.channel_state.can_generate_new_commitment();
-		log_debug!(logger, "Pushing new outbound HTLC with hash {} for {} msat {}",
-			payment_hash, amount_msat,
-			if force_holding_cell { "into holding cell" }
-			else if need_holding_cell { "into holding cell as we're awaiting an RAA or monitor" }
-			else { "to peer" });
+		log_debug!(
+			logger,
+			"Pushing new outbound HTLC with hash {} for {} msat {}",
+			payment_hash,
+			amount_msat,
+			if force_holding_cell {
+				"into holding cell"
+			} else if need_holding_cell {
+				"into holding cell as we're awaiting an RAA or monitor"
+			} else {
+				"to peer"
+			}
+		);
 
 		if need_holding_cell {
 			force_holding_cell = true;
@@ -10685,7 +10715,7 @@ where
 				skimmed_fee_msat,
 				blinding_point,
 			});
-			return Ok(None);
+			return Ok(false);
 		}
 
 		// Record the approximate time when the HTLC is sent to the peer. This timestamp is later used to calculate the
@@ -10706,20 +10736,9 @@ where
 			skimmed_fee_msat,
 			send_timestamp,
 		});
-
-		let res = msgs::UpdateAddHTLC {
-			channel_id: self.context.channel_id,
-			htlc_id: self.context.next_holder_htlc_id,
-			amount_msat,
-			payment_hash,
-			cltv_expiry,
-			onion_routing_packet,
-			skimmed_fee_msat,
-			blinding_point,
-		};
 		self.context.next_holder_htlc_id += 1;
 
-		Ok(Some(res))
+		Ok(true)
 	}
 
 	#[rustfmt::skip]
@@ -10956,24 +10975,35 @@ where
 	///
 	/// Shorthand for calling [`Self::send_htlc`] followed by a commitment update, see docs on
 	/// [`Self::send_htlc`] and [`Self::build_commitment_no_state_update`] for more info.
-	#[rustfmt::skip]
 	pub fn send_htlc_and_commit<F: Deref, L: Deref>(
 		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32,
 		source: HTLCSource, onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option<u64>,
-		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
 	) -> Result<Option<ChannelMonitorUpdate>, ChannelError>
-	where F::Target: FeeEstimator, L::Target: Logger
+	where
+		F::Target: FeeEstimator,
+		L::Target: Logger,
 	{
-		let send_res = self.send_htlc(amount_msat, payment_hash, cltv_expiry, source,
-			onion_routing_packet, false, skimmed_fee_msat, None, fee_estimator, logger);
+		let send_res = self.send_htlc(
+			amount_msat,
+			payment_hash,
+			cltv_expiry,
+			source,
+			onion_routing_packet,
+			false,
+			skimmed_fee_msat,
+			None,
+			fee_estimator,
+			logger,
+		);
 		// All [`LocalHTLCFailureReason`] errors are temporary, so they are [`ChannelError::Ignore`].
-		match send_res.map_err(|(_, msg)| ChannelError::Ignore(msg))? {
-			Some(_) => {
-				let monitor_update = self.build_commitment_no_status_check(logger);
-				self.monitor_updating_paused(false, true, false, Vec::new(), Vec::new(), Vec::new());
-				Ok(self.push_ret_blockable_mon_update(monitor_update))
-			},
-			None => Ok(None)
+		let can_add_htlc = send_res.map_err(|(_, msg)| ChannelError::Ignore(msg))?;
+		if can_add_htlc {
+			let monitor_update = self.build_commitment_no_status_check(logger);
+			self.monitor_updating_paused(false, true, false, Vec::new(), Vec::new(), Vec::new());
+			Ok(self.push_ret_blockable_mon_update(monitor_update))
+		} else {
+			Ok(None)
 		}
 	}