Refactor `ChannelPhase` variants from `ChannelManager` #3513

jkczyz · 2025-01-09T01:02:43Z

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid
this.

This PR is a step in that direction by removing direct use of ChannelPhase variants from ChannelManager. Instead, various methods are added to ChannelPhase in order to obtain the underlying structs held by each variant. Channel is also renamed to FundedChannel such that ChannelPhase can be renamed to Channel. A follow-up PR will hide ChannelPhase inside Channel, if desired.

Based on #3498.

jkczyz · 2025-01-09T01:07:18Z

Reviewers: It is best to review each commit separately. I tried to avoid moving code or introducing indents whenever possible. I could do a follow-up to hide the enum entirely as mentioned in the PR description. I'm uncertain how valuable this would be. At very least it would prevent us from removing and inserting into hash maps when the channel id doesn't change. Zooming out, it would be nice to clearly define what belongs in channel.rs and channelmanager.rs, respectively.

codecov · 2025-01-09T03:26:28Z

Codecov Report

Attention: Patch coverage is 70.67834% with 134 lines in your changes missing coverage. Please review.

Project coverage is 88.35%. Comparing base (42cc4e7) to head (786cf37).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	62.87%	72 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	76.15%	53 Missing and 4 partials ⚠️
lightning/src/ln/functional_tests.rs	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3513      +/-   ##
==========================================
+ Coverage   88.33%   88.35%   +0.01%     
==========================================
  Files         149      149              
  Lines      112839   112951     +112     
  Branches   112839   112951     +112     
==========================================
+ Hits        99672    99793     +121     
+ Misses      10688    10684       -4     
+ Partials     2479     2474       -5

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

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, introduce ChannelPhase::as_funded and ChannelPhase::as_funded_mut for use in ChannelManager when a Channel (to be later renamed FundedChannel) is needed.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, introduce ChannelPhase::is_funded for use in ChannelManager when a Channel (to be later renamed FundedChannel) needs to be tested for.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, rewrite ChannelManager's unfunded_channel_count method to use ChannelPhase::as_funded and a new ChannelPhase::as_unfunded_v2 method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::internal_tx_abort to use ChannelPhase::is_funded and a new ChannelPhase::as_unfunded_v2_mut method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::signer_unblocked to use ChannelPhase::as_funded and a new method on ChannelPhase dispatching to each variant's signer_maybe_unblocked method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::peer_disconnected to use ChannelPhase::as_funded_mut and a new ChannelPhase::is_resumable method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::peer_connected to use ChannelPhase::as_funded_mut and a new ChannelPhase::maybe_get_open_channel method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::handle_error to use a new ChannelPhase::maybe_handle_error_without_close.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager::timer_tick_occurred to use ChannelPhase::as_funded_mut and a new ChannelPhase::unfunded_context_mut method.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager methods to use ChannelPhase::as_unfunded_v2_mut and ChannelPhase::into_unfunded_v2 methods.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update the convert_chan_phase_err macro to use ChannelPhase::as_funded_mut instead.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, update ChannelManager methods to use methods on ChannelPhase for obtaining the appropriate V1 channel types.

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, define a conversion from Channel (to be renamed FundedChannel) to ChannelPhase (to be renamed Channel).

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, define a conversion from OutboundV1Channel to ChannelPhase (to be renamed Channel).

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, define a conversion from InboundV1Channel to ChannelPhase (to be renamed Channel).

Exposing ChannelPhase in ChannelManager has led to verbose match statements, which need to be modified each time a ChannelPhase is added. Making ChannelPhase an implementation detail of Channel would help avoid this. As a step in this direction, define a conversion from PendingV2Channel to ChannelPhase (to be renamed Channel).

dunxen · 2025-01-09T09:57:50Z

Can rebase as 3498 is merged.

dunxen

This generally LGTM and the renames are great.

Thanks for splitting it up like this. Just left some minor remarks/comments that could probably all be NO_OPs.

dunxen · 2025-01-09T11:48:29Z

lightning/src/ln/channel.rs

+			ChannelPhase::Funded(_) => false,
+			ChannelPhase::UnfundedOutboundV1(chan) => chan.is_resumable(),
+			ChannelPhase::UnfundedInboundV1(_) => false,
+			ChannelPhase::UnfundedV2(_) => false,


I suppose I'd need to look at introducing is_resumable() to PendingV2Chan if possible.

dunxen · 2025-01-09T11:56:47Z

lightning/src/ln/channel.rs

@@ -8979,6 +9011,7 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 	/// If we receive an error message, it may only be a rejection of the channel type we tried,
 	/// not of our ability to open any channel at all. Thus, on error, we should first call this
 	/// and see if we get a new `OpenChannelV2` message, otherwise the channel is failed.
+	#[cfg(dual_funding)]


Do we have to introduce the cfg flags again? I think with #3485 we wanted to only have them where necessary.

Discussed offline. This is needed to avoid a compilation error since the calls site needs OpenChannelMessage::V2.

dunxen · 2025-01-09T12:02:12Z

lightning/src/ln/channelmanager.rs

-								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
+							None => {
+								phase.context_mut().maybe_expire_prev_config();
+								let unfunded_context = phase.unfunded_context_mut().expect("channel should be unfunded");


Nice that we got rid of the macro, but sad that we need this expect. That's okay, though.

Yeah, the only way to really avoid this is to have a method returning an enum that has Funded and Unfunded variants, where the latter gives the UnfundedChannelContext. I'm not necessarily opposed to the idea (and maybe it would be useful elsewhere?), but I opted for a minimal change initially.

In preparation for hiding ChannelPhase inside a Channel type, rename Channel to FundedChannel.

In preparation for hiding ChannelPhase inside a Channel type, rename ChannelPhase to Channel.

jkczyz

Pushed some doclink changes caught by CI.

jkczyz · 2025-01-09T15:54:31Z

lightning/src/ln/channel.rs

@@ -8979,6 +9011,7 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 	/// If we receive an error message, it may only be a rejection of the channel type we tried,
 	/// not of our ability to open any channel at all. Thus, on error, we should first call this
 	/// and see if we get a new `OpenChannelV2` message, otherwise the channel is failed.
+	#[cfg(dual_funding)]


Discussed offline. This is needed to avoid a compilation error since the calls site needs OpenChannelMessage::V2.

jkczyz · 2025-01-09T15:57:55Z

lightning/src/ln/channelmanager.rs

-								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
+							None => {
+								phase.context_mut().maybe_expire_prev_config();
+								let unfunded_context = phase.unfunded_context_mut().expect("channel should be unfunded");


Yeah, the only way to really avoid this is to have a method returning an enum that has Funded and Unfunded variants, where the latter gives the UnfundedChannelContext. I'm not necessarily opposed to the idea (and maybe it would be useful elsewhere?), but I opted for a minimal change initially.

optout21

LGTM, fine piece-by-piece work!

Some general comments:

There are a few matches with 2 arms: Some and None (pre-change: FundedChannel and _): the if let Some() construct is probably more appropriate, but I do understand that you tried to keep changes to minimal, which is fine.

I've checked FundedChannel usage, outside of channel.rs it is only used in channelmanager.rs, in about 12 places, mostly as parameter type for methods. I think those are fine at this point, but maybe you should have a look yourself.

optout21 · 2025-01-10T10:35:52Z

lightning/src/ln/channel.rs

+	pub fn is_funded(&self) -> bool {
+		self.as_funded().is_some()
+	}


nit: for a minimal more efficiency, could be unrolled to:

Suggested change

pub fn is_funded(&self) -> bool {

self.as_funded().is_some()

}

pub fn is_funded(&self) -> bool {

matches!(self, Channel::Funded(_channel))

}

jkczyz requested review from dunxen, wpaulino and optout21 January 9, 2025 01:03

jkczyz force-pushed the 2024-12-refactor-channel-phase branch from 0ee63dd to 9a3315d Compare January 9, 2025 03:18

jkczyz added 16 commits January 8, 2025 21:29

jkczyz force-pushed the 2024-12-refactor-channel-phase branch from 9a3315d to 786cf37 Compare January 9, 2025 03:43

dunxen mentioned this pull request Jan 9, 2025

Follow-ups to PR 3137 #3423

Open

dunxen reviewed Jan 9, 2025

View reviewed changes

jkczyz added 3 commits January 9, 2025 09:49

Rename Channel to FundedChannel

f7a5610

In preparation for hiding ChannelPhase inside a Channel type, rename Channel to FundedChannel.

Rename ChannelPhase to Channel

887fefa

In preparation for hiding ChannelPhase inside a Channel type, rename ChannelPhase to Channel.

Remove unnecessary lifetime parameter

1775d64

jkczyz force-pushed the 2024-12-refactor-channel-phase branch from 786cf37 to 1775d64 Compare January 9, 2025 15:50

jkczyz commented Jan 9, 2025

View reviewed changes

jkczyz requested a review from TheBlueMatt January 9, 2025 18:22

jkczyz mentioned this pull request Jan 9, 2025

Channel phase refactor, wrap channel phase, store wrapped phase in map #3418

Draft

optout21 reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `ChannelPhase` variants from `ChannelManager` #3513

Refactor `ChannelPhase` variants from `ChannelManager` #3513

jkczyz commented Jan 9, 2025

jkczyz commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

dunxen commented Jan 9, 2025

dunxen left a comment

dunxen Jan 9, 2025

dunxen Jan 9, 2025

jkczyz Jan 9, 2025 •

edited

Loading

dunxen Jan 9, 2025

jkczyz Jan 9, 2025 •

edited

Loading

jkczyz left a comment

jkczyz Jan 9, 2025 •

edited

Loading

jkczyz Jan 9, 2025 •

edited

Loading

optout21 left a comment

optout21 Jan 10, 2025

Refactor ChannelPhase variants from ChannelManager #3513

Are you sure you want to change the base?

Refactor ChannelPhase variants from ChannelManager #3513

Conversation

jkczyz commented Jan 9, 2025

jkczyz commented Jan 9, 2025

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

dunxen commented Jan 9, 2025

dunxen left a comment

Choose a reason for hiding this comment

dunxen Jan 9, 2025

Choose a reason for hiding this comment

dunxen Jan 9, 2025

Choose a reason for hiding this comment

jkczyz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

dunxen Jan 9, 2025

Choose a reason for hiding this comment

jkczyz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

jkczyz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

optout21 left a comment

Choose a reason for hiding this comment

optout21 Jan 10, 2025

Choose a reason for hiding this comment

Refactor `ChannelPhase` variants from `ChannelManager` #3513

Refactor `ChannelPhase` variants from `ChannelManager` #3513

codecov bot commented Jan 9, 2025 •

edited

Loading

jkczyz Jan 9, 2025 •

edited

Loading

jkczyz Jan 9, 2025 •

edited

Loading

jkczyz Jan 9, 2025 •

edited

Loading

jkczyz Jan 9, 2025 •

edited

Loading