ln/refactor: remove err_str where LocalHTLCFailureReason sufficient

In some places where we have LocalHTLCFailureReason, the string we're
passing around is redundant.

This isn't the case in all uses of LocalHTLCFailureReason, for example
InboundHTLCError provides additional information about payload and
blinding related errors that isn't captured in LocalHTLCFailureReason.
In these cases we keep the error string, tolerating some duplication.

Author: carlaKC

lightning/src/ln/channel.rs

@@ -7838,17 +7838,15 @@ impl<SP: Deref> FundedChannel<SP> where
 
 	fn internal_htlc_satisfies_config(
 		&self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, config: &ChannelConfig,
-	) -> Result<(), (&'static str, LocalHTLCFailureReason)> {
+	) -> Result<(), LocalHTLCFailureReason> {
 		let fee = amt_to_forward.checked_mul(config.forwarding_fee_proportional_millionths as u64)
 			.and_then(|prop_fee| (prop_fee / 1000000).checked_add(config.forwarding_fee_base_msat as u64));
 		if fee.is_none() || htlc.amount_msat < fee.unwrap() ||
 			(htlc.amount_msat - fee.unwrap()) < amt_to_forward {
-			return Err(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones",
-				LocalHTLCFailureReason::FeeInsufficient));
+			return Err(LocalHTLCFailureReason::FeeInsufficient);
 		}
 		if (htlc.cltv_expiry as u64) < outgoing_cltv_value as u64 + config.cltv_expiry_delta as u64 {
-			return Err(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
-				LocalHTLCFailureReason::IncorrectCLTVExpiry));
+			return Err(LocalHTLCFailureReason::IncorrectCLTVExpiry);
 		}
 		Ok(())
 	}
@@ -7858,7 +7856,7 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// unsuccessful, falls back to the previous one if one exists.
 	pub fn htlc_satisfies_config(
 		&self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32,
-	) -> Result<(), (&'static str, LocalHTLCFailureReason)> {
+	) -> Result<(), LocalHTLCFailureReason> {
 		self.internal_htlc_satisfies_config(&htlc, amt_to_forward, outgoing_cltv_value, &self.context.config())
 			.or_else(|err| {
 				if let Some(prev_config) = self.context.prev_config() {
@@ -7873,13 +7871,13 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// this function determines whether to fail the HTLC, or forward / claim it.
 	pub fn can_accept_incoming_htlc<F: Deref, L: Deref>(
 		&self, msg: &msgs::UpdateAddHTLC, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L
-	) -> Result<(), (&'static str, LocalHTLCFailureReason)>
+	) -> Result<(), LocalHTLCFailureReason>
 	where
 		F::Target: FeeEstimator,
 		L::Target: Logger
 	{
 		if self.context.channel_state.is_local_shutdown_sent() {
-			return Err(("Shutdown was already sent", LocalHTLCFailureReason::ChannelClosed))
+			return Err(LocalHTLCFailureReason::ChannelClosed)
 		}
 
 		let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator);
@@ -7890,8 +7888,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			// Note that the total dust exposure includes both the dust HTLCs and the excess mining fees of the counterparty commitment transaction
 			log_info!(logger, "Cannot accept value that would put our total dust exposure at {} over the limit {} on counterparty commitment tx",
 				on_counterparty_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
-			return Err(("Exceeded our total dust exposure limit on counterparty commitment tx",
-				LocalHTLCFailureReason::DustLimitCounterparty))
+			return Err(LocalHTLCFailureReason::DustLimitCounterparty)
 		}
 		let htlc_success_dust_limit = if self.funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
 			0
@@ -7905,8 +7902,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			if on_holder_tx_dust_htlc_exposure_msat > max_dust_htlc_exposure_msat {
 				log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
 					on_holder_tx_dust_htlc_exposure_msat, max_dust_htlc_exposure_msat);
-				return Err(("Exceeded our dust exposure limit on holder commitment tx",
-					LocalHTLCFailureReason::DustLimitHolder))
+				return Err(LocalHTLCFailureReason::DustLimitHolder)
 			}
 		}
 
@@ -7944,7 +7940,7 @@ impl<SP: Deref> FundedChannel<SP> where
 			}
 			if pending_remote_value_msat.saturating_sub(self.funding.holder_selected_channel_reserve_satoshis * 1000).saturating_sub(anchor_outputs_value_msat) < remote_fee_cost_incl_stuck_buffer_msat {
 				log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.context.channel_id());
-				return Err(("Fee spike buffer violation", LocalHTLCFailureReason::FeeSpikeBuffer));
+				return Err(LocalHTLCFailureReason::FeeSpikeBuffer);
 			}
 		}
 

lightning/src/ln/channelmanager.rs

@@ -4343,25 +4343,22 @@ where
 
 	fn can_forward_htlc_to_outgoing_channel(
 		&self, chan: &mut FundedChannel<SP>, msg: &msgs::UpdateAddHTLC, next_packet: &NextPacketDetails
-	) -> Result<(), (&'static str, LocalHTLCFailureReason)> {
+	) -> Result<(), LocalHTLCFailureReason> {
 		if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 			// Note that the behavior here should be identical to the above block - we
 			// should NOT reveal the existence or non-existence of a private channel if
 			// we don't allow forwards outbound over them.
-			return Err(("Refusing to forward to a private channel based on our config.",
-				LocalHTLCFailureReason::PrivateChannelForward));
+			return Err(LocalHTLCFailureReason::PrivateChannelForward);
 		}
 		if let HopConnector::ShortChannelId(outgoing_scid) = next_packet.outgoing_connector {
 			if chan.funding.get_channel_type().supports_scid_privacy() && outgoing_scid != chan.context.outbound_scid_alias() {
 				// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
 				// "refuse to forward unless the SCID alias was used", so we pretend
 				// we don't have the channel here.
-				return Err(("Refusing to forward over real channel SCID as our counterparty requested.",
-					LocalHTLCFailureReason::RealSCIDForward));
+				return Err(LocalHTLCFailureReason::RealSCIDForward);
 			}
 		} else {
-			return Err(("Cannot forward by Node ID without SCID.",
-				LocalHTLCFailureReason::InvalidTrampolineForward));
+			return Err(LocalHTLCFailureReason::InvalidTrampolineForward);
 		}
 
 		// Note that we could technically not return an error yet here and just hope
@@ -4371,16 +4368,13 @@ where
 		// on a small/per-node/per-channel scale.
 		if !chan.context.is_live() {
 			if !chan.context.is_enabled() {
-				return Err(("Forwarding channel has been disconnected for some time.",
-					LocalHTLCFailureReason::ChannelDisabled));
+				return Err(LocalHTLCFailureReason::ChannelDisabled);
 			} else {
-				return Err(("Forwarding channel is not in a ready state.",
-					LocalHTLCFailureReason::ChannelNotReady));
+				return Err(LocalHTLCFailureReason::ChannelNotReady);
 			}
 		}
 		if next_packet.outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() {
-			return Err(("HTLC amount was below the htlc_minimum_msat",
-				LocalHTLCFailureReason::AmountBelowMinimum));
+			return Err(LocalHTLCFailureReason::AmountBelowMinimum);
 		}
 		chan.htlc_satisfies_config(msg, next_packet.outgoing_amt_msat, next_packet.outgoing_cltv_value)?;
 
@@ -4411,12 +4405,11 @@ where
 
 	fn can_forward_htlc(
 		&self, msg: &msgs::UpdateAddHTLC, next_packet_details: &NextPacketDetails
-	) -> Result<(), (&'static str, LocalHTLCFailureReason)> {
+	) -> Result<(), LocalHTLCFailureReason> {
 		let outgoing_scid = match next_packet_details.outgoing_connector {
 			HopConnector::ShortChannelId(scid) => scid,
 			HopConnector::Trampoline(_) => {
-				return Err(("Cannot forward by Node ID without SCID.",
-				LocalHTLCFailureReason::InvalidTrampolineForward));
+				return Err(LocalHTLCFailureReason::InvalidTrampolineForward);
 			}
 		};
 		match self.do_funded_channel_callback(outgoing_scid, |chan: &mut FundedChannel<SP>| {
@@ -4431,8 +4424,7 @@ where
 					fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, outgoing_scid, &self.chain_hash)) ||
 					fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, outgoing_scid, &self.chain_hash)
 				{} else {
-					return Err(("Don't have available channel for forwarding as requested.",
-					LocalHTLCFailureReason::UnknownNextPeer));
+					return Err(LocalHTLCFailureReason::UnknownNextPeer);
 				}
 			}
 		}
@@ -4444,7 +4436,7 @@ where
 	}
 
 	fn htlc_failure_from_update_add_err(
-		&self, msg: &msgs::UpdateAddHTLC, counterparty_node_id: &PublicKey, err_msg: &'static str,
+		&self, msg: &msgs::UpdateAddHTLC, counterparty_node_id: &PublicKey,
 		reason: LocalHTLCFailureReason, is_intro_node_blinded_forward: bool,
 		shared_secret: &[u8; 32]
 	) -> HTLCFailureMsg {
@@ -4468,7 +4460,7 @@ where
 
 		log_info!(
 			WithContext::from(&self.logger, Some(*counterparty_node_id), Some(msg.channel_id), Some(msg.payment_hash)),
-			"Failed to accept/forward incoming HTLC: {}", err_msg
+			"Failed to accept/forward incoming HTLC: {:?} - {}", reason, reason,
 		);
 		// If `msg.blinding_point` is set, we must always fail with malformed.
 		if msg.blinding_point.is_some() {
@@ -5810,9 +5802,9 @@ where
 					)
 				}) {
 					Some(Ok(_)) => {},
-					Some(Err((err, reason))) => {
+					Some(Err(reason)) => {
 						let htlc_fail = self.htlc_failure_from_update_add_err(
-							&update_add_htlc, &incoming_counterparty_node_id, err, reason,
+							&update_add_htlc, &incoming_counterparty_node_id, reason,
 							is_intro_node_blinded_forward, &shared_secret,
 						);
 						let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);
@@ -5825,11 +5817,11 @@ where
 
 				// Now process the HTLC on the outgoing channel if it's a forward.
 				if let Some(next_packet_details) = next_packet_details_opt.as_ref() {
-					if let Err((err, reason)) = self.can_forward_htlc(
+					if let Err(reason) = self.can_forward_htlc(
 						&update_add_htlc, next_packet_details
 					) {
 						let htlc_fail = self.htlc_failure_from_update_add_err(
-							&update_add_htlc, &incoming_counterparty_node_id, err, reason,
+							&update_add_htlc, &incoming_counterparty_node_id, reason,
 							is_intro_node_blinded_forward, &shared_secret,
 						);
 						let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);

lightning/src/ln/onion_payment.rs

@@ -453,11 +453,11 @@ where
 				}),
 			};
 
-			if let Err((err_msg, reason)) = check_incoming_htlc_cltv(
+			if let Err(reason) = check_incoming_htlc_cltv(
 				cur_height, outgoing_cltv_value, msg.cltv_expiry,
 			) {
 				return Err(InboundHTLCErr {
-					msg: err_msg,
+					msg: "incoming cltv check failed",
 					reason,
 					err_data: Vec::new(),
 				});
@@ -601,19 +601,18 @@ where
 
 pub(super) fn check_incoming_htlc_cltv(
 	cur_height: u32, outgoing_cltv_value: u32, cltv_expiry: u32
-) -> Result<(), (&'static str, LocalHTLCFailureReason)> {
+) -> Result<(), LocalHTLCFailureReason> {
 	if (cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
-		return Err(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
-			LocalHTLCFailureReason::IncorrectCLTVExpiry));
+		return Err(LocalHTLCFailureReason::IncorrectCLTVExpiry);
 	}
 	// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
 	// but we want to be robust wrt to counterparty packet sanitization (see
 	// HTLC_FAIL_BACK_BUFFER rationale).
 	if cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 {
-		return Err(("CLTV expiry is too close", LocalHTLCFailureReason::CLTVExpiryTooSoon));
+		return Err(LocalHTLCFailureReason::CLTVExpiryTooSoon);
 	}
 	if cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 {
-		return Err(("CLTV expiry is too far in the future", LocalHTLCFailureReason::CLTVExpiryTooFar));
+		return Err(LocalHTLCFailureReason::CLTVExpiryTooFar);
 	}
 	// If the HTLC expires ~now, don't bother trying to forward it to our
 	// counterparty. They should fail it anyway, but we don't want to bother with
@@ -624,7 +623,7 @@ pub(super) fn check_incoming_htlc_cltv(
 	// but there is no need to do that, and since we're a bit conservative with our
 	// risk threshold it just results in failing to forward payments.
 	if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
-		return Err(("Outgoing CLTV value is too soon", LocalHTLCFailureReason::OutgoingCLTVTooSoon));
+		return Err(LocalHTLCFailureReason::OutgoingCLTVTooSoon);
 	}
 
 	Ok(())

lightning/src/ln/priv_short_conf_tests.rs

@@ -449,7 +449,8 @@ fn test_inbound_scid_privacy() {
 	);
 	check_added_monitors(&nodes[1], 1);
 
-	nodes[1].logger.assert_log_regex("lightning::ln::channelmanager", regex::Regex::new(r"Refusing to forward over real channel SCID as our counterparty requested").unwrap(), 1);
+	nodes[1].logger.assert_log_regex("lightning::ln::channelmanager",
+		regex::Regex::new(r"Forward using real SCID over aliased channel rejected").unwrap(), 1);
 
 	let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);