Fix two issues in Balance generation
#2593
Conversation
This will allow us to use the HTLC resolution tracking to detect the HTLC spend via `OnchainEvent::MaturingOutput` in the next commit.
…C transaction output index
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2593 +/- ##
==========================================
- Coverage 88.86% 88.83% -0.04%
==========================================
Files 113 113
Lines 84419 84427 +8
Branches 84419 84427 +8
==========================================
- Hits 75022 75000 -22
- Misses 7197 7222 +25
- Partials 2200 2205 +5
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Situation is dangerous in the sense than get_claimable_balances might yield back erroneous balance, or even lack to see them. Not a time-sensitive claim direct error as HTLC claims are handled by other ChannelMonitor logics. This still can be an issue for ChainMonitor::get_claimable_balances() consumers.
Looking on Balance detection, I don’t know if we’re covering the claim of the two anchor outputs through the anyone can spend path, once the CSV encumbrance is expired. Not necessarily a must, though nice to clean the UTXO set if we can afford it.
I don’t have time to write test either, at least before catching up on my mempool reviews though available to review more fixes.
| descriptor: SpendableOutputDescriptor::DelayedPaymentOutput(_) } | ||
| if event.transaction.as_ref().map(|tx| tx.input.iter() | ||
| .any(|inp| Some(inp.previous_output.txid) == confirmed_txid && | ||
| inp.previous_output.vout == htlc_commitment_tx_output_idx)) |
There was a problem hiding this comment.
So before this commit 259815f we will match any DelayedPaymentOutput as soon as the htlc_commitment_tx_output_idx match the transaction output. IIUC, I think such case can arise when you have a second-stage HTLC transaction revocable output with an index higher than the balance outputs (according to BOLT3 outputs ordering). Post-anchor, HTLC-timeout transaction can be aggregated so the index can be higher than 1.
The fix adds an additional check than parent commitment transaction is confirmed.
| }, | ||
| OnchainEvent::MaturingOutput { | ||
| descriptor: SpendableOutputDescriptor::DelayedPaymentOutput(ref descriptor) } | ||
| if descriptor.outpoint.into_bitcoin_outpoint() == htlc_output_to_spend => { |
There was a problem hiding this comment.
IIUC this path should check we’re detecting well a second-stage HTLC transaction (HTLC-timeout or HTLC-preimage) CSV encumbered delayed and return further a ClaimableAwaitingConfirmations balance event
4 participants
The second issue I hit in prod on my node - an anchor HTLC timeout transaction confirmed but the output on the HTLC transaction was still locked for another few hundred blocks. During this time, we don't generate a corresponding
Balanceat all, which is a potentially dangerous situation. While fixing it, I noticed the first issue - we shouldn't be deciding if an HTLC is confirming based on the output index in the HTLC transaction matching the output index in the commitment transaction.Sadly I don't have time to write tests here (or real commit messages). The second should be trivial to test, the first shouldn't be too hard, I think if we swap around some of the amounts in the existing balances tests it'll swap the output order and the old code will break.