Prune locktimed packages when inputs are spent

We have to prune locktimed packages when their inputs are spent,
otherwise the notification of the watched outputs might be missed. This
can lead to locktimed packages with spent inputs being added back to
the pending claim requests in the future, and they are never cleaned
up until node restart.

Resolves: #3859

Author: whfuyn

lightning/src/chain/onchaintx.rs

@@ -994,6 +994,17 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 						panic!("Inconsistencies between pending_claim_requests map and claimable_outpoints map");
 					}
 				}
+
+				// Also remove/split any locktimed packages whose inputs have been spent by this transaction.
+				self.locktimed_packages.retain(|_locktime, packages|{
+					packages.retain_mut(|package| {
+						if let Some(p) = package.split_package(&inp.previous_output) {
+							claimed_outputs_material.push(p);
+						}
+						!package.outpoints().is_empty()
+					});
+					!packages.is_empty()
+				});
 			}
 			for package in claimed_outputs_material.drain(..) {
 				let entry = OnchainEventEntry {
@@ -1135,6 +1146,12 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 				//- resurect outpoint back in its claimable set and regenerate tx
 				match entry.event {
 					OnchainEvent::ContentiousOutpoint { package } => {
+						let package_locktime = package.package_locktime(height);
+						if package_locktime > height {
+							self.locktimed_packages.entry(package_locktime).or_default().push(package);
+							continue;
+						}
+
 						if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) {
 							if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) {
 								assert!(request.merge_package(package, height).is_ok());

lightning/src/ln/functional_tests.rs

@@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() {
 		_ => panic!("Unexpected event"),
 	}
 
-	macro_rules! check_tx_local_broadcast {
-		($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{
-			let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap();
-			// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
-			// remote commitment transaction.
-			if $htlc_offered {
-				assert_eq!(node_txn.len(), 2);
-				for tx in node_txn.iter() {
-					check_spends!(tx, $commitment_tx);
-					assert_ne!(tx.lock_time, LockTime::ZERO);
-					assert_eq!(
-						tx.input[0].witness.last().unwrap().len(),
-						OFFERED_HTLC_SCRIPT_WEIGHT
-					);
-					assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
-				}
-				assert_ne!(
-					node_txn[0].input[0].previous_output,
-					node_txn[1].input[0].previous_output
-				);
-			} else {
-				assert_eq!(node_txn.len(), 1);
-				check_spends!(node_txn[0], $commitment_tx);
-				assert_ne!(node_txn[0].lock_time, LockTime::ZERO);
-				assert_eq!(
-					node_txn[0].input[0].witness.last().unwrap().len(),
-					ACCEPTED_HTLC_SCRIPT_WEIGHT
-				);
-				assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment
-				assert_ne!(
-					node_txn[0].input[0].previous_output,
-					node_txn[0].input[1].previous_output
-				);
-			}
-			node_txn.clear();
-		}};
-	}
-	// nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success.
-	check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]);
+	// nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it
+	// via success.
+	assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
 
 	// Broadcast legit commitment tx from A on B's chain
 	// Broadcast preimage tx by B on offered output from A commitment tx  on A's chain
@@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() {
 			_ => panic!("Unexpected event"),
 		}
 	}
-	check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]);
+	// HTLC timeout claims for non-anchor channels are only aggregated when claimed from the
+	// remote commitment transaction.
+	let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(node_txn.len(), 2);
+	for tx in node_txn.iter() {
+		check_spends!(tx, node_a_commitment_tx[0]);
+		assert_ne!(tx.lock_time, LockTime::ZERO);
+		assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+		assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output
+	}
+	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
 }
 
 fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {

lightning/src/ln/monitor_tests.rs

@@ -732,8 +732,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
 	test_spendable_output(&nodes[0], &remote_txn[0], false);
 	assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
 
-	// After broadcasting the HTLC claim transaction, node A will still consider the HTLC
-	// possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it.
+	// After confirming the HTLC claim transaction, node A will no longer attempt to claim said
+	// HTLC, unless the transaction is reorged. However, we'll still report a
+	// `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations.
 	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
 	if prev_commitment_tx {
 		expect_payment_path_successful!(nodes[0]);
@@ -749,18 +750,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) {
 	// When the HTLC timeout output is spendable in the next block, A should broadcast it
 	connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1);
 	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	// Aggregated claim transaction.
 	assert_eq!(a_broadcast_txn.len(), 1);
 	check_spends!(a_broadcast_txn[0], remote_txn[0]);
-	assert_eq!(a_broadcast_txn[0].input.len(), 2);
-	assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout);
-	// a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx
-	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000));
+	assert_eq!(a_broadcast_txn[0].input.len(), 1);
 	assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000));
-
-	// Confirm node B's claim for node A to remove that claim from the aggregated claim transaction.
-	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
-	let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap();
 
 	// Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC