Skip to content

Conversation

TheBlueMatt
Copy link
Collaborator

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

@TheBlueMatt TheBlueMatt added this to the 0.2 milestone Sep 18, 2025
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Sep 18, 2025

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

@TheBlueMatt
Copy link
Collaborator Author

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

Copy link

codecov bot commented Sep 18, 2025

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 TheBlueMatt force-pushed the 2025-09-drop-inb-unfunded-monitors branch 2 times, most recently from d0018e3 to f7d3296 Compare September 18, 2025 16:16
@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Sep 18, 2025

$ 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 TheBlueMatt requested a review from tnull September 18, 2025 16:17
tnull
tnull previously approved these changes Sep 18, 2025
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 lightningdevkit#3384
@TheBlueMatt
Copy link
Collaborator Author

Oops sorry test was wrong.

@wpaulino wpaulino merged commit 981e95f into lightningdevkit:main Sep 18, 2025
24 of 25 checks passed
@tankyleo tankyleo removed their request for review September 18, 2025 23:52
@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Oct 6, 2025

Backported in #4143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop ChannelMonitors for inbound, no-local-funding channels after they time out

4 participants