Require counterparty_node_id TLV for ChannelMonitor by wpaulino · Pull Request #3638 · lightningdevkit/rust-lightning

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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 · 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

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.

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

+ "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

@@ -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

+	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 027b2eaCompare March 4, 2025 15:08

wpaulino force-pushed the bye-bye-outpoint-to-peer branch 2 times, most recently from d5f5469 to fc4ee83Compare 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 7063531Compare 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

Comment threadlightning/src/ln/functional_tests.rs

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

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from 7063531 to caace0cCompare 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

Comment threadlightning/src/ln/channel.rs Outdated

wpaulino force-pushed the bye-bye-outpoint-to-peer branch from caace0c to 5db6b24Compare 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 fa4ff0cCompare 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:mainMar 6, 2025

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

jkczyz mentioned this pull request Jun 21, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks