Only claim HTLCs with matching payment hash upon preimage monitor update #4154
Conversation
We plan to rework this method in the following commit, so we start by formatting it first.
Previously, we'd attempt to claim all HTLCs that have expired or that we have the preimage for on each preimage monitor update. This happened due to reusing the code path (`get_counterparty_output_claim_info`) used when producing all claims for a newly confirmed counterparty commitment. Unfortunately, this can result in invalid claim transactions and ultimately in loss of funds (if the HTLC expires and the counterparty claims it via the timeout), as it didn't consider that some of those HTLCs may have already been claimed by a separate transaction. This commit changes the behavior when handling preimage monitor updates only. We will now only attempt to claim HTLCs for the specific preimage that we learned via the monitor update. This is safe to do, as even if a preimage HTLC claim transaction is reorged out, the `OnchainTxHandler` is responsible for continuous claiming attempts until we see a reorg of the corresponding commitment transaction.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4154 +/- ##
==========================================
+ Coverage 88.65% 88.72% +0.06%
==========================================
Files 180 180
Lines 135241 135732 +491
Branches 135241 135732 +491
==========================================
+ Hits 119895 120425 +530
+ Misses 12579 12538 -41
- Partials 2767 2769 +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:
|
|
👋 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. |
|
✅ Added second reviewer: @tankyleo |
| let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); | ||
| assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::<Vec<_>>()); | ||
| let htlc_claim_tx = txn.remove(0); | ||
| assert_eq!(htlc_claim_tx.input.len(), 1); |
There was a problem hiding this comment.
Add coverage for the case where at this point we should claim multiple outputs on the commit tx ? I'm thinking of the MPP case. The actual code does this correctly.
| fn get_counterparty_output_claims_for_preimage( | ||
| &self, preimage: PaymentPreimage, funding_spent: &FundingScope, commitment_number: u64, | ||
| commitment_txid: Txid, | ||
| per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>, |
There was a problem hiding this comment.
Wondering if we can be more aggressive and remove the Option here - we have this data at all places where this is called. If not, we lose funds, and we should panic ? Also applies to get_point_for_commitment_number.
4 participants
Previously, we'd attempt to claim all HTLCs that have expired or that we have the preimage for on each preimage monitor update. This happened due to reusing the code path (
get_counterparty_output_claim_info) used when producing all claims for a newly confirmed counterparty commitment. Unfortunately, this can result in invalid claim transactions and ultimately in loss of funds (if the HTLC expires and the counterparty claims it via the timeout), as it didn't consider that some of those HTLCs may have already been claimed by a separate transaction.This commit changes the behavior when handling preimage monitor updates only. We will now only attempt to claim HTLCs for the specific preimage that we learned via the monitor update. This is safe to do, as even if a preimage HTLC claim transaction is reorged out, the
OnchainTxHandleris responsible for continuous claiming attempts until we see a reorg of the corresponding commitment transaction.