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

[optout21]

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]

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

[dunxen]

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]

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]

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]

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.

[jkczyz]

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

[optout21]

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]

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.

[optout21]

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.

[jkczyz]

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.

[optout21]

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.

[jkczyz]

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

[optout21]

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]

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.