Immediately archive ChannelMonitors for inbound unfuned channels

If a peer opens a channel to us, but never actually broadcasts the
funding transaction, we'll still keep a `ChannelMonitor` around for
the channel. While we maybe shouldn't do this either, when the
channel ultimately times out 2016 blocks later, we should at least
immediately archive the `ChannelMonitor`, which we do here.

Fixes #3384

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -2571,6 +2571,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		let current_height = self.current_best_block().height;
 		let mut inner = self.inner.lock().unwrap();
 
+		if inner.is_closed_without_updates()
+			&& is_all_funds_claimed
+			&& !inner.funding_spend_seen
+		{
+			// We closed the channel without ever advancing it and didn't have any funds in it.
+			// We should immediately archive this monitor as there's nothing for us to ever do with
+			// it.
+			return (true, false);
+		}
+
 		if is_all_funds_claimed && !inner.funding_spend_seen {
 			debug_assert!(false, "We should see funding spend by the time a monitor clears out");
 			is_all_funds_claimed = false;
@@ -3044,7 +3054,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 						transaction_fee_satoshis,
 					}
 				})
-				.collect();
+				.collect::<Vec<_>>();
 			let confirmed_balance_candidate_index = core::iter::once(&us.funding)
 				.chain(us.pending_funding.iter())
 				.enumerate()
@@ -3057,14 +3067,25 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 				})
 				.map(|(idx, _)| idx)
 				.expect("We must have one FundingScope that is confirmed");
-			res.push(Balance::ClaimableOnChannelClose {
-				balance_candidates,
-				confirmed_balance_candidate_index,
-				outbound_payment_htlc_rounded_msat,
-				outbound_forwarded_htlc_rounded_msat,
-				inbound_claiming_htlc_rounded_msat,
-				inbound_htlc_rounded_msat,
-			});
+
+			// Only push a primary balance if either the channel isn't closed or we've advanced the
+			// channel state machine at least once (implying there are multiple previous commitment
+			// transactions) or we actually have a balance.
+			// Avoiding including a `Balance` if none of these are true allows us to prune monitors
+			// for chanels that were opened inbound to us but where the funding transaction never
+			// confirmed at all.
+			if !us.is_closed_without_updates()
+				|| balance_candidates.iter().any(|bal| bal.amount_satoshis != 0)
+			{
+				res.push(Balance::ClaimableOnChannelClose {
+					balance_candidates,
+					confirmed_balance_candidate_index,
+					outbound_payment_htlc_rounded_msat,
+					outbound_forwarded_htlc_rounded_msat,
+					inbound_claiming_htlc_rounded_msat,
+					inbound_htlc_rounded_msat,
+				});
+			}
 		}
 
 		res
@@ -4352,6 +4373,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		} else { ret }
 	}
 
+	/// Returns true if the channel has been closed (i.e. no further updates are allowed) and no
+	/// commitment state updates ever happened.
+	fn is_closed_without_updates(&self) -> bool {
+		let mut commitment_not_advanced =
+			self.current_counterparty_commitment_number == INITIAL_COMMITMENT_NUMBER;
+		commitment_not_advanced &=
+			self.current_holder_commitment_number == INITIAL_COMMITMENT_NUMBER;
+		(self.holder_tx_signed || self.lockdown_from_offchain) && commitment_not_advanced
+	}
+
 	fn no_further_updates_allowed(&self) -> bool {
 		self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed
 	}

lightning/src/ln/functional_tests.rs

@@ -18,6 +18,7 @@ use crate::chain::channelmonitor::{
 	Balance, ANTI_REORG_DELAY, CLTV_CLAIM_BUFFER, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE,
 	LATENCY_GRACE_PERIOD_BLOCKS,
 };
+use crate::chain::transaction::OutPoint;
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::events::{
 	ClosureReason, Event, HTLCHandlingFailureType, PathFailure, PaymentFailureReason,
@@ -6631,9 +6632,13 @@ pub fn test_channel_conf_timeout() {
 
 	let node_a_id = nodes[0].node.get_our_node_id();
 
-	let _funding_tx =
+	let funding_tx =
 		create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 100_000);
 
+	// Inbound channels which haven't advanced state at all and never were funded will never
+	// generate any claimable `Balance`s.
+	assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
 	// The outbound node should wait forever for confirmation:
 	// This matches `channel::FUNDING_CONF_DEADLINE_BLOCKS` and BOLT 2's suggested timeout, thus is
 	// copied here instead of directly referencing the constant.
@@ -6645,6 +6650,10 @@ pub fn test_channel_conf_timeout() {
 	check_added_monitors(&nodes[1], 0);
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
 
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
 	connect_blocks(&nodes[1], 1);
 	check_added_monitors(&nodes[1], 1);
 	check_closed_event!(nodes[1], 1, ClosureReason::FundingTimedOut, [node_a_id], 1000000);
@@ -6663,6 +6672,19 @@ pub fn test_channel_conf_timeout() {
 		},
 		_ => panic!("Unexpected event"),
 	}
+
+	assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+	// Once the funding times out the monitor should be immediately archived.
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 0);
+	assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
+	// Remove the corresponding outputs and transactions the chain source is
+	// watching. This is to make sure the `Drop` function assertions pass.
+	nodes[1].chain_source.remove_watched_txn_and_outputs(
+		OutPoint { txid: funding_tx.compute_txid(), index: 0 },
+		funding_tx.output[0].script_pubkey.clone(),
+	);
 }
 
 #[xtest(feature = "_externalize_tests")]