Move HTLC holding into HTLC decode from `forward_htlcs`
What the developer wrote
Authored by Matt Corallo
Move HTLC holding into HTLC decode from `forward_htlcs`
When we added support for async payments (which requires holding
HTLCs until we receive an onion message), we added the hold logic
to `ChannelManager::forward_htlcs`. This made sense as we reused
the forwarding datastructure in the holding logic so already had
the right types in place, but it turns out only a single call of
`forward_htlcs` should ever result in an HTLC being held.
All of the other calls (un-holding an HTLC, forwarding an
intercepted HTLC, forwarding an HTLC decoded by LDK prior to 0.2,
or processing a phantom receive) should never result in an HTLC
being held. Instead, HTLCs should actually only ever be held when
the HTLC is decoded in `process_pending_update_add_htlcs` before
forwarding.
Because of this, and because we want to move the interception (and
thus also the holding logic) out of `forward_htlcs`, here we move
the holding logic into `process_pending_update_add_htlcs`.
What changed, and why it matters
This commit is a code cleanup that moves where in the program Lightning payments are intentionally held (paused) for async delivery. It does not add or remove security behavior, but it changes which internal function is responsible for deciding to hold a payment. Because the change is described as a prerequisite for moving interception logic out of a central forwarding function, it could affect correctness of payment handling if done wrong, but the diff itself appears to preserve the same hold logic in a different location.
Treat as a normal refactor review. Verify that the moved hold logic is reached exactly once per HTLC and that no other call paths into forward_htlcs still expect to evaluate should_hold_htlc(). Confirm the duplicate intercept_id failure path remains reachable and that lock ordering of pending_intercepted_htlcs is unchanged relative to other locks held in process_pending_update_add_htlcs.
Security signals we found
Logic relocation of HTLC hold/intercept decision point
Duplicate intercept_id now handled at decode time with debug_assert and failure fallback
No new cryptographic, network, or permission checks introduced
Commit message frames change as enabling future interception refactoring
Evidence from the diff
The patch relocates HTLC-holding logic from ChannelManager::forward_htlcs into process_pending_update_add_htlcs. Previously, should_hold_htlc() was evaluated inside forward_htlcs, which is also called for un-holding, forwarding intercepted HTLCs, forwarding pre-0.2 decoded HTLCs, and phantom receives. The change restricts the hold decision to the point where HTLCs are freshly decoded, matching the intended semantics that only one call path should ever hold an HTLC. The same Vacant/Occupied map handling and duplicate-ID failure path are preserved, including the debug_assert and TemporaryNodeFailure fallback.
Changed components
lightning/src/ln/channelmanager.rsChannelManager::process_pending_update_add_htlcsChannelManager::forward_htlcsasync payment HTLC holding / intercept logicInspect captured patch +49 / −31
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 95442ea..04f25bc 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6967,7 +6967,53 @@ where
incoming_accept_underpaying_htlcs,
next_packet_details_opt.map(|d| d.next_packet_pubkey),
) {
- Ok(info) => htlc_forwards.push((info, update_add_htlc.htlc_id)),
+ Ok(info) => {
+ if info.routing.should_hold_htlc() {
+ let intercept_id = InterceptId::from_htlc_id_and_chan_id(
+ update_add_htlc.htlc_id,
+ &incoming_channel_id,
+ &incoming_counterparty_node_id,
+ );
+ let mut held_htlcs = self.pending_intercepted_htlcs.lock().unwrap();
+ match held_htlcs.entry(intercept_id) {
+ hash_map::Entry::Vacant(entry) => {
+ log_trace!(
+ self.logger,
+ "Intercepted held HTLC with id {}, holding until the recipient is online",
+ intercept_id
+ );
+ let pending_add = PendingAddHTLCInfo {
+ prev_outbound_scid_alias: incoming_scid_alias,
+ prev_counterparty_node_id: incoming_counterparty_node_id,
+ prev_funding_outpoint: incoming_funding_txo,
+ prev_channel_id: incoming_channel_id,
+ prev_htlc_id: update_add_htlc.htlc_id,
+ prev_user_channel_id: incoming_user_channel_id,
+ forward_info: info,
+ };
+ entry.insert(pending_add);
+ },
+ hash_map::Entry::Occupied(_) => {
+ debug_assert!(false, "Should never have two HTLCs with the same channel id and htlc id");
+ 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()));
+ },
+ }
+ } else {
+ htlc_forwards.push((info, update_add_htlc.htlc_id))
+ }
+ },
Err(inbound_err) => {
let failure_type =
get_htlc_failure_type(outgoing_scid_opt, update_add_htlc.payment_hash);
@@ -6991,7 +7037,7 @@ where
incoming_funding_txo,
incoming_channel_id,
incoming_user_channel_id,
- htlc_forwards.drain(..).collect(),
+ htlc_forwards,
);
self.forward_htlcs(&mut [pending_forwards]);
for (htlc_fail, failure_type, failure_reason) in htlc_fails.drain(..) {
@@ -11902,35 +11948,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
));
};
- // In the case that we have an HTLC that we're supposed to hold onto until the
- // recipient comes online *and* the outbound scid is encoded as
- // `fake_scid::is_valid_intercept`, we should first wait for the recipient to come
- // online before generating an `HTLCIntercepted` event, since the event cannot be
- // acted on until the recipient is online to cooperatively open the JIT channel. Once
- // we receive the `ReleaseHeldHtlc` message from the recipient, we will circle back
- // here and resume generating the event below.
- if pending_add.forward_info.routing.should_hold_htlc() {
- let intercept_id = InterceptId::from_htlc_id_and_chan_id(
- prev_htlc_id,
- &prev_channel_id,
- &prev_counterparty_node_id,
- );
- let mut held_htlcs = self.pending_intercepted_htlcs.lock().unwrap();
- match held_htlcs.entry(intercept_id) {
- hash_map::Entry::Vacant(entry) => {
- log_trace!(
- logger,
- "Intercepted held HTLC with id {}, holding until the recipient is online",
- intercept_id
- );
- entry.insert(pending_add);
- },
- hash_map::Entry::Occupied(_) => {
- debug_assert!(false, "Should never have two HTLCs with the same channel id and htlc id");
- fail_intercepted_htlc(pending_add);
- },
- }
- } else if !is_our_scid
+ if !is_our_scid
&& pending_add.forward_info.incoming_amt_msat.is_some()
&& fake_scid::is_valid_intercept(
&self.fake_scid_rand_bytes,
Why this scored 35/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.