From 24b52f39b223dabb44f02d6e613b2156b95e4dd6 Mon Sep 17 00:00:00 2001 From: amackillop Date: Thu, 16 Apr 2026 21:18:03 +0100 Subject: [PATCH 1/2] Persist HTLCs before action calculation htlc_intercepted only persisted HTLCs when a liquidity action (splice/open) was needed. This prevents calculate_htlc_actions from returning empty actions (i.e. "do nothing, let the timer handle it") because the HTLC would never make it into the store and the timer would never see it. Move the insert before calculate_htlc_actions_for_peer so every intercepted HTLC is in the store before we decide what to do with it. execute_htlc_actions already removes the HTLC on successful forward, so the store entry is short-lived on the happy path. The cost is two extra KV store round-trips (S3 write + delete) per HTLC on the forward-only path, where previously there were none. This hits every payment since LSPS4 intercepts all of them. Unfortunately unavoidable: the insert must happen before action calculation because we don't know yet whether the result will be "forward now" or "do nothing and defer to the timer." If we wait until after and the result is "do nothing," we've already lost the HTLC. Replace the unconditional unwrap on insert with explicit error handling. On persistence failure: - Forward-only: proceed. If the forward succeeds the store entry was redundant. If persistence failed and the channel becomes unusable between calculate and execute, the HTLC is orphaned (not in store for timer retry, not forwarded) until LDK's CLTV timeout cleans it up. Requires both S3 failure and a channel state change between two adjacent calls. - Liquidity action needed: fail the HTLC back to sender. The splice/open is async and the timer needs the stored HTLC to forward once the channel is ready. Without it, we'd spend on-chain fees on a splice that never results in a forward. --- lightning-liquidity/src/lsps4/service.rs | 46 +++++++++++++++++++++--- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/lightning-liquidity/src/lsps4/service.rs b/lightning-liquidity/src/lsps4/service.rs index 55c59497900..bd7f01c589d 100644 --- a/lightning-liquidity/src/lsps4/service.rs +++ b/lightning-liquidity/src/lsps4/service.rs @@ -239,15 +239,30 @@ where ); } + // Always persist before calculating actions. execute_htlc_actions + // removes the HTLC on successful forward. For the liquidity path + // (splice/open) the HTLC must survive in the store so the timer + // can forward it once the new channel is ready. + let persisted = match self.htlc_store.insert(htlc.clone()) { + Ok(_) => true, + Err(e) => { + log_error!( + self.logger, + "[LSPS4] htlc_intercepted: failed to persist HTLC {:?}, \ + payment_hash: {}, error: {}", + intercept_id, + payment_hash, + e + ); + false + } + }; + let actions = self.calculate_htlc_actions_for_peer( counterparty_node_id, - vec![htlc.clone()], + vec![htlc], ); - if actions.needs_liquidity_action() { - self.htlc_store.insert(htlc).unwrap(); - } - log_debug!( self.logger, "[LSPS4] htlc_intercepted: calculated actions for peer {}: {:?}", @@ -255,6 +270,27 @@ where actions ); + // Liquidity actions (splice/open) are async — the timer forwards + // the HTLC once the channel is ready. Without persistence the + // splice/open fires but the HTLC is never forwarded, wasting + // on-chain fees for a payment that times out anyway. + if !persisted && actions.needs_liquidity_action() { + log_error!( + self.logger, + "[LSPS4] htlc_intercepted: liquidity action needed but HTLC {:?} \ + not persisted, failing back to sender. payment_hash: {}", + intercept_id, + payment_hash + ); + let _ = self.channel_manager.get_cm().fail_intercepted_htlc(intercept_id); + return Ok(()); + } + + // Forward-only path is fine without persistence — the HTLC is + // consumed immediately by forward_intercepted_htlc. When persisted, + // the store entry also covers the TOCTOU race where the channel + // becomes unusable between calculate and execute; the timer retries + // from the store. self.execute_htlc_actions(actions, counterparty_node_id.clone()); } } else { From 0651fb0782954b0cb731d6f7491f470465e6f7f2 Mon Sep 17 00:00:00 2001 From: amackillop Date: Thu, 16 Apr 2026 21:28:37 +0100 Subject: [PATCH 2/2] Prevent spurious splice on reconnect When the LSP intercepts an HTLC for an offline peer, the peer reconnects and peer_connected fires before channel_reestablish completes. All channels report is_usable=false at this point. calculate_htlc_actions_for_peer builds its capacity map from is_usable channels (empty set), finds zero capacity, then falls through to the splice candidate filter. That filter used is_channel_ready, which matches mid-reestablish channels, so it emits a splice. Once reestablish finishes, the HTLC gets forwarded through the existing channel. The splice was wasted, and the 30s liquidity cooldown it sets blocks all subsequent liquidity actions for that peer. Two fixes in calculate_htlc_actions_for_peer: 1. Early return when channels exist but none are usable. The capacity map is empty so any decision would be wrong. The HTLC stays in the store (persisted by the prior commit) and the 1Hz timer retries once channels become usable. 2. Splice candidate filter changed from is_channel_ready to is_usable. A mid-reestablish channel maye already have sufficient capacity but it's just not visible yet. Splicing into it adds unnecessary on-chain cost and a 30s cooldown for capacity that was never actually insufficient. splice_channel() would also fail if the channel does become usable in time. --- lightning-liquidity/src/lsps4/service.rs | 30 ++++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/lightning-liquidity/src/lsps4/service.rs b/lightning-liquidity/src/lsps4/service.rs index bd7f01c589d..95c626f6020 100644 --- a/lightning-liquidity/src/lsps4/service.rs +++ b/lightning-liquidity/src/lsps4/service.rs @@ -706,6 +706,25 @@ where htlcs.len() ); + // Channels exist but none are usable (reestablish in progress). + // Return empty actions. We can't forward and we must not decide to splice or + // open a new channel based on stale capacity. The timer retries once usable. + if !channels.is_empty() && channel_capacity_map.is_empty() { + log_info!( + self.logger, + "[LSPS4] calculate_htlc_actions: {} has {} channels but none usable yet \ + - deferring decision", + their_node_id, + channels.len() + ); + return HtlcProcessingActions { + forwards: vec![], + new_channel_needed_msat: None, + splice_needed: None, + channel_count, + }; + } + struct ComputedHtlc { htlc: InterceptedHtlc, amount_to_forward_msat: u64, @@ -783,12 +802,13 @@ where .fold(required_amount, |acc, h| acc.saturating_add(h.amount_to_forward_msat)); // Prefer splicing into the largest usable channel over opening a new one. - // Use is_channel_ready (not is_usable) so we prefer splice even during - // channel_reestablish. splice_channel() will fail if the channel isn't - // usable yet, and the timer will retry once reestablishment completes. - let splice_candidate = channels + // Only splice into usable channels. A mid-reestablish channel may + // already have sufficient capacity that just isn't visible yet; + // splice_channel() would also reject if the channel does become + // usable in time. + let splice_candidate = channels .iter() - .filter(|c| c.is_channel_ready) + .filter(|c| c.is_usable) .max_by_key(|c| c.channel_value_satoshis); if let Some(candidate) = splice_candidate {