Skip to content

(3/3) Add Failure Reason to HTLCHandlingFailed #3700

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 6 commits into from
Apr 29, 2025
Merged

(3/3) Add Failure Reason to HTLCHandlingFailed #3700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
45a4f00
ln+events+liquidity/refactor: HTLCDestination/HTLCHandlingFailureType
carlaKC Apr 11, 2025
d98a254
ln+events+liquidity/refactor: rename FailedPayment to Receive
carlaKC Apr 25, 2025
99bcd69
ln+events+liquidity/refactor: NextHopChannel renamed Forward
carlaKC Apr 25, 2025
d1289ce
ln+events: add htlc failure reason to HTLCHandlingFailed
carlaKC Apr 25, 2025
8fc8b3b
ln+events: deprecate UnknownNextPeer in HTLCHandlingFailureType
carlaKC Apr 25, 2025
af301f4
pending_changelog: release notes for HTLCHandlingFailure changes
carlaKC Apr 26, 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
6 changes: 3 additions & 3 deletions lightning-liquidity/src/lsps2/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::prelude::hash_map::Entry;
use crate::prelude::{new_hash_map, HashMap};
use crate::sync::{Arc, Mutex, MutexGuard, RwLock};

use lightning::events::HTLCDestination;
use lightning::events::HTLCHandlingFailureType;
use lightning::ln::channelmanager::{AChannelManager, InterceptId};
use lightning::ln::msgs::{ErrorAction, LightningError};
use lightning::ln::types::ChannelId;
Expand Down Expand Up @@ -883,9 +883,9 @@ where
///
/// [`Event::HTLCHandlingFailed`]: lightning::events::Event::HTLCHandlingFailed
pub fn htlc_handling_failed(
&self, failed_next_destination: HTLCDestination,
&self, failure_type: HTLCHandlingFailureType,
) -> Result<(), APIError> {
if let HTLCDestination::NextHopChannel { channel_id, .. } = failed_next_destination {
if let HTLCHandlingFailureType::Forward { channel_id, .. } = failure_type {
let peer_by_channel_id = self.peer_by_channel_id.read().unwrap();
if let Some(counterparty_node_id) = peer_by_channel_id.get(&channel_id) {
let outer_state_lock = self.per_peer_state.read().unwrap();
Expand Down
75 changes: 60 additions & 15 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::ln::channel::FUNDING_CONF_DEADLINE_BLOCKS;
use crate::offers::invoice::Bolt12Invoice;
use crate::offers::static_invoice::StaticInvoice;
use crate::types::features::ChannelTypeFeatures;
use crate::ln::msgs;
use crate::ln::{msgs, LocalHTLCFailureReason};
use crate::ln::types::ChannelId;
use crate::types::payment::{PaymentPreimage, PaymentHash, PaymentSecret};
use crate::onion_message::messenger::Responder;
Expand Down Expand Up @@ -466,12 +466,12 @@ impl_writeable_tlv_based_enum_upgradable!(ClosureReason,
},
);

/// Intended destination of a failed HTLC as indicated in [`Event::HTLCHandlingFailed`].
/// The type of HTLC handling performed in [`Event::HTLCHandlingFailed`].
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum HTLCDestination {
pub enum HTLCHandlingFailureType {
/// We tried forwarding to a channel but failed to do so. An example of such an instance is when
/// there is insufficient capacity in our outbound channel.
NextHopChannel {
Forward {
/// The `node_id` of the next node. For backwards compatibility, this field is
/// marked as optional, versions prior to 0.0.110 may not always be able to provide
/// counterparty node information.
Expand All @@ -480,12 +480,17 @@ pub enum HTLCDestination {
channel_id: ChannelId,
},
/// Scenario where we are unsure of the next node to forward the HTLC to.
///
/// Deprecated: will only be used in versions before LDK v0.2.0. Downgrades will result in
/// this type being represented as [`Self::InvalidForward`].
UnknownNextHop {
/// Short channel id we are requesting to forward an HTLC to.
requested_forward_scid: u64,
},
/// We couldn't forward to the outgoing scid. An example would be attempting to send a duplicate
/// intercept HTLC.
///
/// In LDK v0.2.0 and greater, this variant replaces [`Self::UnknownNextHop`].
InvalidForward {
/// Short channel id we are requesting to forward an HTLC to.
requested_forward_scid: u64
Expand All @@ -502,14 +507,14 @@ pub enum HTLCDestination {
/// * The counterparty node modified the HTLC in transit,
/// * A probing attack where an intermediary node is trying to detect if we are the ultimate
/// recipient for a payment.
FailedPayment {
Receive {
/// The payment hash of the payment we attempted to process.
payment_hash: PaymentHash
},
}

impl_writeable_tlv_based_enum_upgradable!(HTLCDestination,
(0, NextHopChannel) => {
impl_writeable_tlv_based_enum_upgradable!(HTLCHandlingFailureType,
(0, Forward) => {
(0, node_id, required),
(2, channel_id, required),
},
Expand All @@ -520,11 +525,36 @@ impl_writeable_tlv_based_enum_upgradable!(HTLCDestination,
(0, requested_forward_scid, required),
},
(3, InvalidOnion) => {},
(4, FailedPayment) => {
(4, Receive) => {
(0, payment_hash, required),
},
);

/// The reason for HTLC failures in [`Event::HTLCHandlingFailed`].
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum HTLCHandlingFailureReason {
/// The forwarded HTLC was failed back by the downstream node with an encrypted error reason.
Downstream,
/// The HTLC was failed locally by our node.
Local {
/// The reason that our node chose to fail the HTLC.
reason: LocalHTLCFailureReason,
},
Comment on lines +537 to +542
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be nicer to avoid this outer enum and just document on the field "Will be None if the HTLC failed downstream or this event was created prior to 0.2." Seems simpler, loses a bit of information for a period of time though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it would be nice to surface hold_time along with Downstream in future (nice thing to put on a dashboard IMO) so tend towards keeping the enum for the sake of being able to easily add that.

Opinion not very strongly held though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't realize that was a potential goal but that makes sense! We may want to document this on the attributable failures issue, if there is one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@joostjager do we have a tracking issue for follow-ups for attributable failures? IIRC we still need to support the hold_times part of the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted in #3753.

}

impl_writeable_tlv_based_enum!(HTLCHandlingFailureReason,
(1, Downstream) => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't those be even because it is a completely new type? I somehow feel that we've been here before, but maybe it was another PR. Enum always odd regardless, was that the outcome? Doesn't seem to matter all that much though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Odd/even enum variant types only have different behavior in impl_writeable_tlv_based_enum_upgradable :) We could use that macro instead if we think we'll want to add more odd variants in the future, though I don't feel strongly about that because that doesn't seem likely IMO.

(3, Local) => {
(0, reason, required),
},
);

impl From<LocalHTLCFailureReason> for HTLCHandlingFailureReason {
fn from(value: LocalHTLCFailureReason) -> Self {
HTLCHandlingFailureReason::Local { reason: value }
}
}

/// Will be used in [`Event::HTLCIntercepted`] to identify the next hop in the HTLC's path.
/// Currently only used in serialization for the sake of maintaining compatibility. More variants
/// will be added for general-purpose HTLC forward intercepts as well as trampoline forward
Expand Down Expand Up @@ -1460,8 +1490,12 @@ pub enum Event {
HTLCHandlingFailed {
/// The channel over which the HTLC was received.
prev_channel_id: ChannelId,
/// Destination of the HTLC that failed to be processed.
failed_next_destination: HTLCDestination,
/// The type of HTLC handling that failed.
failure_type: HTLCHandlingFailureType,
/// The reason that the HTLC failed.
///
/// This field will be `None` only for objects serialized prior to LDK 0.2.0.
failure_reason: Option<HTLCHandlingFailureReason>
},
/// Indicates that a transaction originating from LDK needs to have its fee bumped. This event
/// requires confirmed external funds to be readily available to spend.
Expand Down Expand Up @@ -1766,11 +1800,12 @@ impl Writeable for Event {
(8, path.blinded_tail, option),
})
},
&Event::HTLCHandlingFailed { ref prev_channel_id, ref failed_next_destination } => {
&Event::HTLCHandlingFailed { ref prev_channel_id, ref failure_type, ref failure_reason } => {
25u8.write(writer)?;
write_tlv_fields!(writer, {
(0, prev_channel_id, required),
(2, failed_next_destination, required),
(1, failure_reason, option),
(2, failure_type, required),
})
},
&Event::BumpTransaction(ref event)=> {
Expand Down Expand Up @@ -2218,14 +2253,24 @@ impl MaybeReadable for Event {
25u8 => {
let mut f = || {
let mut prev_channel_id = ChannelId::new_zero();
let mut failed_next_destination_opt = UpgradableRequired(None);
let mut failure_reason = None;
let mut failure_type_opt = UpgradableRequired(None);
read_tlv_fields!(reader, {
(0, prev_channel_id, required),
(2, failed_next_destination_opt, upgradable_required),
(1, failure_reason, option),
(2, failure_type_opt, upgradable_required),
});

// If a legacy HTLCHandlingFailureType::UnknownNextHop was written, upgrade
// it to its new representation, otherwise leave unchanged.
if let Some(HTLCHandlingFailureType::UnknownNextHop { requested_forward_scid }) = failure_type_opt.0 {
failure_type_opt.0 = Some(HTLCHandlingFailureType::InvalidForward { requested_forward_scid });
failure_reason = Some(LocalHTLCFailureReason::UnknownNextPeer.into());
}
Ok(Some(Event::HTLCHandlingFailed {
prev_channel_id,
failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required),
failure_type: _init_tlv_based_struct_field!(failure_type_opt, upgradable_required),
failure_reason
}))
};
f()
Expand Down
12 changes: 6 additions & 6 deletions lightning/src/ln/async_payments_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::blinded_path::message::{MessageContext, OffersContext};
use crate::blinded_path::payment::PaymentContext;
use crate::blinded_path::payment::{AsyncBolt12OfferContext, BlindedPaymentTlvs};
use crate::chain::channelmonitor::{HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
use crate::events::{Event, HTLCDestination, PaidBolt12Invoice, PaymentFailureReason};
use crate::events::{Event, HTLCHandlingFailureType, PaidBolt12Invoice, PaymentFailureReason};
use crate::ln::blinded_payment_tests::{fail_blinded_htlc_backwards, get_blinded_route_parameters};
use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
use crate::ln::functional_test_utils::*;
Expand Down Expand Up @@ -172,7 +172,7 @@ fn invalid_keysend_payment_secret() {
PassAlongPathArgs::new(&nodes[0], &expected_route[0], amt_msat, payment_hash, ev.clone())
.with_payment_secret(invalid_payment_secret)
.with_payment_preimage(keysend_preimage)
.expect_failure(HTLCDestination::FailedPayment { payment_hash });
.expect_failure(HTLCHandlingFailureType::Receive { payment_hash });
do_pass_along_path(args);

let updates_2_1 = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
Expand Down Expand Up @@ -701,7 +701,7 @@ fn amount_doesnt_match_invreq() {
let args = PassAlongPathArgs::new(&nodes[0], route[0], amt_msat, payment_hash, ev)
.with_payment_preimage(keysend_preimage)
.without_claimable_event()
.expect_failure(HTLCDestination::FailedPayment { payment_hash });
.expect_failure(HTLCHandlingFailureType::Receive { payment_hash });
do_pass_along_path(args);

// Modify the invoice request stored in our outbounds to be the correct one, to make sure the
Expand Down Expand Up @@ -917,7 +917,7 @@ fn invalid_async_receive_with_retry<F1, F2>(
nodes[2].node.fail_htlc_backwards(&payment_hash);
expect_pending_htlcs_forwardable_conditions(
nodes[2].node.get_and_clear_pending_events(),
&[HTLCDestination::FailedPayment { payment_hash }],
&[HTLCHandlingFailureType::Receive { payment_hash }],
);
nodes[2].node.process_pending_htlc_forwards();
check_added_monitors!(nodes[2], 1);
Expand All @@ -937,7 +937,7 @@ fn invalid_async_receive_with_retry<F1, F2>(
let args = PassAlongPathArgs::new(&nodes[0], route[0], amt_msat, payment_hash, ev)
.with_payment_preimage(keysend_preimage)
.without_claimable_event()
.expect_failure(HTLCDestination::FailedPayment { payment_hash });
.expect_failure(HTLCHandlingFailureType::Receive { payment_hash });
do_pass_along_path(args);
fail_blinded_htlc_backwards(payment_hash, 1, &[&nodes[0], &nodes[1], &nodes[2]], true);

Expand Down Expand Up @@ -1103,7 +1103,7 @@ fn expired_static_invoice_payment_path() {
let args = PassAlongPathArgs::new(&nodes[0], route[0], amt_msat, payment_hash, ev)
.with_payment_preimage(keysend_preimage)
.without_claimable_event()
.expect_failure(HTLCDestination::FailedPayment { payment_hash });
.expect_failure(HTLCHandlingFailureType::Receive { payment_hash });
do_pass_along_path(args);
fail_blinded_htlc_backwards(payment_hash, 1, &[&nodes[0], &nodes[1], &nodes[2]], false);
nodes[2].logger.assert_log_contains(
Expand Down
Loading
Loading