Skip to content
Open
33 changes: 33 additions & 0 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6406,6 +6406,32 @@ where
});
let shared_secret = next_hop.shared_secret().secret_bytes();

// Nodes shouldn't expect us to hold HTLCs for them if we don't advertise htlc_hold feature
// support.
//
// If we wanted to pretend to be a node that didn't understand the feature at all here, the
// correct behavior would've been to disconnect the sender when we first received the
// update_add message. However, this would make the `UserConfig::enable_htlc_hold` option
// unsafe -- if our node switched the config option from on to off just after the sender
// enqueued their update_add + CS, the sender would continue retransmitting those messages
// and we would keep disconnecting them until the HTLC timed out.
if update_add_htlc.hold_htlc.is_some()
&& !BaseMessageHandler::provided_node_features(self).supports_htlc_hold()
{
let reason = LocalHTLCFailureReason::TemporaryNodeFailure;
let htlc_fail = self.htlc_failure_from_update_add_err(
&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);
htlc_fails.push((htlc_fail, failure_type, reason.into()));
continue;
}
Comment on lines +6473 to +6497
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seeking conceptual feedback on this approach, including the failure reason

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it isn't better and simpler to just let the htlc through and log? Maybe the receiver is online or can be woken up and it just works. And otherwise that error will come back anyway.

It probably also depends on how the sender is going to interpret this failure. If it handles it like any other, it isn't necessary to be specific? Zooming out, I don't think any of this matters much in a client<->lsp setup, except for some debugging with can be done through logs too.


// Process the HTLC on the incoming channel.
match self.do_funded_channel_callback(
incoming_scid,
Expand Down Expand Up @@ -14792,6 +14818,13 @@ pub fn provided_init_features(config: &UserConfig) -> InitFeatures {
features.set_anchor_zero_fee_commitments_optional();
}

// If we are configured to be an announced node, we are expected to be always-online and can
// advertise the htlc_hold feature.
#[cfg(test)]
if config.enable_htlc_hold {
features.set_htlc_hold_optional();
}

features
}

Expand Down
14 changes: 14 additions & 0 deletions lightning/src/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,18 @@ pub struct UserConfig {
///
/// Default value: `false`
pub enable_dual_funded_channels: bool,
/// LDK supports a feature for always-online nodes such that these nodes can hold onto an HTLC
/// from an often-offline channel peer until the often-offline payment recipient sends an onion
/// message telling the always-online node to release the HTLC. If this is set to `true`, our node
/// will carry out this feature for channel peers that request it.
///
/// This should only be set to `true` for nodes which expect to be online reliably.
///
/// Setting this to `true` may break backwards compatibility with LDK versions < 0.2.
///
/// Default value: `false`
#[cfg(test)]
pub enable_htlc_hold: bool,
}

impl Default for UserConfig {
Expand All @@ -949,6 +961,8 @@ impl Default for UserConfig {
accept_intercept_htlcs: false,
manually_handle_bolt12_invoices: false,
enable_dual_funded_channels: false,
#[cfg(test)]
enable_htlc_hold: false,
}
}
}
Expand Down