Avoid duplicate on-chain HTLC claims after replay#4583
Avoid duplicate on-chain HTLC claims after replay#4583joostjager 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.16% +0.01%
==========================================
Files 161 161
Lines 109251 109512 +261
Branches 109251 109512 +261
==========================================
+ Hits 95215 95459 +244
- Misses 11560 11575 +15
- Partials 2476 2478 +2
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.
joostjager
force-pushed
the
onchain-claim-replay-fixes
branch
from
531576d to
9e0f886
Compare
joostjager
changed the title
|
I've completed a thorough review of the entire PR diff, examining all files and hunks. I also leveraged my prior architectural knowledge of the codebase and ran multiple exploration agents to deeply analyze the key logic changes. Key areas verified:
No issues found. |
| requests.retain(|req| { | ||
| let outpoint = req.outpoint(); | ||
| if self.claimable_outpoints.get(outpoint).is_some() { | ||
| if self.is_outpoint_spend_waiting_threshold_conf(outpoint) { |
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?
There was a problem hiding this comment.
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?
|
|
||
| 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 |
There was a problem hiding this comment.
So why exactly do we need the duplicate?
There was a problem hiding this comment.
It stems from OnchainTxHandler not persisting state
| // 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 { |
There was a problem hiding this comment.
Same issue as the previous commit - does this break broadcasting our conflicting claim on reorg?
There was a problem hiding this comment.
Hm, here indeed a claim is never submitted and also cannot be resurrected.
There was a problem hiding this comment.
Now only checking for deeply confirmed outpoints, but need to look at the consequences of that.
Filter regenerated HTLC claim requests only after ChannelMonitor has persisted final HTLC resolution for the commitment output. This keeps replayed preimage updates from recreating claims once the monitor has durable resolution state, while preserving live conflicting claims before final resolution so they can be retried if a counterparty spend reorgs out.
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
9e0f886 to
37d6b77
Compare
4 participants
Fixes #4572