Skip to content

Use LocalHTLCFailureReason in Onion Processing #3744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 2, 2025
Merged

Use LocalHTLCFailureReason in Onion Processing #3744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27b1321
ln/refactor: remove err_str where LocalHTLCFailureReason sufficient
carlaKC Apr 16, 2025
81fb07d
ln/fix: remove legacy payment related error codes
carlaKC Apr 16, 2025
5e9ae6b
ln/refactor: use LocalHTLCFailureCodes in onion error processing
carlaKC Apr 16, 2025
935ffde
ln/refactor: macro to impl From<u16> for LocalHTLCFailureReason
carlaKC Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand All @@ -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() {
Expand All @@ -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);
Expand All @@ -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
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down
40 changes: 16 additions & 24 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)?;

Expand Down Expand Up @@ -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>| {
Expand All @@ -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);
}
}
}
Expand All @@ -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 {
Expand All @@ -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() {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
15 changes: 7 additions & 8 deletions lightning/src/ln/onion_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor Author

@carlaKC carlaKC May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note here for review: this msg loses a bit of granularity - previously it had the descriptive string, and now it's just got this generic statement because we can't get a &str from format!({:?}, reason)'s String that lives long enough (afaik).

I think that this is okay because peel_payment_onion isn't used anywhere in the codebase, and the new reason is now available to provide the information that's lost in the string. If it's not ok, I can revert check_incoming_htlc_cltv to still return &str, LocalHTLCFailureReason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit 👍 Thanks for the diligence!

reason,
err_data: Vec::new(),
});
Expand Down Expand Up @@ -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
Expand All @@ -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(())
Expand Down
Loading
Loading