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

@@ -3356,6 +3356,58 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		}
 	}
 
+	/// Cancels any existing pending claims for a commitment that previously confirmed and has now
+	/// been replaced by another.
+	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 {
+			// 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<L: Deref>(
 		&mut self, logger: &WithChannelMonitor<L>,
 	) -> Vec<Transaction> where L::Target: Logger {
@@ -3568,6 +3620,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							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 {

lightning/src/chain/onchaintx.rs

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

@@ -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);
 	}
 }
 

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.