Avoid duplicate on-chain HTLC claims after replay#4583
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4583 +/- ##
==========================================
- Coverage 87.16% 86.22% -0.95%
==========================================
Files 161 156 -5
Lines 109251 108965 -286
Branches 109251 108965 -286
==========================================
- Hits 95230 93950 -1280
- Misses 11547 12402 +855
- Partials 2474 2613 +139
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
2 times, most recently
from
531576d to
9e0f886
Compare
joostjager
changed the title
|
I've re-reviewed the entire diff, focusing on areas not covered in my prior pass. I traced the index-only matching in No issues found. No new line-specific issues to report. The dedup layering (ChannelMonitor One non-blocking observation (not a bug, no inline comment): |
| 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
There was a problem hiding this comment.
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.
| // 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.
There was a problem hiding this comment.
Yea, I would imagine any issues we have with deeply-confirmed would apply to less-confirmed as well, only more likely.
There was a problem hiding this comment.
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.
joostjager
force-pushed
the
onchain-claim-replay-fixes
branch
2 times, most recently
from
37d6b77 to
1c3b725
Compare
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.
joostjager
force-pushed
the
onchain-claim-replay-fixes
branch
2 times, most recently
from
a3fbe2d to
3196617
Compare
| OnchainEvent::HTLCSpendConfirmation { on_to_local_output_csv: Some(csv), .. } => { | ||
| // A CSV'd transaction is confirmable in block (input height) + CSV delay, which means | ||
| // it's broadcastable when we see the previous block. | ||
| OnchainEvent::FundingSpendConfirmation { on_local_output_csv: Some(csv), .. } => { |
There was a problem hiding this comment.
Shouldn't the same apply to FundingSpendConfirmation? Maybe its worth asserting this exists when pushing HTLCSpendConfirmations (might have to move self.check_tx_and_push_spendable_outputs above self.is_resolving_htlc_output)?
There was a problem hiding this comment.
I considered making the change for FundingSpendConfirmation too, but it breaks more things. Balance calc needs to be updated, but also the changed behavior permeates up and leads to problems. I don't think it is needed functionally because there's no preimage claim coming in that triggers a sweep?
I did add a fixup for the assert you describe, that indeed also swaps those two calls).
There was a problem hiding this comment.
Hmmmmm, it would be really nice if we made this behavior consistent, though? Its strange that we are now so inconsistent. Also, did you check through git history to find out why we do this? Will this implicitly break downgrade to some (potentially-ancient) version?
There was a problem hiding this comment.
I think FundingSpendConfirmation got CSV-delayed because it was used to drive claimable balance visibility for the holder commitment to-self output. HTLCSpendConfirmation later copied that same threshold behavior, but it probably didn't need to because the delayed HTLC-success output is already tracked separately as a MaturingOutput, which this PR makes use of.
If starting from scratch, it seems better to be more clear about the distinction between confirmed and spendable. But for this PR, I wouldn't want to extend the scope to include that.
For downgrades, I think get_htlc_balance will keep working because it gives precedence to the MaturingOutput event.
| if htlc.offered && htlc.payment_hash == matching_payment_hash { | ||
| if htlc.offered | ||
| && htlc.payment_hash == matching_payment_hash | ||
| && !self.is_htlc_output_resolved_on_chain(htlc) |
There was a problem hiding this comment.
Should we at least log a huge "we lost money" error here (same elsewhere)?
There was a problem hiding this comment.
Added logging fixup, but not completely certain it's now in the exact right places.
| } | ||
|
|
||
| fn is_outpoint_spend_waiting_threshold_conf(&self, outpoint: &BitcoinOutPoint) -> bool { | ||
| self.onchain_events_awaiting_threshold_conf.iter().any(|entry| { |
There was a problem hiding this comment.
Should this look at the timelocked set too to simplify the callsite?
There was a problem hiding this comment.
Added fixup that refactors to outpoint_claim_state helper
|
👋 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. |
Check that any HTLCSpendConfirmation carrying a local-output CSV has a matching delayed MaturingOutput. Scan spendable outputs before recording HTLC spend confirmations so the invariant is present when the assertion runs.
joostjager
force-pushed
the
onchain-claim-replay-fixes
branch
from
3196617 to
f73f75c
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
Waiting for a response on #4583 (comment) and @wpaulino's review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @wpaulino! This PR has been waiting for your review. |
| && match &entry.event { | ||
| OnchainEvent::MaturingOutput { | ||
| descriptor: SpendableOutputDescriptor::DelayedPaymentOutput(descriptor), | ||
| } => descriptor.to_self_delay == csv, |
There was a problem hiding this comment.
Nit: this should probably also match on the HTLC output index
There was a problem hiding this comment.
Added in f: pin HTLC spend assert to the delayed output
| assert!(tx_handler.claimable_outpoints.is_empty()); | ||
|
|
||
| // If the spend reorgs out, the contentious outpoint is resurrected into | ||
| // the delayed package. |
There was a problem hiding this comment.
Nit: may be worth checking that replaying the original claim requests after the reorg doesn't result in duplicates as well
There was a problem hiding this comment.
Added in "f: cover claim replay after reorg resurrection"
Aggregated HTLC spends create one delayed output per HTLC input, all sharing the same transaction and CSV delay, so txid and CSV alone do not identify the output. Match on the output index paired with the spending input as well. AI tools were used in preparing this commit.
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.
Classify duplicate outpoint state in one helper. Preserve existing filter ordering and timelock logging.
Replay the original claim requests once the contentious spend reorgs out and verify the resurrected delayed package is not duplicated. AI tools were used in preparing this commit.
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.
Log when a replayed preimage claim is skipped because the HTLC output reached anti-reorg finality without that preimage.
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
f73f75c to
977444d
Compare
|
LGTM after squash |
|
Regarding the back port question: I do wonder if without this PR there can be zombie claims that never disappear. But even if that is a possibility, it might not be critical enough. Unless it does something like using up wallet utxos for bumping. |
|
Good point, using up wallet UTXOs would be annoying. AFAICT though this PR doesn't clean up zombie claims, it merely prevents them? |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
5 participants
Fixes #4572