ln: surface LocalHTLCFailureReason in queue_add_htlc

Author: carlaKC

lightning/src/ln/channel.rs

@@ -5852,22 +5852,14 @@ impl<SP: Deref> FundedChannel<SP> where
 								);
 								update_add_count += 1;
 							},
-							Err(e) => {
-								match e {
-									ChannelError::Ignore(ref msg) => {
-										log_info!(logger, "Failed to send HTLC with payment_hash {} due to {} in channel {}", &payment_hash, msg, &self.context.channel_id());
-										// If we fail to send here, then this HTLC should
-										// be failed backwards. Failing to send here
-										// indicates that this HTLC may keep being put back
-										// into the holding cell without ever being
-										// successfully forwarded/failed/fulfilled, causing
-										// our counterparty to eventually close on us.
-										htlcs_to_fail.push((source.clone(), *payment_hash));
-									},
-									_ => {
-										panic!("Got a non-IgnoreError action trying to send holding cell HTLC");
-									},
-								}
+							Err((_, msg)) => {
+								log_info!(logger, "Failed to send HTLC with payment_hash {} due to {} in channel {}", &payment_hash, msg, &self.context.channel_id());
+								// If we fail to send here, then this HTLC should be failed
+								// backwards. Failing to send here indicates that this HTLC may
+								// keep being put back into the holding cell without ever being
+								// successfully forwarded/failed/fulfilled, causing our
+								// counterparty to eventually close on us.
+								htlcs_to_fail.push((source.clone(), *payment_hash));
 							}
 						}
 						None
@@ -8524,22 +8516,19 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// 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.
-	///
-	/// `Err`s will only be [`ChannelError::Ignore`].
 	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
-	) -> Result<(), ChannelError>
+	) -> Result<(), (LocalHTLCFailureReason, String)>
 	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| {
-				if let ChannelError::Ignore(_) = err { /* fine */ }
-				else { debug_assert!(false, "Queueing cannot trigger channel failure"); }
+				debug_assert!(err.0.is_temporary(), "Queuing HTLC should return temporary error");
 				err
 			})
 	}
@@ -8559,38 +8548,40 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// You MUST call [`Self::send_commitment_no_state_update`] prior to calling any other methods
 	/// on this [`FundedChannel`] if `force_holding_cell` is false.
 	///
-	/// `Err`s will only be [`ChannelError::Ignore`].
+	/// `Err`'s will always be temporary channel failures.
 	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,
 		skimmed_fee_msat: Option<u64>, blinding_point: Option<PublicKey>,
 		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
-	) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError>
+	) -> Result<Option<msgs::UpdateAddHTLC>, (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()
 		{
-			return Err(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
+			return Err((LocalHTLCFailureReason::ChannelNotReady,
+				"Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
 		}
 		let channel_total_msat = self.funding.get_value_satoshis() * 1000;
 		if amount_msat > channel_total_msat {
-			return Err(ChannelError::Ignore(format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
+			return Err((LocalHTLCFailureReason::AmountExceedsCapacity,
+				format!("Cannot send amount {}, because it is more than the total value of the channel {}", amount_msat, channel_total_msat)));
 		}
 
 		if amount_msat == 0 {
-			return Err(ChannelError::Ignore("Cannot send 0-msat HTLC".to_owned()));
+			return Err((LocalHTLCFailureReason::ZeroAmount, "Cannot send 0-msat HTLC".to_owned()));
 		}
 
 		let available_balances = self.context.get_available_balances(&self.funding, fee_estimator);
 		if amount_msat < available_balances.next_outbound_htlc_minimum_msat {
-			return Err(ChannelError::Ignore(format!("Cannot send less than our next-HTLC minimum - {} msat",
+			return Err((LocalHTLCFailureReason::LiquidityMinimum, 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(ChannelError::Ignore(format!("Cannot send more than our next-HTLC maximum - {} msat",
+			return Err((LocalHTLCFailureReason::LiquidityMaximum, format!("Cannot send more than our next-HTLC maximum - {} msat",
 				available_balances.next_outbound_htlc_limit_msat)));
 		}
 
@@ -8601,7 +8592,8 @@ impl<SP: Deref> FundedChannel<SP> 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(ChannelError::Ignore("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();
@@ -8821,8 +8813,8 @@ impl<SP: Deref> FundedChannel<SP> where
 	{
 		let send_res = self.send_htlc(amount_msat, payment_hash, cltv_expiry, source,
 			onion_routing_packet, false, skimmed_fee_msat, None, fee_estimator, logger);
-		if let Err(e) = &send_res { if let ChannelError::Ignore(_) = e {} else { debug_assert!(false, "Sending cannot trigger channel failure"); } }
-		match send_res? {
+		// 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());

lightning/src/ln/channelmanager.rs

@@ -6090,22 +6090,17 @@ where
 								};
 								log_trace!(logger, "Forwarding HTLC from SCID {} with payment_hash {} and next hop SCID {} over {} channel {} with corresponding peer {}",
 									prev_short_channel_id, &payment_hash, short_chan_id, channel_description, optimal_channel.context.channel_id(), &counterparty_node_id);
-								if let Err(e) = optimal_channel.queue_add_htlc(outgoing_amt_msat,
+								if let Err((reason, msg)) = optimal_channel.queue_add_htlc(outgoing_amt_msat,
 										payment_hash, outgoing_cltv_value, htlc_source.clone(),
 										onion_packet.clone(), skimmed_fee_msat, next_blinding_point, &self.fee_estimator,
 										&&logger)
 								{
-									if let ChannelError::Ignore(msg) = e {
-										log_trace!(logger, "Failed to forward HTLC with payment_hash {} to peer {}: {}", &payment_hash, &counterparty_node_id, msg);
-									} else {
-										panic!("Stated return value requirements in send_htlc() were not met");
-									}
+									log_trace!(logger, "Failed to forward HTLC with payment_hash {} to peer {}: {}", &payment_hash, &counterparty_node_id, msg);
 
 									if let Some(chan) = peer_state.channel_by_id
 										.get_mut(&forward_chan_id)
 										.and_then(Channel::as_funded_mut)
 									{
-										let reason = LocalHTLCFailureReason::TemporaryChannelFailure;
 										let data = self.get_htlc_inbound_temp_fail_data(reason);
 										failed_forwards.push((htlc_source, payment_hash,
 											HTLCFailReason::reason(reason, data),

lightning/src/ln/onion_utils.rs

@@ -1497,7 +1497,19 @@ pub enum LocalHTLCFailureReason {
 	DroppedPending,
 	/// The HTLC was failed back because its channel is closed and it has timed out on chain.
 	ChannelClosed,
-	/// UnknownFailureCode represents BOLT04 failure codes that we are not familiar with. We will
+	/// The HTLC was failed because its amount is greater than the capacity of the channel.
+	AmountExceedsCapacity,
+	/// The HTLC was failed because zero amount HTLCs are not allowed.
+	ZeroAmount,
+	/// The HTLC was failed because its amount is less than the smallest HTLC that the channel
+	/// can currently accept.
+	LiquidityMinimum,
+	/// The HTLC was failed because its amount is more than then largest HTLC that the channel
+	/// can currently accept.
+	LiquidityMaximum,
+	/// The HTLC was failed because our remote peer is offline.
+	PeerOffline,
+	/// UnknownFailureCode  represents BOLT04 failure codes that we are not familiar with. We will
 	/// encounter this if:
 	/// - A peer sends us a new failure code that LDK has not yet been upgraded to understand.
 	/// - We read a deprecated failure code from disk that LDK no longer uses.
@@ -1523,7 +1535,12 @@ impl LocalHTLCFailureReason {
 			| Self::DustLimitHolder
 			| Self::DustLimitCounterparty
 			| Self::FeeSpikeBuffer
-			| Self::ChannelNotReady => UPDATE | 7,
+			| Self::ChannelNotReady
+			| Self::AmountExceedsCapacity
+			| Self::ZeroAmount
+			| Self::LiquidityMinimum
+			| Self::LiquidityMaximum
+			| Self::PeerOffline => UPDATE | 7,
 			Self::PermanentChannelFailure | Self::ChannelClosed | Self::DroppedPending => PERM | 8,
 			Self::RequiredChannelFeature => PERM | 9,
 			Self::UnknownNextPeer
@@ -1650,7 +1667,12 @@ impl_writeable_tlv_based_enum!(LocalHTLCFailureReason,
 	(72, ChannelClosed) => {},
 	(74, UnknownFailureCode) => {
 		(0, code, required),
-	}
+	},
+	(76, AmountExceedsCapacity) => {},
+	(78, ZeroAmount) => {},
+	(80, LiquidityMinimum) => {},
+	(82, LiquidityMaximum) => {},
+	(84, PeerOffline) => {},
 );
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
@@ -1731,7 +1753,12 @@ impl HTLCFailReason {
 			| LocalHTLCFailureReason::DustLimitHolder
 			| LocalHTLCFailureReason::DustLimitCounterparty
 			| LocalHTLCFailureReason::FeeSpikeBuffer
-			| LocalHTLCFailureReason::ChannelNotReady => {
+			| LocalHTLCFailureReason::ChannelNotReady
+			| LocalHTLCFailureReason::AmountExceedsCapacity
+			| LocalHTLCFailureReason::ZeroAmount
+			| LocalHTLCFailureReason::LiquidityMinimum
+			| LocalHTLCFailureReason::LiquidityMaximum
+			| LocalHTLCFailureReason::PeerOffline => {
 				debug_assert_eq!(
 					data.len() - 2,
 					u16::from_be_bytes(data[0..2].try_into().unwrap()) as usize