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.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -3300,6 +3300,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
+	pub fn cancel_prev_commitment_claims<L: Deref>(
+		&mut self, logger: &L, confirmed_commitment_txid: &Txid
+	) where L::Target: Logger {
+		for (counterparty_commitment_txid, _) in &self.counterparty_commitment_txn_on_chain {
+			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 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.
+		}
+	}
+
 	pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
 		log_debug!(logger, "Getting signed latest holder commitment transaction!");
 		self.holder_tx_signed = true;
@@ -3505,6 +3552,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							commitment_tx_to_counterparty_output,
 						},
 					});
+					self.cancel_prev_commitment_claims(&logger, &txid);
 				}
 			}
 			if tx.input.len() >= 1 {

lightning/src/chain/onchaintx.rs

@@ -679,6 +679,25 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 		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

lightning/src/ln/functional_tests.rs

@@ -8538,10 +8538,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.
@@ -8610,11 +8611,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);
 	}
 }
 

lightning/src/ln/reorg_tests.rs

@@ -664,6 +664,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.