Skip to content

Conversation

TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented Aug 5, 2025

If we have pending HTLCs which we intended to forward, but which were waiting on a ChannelMonitorUpdate to be forwarded when we closed, they will neither be in the ChannelMonitor nor in the Channel in a state which indicates they need to be failed (i.e. in the holding cell). As a result, we previously did not fail such HTLCs back. Note that the catch-all
fail-back-before-channel-closure logic is run by the ChannelMonitor so is also unaware of these HTLCs.

Here we fix this by detecting the specific case - HTLCs which are in LocalSent (i.e. the counterparty has not provided an RAA yet) and we have a blocked ChannelMonitorUpdate containing a remote commitment transaction update (which will always contain the HTLC).

In such a case, we can be confident the counterparty does not have a commitment transaction containing the HTLC, and can fail it back immediately.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Aug 5, 2025

I've assigned @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@TheBlueMatt TheBlueMatt mentioned this pull request Aug 4, 2025
24 tasks
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

❌ Patch coverage is 88.42105% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.98%. Comparing base (e01663a) to head (7c39480).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 74.07% 6 Missing and 1 partial ⚠️
lightning/src/ln/shutdown_tests.rs 94.11% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3989      +/-   ##
==========================================
+ Coverage   88.97%   88.98%   +0.01%     
==========================================
  Files         174      174              
  Lines      124161   124394     +233     
  Branches   124161   124394     +233     
==========================================
+ Hits       110470   110693     +223     
- Misses      11216    11219       +3     
- Partials     2475     2482       +7     
Flag Coverage Δ
fuzzing 22.22% <7.40%> (-0.43%) ⬇️
tests 88.81% <88.42%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@tankyleo tankyleo left a comment

Choose a reason for hiding this comment

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

Quick first pass

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@TheBlueMatt TheBlueMatt force-pushed the 2023-10-pending-htlcs-lost-on-mon-delay branch from 54fff98 to 34abcf6 Compare August 5, 2025 20:33
If we have pending HTLCs which we intended to forward, but which
were waiting on a `ChannelMonitorUpdate` to be forwarded when we
closed, they will neither be in the `ChannelMonitor` nor in the
`Channel` in a state which indicates they need to be failed (i.e.
in the holding cell). As a result, we previously did not fail such
HTLCs back immediately. Note that we cannot rely on the catch-all
fail-back-before-channel-closure logic either as it is done by the
`ChannelMonitor` that is unaware of these HTLCs.

Here we fix this by detecting the specific case - HTLCs which are
in `LocalSent` (i.e. the counterparty has not provided an RAA yet)
and we have a blocked `ChannelMonitorUpdate` containing a remote
commitment transaction update (which will always contain the HTLC).

In such a case, we can be confident the counterparty does not have
a commitment transaction containing the HTLC, and can fail it back
immediately.
@TheBlueMatt TheBlueMatt force-pushed the 2023-10-pending-htlcs-lost-on-mon-delay branch from 34abcf6 to 7c39480 Compare August 5, 2025 21:03
@TheBlueMatt
Copy link
Collaborator Author

Fixed handling the missing monitor update type:

$ git diff-tree -U2 34abcf6603^1 7c394804b0
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 5a95295f8f..115b917649 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5351,25 +5351,33 @@ where
 				for update in self.blocked_monitor_updates.iter() {
 					for update in update.update.updates.iter() {
-						match update {
-							ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
-								htlc_outputs,
+						let have_htlc = match update {
+							ChannelMonitorUpdateStep::LatestCounterpartyCommitment {
+								htlc_data,
 								..
 							} => {
-								let have_htlc = htlc_outputs.iter().any(|(_, source)| {
-									source.as_ref().map(|s| &**s) == Some(&htlc.source)
-								});
-								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 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;
 					}
 				}

@TheBlueMatt TheBlueMatt merged commit 0a23664 into lightningdevkit:main Aug 6, 2025
23 of 24 checks passed
@TheBlueMatt TheBlueMatt self-assigned this Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants