From 7dcee4f2e54988dbaceefad7a352bbd15263622b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 31 Oct 2023 01:12:58 -0700 Subject: [PATCH] Cancel previous commitment claims on newly confirmed commitment Once a commitment transaction is broadcast/confirms, we may need to claim some of the HTLCs in it. These claims are sent as requests to the `OnchainTxHandler`, which will bump their feerate as they remain unconfirmed. When said commitment transaction becomes unconfirmed though, and another commitment confirms instead, i.e., a reorg happens, the `OnchainTxHandler` doesn't have any insight into whether these claims are still valid or not, so it continues attempting to claim the HTLCs from the previous commitment (now unconfirmed) forever, along with the HTLCs from the newly confirmed commitment. --- lightning/src/chain/channelmonitor.rs | 56 +++++++++++++++++++++++++++ lightning/src/chain/onchaintx.rs | 19 +++++++++ lightning/src/ln/functional_tests.rs | 10 ++--- lightning/src/ln/reorg_tests.rs | 3 ++ 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index ce1ef9128..472c768d3 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3356,6 +3356,58 @@ impl ChannelMonitorImpl { } } + /// Cancels any existing pending claims for a commitment that previously confirmed and has now + /// been replaced by another. + pub fn cancel_prev_commitment_claims( + &mut self, logger: &L, confirmed_commitment_txid: &Txid + ) where L::Target: Logger { + for (counterparty_commitment_txid, _) in &self.counterparty_commitment_txn_on_chain { + // Cancel any pending claims for counterparty commitments we've seen confirm. + if counterparty_commitment_txid == confirmed_commitment_txid { + continue; + } + for (htlc, _) in self.counterparty_claimable_outpoints.get(counterparty_commitment_txid).unwrap_or(&vec![]) { + log_trace!(logger, "Canceling claims for previously confirmed counterparty commitment {}", + counterparty_commitment_txid); + let mut outpoint = BitcoinOutPoint { txid: *counterparty_commitment_txid, vout: 0 }; + if let Some(vout) = htlc.transaction_output_index { + outpoint.vout = vout; + self.onchain_tx_handler.abandon_claim(&outpoint); + } + } + } + if self.holder_tx_signed { + // If we've signed, we may have broadcast either commitment (prev or current), and + // attempted to claim from it immediately without waiting for a confirmation. + if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid { + log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", + self.current_holder_commitment_tx.txid); + let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 }; + for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs { + if let Some(vout) = htlc.transaction_output_index { + outpoint.vout = vout; + self.onchain_tx_handler.abandon_claim(&outpoint); + } + } + } + if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx { + if prev_holder_commitment_tx.txid != *confirmed_commitment_txid { + log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", + prev_holder_commitment_tx.txid); + let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 }; + for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs { + if let Some(vout) = htlc.transaction_output_index { + outpoint.vout = vout; + self.onchain_tx_handler.abandon_claim(&outpoint); + } + } + } + } + } else { + // No previous claim. + } + } + fn get_latest_holder_commitment_txn( &mut self, logger: &WithChannelMonitor, ) -> Vec where L::Target: Logger { @@ -3568,6 +3620,10 @@ impl ChannelMonitorImpl { commitment_tx_to_counterparty_output, }, }); + // Now that we've detected a confirmed commitment transaction, attempt to cancel + // pending claims for any commitments that were previously confirmed such that + // we don't continue claiming inputs that no longer exist. + self.cancel_prev_commitment_claims(&logger, &txid); } } if tx.input.len() >= 1 { diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index bbed782bb..59c98f05e 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -676,6 +676,25 @@ impl OnchainTxHandler None } + pub fn abandon_claim(&mut self, outpoint: &BitcoinOutPoint) { + let claim_id = self.claimable_outpoints.get(outpoint).map(|(claim_id, _)| *claim_id) + .or_else(|| { + self.pending_claim_requests.iter() + .find(|(_, claim)| claim.outpoints().iter().any(|claim_outpoint| *claim_outpoint == outpoint)) + .map(|(claim_id, _)| *claim_id) + }); + if let Some(claim_id) = claim_id { + if let Some(claim) = self.pending_claim_requests.remove(&claim_id) { + for outpoint in claim.outpoints() { + self.claimable_outpoints.remove(&outpoint); + } + } + } else { + self.locktimed_packages.values_mut().for_each(|claims| + claims.retain(|claim| !claim.outpoints().iter().any(|claim_outpoint| *claim_outpoint == outpoint))); + } + } + /// Upon channelmonitor.block_connected(..) or upon provision of a preimage on the forward link /// for this channel, provide new relevant on-chain transactions and/or new claim requests. /// Together with `update_claims_view_from_matched_txn` this used to be named diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index fee3a3b39..979ef774e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8568,10 +8568,11 @@ fn test_concurrent_monitor_claim() { watchtower_alice.chain_monitor.block_connected(&block, HTLC_TIMEOUT_BROADCAST); // Watchtower Alice should have broadcast a commitment/HTLC-timeout - let alice_state = { + { let mut txn = alice_broadcaster.txn_broadcast(); assert_eq!(txn.len(), 2); - txn.remove(0) + check_spends!(txn[0], chan_1.3); + check_spends!(txn[1], txn[0]); }; // Copy ChainMonitor to simulate watchtower Bob and make it receive a commitment update first. @@ -8640,11 +8641,8 @@ fn test_concurrent_monitor_claim() { check_added_monitors(&nodes[0], 1); { let htlc_txn = alice_broadcaster.txn_broadcast(); - assert_eq!(htlc_txn.len(), 2); + assert_eq!(htlc_txn.len(), 1); check_spends!(htlc_txn[0], bob_state_y); - // Alice doesn't clean up the old HTLC claim since it hasn't seen a conflicting spend for - // it. However, she should, because it now has an invalid parent. - check_spends!(htlc_txn[1], alice_state); } } diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index badb78f24..d65f258e6 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -666,6 +666,9 @@ fn test_htlc_preimage_claim_holder_commitment_after_counterparty_commitment_reor mine_transaction(&nodes[0], &commitment_tx_b); mine_transaction(&nodes[1], &commitment_tx_b); + if nodes[1].connect_style.borrow().updates_best_block_first() { + let _ = nodes[1].tx_broadcaster.txn_broadcast(); + } // Provide the preimage now, such that we only claim from the holder commitment (since it's // currently confirmed) and not the counterparty's. -- 2.39.5