Immediately archive ChannelMonitors for inbound unfuned channels

[TheBlueMatt]

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

[ldk-reviews-bot]

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignment, please click here.

[TheBlueMatt]

Tagged this "backport 0.1" cause it seems like an important bugfix, but I'm open to feedback from reviewers on that.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.60%. Comparing base (5ae19b4) to head (744664e).
⚠️ Report is 53 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4081      +/-   ##
==========================================
+ Coverage   88.33%   88.60%   +0.26%     
==========================================
  Files         177      176       -1     
  Lines      131896   132097     +201     
  Branches   131896   132097     +201     
==========================================
+ Hits       116512   117045     +533     
+ Misses      12728    12387     -341     
- Partials     2656     2665       +9     

Flag	Coverage Δ
fuzzing	21.93% <25.80%> (+0.32%)	⬆️
tests	88.44% <100.00%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

$ git diff-tree -U3 278aaa1fc 744664e88
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 0237e653db..c17cd87d49 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2571,12 +2571,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		let current_height = self.current_best_block().height;
 		let mut inner = self.inner.lock().unwrap();

-		let mut commitment_not_advanced =
-			inner.current_counterparty_commitment_number == INITIAL_COMMITMENT_NUMBER;
-		commitment_not_advanced &=
-			inner.current_holder_commitment_number == INITIAL_COMMITMENT_NUMBER;
-		if (inner.holder_tx_signed || inner.lockdown_from_offchain)
-			&& commitment_not_advanced
+		if inner.is_closed_without_updates()
 			&& is_all_funds_claimed
 			&& !inner.funding_spend_seen
 		{
@@ -3079,12 +3074,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			// 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.
-			let mut commitment_not_advanced =
-				us.current_counterparty_commitment_number == INITIAL_COMMITMENT_NUMBER;
-			commitment_not_advanced &=
-				us.current_holder_commitment_number == INITIAL_COMMITMENT_NUMBER;
-			if (!us.holder_tx_signed && !us.lockdown_from_offchain)
-				|| !commitment_not_advanced
+			if !us.is_closed_without_updates()
 				|| balance_candidates.iter().any(|bal| bal.amount_satoshis != 0)
 			{
 				res.push(Balance::ClaimableOnChannelClose {
@@ -4383,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
 	}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 46ff741a54..bc089d495b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -6635,6 +6635,10 @@ pub fn test_channel_conf_timeout() {
 	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 generate
+	// claimable `Balance`s until they're closed.
+	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.
@@ -6648,6 +6652,7 @@ pub fn test_channel_conf_timeout() {

 	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);
@@ -6668,9 +6673,14 @@ pub fn test_channel_conf_timeout() {
 		_ => panic!("Unexpected event"),
 	}

+	// Once an inbound never-confirmed channel is closed, it will no longer generate any claimable
+	// `Balance`s.
+	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.

[TheBlueMatt]

Oops sorry test was wrong.

[TheBlueMatt]

Backported in #4143