Avoid duplicate on-chain HTLC claims after replay by joostjager · Pull Request #4583 · lightningdevkit/rust-lightning

joostjager · 2026-04-30T13:57:15Z

ldk-reviews-bot · 2026-04-30T13:57:18Z

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignment, please click here.

codecov · 2026-04-30T15:13:17Z

Codecov Report

❌ Patch coverage is 99.10714% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.18%. Comparing base (0c7e6e7) to head (3196617).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	94.87%	2 Missing ⚠️
lightning/src/chain/onchaintx.rs	99.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4583      +/-   ##
==========================================
+ Coverage   87.16%   87.18%   +0.02%     
==========================================
  Files         161      161              
  Lines      109251   109520     +269     
  Branches   109251   109520     +269     
==========================================
+ Hits        95230    95487     +257     
- Misses      11547    11558      +11     
- Partials     2474     2475       +1

Flag	Coverage Δ
fuzzing-fake-hashes	`31.22% <51.61%> (+0.07%)`	⬆️
fuzzing-real-hashes	`22.91% <0.00%> (-0.02%)`	⬇️
tests	`86.25% <99.10%> (+0.02%)`	⬆️

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.

ldk-claude-review-bot · 2026-05-01T11:38:46Z

I've completed a thorough review of every hunk in this PR, tracing through the following key areas:

is_htlc_output_resolved_on_chain and all 4 call sites — correct suppression of duplicate claims
HTLCSpendConfirmation removal from confirmation_threshold CSV branch — correctly separates HTLC spend finality from delayed output maturity
Three-level dedup in update_claims_view_from_requests — contentious outpoints, claimable_outpoints, and locktimed_packages all work correctly together
ClaimId::from_htlcs canonicalization — sorting is correct; no backward compatibility issue since ClaimId is loaded from persistence, never recomputed for existing claims
htlcs_resolved_on_chain append-only behavior during reorgs — consistent with existing ANTI_REORG_DELAY security model
Balance reporting — MaturingOutput (with CSV threshold) is always co-created with HTLCSpendConfirmation, so the balance correctly shows through the gap between anti-reorg finality and CSV maturity
blocks_disconnected resurrection path — contentious outpoints correctly return to locktimed_packages when locktime > new_best_height
All new tests — correctly exercise the intended scenarios

No issues found.

TheBlueMatt · 2026-05-01T13:17:51Z

 		requests.retain(|req| {
 			let outpoint = req.outpoint();
-			if self.claimable_outpoints.get(outpoint).is_some() {
+			if self.is_outpoint_spend_waiting_threshold_conf(outpoint) {


Does this break for reorgs where a counterparty's transaction gets unconfirmed? ie if we have a event awaiting confirmations for the counterparty claiming an outpoint, but then that gets reorg'd out and not replayed, do we still manage to broadcast our own claim (and RBF it)?

I assume that this case is only reachable if we receive a preimage after a counterparty's timeout claim is confirming?

I think on a reorg the item in onchain_events_awaiting_threshold_conf is revived again? Basically the claim is passed around to different data structures, but never lost?

TheBlueMatt · 2026-05-01T13:23:04Z


+	fn is_htlc_output_spent_on_chain(&self, htlc: &HTLCOutputInCommitment) -> bool {
+		if let Some(transaction_output_index) = htlc.transaction_output_index {
+			// This is a monitor-level HTLC generation filter. OnchainTxHandler


So why exactly do we need the duplicate?

It stems from OnchainTxHandler not persisting state

The duplication covers a restart/replay gap after finality.

OnchainTxHandler persists active claim state, but drops it once the claim reaches anti-reorg finality. It does not keep an ever-growing resolved-outpoint tombstone set.

After that cleanup, a replayed preimage update can look like a fresh claim request to OnchainTxHandler. ChannelMonitor does persist final HTLC resolution in htlcs_resolved_on_chain, so this filter uses that state to suppress only already-resolved HTLC outpoints.

TheBlueMatt · 2026-05-01T13:23:21Z

+			// still guards package state for outpoints split out by confirmed
+			// spends; here we avoid recreating HTLC claim requests once the
+			// monitor has observed resolution.
+			self.onchain_events_awaiting_threshold_conf.iter().any(|entry| match entry.event {


Same issue as the previous commit - does this break broadcasting our conflicting claim on reorg?

Hm, here indeed a claim is never submitted and also cannot be resurrected.

Now only checking for deeply confirmed outpoints, but need to look at the consequences of that.

Yea, I would imagine any issues we have with deeply-confirmed would apply to less-confirmed as well, only more likely.

I've added resolve HTLC spends at anti-reorg finality. Previously, there was a gap between anti-reorg finality and CSV maturity where the monitor could resubmit a claim to the onchain handler. The commit closes that by adding HTLC spends to htlcs_resolved_on_chain at anti-reorg finality.

That should not affect balance reporting, since any CSV-delayed output remains tracked separately. Before anti-reorg finality, the claim still goes to the onchain handler, so its reorg handling stays intact. After anti-reorg finality, replayed claims are suppressed by the monitor.

Let me know what you think.

Have ChannelMonitor hand singular ClaimRequests to OnchainTxHandler. Convert them to PackageTemplates only after duplicate filtering. This makes the single-outpoint invariant explicit at that boundary.

Clarify ChannelMonitor comments around on-chain event thresholds. Some events only wait for anti-reorg finality, while CSV-delayed outputs wait until spendable through the same threshold queue.

Move repeated OnchainTxHandler setup into shared test helpers so the claim-replay coverage can focus on the behavior under test.

Add a monitor test for an inbound HTLC claimed by preimage from a holder commitment. Confirm that the claimable balance remains unchanged after the HTLC-success spend reaches anti-reorg finality but before the CSV-delayed output is spendable.

Treat HTLCSpendConfirmation entries as irrevocably resolved once the commitment HTLC output spend reaches anti-reorg finality. Do not wait for CSV maturity of any delayed output created by that spend. Delayed outputs remain tracked separately as MaturingOutput entries, keeping claimable balances alive until they are CSV-mature and can be surfaced as SpendableOutputs.

A replayed holder HTLC claim may arrive as a single-outpoint request after earlier requests were merged into a delayed package. Check whether an existing delayed package already covers the new request instead of requiring exact outpoint-set equality. Add focused OnchainTxHandler coverage and a ChannelMonitor regression through claim_funds for both current anchor variants.

When a transaction spends one outpoint from a delayed package, the split outpoint is tracked as a ContentiousOutpoint until the spend reaches anti-reorg finality. Reject replayed claim requests for those pending-spent outpoints so they are not added back before the spend reaches anti-reorg finality or reorgs out. Add an OnchainTxHandler regression that replays a holder claim during that pending-spent window and verifies reorg resurrection still works.

Filter regenerated HTLC claim requests once ChannelMonitor has persisted anti-reorg finality for the commitment HTLC output spend. This keeps replayed preimage updates from recreating claims after OnchainTxHandler has cleaned up its active retry state, relying on the monitor's persisted HTLC resolution state.

Hash HTLC claim outpoints in canonical order so the same logical HTLC set produces the same ClaimId regardless of descriptor order. Add a unit test covering reversed descriptor order.

joostjager self-assigned this Apr 30, 2026

joostjager added this to Weekly Goals Apr 30, 2026

joostjager force-pushed the onchain-claim-replay-fixes branch 2 times, most recently from 531576d to 9e0f886 Compare May 1, 2026 11:06

joostjager marked this pull request as ready for review May 1, 2026 11:22

ldk-reviews-bot requested a review from valentinewallace May 1, 2026 11:23

joostjager changed the title ~~Onchain claim replay fixes~~ Avoid duplicate on-chain HTLC claims after replay May 1, 2026

joostjager removed the request for review from valentinewallace May 1, 2026 11:24

TheBlueMatt reviewed May 1, 2026

View reviewed changes

joostjager force-pushed the onchain-claim-replay-fixes branch 2 times, most recently from 37d6b77 to 1c3b725 Compare May 1, 2026 18:26

joostjager added 5 commits May 4, 2026 09:36

lightning: introduce singular claim requests

8e57ada

Have ChannelMonitor hand singular ClaimRequests to OnchainTxHandler. Convert them to PackageTemplates only after duplicate filtering. This makes the single-outpoint invariant explicit at that boundary.

lightning: clarify channelmonitor event thresholds

c8ce370

Clarify ChannelMonitor comments around on-chain event thresholds. Some events only wait for anti-reorg finality, while CSV-delayed outputs wait until spendable through the same threshold queue.

lightning: refactor onchain tx handler tests

ac00ab6

Move repeated OnchainTxHandler setup into shared test helpers so the claim-replay coverage can focus on the behavior under test.

joostjager force-pushed the onchain-claim-replay-fixes branch from 1c3b725 to a3fbe2d Compare May 4, 2026 10:14

joostjager added 4 commits May 4, 2026 12:32

lightning: canonicalize htlc claim ids

3196617

Hash HTLC claim outpoints in canonical order so the same logical HTLC set produces the same ClaimId regardless of descriptor order. Add a unit test covering reversed descriptor order.

joostjager force-pushed the onchain-claim-replay-fixes branch from a3fbe2d to 3196617 Compare May 4, 2026 10:44

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

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!

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!

joostjager May 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joostjager commented Apr 30, 2026 •

edited

Loading

ldk-reviews-bot commented Apr 30, 2026 •

edited

Loading

codecov Bot commented Apr 30, 2026 •

edited

Loading

ldk-claude-review-bot commented May 1, 2026 •

edited

Loading

joostjager May 4, 2026 •

edited

Loading