Avoid duplicate on-chain HTLC claims after replay#4583
Open
joostjager wants to merge 7 commits intolightningdevkit:mainfrom
Open
Avoid duplicate on-chain HTLC claims after replay#4583joostjager wants to merge 7 commits intolightningdevkit:mainfrom
joostjager wants to merge 7 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @valentinewallace was un-assigned. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4583 +/- ##
==========================================
+ Coverage 87.15% 87.20% +0.04%
==========================================
Files 161 161
Lines 109251 109518 +267
Branches 109251 109518 +267
==========================================
+ Hits 95215 95501 +286
+ Misses 11560 11541 -19
Partials 2476 2476
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
force-pushed
the
onchain-claim-replay-fixes
branch
from
dca8cfb to
531576d
Compare
Apply rustfmt to the chanmon consistency fuzz target.
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.
Move repeated OnchainTxHandler setup into shared test helpers so the claim-replay coverage can focus on the behavior under test.
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 while it awaits anti-reorg confirmation. Reject replayed claim requests for those pending-spent outpoints so they are not added back before the spend either matures 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 when the commitment output already has a confirmed spend tracked by the ChannelMonitor. This keeps replayed preimage updates from recreating claims after OnchainTxHandler has cleaned up its anti-reorg tracking, without adding a new persisted outpoint tombstone set.
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
531576d to
9e0f886
Compare
joostjager
changed the title
Collaborator
|
OK, that's fine. The test uses the correct API. I've completed my thorough review. The PR is well-structured and the changes are logically sound. I found no significant bugs or security issues. The changes are:
The overall approach of defense-in-depth (monitor-level filtering + handler-level filtering) is sound. No issues found. |
TheBlueMatt
reviewed
May 1, 2026
| requests.retain(|req| { | ||
| let outpoint = req.outpoint(); | ||
| if self.claimable_outpoints.get(outpoint).is_some() { | ||
| if self.is_outpoint_spend_waiting_threshold_conf(outpoint) { |
Collaborator
There was a problem hiding this comment.
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?
|
|
||
| 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 |
Collaborator
There was a problem hiding this comment.
So why exactly do we need the duplicate?
| // 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 { |
Collaborator
There was a problem hiding this comment.
Same issue as the previous commit - does this break broadcasting our conflicting claim on reorg?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4572