From 27b13219127d34ef30d1e83e54847a638edc4f37 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 16 Apr 2025 11:51:47 -0400 Subject: [PATCH 1/4] 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. --- lightning/src/ln/channel.rs | 22 +++++-------- lightning/src/ln/channelmanager.rs | 40 +++++++++-------------- lightning/src/ln/onion_payment.rs | 15 ++++----- lightning/src/ln/priv_short_conf_tests.rs | 3 +- 4 files changed, 34 insertions(+), 46 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b4210a7980c..601c5c2747b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7838,17 +7838,15 @@ impl FundedChannel 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 FundedChannel 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 FundedChannel where /// this function determines whether to fail the HTLC, or forward / claim it. pub fn can_accept_incoming_htlc( &self, msg: &msgs::UpdateAddHTLC, fee_estimator: &LowerBoundedFeeEstimator, 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 FundedChannel 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 FundedChannel 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 FundedChannel 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); } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 363f2ffdb65..541b5e73c25 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4343,25 +4343,22 @@ where fn can_forward_htlc_to_outgoing_channel( &self, chan: &mut FundedChannel, 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| { @@ -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, ); // 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); diff --git a/lightning/src/ln/onion_payment.rs b/lightning/src/ln/onion_payment.rs index 09d453d0020..2519372f11b 100644 --- a/lightning/src/ln/onion_payment.rs +++ b/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(()) diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index ed707771f84..d297f5766f1 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/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"Failed to accept/forward incoming HTLC: RealSCIDForward").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]); From 81fb07d6101039e33c3b26b799533af39455f3d9 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 16 Apr 2025 11:30:32 -0400 Subject: [PATCH 2/4] ln/fix: remove legacy payment related error codes These error codes were removed from the specification seven years ago to prevent probing, so we don't need to handle these cases anymore. --- lightning/src/ln/onion_utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index f4cd412cc1d..555415805db 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1287,7 +1287,7 @@ where // indicate that payment parameter has failed and no need to update Route object let payment_failed = match error_code & 0xff { - 15 | 16 | 17 | 18 | 19 | 23 => true, + 15 | 18 | 19 | 23 => true, _ => false, } && is_from_final_non_blinded_node; // PERM bit observed below even if this error is from the intermediate nodes From 5e9ae6bd751213a8db53191dec3ecd57f9d692e5 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 16 Apr 2025 11:32:12 -0400 Subject: [PATCH 3/4] ln/refactor: use LocalHTLCFailureCodes in onion error processing --- lightning/src/ln/onion_utils.rs | 108 +++++++++++++++++---------- lightning/src/ln/outbound_payment.rs | 2 +- lightning/src/util/errors.rs | 47 ------------ 3 files changed, 70 insertions(+), 87 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 555415805db..ab6eea262cf 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -20,7 +20,7 @@ use crate::routing::router::{BlindedTail, Path, RouteHop, RouteParameters, Tramp use crate::sign::{NodeSigner, Recipient}; use crate::types::features::{ChannelFeatures, NodeFeatures}; use crate::types::payment::{PaymentHash, PaymentPreimage}; -use crate::util::errors::{self, APIError}; +use crate::util::errors::APIError; use crate::util::logger::Logger; use crate::util::ser::{ LengthCalculatingWriter, Readable, ReadableArgs, VecWriter, Writeable, Writer, @@ -975,7 +975,7 @@ mod fuzzy_onion_utils { #[allow(dead_code)] pub(crate) hold_times: Vec, #[cfg(any(test, feature = "_test_utils"))] - pub(crate) onion_error_code: Option, + pub(crate) onion_error_code: Option, #[cfg(any(test, feature = "_test_utils"))] pub(crate) onion_error_data: Option>, } @@ -1126,7 +1126,7 @@ where Some(hop) => hop, None => { // Got an error from within a blinded route. - _error_code_ret = Some(BADONION | PERM | 24); // invalid_onion_blinding + _error_code_ret = Some(LocalHTLCFailureReason::InvalidOnionBlinding); _error_packet_ret = Some(vec![0; 32]); res = Some(FailureLearnings { network_update: None, @@ -1154,7 +1154,7 @@ where // The failing hop is within a multi-hop blinded path. #[cfg(not(test))] { - _error_code_ret = Some(BADONION | PERM | 24); // invalid_onion_blinding + _error_code_ret = Some(LocalHTLCFailureReason::InvalidOnionBlinding); _error_packet_ret = Some(vec![0; 32]); } #[cfg(test)] @@ -1166,9 +1166,12 @@ where &encrypted_packet.data, )) .unwrap(); - _error_code_ret = Some(u16::from_be_bytes( - err_packet.failuremsg.get(0..2).unwrap().try_into().unwrap(), - )); + _error_code_ret = Some( + u16::from_be_bytes( + err_packet.failuremsg.get(0..2).unwrap().try_into().unwrap(), + ) + .into(), + ); _error_packet_ret = Some(err_packet.failuremsg[2..].to_vec()); } @@ -1279,22 +1282,19 @@ where }, }; - let error_code = u16::from_be_bytes(error_code_slice.try_into().expect("len is 2")); + let error_code = u16::from_be_bytes(error_code_slice.try_into().expect("len is 2")).into(); _error_code_ret = Some(error_code); _error_packet_ret = Some(err_packet.failuremsg[2..].to_vec()); - let (debug_field, debug_field_size) = errors::get_onion_debug_field(error_code); + let (debug_field, debug_field_size) = error_code.get_onion_debug_field(); // indicate that payment parameter has failed and no need to update Route object - let payment_failed = match error_code & 0xff { - 15 | 18 | 19 | 23 => true, - _ => false, - } && is_from_final_non_blinded_node; // PERM bit observed below even if this error is from the intermediate nodes + let payment_failed = error_code.is_recipient_failure() && is_from_final_non_blinded_node; let mut network_update = None; let mut short_channel_id = None; - if error_code & BADONION == BADONION { + if error_code.is_badonion() { // If the error code has the BADONION bit set, always blame the channel from the node // "originating" the error to its next hop. The "originator" is ultimately actually claiming // that its counterparty is the one who is failing the HTLC. @@ -1308,12 +1308,13 @@ where is_permanent: true, }); } - } else if error_code & NODE == NODE { - let is_permanent = error_code & PERM == PERM; - network_update = - Some(NetworkUpdate::NodeFailure { node_id: *route_hop.pubkey(), is_permanent }); + } else if error_code.is_node() { + network_update = Some(NetworkUpdate::NodeFailure { + node_id: *route_hop.pubkey(), + is_permanent: error_code.is_permanent(), + }); short_channel_id = route_hop.short_channel_id(); - } else if error_code & PERM == PERM { + } else if error_code.is_permanent() { if !payment_failed { if let ErrorHop::RouteHop(failing_route_hop) = failing_route_hop { network_update = Some(NetworkUpdate::ChannelFailure { @@ -1323,7 +1324,7 @@ where } short_channel_id = failing_route_hop.short_channel_id(); } - } else if error_code & UPDATE == UPDATE { + } else if error_code.is_temporary() { if let Some(update_len_slice) = err_packet.failuremsg.get(debug_field_size + 2..debug_field_size + 4) { @@ -1357,8 +1358,9 @@ where } else if payment_failed { // Only blame the hop when a value in the HTLC doesn't match the corresponding value in the // onion. - short_channel_id = match error_code & 0xff { - 18 | 19 => route_hop.short_channel_id(), + short_channel_id = match error_code { + LocalHTLCFailureReason::FinalIncorrectCLTVExpiry + | LocalHTLCFailureReason::FinalIncorrectHTLCAmount => route_hop.short_channel_id(), _ => None, }; } else { @@ -1374,30 +1376,27 @@ where res = Some(FailureLearnings { network_update, short_channel_id, - payment_failed_permanently: error_code & PERM == PERM && is_from_final_non_blinded_node, + payment_failed_permanently: error_code.is_permanent() && is_from_final_non_blinded_node, failed_within_blinded_path: false, }); - let (description, title) = errors::get_onion_error_description(error_code); if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size { log_info!( logger, - "Onion Error[from {}: {}({:#x}) {}({})] {}", + "Onion Error[from {}: {:?}({:#x}) {}({})]", route_hop.pubkey(), - title, error_code, + error_code.failure_code(), debug_field, log_bytes!(&err_packet.failuremsg[4..4 + debug_field_size]), - description ); } else { log_info!( logger, - "Onion Error[from {}: {}({:#x})] {}", + "Onion Error[from {}: {:?}({:#x})]", route_hop.pubkey(), - title, error_code, - description + error_code.failure_code(), ); } @@ -1651,14 +1650,45 @@ impl LocalHTLCFailureReason { } } + /// Returns the name of an error's data field and its expected length. + fn get_onion_debug_field(&self) -> (&'static str, usize) { + match self { + Self::InvalidOnionVersion | Self::InvalidOnionHMAC | Self::InvalidOnionKey => { + ("sha256_of_onion", 32) + }, + Self::AmountBelowMinimum | Self::FeeInsufficient => ("htlc_msat", 8), + Self::IncorrectCLTVExpiry | Self::FinalIncorrectCLTVExpiry => ("cltv_expiry", 4), + Self::FinalIncorrectHTLCAmount => ("incoming_htlc_msat", 8), + Self::ChannelDisabled => ("flags", 2), + _ => ("", 0), + } + } + pub(super) fn is_temporary(&self) -> bool { self.failure_code() & UPDATE == UPDATE } - #[cfg(test)] pub(super) fn is_permanent(&self) -> bool { self.failure_code() & PERM == PERM } + + fn is_badonion(&self) -> bool { + self.failure_code() & BADONION == BADONION + } + + fn is_node(&self) -> bool { + self.failure_code() & NODE == NODE + } + + /// Returns true if the failure is only sent by the final recipient. Note that this function + /// only checks [`LocalHTLCFailureReason`] variants that represent bolt 04 errors directly, + /// as it's intended to analyze errors we've received as a sender. + fn is_recipient_failure(&self) -> bool { + self.failure_code() == LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code() + || *self == LocalHTLCFailureReason::FinalIncorrectCLTVExpiry + || *self == LocalHTLCFailureReason::FinalIncorrectHTLCAmount + || *self == LocalHTLCFailureReason::MPPTimeout + } } impl From for LocalHTLCFailureReason { @@ -2006,7 +2036,7 @@ impl HTLCFailReason { failed_within_blinded_path: false, hold_times: Vec::new(), #[cfg(any(test, feature = "_test_utils"))] - onion_error_code: Some(failure_reason.failure_code()), + onion_error_code: Some(*failure_reason), #[cfg(any(test, feature = "_test_utils"))] onion_error_data: Some(data.clone()), } @@ -3093,7 +3123,7 @@ mod tests { test_attributable_failure_packet_onion_with_mutation(Some(mutation)); if decrypted_failure.onion_error_code - == Some(LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code()) + == Some(LocalHTLCFailureReason::IncorrectPaymentDetails) { continue; } @@ -3109,7 +3139,7 @@ mod tests { let decrypted_failure = test_attributable_failure_packet_onion_with_mutation(None); assert_eq!( decrypted_failure.onion_error_code, - Some(LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code()) + Some(LocalHTLCFailureReason::IncorrectPaymentDetails) ); assert_eq!(decrypted_failure.hold_times, [5, 4, 3, 2, 1]); } @@ -3334,7 +3364,7 @@ mod tests { ); assert_eq!( decrypted_failure.onion_error_code, - Some(LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code()) + Some(LocalHTLCFailureReason::IncorrectPaymentDetails), ); } @@ -3380,7 +3410,7 @@ mod tests { let decrypted_failure = process_onion_failure(&secp_ctx, &logger, &htlc_source, first_hop_error_packet); - assert_eq!(decrypted_failure.onion_error_code, Some(error_code.failure_code())); + assert_eq!(decrypted_failure.onion_error_code, Some(error_code)); }; { @@ -3411,7 +3441,7 @@ mod tests { &htlc_source, trampoline_outer_hop_error_packet, ); - assert_eq!(decrypted_failure.onion_error_code, Some(error_code.failure_code())); + assert_eq!(decrypted_failure.onion_error_code, Some(error_code)); }; { @@ -3447,7 +3477,7 @@ mod tests { &htlc_source, trampoline_inner_hop_error_packet, ); - assert_eq!(decrypted_failure.onion_error_code, Some(error_code.failure_code())); + assert_eq!(decrypted_failure.onion_error_code, Some(error_code)); } { @@ -3488,7 +3518,7 @@ mod tests { &htlc_source, trampoline_second_hop_error_packet, ); - assert_eq!(decrypted_failure.onion_error_code, Some(error_code.failure_code())); + assert_eq!(decrypted_failure.onion_error_code, Some(error_code)); } } } diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 8cab698a59a..92a652b768e 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2288,7 +2288,7 @@ impl OutboundPayments { path: path.clone(), short_channel_id, #[cfg(any(test, feature = "_test_utils"))] - error_code: onion_error_code, + error_code: onion_error_code.map(|f| f.failure_code()), #[cfg(any(test, feature = "_test_utils"))] error_data: onion_error_data } diff --git a/lightning/src/util/errors.rs b/lightning/src/util/errors.rs index 7b9a24f891f..eaaf0130ca2 100644 --- a/lightning/src/util/errors.rs +++ b/lightning/src/util/errors.rs @@ -101,50 +101,3 @@ impl_writeable_tlv_based_enum_upgradable!(APIError, (8, MonitorUpdateInProgress) => {}, (10, IncompatibleShutdownScript) => { (0, script, required), }, ); - -#[inline] -pub(crate) fn get_onion_debug_field(error_code: u16) -> (&'static str, usize) { - match error_code & 0xff { - 4 | 5 | 6 => ("sha256_of_onion", 32), - 11 | 12 => ("htlc_msat", 8), - 13 | 18 => ("cltv_expiry", 4), - 19 => ("incoming_htlc_msat", 8), - 20 => ("flags", 2), - _ => ("", 0), - } -} - -#[inline] -pub(crate) fn get_onion_error_description(error_code: u16) -> (&'static str, &'static str) { - const BADONION: u16 = 0x8000; - const PERM: u16 = 0x4000; - const NODE: u16 = 0x2000; - const UPDATE: u16 = 0x1000; - match error_code { - _c if _c == PERM|1 => ("The realm byte was not understood by the processing node", "invalid_realm"), - _c if _c == NODE|2 => ("Node indicated temporary node failure", "temporary_node_failure"), - _c if _c == PERM|NODE|2 => ("Node indicated permanent node failure", "permanent_node_failure"), - _c if _c == PERM|NODE|3 => ("Node indicated the required node feature is missing in the onion", "required_node_feature_missing"), - _c if _c == BADONION|PERM|4 => ("Node indicated the version by is not understood", "invalid_onion_version"), - _c if _c == BADONION|PERM|5 => ("Node indicated the HMAC of the onion is incorrect", "invalid_onion_hmac"), - _c if _c == BADONION|PERM|6 => ("Node indicated the ephemeral public keys is not parseable", "invalid_onion_key"), - _c if _c == UPDATE|7 => ("Node indicated the outgoing channel is unable to handle the HTLC temporarily", "temporary_channel_failure"), - _c if _c == PERM|8 => ("Node indicated the outgoing channel is unable to handle the HTLC peramanently", "permanent_channel_failure"), - _c if _c == PERM|9 => ("Node indicated the required feature for the outgoing channel is not satisfied", "required_channel_feature_missing"), - _c if _c == PERM|10 => ("Node indicated the outbound channel is not found for the specified short_channel_id in the onion packet", "unknown_next_peer"), - _c if _c == UPDATE|11 => ("Node indicated the HTLC amount was below the required minmum for the outbound channel", "amount_below_minimum"), - _c if _c == UPDATE|12 => ("Node indicated the fee amount does not meet the required level", "fee_insufficient"), - _c if _c == UPDATE|13 => ("Node indicated the cltv_expiry does not comply with the cltv_expiry_delta required by the outgoing channel", "incorrect_cltv_expiry"), - _c if _c == UPDATE|14 => ("Node indicated the CLTV expiry too close to the current block height for safe handling", "expiry_too_soon"), - _c if _c == PERM|15 => ("The final node indicated the payment hash is unknown or amount is incorrect", "incorrect_or_unknown_payment_details"), - _c if _c == PERM|16 => ("The final node indicated the payment amount is incorrect", "incorrect_payment_amount"), - _c if _c == 17 => ("The final node indicated the CLTV expiry is too close to the current block height for safe handling", "final_expiry_too_soon"), - _c if _c == 18 => ("The final node indicated the CLTV expiry in the HTLC does not match the value in the onion", "final_incorrect_cltv_expiry"), - _c if _c == 19 => ("The final node indicated the amount in the HTLC does not match the value in the onion", "final_incorrect_htlc_amount"), - _c if _c == UPDATE|20 => ("Node indicated the outbound channel has been disabled", "channel_disabled"), - _c if _c == 21 => ("Node indicated the CLTV expiry in the HTLC is too far in the future", "expiry_too_far"), - _c if _c == PERM|22 => ("Node indicated that the decrypted onion per-hop payload was not understood by it or is incomplete", "invalid_onion_payload"), - _c if _c == 23 => ("The final node indicated the complete amount of the multi-part payment was not received within a reasonable time", "mpp_timeout"), - _ => ("Unknown", ""), - } -} From 935ffde99bc98d41823a6aa59e68aab293d56141 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Thu, 17 Apr 2025 16:33:38 -0400 Subject: [PATCH 4/4] ln/refactor: macro to impl From for LocalHTLCFailureReason De-duplicate use of u16 failure codes by using a macro that will match against each variant's failure_code instead. --- lightning/src/ln/onion_utils.rs | 97 ++++++++++++++++----------------- 1 file changed, 46 insertions(+), 51 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index ab6eea262cf..69db26c8854 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1457,6 +1457,9 @@ const UPDATE: u16 = 0x1000; /// [`Self::FeeInsufficient`] is a direct representation of its underlying BOLT04 error code. /// [`Self::PrivateChannelForward`] provides additional information that is not provided by its /// BOLT04 error code. +// +// Note that variants that directly represent BOLT04 error codes must implement conversion from u16 +// values using [`impl_from_u16_for_htlc_reason`] #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum LocalHTLCFailureReason { /// There has been a temporary processing failure on the node which may resolve on retry. @@ -1691,57 +1694,49 @@ impl LocalHTLCFailureReason { } } -impl From for LocalHTLCFailureReason { - fn from(value: u16) -> Self { - if value == (NODE | 2) { - LocalHTLCFailureReason::TemporaryNodeFailure - } else if value == (PERM | NODE | 2) { - LocalHTLCFailureReason::PermanentNodeFailure - } else if value == (PERM | NODE | 3) { - LocalHTLCFailureReason::RequiredNodeFeature - } else if value == (BADONION | PERM | 4) { - LocalHTLCFailureReason::InvalidOnionVersion - } else if value == (BADONION | PERM | 5) { - LocalHTLCFailureReason::InvalidOnionHMAC - } else if value == (BADONION | PERM | 6) { - LocalHTLCFailureReason::InvalidOnionKey - } else if value == (UPDATE | 7) { - LocalHTLCFailureReason::TemporaryChannelFailure - } else if value == (PERM | 8) { - LocalHTLCFailureReason::PermanentChannelFailure - } else if value == (PERM | 9) { - LocalHTLCFailureReason::RequiredChannelFeature - } else if value == (PERM | 10) { - LocalHTLCFailureReason::UnknownNextPeer - } else if value == (UPDATE | 11) { - LocalHTLCFailureReason::AmountBelowMinimum - } else if value == (UPDATE | 12) { - LocalHTLCFailureReason::FeeInsufficient - } else if value == (UPDATE | 13) { - LocalHTLCFailureReason::IncorrectCLTVExpiry - } else if value == (UPDATE | 14) { - LocalHTLCFailureReason::CLTVExpiryTooSoon - } else if value == (PERM | 15) { - LocalHTLCFailureReason::IncorrectPaymentDetails - } else if value == 18 { - LocalHTLCFailureReason::FinalIncorrectCLTVExpiry - } else if value == 19 { - LocalHTLCFailureReason::FinalIncorrectHTLCAmount - } else if value == (UPDATE | 20) { - LocalHTLCFailureReason::ChannelDisabled - } else if value == 21 { - LocalHTLCFailureReason::CLTVExpiryTooFar - } else if value == (PERM | 22) { - LocalHTLCFailureReason::InvalidOnionPayload - } else if value == 23 { - LocalHTLCFailureReason::MPPTimeout - } else if value == (BADONION | PERM | 24) { - LocalHTLCFailureReason::InvalidOnionBlinding - } else { - LocalHTLCFailureReason::UnknownFailureCode { code: value } - } - } -} +macro_rules! impl_from_u16_for_htlc_reason { + ($enum:ident, [$($variant:ident),* $(,)?]) => { + impl From for $enum { + fn from(value: u16) -> Self { + $( + if value == $enum::$variant.failure_code() { + return $enum::$variant; + } + )* + $enum::UnknownFailureCode { code: value } + } + } + }; +} + +// Error codes that represent BOLT04 error codes must be included here. +impl_from_u16_for_htlc_reason!( + LocalHTLCFailureReason, + [ + TemporaryNodeFailure, + PermanentNodeFailure, + RequiredNodeFeature, + InvalidOnionVersion, + InvalidOnionHMAC, + InvalidOnionKey, + TemporaryChannelFailure, + PermanentChannelFailure, + RequiredChannelFeature, + UnknownNextPeer, + AmountBelowMinimum, + FeeInsufficient, + IncorrectCLTVExpiry, + CLTVExpiryTooSoon, + IncorrectPaymentDetails, + FinalIncorrectCLTVExpiry, + FinalIncorrectHTLCAmount, + ChannelDisabled, + CLTVExpiryTooFar, + InvalidOnionPayload, + MPPTimeout, + InvalidOnionBlinding, + ] +); impl_writeable_tlv_based_enum!(LocalHTLCFailureReason, (1, TemporaryNodeFailure) => {},