Detect and fail-back monitor-blocked un-forwarded HTLCs at close
What changed, and why it matters
This commit fixes a bug in the Lightning Dev Kit where certain pending forwarded payments could get stuck and not be returned to the sender if a channel closed at exactly the wrong moment. Normally, stuck payments are automatically failed back, but a specific edge case involving a delayed channel-monitor update caused the software to lose track of them. The fix detects these stranded payments during channel closure and immediately fails them back so funds are not left in limbo.
Review the new closure-time HTLC scan for correctness and completeness, ensure the regression test covers both monitor-update step variants, and consider whether any other blocked-update states could strand HTLCs similarly.
Security signals we found
Funds-availability / stuck-HTLC bug fixed
Channel closure edge case with incomplete monitor update
Missing fail-back of un-forwarded HTLCs
Regression test added for the exact failure path
Evidence from the diff
When an outbound HTLC is in OutboundHTLCState::LocalAnnounced (the counterparty has not yet sent a revoke_and_ack) and its forwarding is blocked because a ChannelMonitorUpdate containing the latest counterparty commitment has not completed, the ChannelMonitor is unaware of the HTLC. If the channel closes in this state, the existing catch-all fail-back logic (which is monitor-driven) cannot act, so the HTLC was not failed back. The patch adds a scan in channel.rs over pending outbound HTLCs and blocked monitor updates; when it finds an HTLC referenced in a LatestCounterpartyCommitment or LatestCounterpartyCommitmentTXInfo update step, it pushes the HTLC into dropped_outbound_htlcs so it is failed back immediately on close. A regression test simulates the exact monitor-delay scenario and asserts that the HTLC is failed back to the upstream node.
Changed components
lightning/src/ln/channel.rslightning/src/ln/shutdown_tests.rsInspect captured patch +154 / −1
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0796369..115b917 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5340,6 +5340,50 @@ where
_ => {},
}
}
+
+ // Once we're closed, the `ChannelMonitor` is responsible for resolving any remaining
+ // HTLCs. However, in the specific case of us pushing new HTLC(s) to the counterparty in
+ // the latest commitment transaction that we haven't actually sent due to a block
+ // `ChannelMonitorUpdate`, we may have some HTLCs that the `ChannelMonitor` won't know
+ // about and thus really need to be included in `dropped_outbound_htlcs`.
+ 'htlc_iter: for htlc in self.pending_outbound_htlcs.iter() {
+ if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
+ for update in self.blocked_monitor_updates.iter() {
+ for update in update.update.updates.iter() {
+ let have_htlc = match update {
+ ChannelMonitorUpdateStep::LatestCounterpartyCommitment {
+ htlc_data,
+ ..
+ } => {
+ let dust =
+ htlc_data.dust_htlcs.iter().map(|(_, source)| source.as_ref());
+ let nondust =
+ htlc_data.nondust_htlc_sources.iter().map(|s| Some(s));
+ dust.chain(nondust).any(|source| source == Some(&htlc.source))
+ },
+ ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
+ htlc_outputs,
+ ..
+ } => htlc_outputs.iter().any(|(_, source)| {
+ source.as_ref().map(|s| &**s) == Some(&htlc.source)
+ }),
+ _ => continue,
+ };
+ debug_assert!(have_htlc);
+ if have_htlc {
+ dropped_outbound_htlcs.push((
+ htlc.source.clone(),
+ htlc.payment_hash,
+ counterparty_node_id,
+ self.channel_id,
+ ));
+ }
+ continue 'htlc_iter;
+ }
+ }
+ }
+ }
+
let monitor_update = if let Some(funding_txo) = funding.get_funding_txo() {
// If we haven't yet exchanged funding signatures (ie channel_state < AwaitingChannelReady),
// returning a channel monitor update here would imply a channel monitor update before
diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs
index 6e0845d..dc89981 100644
--- a/lightning/src/ln/shutdown_tests.rs
+++ b/lightning/src/ln/shutdown_tests.rs
@@ -12,7 +12,7 @@
use crate::chain::transaction::OutPoint;
use crate::chain::ChannelMonitorUpdateStatus;
-use crate::events::{ClosureReason, Event, HTLCHandlingFailureType};
+use crate::events::{ClosureReason, Event, HTLCHandlingFailureReason, HTLCHandlingFailureType};
use crate::ln::channel_state::{ChannelDetails, ChannelShutdownState};
use crate::ln::channelmanager::{self, PaymentId, RecipientOnionFields, Retry};
use crate::ln::msgs;
@@ -1825,3 +1825,112 @@ fn test_force_closure_on_low_stale_fee() {
};
check_closed_events(&nodes[1], &[ExpectedCloseEvent::from_id_reason(chan_id, false, reason)]);
}
+
+#[test]
+fn test_pending_htlcs_arent_lost_on_mon_delay() {
+ // Test that HTLCs which were queued to be sent to peers but which never made it out due to a
+ // pending, not-completed `ChannelMonitorUpdate` which got dropped with the `Channel`. This is
+ // only possible when the `ChannelMonitorUpdate` is blocked, as otherwise it will be queued in
+ // the `ChannelManager` and go out before any closure update.
+ let chanmon_cfgs = create_chanmon_cfgs(3);
+ let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+
+ let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+ let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+ let node_a_id = nodes[0].node.get_our_node_id();
+ let node_b_id = nodes[1].node.get_our_node_id();
+ let node_c_id = nodes[2].node.get_our_node_id();
+
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+ let (_, _, chan_id_bc, ..) = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+ // First route a payment from node B to C, which will allow us to block `ChannelMonitorUpdate`s
+ // by not processing the `PaymentSent` event upon claim.
+ let (preimage_a, payment_hash_a, ..) = route_payment(&nodes[1], &[&nodes[2]], 500_000);
+
+ nodes[2].node.claim_funds(preimage_a);
+ check_added_monitors(&nodes[2], 1);
+ expect_payment_claimed!(nodes[2], payment_hash_a, 500_000);
+
+ let mut claim = get_htlc_update_msgs(&nodes[2], &node_b_id);
+ nodes[1].node.handle_update_fulfill_htlc(node_c_id, claim.update_fulfill_htlcs.pop().unwrap());
+
+ // Now, while sitting on the `PaymentSent` event, move the B <-> C channel forward until B is
+ // just waiting on C's last RAA.
+ nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &claim.commitment_signed);
+ check_added_monitors(&nodes[1], 1);
+
+ let (raa, cs) = get_revoke_commit_msgs(&nodes[1], &node_c_id);
+
+ nodes[2].node.handle_revoke_and_ack(node_b_id, &raa);
+ check_added_monitors(&nodes[2], 1);
+ nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &cs);
+ check_added_monitors(&nodes[2], 1);
+
+ let cs_last_raa = get_event_msg!(nodes[2], MessageSendEvent::SendRevokeAndACK, node_b_id);
+
+ // Now, while still sitting on the `PaymentSent` event, send an HTLC which will be relayed the
+ // moment `cs_last_raa` is received by B.
+ let (route_b, payment_hash_b, _preimage, payment_secret_b) =
+ get_route_and_payment_hash!(&nodes[0], nodes[2], 900_000);
+ let onion = RecipientOnionFields::secret_only(payment_secret_b);
+ let id = PaymentId(payment_hash_b.0);
+ nodes[0].node.send_payment_with_route(route_b, payment_hash_b, onion, id).unwrap();
+ check_added_monitors(&nodes[0], 1);
+ let as_send = get_htlc_update_msgs(&nodes[0], &node_b_id);
+
+ nodes[1].node.handle_update_add_htlc(node_a_id, &as_send.update_add_htlcs[0]);
+ commitment_signed_dance!(nodes[1], nodes[0], as_send.commitment_signed, false);
+
+ // Place the HTLC in the B <-> C channel holding cell for release upon RAA and finally deliver
+ // `cs_last_raa`. Because we're still waiting to handle the `PaymentSent` event, the
+ // `ChannelMonitorUpdate` and update messages will be held.
+ nodes[1].node.process_pending_htlc_forwards();
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+ check_added_monitors(&nodes[1], 0);
+
+ nodes[1].node.handle_revoke_and_ack(node_c_id, &cs_last_raa);
+ check_added_monitors(&nodes[1], 0);
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+ // Now force-close the B <-> C channel, making sure that we (finally) see the `PaymentSent`, as
+ // well as the channel closure and, importantly, the HTLC fail-back to A.
+ let message = "".to_string();
+ nodes[1].node.force_close_broadcasting_latest_txn(&chan_id_bc, &node_c_id, message).unwrap();
+ check_closed_broadcast(&nodes[1], 1, true);
+ check_added_monitors(&nodes[1], 1);
+
+ let events = nodes[1].node.get_and_clear_pending_events();
+ assert_eq!(events.len(), 3, "{events:?}");
+ assert!(events.iter().any(|ev| {
+ if let Event::PaymentSent { payment_preimage: ev_preimage, .. } = ev {
+ assert_eq!(*ev_preimage, preimage_a);
+ true
+ } else {
+ false
+ }
+ }));
+ assert!(events.iter().any(|ev| matches!(ev, Event::ChannelClosed { .. })));
+ assert!(events.iter().any(|ev| {
+ if let Event::HTLCHandlingFailed { failure_type, failure_reason, .. } = ev {
+ assert!(matches!(failure_type, HTLCHandlingFailureType::Forward { .. }));
+ if let Some(HTLCHandlingFailureReason::Local { reason }) = failure_reason {
+ assert_eq!(*reason, LocalHTLCFailureReason::ChannelClosed);
+ } else {
+ panic!("Unexpected failure reason {failure_reason:?}");
+ }
+ true
+ } else {
+ false
+ }
+ }));
+
+ nodes[1].node.process_pending_htlc_forwards();
+ check_added_monitors(&nodes[1], 1);
+
+ let failures = get_htlc_update_msgs(&nodes[1], &node_a_id);
+ nodes[0].node.handle_update_fail_htlc(node_b_id, &failures.update_fail_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[1], failures.commitment_signed, false);
+ expect_payment_failed!(nodes[0], payment_hash_b, false);
+}
Why this scored 66/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.