Merge pull request #3744 from carlaKC/3541-followups

Use LocalHTLCFailureReason in Onion Processing

Author: joostjager

lightning/src/ln/channel.rs

@@ -7879,17 +7879,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(())
 	}
@@ -7899,7 +7897,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() {
@@ -7914,13 +7912,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);
@@ -7931,8 +7929,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
@@ -7946,8 +7943,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)
 			}
 		}
 
@@ -7985,7 +7981,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

@@ -4352,25 +4352,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
@@ -4380,16 +4377,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)?;
 
@@ -4420,12 +4414,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>| {
@@ -4440,8 +4433,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);
 				}
 			}
 		}
@@ -4453,7 +4445,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 {
@@ -4477,7 +4469,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,
 		);
 		// If `msg.blinding_point` is set, we must always fail with malformed.
 		if msg.blinding_point.is_some() {
@@ -5820,9 +5812,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 failure_type = get_htlc_failure_type(outgoing_scid_opt, update_add_htlc.payment_hash);
@@ -5835,11 +5827,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 failure_type = get_htlc_failure_type(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(())