Require counterparty_node_id TLV for ChannelMonitor #3638

wpaulino · 2025-03-03T21:42:13Z

New ChannelMonitors created starting from v0.0.110 will already have this field set, and those created before then will have it set if a ChannelMonitorUpdate created in v0.0.116 or later has been applied.

It would be extremely rare for a user to not fall under either of these conditions: they opened a channel almost 3 years ago, and haven't had any activity on it in the last 2 years. Nonetheless, a panic has been added on ChannelMonitor deserialization to ensure users can move forward by first running a v0.1.* release and sending/routing a payment or closing the channel before upgrading to v0.2.0.

As a result, the ChannelManager::outpoint_to_peer map can now be dropped.

ldk-reviews-bot · 2025-03-03T21:42:16Z

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-04T16:22:13Z

Codecov Report

Attention: Patch coverage is 91.52542% with 15 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (c4d0560) to head (fa4ff0c).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	92.42%	6 Missing and 4 partials ⚠️
lightning/src/chain/channelmonitor.rs	81.25%	3 Missing ⚠️
lightning/src/chain/chainmonitor.rs	50.00%	1 Missing ⚠️
lightning/src/ln/channel.rs	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3638      +/-   ##
==========================================
- Coverage   89.22%   89.19%   -0.03%     
==========================================
  Files         155      155              
  Lines      119246   119089     -157     
  Branches   119246   119089     -157     
==========================================
- Hits       106394   106220     -174     
- Misses      10260    10277      +17     
  Partials     2592     2592

☔ 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.

jkczyz

All looks fairly straightforward. Anything that I noticed was addressed in later commits. Could you add a pending changelog file indicating the compatibility changes?

ldk-reviews-bot · 2025-03-05T00:20:27Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt

one comment otherwise lgtm

lightning/src/ln/functional_tests.rs

wpaulino · 2025-03-05T20:13:27Z

Had to rebase after #3594 and it was a bit annoying, please re-review. I also restored test_duplicate_conflicting_funding_from_second_peer which required calling unset_funding_info for an OutboundV1Channel.

ldk-reviews-bot · 2025-03-05T21:42:30Z

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/channel.rs

New `ChannelMonitor`s created starting from v0.0.110 will already have this field set, and those created before then will have it set if a `ChannelMonitorUpdate` created in v0.0.116 or later has been applied. It would be extremely rare for a user to not fall under either of these conditions: they opened a channel almost 3 years ago, and haven't had any activity on it in the last 2 years. Nonetheless, a panic has been added on `ChannelMonitor` deserialization to ensure users can move forward by first running a v0.1.* release and sending/routing a payment or closing the channel before upgrading to v0.2.0.

Now that we require `ChannelMonitor`s to track the channel's counterparty node ID, we can remove the `outpoint_to_peer` map that was used throughout `MonitorEvent` handling. This is a big win for a post-splicing future, as we'll no longer have to bother updating this map when a splice is proposed. Some existing tests providing coverage have been removed as a result.

The `counterparty_node_id` in `ChannelMonitorUpdate`s was only tracked to guarantee we could backfill the `ChannelMonitor`'s field once the update is applied. Now that we require the field to already be set at the monitor level, we can remove it from the updates without backwards compatibility concerns as it was written as an odd TLV.

wpaulino · 2025-03-06T15:47:57Z

Had to rebase again due to a minor conflict

jkczyz · 2025-03-06T16:30:07Z

lightning/src/ln/channel.rs

+			"temporary_channel_id should be set since unset_funding_info is only called on funded \
+			 channels that were unfunded immediately beforehand"


Eh, will need to update this message, I guess.

I think it's still correct-ish? The OutboundV1Channel was funded, it just hadn't transitioned to FundedChannel yet.

Ah, fair enough!

TheBlueMatt · 2025-03-06T21:30:35Z

pending_changelog/3638-0.2-upgrade-without-counterparty-node-id-in-monitor-not-supported.txt

@@ -0,0 +1,5 @@
+## API Updates (0.2)
+
+* Upgrading to v0.2.0 is not allowed when a `ChannelMonitor` that does not track the channel's


This isnt particularly useful information for a user. How do they know what channelmonitors track the channel's node id vs not? We should list the exact reasons (monitors created before version X that have had no updates since version Y) instead.

TheBlueMatt · 2025-03-06T21:40:57Z

lightning/src/ln/functional_tests.rs

+	assert!(err_msg.contains("An existing channel using ID"));
+	assert!(err_msg.contains("is open with peer"));
+	let channel_id = ChannelId::v1_from_funding_outpoint(funding_output);
+	let reason = ClosureReason::ProcessingError { err: format!("An existing channel using ID {} is open with peer {}", channel_id, nodes[1].node.get_our_node_id()), };


We really need a copy of this test for v2 channel opens @dunxen

Thanks, tracking it.

wpaulino requested a review from jkczyz March 3, 2025 21:45

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from d7cd76e to 027b2ea Compare March 4, 2025 15:08

wpaulino force-pushed the bye-bye-outpoint-to-peer branch 2 times, most recently from d5f5469 to fc4ee83 Compare March 4, 2025 17:48

jkczyz reviewed Mar 5, 2025

View reviewed changes

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from fc4ee83 to 7063531 Compare March 5, 2025 16:06

wpaulino requested review from TheBlueMatt and jkczyz March 5, 2025 16:06

jkczyz previously approved these changes Mar 5, 2025

View reviewed changes

TheBlueMatt reviewed Mar 5, 2025

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

wpaulino dismissed jkczyz’s stale review via caace0c March 5, 2025 20:13

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from 7063531 to caace0c Compare March 5, 2025 20:13

wpaulino requested review from TheBlueMatt and jkczyz March 5, 2025 20:13

jkczyz reviewed Mar 5, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from caace0c to 5db6b24 Compare March 6, 2025 15:43

wpaulino added 7 commits March 6, 2025 07:47

Remove unused funding_txo parameter to channel_monitor_updated

222cd42

Remove unused ClosedMonitorUpdateRegeneratedOnStartup variant

d9721e6

Make next_channel_counterparty_node_id non-optional

2d24a89

Remove unused FundingScope argument in locked_close_channel

fa4ff0c

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from 5db6b24 to fa4ff0c Compare March 6, 2025 15:47

jkczyz approved these changes Mar 6, 2025

View reviewed changes

TheBlueMatt approved these changes Mar 6, 2025

View reviewed changes

TheBlueMatt merged commit b732865 into lightningdevkit:main Mar 6, 2025
25 of 27 checks passed

wpaulino deleted the bye-bye-outpoint-to-peer branch March 6, 2025 23:23

dunxen mentioned this pull request Mar 7, 2025

Test that dual-funded channel opens error for existing channel ID #3652

Open

		"temporary_channel_id should be set since unset_funding_info is only called on funded \
		channels that were unfunded immediately beforehand"

		@@ -0,0 +1,5 @@
		## API Updates (0.2)

		* Upgrading to v0.2.0 is not allowed when a `ChannelMonitor` that does not track the channel's

Require counterparty_node_id TLV for ChannelMonitor #3638

Require counterparty_node_id TLV for ChannelMonitor #3638

Uh oh!

Conversation

wpaulino commented Mar 3, 2025

Uh oh!

ldk-reviews-bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 5, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wpaulino commented Mar 5, 2025

Uh oh!

ldk-reviews-bot commented Mar 5, 2025

Uh oh!

Uh oh!

wpaulino commented Mar 6, 2025

Uh oh!

jkczyz Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

wpaulino Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

dunxen Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 3, 2025 •

edited

Loading

codecov bot commented Mar 4, 2025 •

edited

Loading