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

optout21 · 2024-11-21T10:33:24Z

Rationale:

Better encapsulate channel phase, shield from ChannelManager
In channel map, store wrapped channel phase, not phase
Can change the channel phase without changing the map
Preparation for easier introduction of new channel phases, for splicing

Changes in this PR:

New ChannelWrapper struct to wrap a ChannelPhase
ChannelManager.channel_by_id map holds ChannelWrapper structs, as opposed to ChannelPhase enums
Transition from pending to funded channel can be done without removing the enrtry from the map and adding again, chaning only inside the wrapper struct around the phase.

Next steps:

Rename Channel -> FundedChannel and ChannelWrapper -> Channel
Create more handling method on Channel (channel wrapper), that dispatch into different phases.

optout21 · 2024-11-21T10:40:30Z

Preferably we should get this in before further substantial dual funding or splicing changes

dunxen · 2024-11-21T10:51:25Z

Thanks!

Discussed this @jkczyz in some calls, makes sense and agree with moving the enum inward effectively. Happy to prioritise this as it will reduce matching hell in ChannelManager where this phase stuff never really belonged, but just need to review implementation first. At first glance it seems to create even more indirection. That's fine if it's temporary and cleans up ChannelManager now already. I'll do some more in-depth review.

optout21 · 2024-11-21T10:55:10Z

At first glance it seems to create even more indirection. That's fine if it's temporary and cleans up ChannelManager now already. I'll do some more in-depth review.

Please note that this is not complete, but only the basic change to the map, further steps are possible (as noted in description). Various handling logic in ChannelManager (of type match channel.phase()) can be moved into Channel, where it can inspect its own phase.
Yes, more encapsulation sometimes goes with more indirection.

dunxen · 2024-11-21T11:01:33Z

Please note that this is not complete, but only the basic change to the map, further steps are possible (as noted in description). Various handling logic in ChannelManager (of type match channel.phase()) can be moved into Channel, where it can inspect its own phase. Yes, more encapsulation sometimes goes with more indirection.

Sounds good! :) I wasn't too worried about the extra wrapper, just wanted to get the bigger picture of its specific use.

codecov · 2024-11-21T11:14:41Z

Codecov Report

Attention: Patch coverage is 71.03275% with 115 lines in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (0c31021) to head (a8f83dc).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	68.22%	91 Missing and 18 partials ⚠️
lightning/src/ln/channel.rs	86.36%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3418      +/-   ##
==========================================
- Coverage   89.22%   89.21%   -0.01%     
==========================================
  Files         130      130              
  Lines      106965   107038      +73     
  Branches   106965   107038      +73     
==========================================
+ Hits        95438    95496      +58     
- Misses       8734     8748      +14     
- Partials     2793     2794       +1

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

jkczyz

Next steps:

Rename Channel -> FundedChannel and ChannelWrapper -> Channel

Create more handling method on Channel (channel wrapper), that dispatch into different phases.

We may as well do this now. At very least the rename.

lightning/src/ln/channel.rs

jkczyz · 2024-11-22T15:48:42Z

lightning/src/ln/channelmanager.rs

-						return close_chan!(chan_err, api_err, chan);
-					},
-				}
+			Some(channel) => {


Could you pattern match on the struct to reduce the diff size here and elsewhere?

The change here comes from the fact that the Option and the Enum are now in different level, hence it's not possible to match them in one.
For the second match with one positive and one error arm I think the if let() construct is more readable. If I change to match, the diff is still there, the unchanged inner part is only indented.

jkczyz · 2024-11-22T15:53:30Z

lightning/src/ln/channel.rs

+	pub fn phase_take(self) -> ChannelPhase<SP> {
+		self.phase
+	}


Seems like we could move context to ChannelWrapper to avoid needing to deconstruct it. Otherwise, we still need to re-insert into the map. So, IIUC, this change doesn't really help us any.

Maybe we should discuss this offline, I don't fully get it.
When a pending channel becomes funded, we need to remove&insert because the channel ID changed.
But when only the phase changes, there is no longer need for removal, see move_v2_to_funded.
The method to change the phase internally is a bit tricky, as the context owned by the old phase needs to be moved to the new phase.
Moving the context to ChannelWrapper would not solve the cases when there are (will be) more than one channel contexts.

Maybe we should discuss this offline, I don't fully get it. When a pending channel becomes funded, we need to remove&insert because the channel ID changed. But when only the phase changes, there is no longer need for removal, see move_v2_to_funded.

Right, for outbound channels the id will change. I had splicing on my mind where we shouldn't change once already funded, IIRC.

The method to change the phase internally is a bit tricky, as the context owned by the old phase needs to be moved to the new phase. Moving the context to ChannelWrapper would not solve the cases when there are (will be) more than one channel contexts.

That's why I was saying to move context into ChannelWrapper. Then you can simply override phase with a new variant. Could you remind me when we'll have more than one channel context and how they'll be stored? Is that for splicing? Will that involve a phase transition? They could possibly be swaped if contained within a Vec, for instance, or taken from an internal Option if not required.

Could you remind me when we'll have more than one channel context and how they'll be stored?

enum ChannelPhase { ... Funded(FundedChannel<SP>), RefundingV2((FundedChannel<SP>, ChannelVariants<SP>)),

First is the pre-splice channel, with its context. Second has the negotiating splice channel, and possible 0 or more negotiated unconfirmed splice variants.

Is the phase transition then Funded -> RefundingV2 -> Funded?

I'm looking at your draft prototype. Is this up-to-date with your current plan? If so, could you explain funded_channels and unfunded_channel in ChannelVariants? Just trying to wrap my head around the organization.

Funded -> RefundingV2 -> Funded
In RefundingV2 there is the original (pre-splice) context, and a pending one (inbound or outbound, V2 negotiating). When that's negotiating, it becomes (Funded)Channel, but still within RefundingV2 phase. When it is confirmed, the splice is completed, and becomes Funded.
With splice RBF variants there can be another pending, and several funding variants.
So in RefundingV2 there is at least two contexts: pre-splice and post-splice, or even more with RBF.
In places where the Funded channel is taken, for RefundingV2 we sometimes have to take the pre-splice channel (e.g. when listing funded channels, or forwarding a payment), sometimes the post-splice (e.g. when tx negotiation msgs are handled), and sometimes both -- e.g. in commitment update.

Moving the context to ChannelWrapper: I don't see how this could work. What would be left in the ChannelPhases then?

Moving the context to ChannelWrapper: I don't see how this could work. What would be left in the ChannelPhases then?

Not much else for FundedChannel, which would mean RefundingV2 would only need to hold ChannelVariants since the ChannelWrapper would contain the "current" ChannelContext, leaving any other "pending" ChannelContexts within the corresponding phase. Then, for splicing, the pending ChannelContext can be moved / swapped to ChannelWrapper when transitioning to Funded. At least that's what I was thinking at a high-level to avoid the Option.

jkczyz · 2024-11-22T15:55:28Z

lightning/src/ln/channel.rs

+	/// The inner channel phase
+	/// Option is used to facilitate in-place replacement (see e.g. move_v2_to_funded),
+	/// but it is never None, ensured in new() and set_phase()
+	phase: Option<ChannelPhase<SP>>,


See other comment about moving ChannelContext here. Seems we shouldn't need to use an Option.

I agree that logically it should not be Option, but I found no other way to transport the context from one phase to another. This may be Rust technicality. Option can be taken out and left empty, which is needed as I was not able to make the transfer in one atomic operation. Maybe another solution would be if ChannelContext had a default().

Would swap work?

optout21 · 2024-11-25T11:20:38Z

We may as well do this now. At very least the rename.

The rename of Channel would create a rather large diff, that's why I was a bit hesitant to do it right away. Considering.

jkczyz · 2024-11-25T21:35:33Z

The rename of Channel would create a rather large diff, that's why I was a bit hesitant to do it right away. Considering.

I don't think Channel is used in that many places. Definitely fewer than ChannelPhase, which you are already changing to ChannelWrapper here. Would be less churn that way.

optout21 · 2024-12-05T00:12:07Z

I've continued with this on another branch

rename (Channel & FundedChannel)
start removing ChannelPhase usage from ChannelManager

@jkczyz , should I include these in this PR or keep separate?

jkczyz · 2024-12-05T01:18:01Z

I've continued with this on another branch

rename (Channel & FundedChannel)

start removing ChannelPhase usage from ChannelManager

@jkczyz , should I include these in this PR or keep separate?

At very least we should do the rename in this PR. We can remove ChannelPhase usage in a separate PR if needed, IMO.

optout21 · 2024-12-05T18:26:01Z

Note: splice PR #3444 contains a new phase, shows more concretely some goals for this change

optout21 · 2024-12-05T18:32:33Z

Rename added

optout21 requested review from jkczyz and dunxen November 21, 2024 10:40

optout21 changed the title ~~Channel phase refactor, wrap channel phase, store wrapped phase in mapChannel phase~~ Channel phase refactor, wrap channel phase, store wrapped phase in map Nov 21, 2024

optout21 force-pushed the channel-phase branch from ed42039 to a8f83dc Compare November 21, 2024 11:14

optout21 marked this pull request as ready for review November 21, 2024 11:34

jkczyz reviewed Nov 22, 2024

View reviewed changes

optout21 force-pushed the channel-phase branch from 92f8bc3 to 4d886fe Compare November 25, 2024 11:20

jkczyz mentioned this pull request Nov 25, 2024

Follow-ups to PR 3137 #3423

Open

optout21 added 3 commits December 5, 2024 00:47

Store ChannelWrappers in channel_by_id map

60a3f36

Handle transition to Funded inline, without remove+readd from map

edffca2

Fixes (review, build)

dd19f65

optout21 force-pushed the channel-phase branch from 4d886fe to dd19f65 Compare December 4, 2024 23:47

This was referenced Dec 5, 2024

[Draft][Splicing] Partial, perform interactive transaction negotiation for new funding #3444

Draft

[Splicing] Clone for ChannelContext #3332

Open

Rename Channel to FundedChannel, ChannelWrapper to Channel

f99c2a5

jkczyz mentioned this pull request Dec 11, 2024

Dual funding extension needed also for splicing: begin_interactive_funding_tx_construction #3443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

optout21 commented Nov 21, 2024

optout21 commented Nov 21, 2024

dunxen commented Nov 21, 2024

optout21 commented Nov 21, 2024

dunxen commented Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

jkczyz left a comment

jkczyz Nov 22, 2024

optout21 Nov 25, 2024

jkczyz Nov 22, 2024

optout21 Nov 25, 2024

jkczyz Nov 25, 2024

optout21 Nov 26, 2024

jkczyz Nov 26, 2024

optout21 Dec 3, 2024

optout21 Dec 3, 2024

jkczyz Dec 3, 2024

jkczyz Nov 22, 2024

optout21 Dec 3, 2024

jkczyz Dec 3, 2024

optout21 commented Nov 25, 2024

jkczyz commented Nov 25, 2024

optout21 commented Dec 5, 2024

jkczyz commented Dec 5, 2024

optout21 commented Dec 5, 2024

optout21 commented Dec 5, 2024

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

Are you sure you want to change the base?

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

Conversation

optout21 commented Nov 21, 2024

optout21 commented Nov 21, 2024

dunxen commented Nov 21, 2024

optout21 commented Nov 21, 2024

dunxen commented Nov 21, 2024

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optout21 commented Nov 25, 2024

jkczyz commented Nov 25, 2024

optout21 commented Dec 5, 2024

jkczyz commented Dec 5, 2024

optout21 commented Dec 5, 2024

optout21 commented Dec 5, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading