Fix async release before HTLC decode by valentinewallace · Pull Request #4548 · lightningdevkit/rust-lightning

valentinewallace · 2026-04-08T15:30:34Z

Handle ReleaseHeldHtlc messages that arrive before the sender-side LSP has even queued the held HTLC for onion decoding. Unlike #4106, which covers releases arriving after the HTLC is in decode_update_add_htlcs but before it reaches pending_intercepted_htlcs, this preserves releases that arrive one step earlier and would otherwise be dropped as HTLC not found.

Supersedes #4542

ldk-reviews-bot · 2026-04-08T15:30:38Z

👋 Thanks for assigning @TheBlueMatt 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.

lightning/src/ln/channelmanager.rs

ldk-claude-review-bot · 2026-04-08T15:58:28Z

After thoroughly reviewing every file and hunk in this PR, including tracing all race condition windows, lock ordering, and state machine transitions:

Review Summary

No new issues found beyond those already flagged in prior reviews.

Prior comment status update

lightning/src/ln/channel.rs:8157 — Resolved: the comment now correctly references Self::should_hold_inbound_htlc.
lightning/src/ln/channelmanager.rs:7504 — False positive in prior review: there is only one "arrived" in the text. No double word.
lightning/src/ln/channel.rs:8171 — Still applicable: "HTlC" should be "HTLC" (lowercase 'l' in doc comment).
lightning/src/ln/channelmanager.rs:7534 — Resolved: when should_hold_inbound_htlc returns false (due to either Released or Signaled clearing the channel flag), the code correctly falls through to else if intercept_forward at line 7534, preserving user-level interception behavior.

Architecture assessment

The three-stage release in release_pending_inbound_held_htlc correctly covers the race window:

monitor_pending_update_adds (clone exists after Committed promotion, pre-drain): clearing here affects the clone that enters decode_update_add_htlcs → Released.
Pre-Committed states (RemoteAnnounced/AwaitingRemoteRevokeToAnnounce/AwaitingAnnouncedRemoteRevoke): HTLC hasn't been cloned. Clearing affects the clone taken at promotion → Released.
Committed/WithOnion (post-clone): clone already taken. Channel's copy cleared as signal for should_hold_inbound_htlc → Signaled.

The should_hold_inbound_htlc callback at line 7508 correctly detects the Signaled case by re-checking the channel's copy under peer_state lock. Lock ordering (peer_state → decode_update_add_htlcs → pending_intercepted_htlcs) is consistent between handle_release_held_htlc and process_pending_update_add_htlcs. The test accurately reproduces the pre-promotion race.

tnull

Mostly looks good, CI is just failing due to some broken doc links AFAICT.

Also validated this still works in conjunction with #4463 / for lightningdevkit/ldk-node#817.

lightning/src/ln/channel.rs

tnull · 2026-04-09T09:19:16Z

lightning/src/ln/channel.rs

 	state: InboundHTLCState,
 }

+/// Result of [`FundedChannel::release_pending_inbound_held_htlc`].


Might be worth adding some more rationale why this is even necessary? I.e. describing the race this fixes?

Updated most of the docs, lmk your thoughts.

tnull · 2026-04-09T09:24:38Z

lightning/src/ln/channelmanager.rs

-										LocalHTLCFailureReason::TemporaryNodeFailure
-									);
-								},
+							// Check whether a ReleaseHeldHtlc arrived while the HTLC was in transit from channel


Also might be worth adding some more context here.

lightning/src/ln/channelmanager.rs

valentinewallace · 2026-04-09T18:11:12Z

Addressed feedback with diff (plus an extra push for a whitespace fix).

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

tnull

LGTM, just some doc nits.

Feel free to land as is though, assuming CI passes. (mod Claude's comments, maybe)

tnull · 2026-04-09T18:13:01Z

lightning/src/ln/channel.rs

+pub(super) enum HeldHtlcReleaseResult {
+	/// `hold_htlc` was cleared. The release of the HTLC is fully handled.
+	Released,
+	/// The ChannelManager has already pulled a copy of this HTLC out of the Channel for processing,


nit:

Suggested change

/// The ChannelManager has already pulled a copy of this HTLC out of the Channel for processing,

/// The `ChannelManager` has already pulled a copy of this HTLC out of the `Channel` for processing,

tnull · 2026-04-09T18:13:15Z

lightning/src/ln/channel.rs

+	Released,
+	/// The ChannelManager has already pulled a copy of this HTLC out of the Channel for processing,
+	/// but may not yet be persisting the HTLC in its internal state. This variant indicates that
+	/// we've cleared the `hold_htlc` flag in the Channel's cached version of the HTLC, which the


Suggested change

/// we've cleared the `hold_htlc` flag in the Channel's cached version of the HTLC, which the

/// we've cleared the `hold_htlc` flag in the `Channel`'s cached version of the HTLC, which the

tnull · 2026-04-09T18:13:25Z

lightning/src/ln/channel.rs

+	/// The ChannelManager has already pulled a copy of this HTLC out of the Channel for processing,
+	/// but may not yet be persisting the HTLC in its internal state. This variant indicates that
+	/// we've cleared the `hold_htlc` flag in the Channel's cached version of the HTLC, which the
+	/// manager will re-check when it goes to actually forward in process_pending_htlc_forwards.


Suggested change

/// manager will re-check when it goes to actually forward in process_pending_htlc_forwards.

/// manager will re-check when it goes to actually forward in `process_pending_htlc_forwards`.

tnull · 2026-04-09T18:14:00Z

lightning/src/ln/channel.rs

+		HeldHtlcReleaseResult::NotFound
+	}
+
+	/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of


Suggested change

/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of

/// Returns whether an inbound HTLC has the `hold_htlc` flag set. Useful if the `Channel`'s copy of

tnull · 2026-04-09T18:14:10Z

lightning/src/ln/channel.rs

+	}
+
+	/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of
+	/// an HTLC was updated to be released but the ChannelManager's copy of the HTLC wasn't.


Suggested change

/// an HTLC was updated to be released but the ChannelManager's copy of the HTLC wasn't.

/// an HTLC was updated to be released but the `ChannelManager`'s copy of the HTLC wasn't.

Handle `ReleaseHeldHtlc` messages that arrive before the sender-side LSP has even queued the held HTLC for onion decoding. Unlike lightningdevkit#4106, which covers releases arriving after the HTLC is in `decode_update_add_htlcs` but before it reaches `pending_intercepted_htlcs`, this preserves releases that arrive one step earlier and would otherwise be dropped as HTLC not found. Co-Authored-By: HAL 9000 Co-Authored-By: Elias Rohrer <dev@tnull.de>

ldk-claude-review-bot · 2026-04-09T18:22:01Z

lightning/src/ln/channel.rs

+		HeldHtlcReleaseResult::NotFound
+	}
+
+	/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of


Nit: "HTlC" → "HTLC" (lowercase 'l').

Suggested change

/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of

/// Returns whether an inbound HTLC has the `hold_htlc` flag set. Useful if the Channel's copy of

codecov · 2026-04-09T19:37:17Z

Codecov Report

❌ Patch coverage is 76.19048% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.00%. Comparing base (5704e8e) to head (f764872).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	60.00%	13 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	96.42%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4548   +/-   ##
=======================================
  Coverage   86.99%   87.00%           
=======================================
  Files         163      163           
  Lines      108635   108720   +85     
  Branches   108635   108720   +85     
=======================================
+ Hits        94511    94587   +76     
- Misses      11647    11650    +3     
- Partials     2477     2483    +6

Flag	Coverage Δ
fuzzing	`40.17% <4.76%> (-0.14%)`	⬇️
tests	`86.09% <76.19%> (+<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.

TheBlueMatt · 2026-04-09T20:40:10Z

lightning/src/ln/channelmanager.rs

 						);
-						if pending_add.forward_info.routing.should_hold_htlc() {
+						let hold_htlc = if pending_add.forward_info.routing.should_hold_htlc() {
+							// Check whether a ReleaseHeldHtlc arrived after the manager pulled a copy of this


This feel like it should be fixed by taking an additional lock in handle_release_held_htlc rather than checking for it here?

I don't believe so -- did you see this race condition? #4542 (comment) Maybe we don't support that kind of multithreaded usage anyway?

Btw, if we try to acquire the decode_update_add_htlcs lock prior to taking the channel lock, which I think would fix a different race you may be pointing out here, it currently results in a lock order violation.

valentinewallace added this to the 0.3 milestone Apr 8, 2026

valentinewallace self-assigned this Apr 8, 2026

valentinewallace added this to Weekly Goals Apr 8, 2026

valentinewallace added the weekly goal Someone wants to land this this week label Apr 8, 2026

valentinewallace mentioned this pull request Apr 8, 2026

Fix async release before HTLC decode #4542

Closed

valentinewallace requested a review from tnull April 8, 2026 15:32

ldk-claude-review-bot reviewed Apr 8, 2026

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

tnull reviewed Apr 9, 2026

View reviewed changes

valentinewallace force-pushed the 2026-04-async-held-htlc-ordering-v2 branch 2 times, most recently from ae776b9 to bd632fb Compare April 9, 2026 18:10

ldk-claude-review-bot reviewed Apr 9, 2026

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

ldk-claude-review-bot reviewed Apr 9, 2026

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

tnull previously approved these changes Apr 9, 2026

View reviewed changes

valentinewallace dismissed tnull’s stale review via f764872 April 9, 2026 18:15

valentinewallace force-pushed the 2026-04-async-held-htlc-ordering-v2 branch from bd632fb to f764872 Compare April 9, 2026 18:15

tnull requested a review from TheBlueMatt April 9, 2026 18:18

ldk-claude-review-bot reviewed Apr 9, 2026

View reviewed changes

TheBlueMatt reviewed Apr 9, 2026

View reviewed changes

	/// The ChannelManager has already pulled a copy of this HTLC out of the Channel for processing,
	/// The `ChannelManager` has already pulled a copy of this HTLC out of the `Channel` for processing,

	/// we've cleared the `hold_htlc` flag in the Channel's cached version of the HTLC, which the
	/// we've cleared the `hold_htlc` flag in the `Channel`'s cached version of the HTLC, which the

	/// manager will re-check when it goes to actually forward in process_pending_htlc_forwards.
	/// manager will re-check when it goes to actually forward in `process_pending_htlc_forwards`.

	/// Returns whether an inbound HTlC has the `hold_htlc` flag set. Useful if the Channel's copy of
	/// Returns whether an inbound HTLC has the `hold_htlc` flag set. Useful if the `Channel`'s copy of

	/// an HTLC was updated to be released but the ChannelManager's copy of the HTLC wasn't.
	/// an HTLC was updated to be released but the `ChannelManager`'s copy of the HTLC wasn't.

Conversation

valentinewallace commented Apr 8, 2026

Uh oh!

ldk-reviews-bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ldk-claude-review-bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Summary

Prior comment status update

Architecture assessment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace commented Apr 9, 2026

Uh oh!

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Apr 8, 2026 •

edited

Loading

ldk-claude-review-bot commented Apr 8, 2026 •

edited

Loading

tnull left a comment •

edited

Loading

codecov bot commented Apr 9, 2026 •

edited

Loading

valentinewallace Apr 9, 2026 •

edited

Loading