Encapsulate `Channel` enum variants inside a struct #3550

jkczyz · 2025-01-17T20:51:33Z

Instead of exposing the Channel enum variants, make a Channel struct that wraps a ChannelPhase enum. This allows updating a Channel's phase without consuming it, which isn't possible when it is in a map without removing the entry first. Then move phase transitions can from ChannelManager to Channel. This allows for simpler logic in ChannelManager since the channel does not need to removed and then re-added into the channel_by_id map during a phase transition (or failed one).

optout21

Looks good (so far)

lightning/src/ln/channel.rs

jkczyz · 2025-01-21T19:49:27Z

Ok, AFAICT, the PR as-is should have changed all the possible phase transitions where we were removing and re-inserting into a map (except when going from a temporary channel id). Reviewers, please check that this is the case by looking at the commits introducing From trait implementations in #3513.

FYI, the changes in the last commit may interfere with #3423.

jkczyz · 2025-01-21T19:54:22Z

lightning/src/ln/channelmanager.rs

+						self.chain_monitor
+							.watch_channel(funded_chan.context.get_funding_txo().unwrap(), monitor)
+							.map_err(|()| {
 								// We weren't able to watch the channel to begin with, so no
 								// updates should be made on it. Previously, full_stack_target
 								// found an (unreachable) panic when the monitor update contained
 								// within `shutdown_finish` was applied.
-								chan.unset_funding_info(msg.channel_id);
-								return Err(convert_channel_err!(self, peer_state, e, chan, &msg.channel_id, FUNDED_CHANNEL).1);


I was a little confused by this comment given the earlier version removed the entry from the map, and this new version uses try_channel_entry to remove it on error below. i.e., Why do we need to call unset_funding_info if the channel is no longer in the map?

We only need to call it in cases where the new channel conflicts with an existing channel. When locked_close_channel eventually gets called on the new channel, it needs to have its funding info removed, otherwise we'll remove the mapping in outpoint_to_peer for the original channel, which is crucial for its handling of MonitorEvents. If we pursue #3554 all the way, that map can finally be removed.

wpaulino

Some other cases where I'd imagine a phase transition happens:

Failed splice negotiation on an already funded channel: not yet implemented, OK for now
Transaction reorg after confirmation: turns out we just force close anyway, and we don't transition back to unfunded regardless of the changes proposed here

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

codecov · 2025-01-22T18:08:27Z

Codecov Report

Attention: Patch coverage is 71.69043% with 139 lines in your changes missing coverage. Please review.

Project coverage is 89.54%. Comparing base (4579e63) to head (f3cd992).
Report is 83 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	50.34%	69 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	79.93%	58 Missing and 7 partials ⚠️
lightning/src/ln/functional_tests.rs	88.23%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3550      +/-   ##
==========================================
+ Coverage   88.57%   89.54%   +0.96%     
==========================================
  Files         149      149              
  Lines      114539   122358    +7819     
  Branches   114539   122358    +7819     
==========================================
+ Hits       101449   109561    +8112     
+ Misses      10602    10370     -232     
+ Partials     2488     2427      -61

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

lightning/src/ln/channel.rs

wpaulino

LGTM after squash

lightning/src/ln/channel.rs

jkczyz · 2025-01-23T22:06:38Z

LGTM after squash

@optout21 Let me know if you'd like to me to squash the fixups

carlaKC

New to the codebase so comments are all non-blocking, nice refactor!

lightning/src/ln/functional_tests.rs

lightning/src/ln/channel.rs

optout21

LGTM, OK to squash

jkczyz · 2025-01-24T23:06:28Z

Squashed. Looks like I need to rebase.

Now that ChannelPhase has been renamed, drop phase from related identifiers.

The old ChannelPhase variants will be used internally in Channel, so they should no longer be used elsewhere.

Now that ChannelPhase has been renamed, drop phase from related identifiers. Also, qualify uses of chan to avoid overloading the identifier.

Instead of exposing the Channel enum variants, make a Channel struct that wraps a ChannelPhase enum. This allows updating a Channel's phase without consuming it, which isn't possible when it is in a map without removing the entry first (e.g., as is done in ChannelManager).

When attempting a ChannelPhase transition, the variant-specific channel struct needs to be taken by self in order to move its ChannelContext into the struct for the new phase. Add a variant for an intermediate state, allowing such actions.

When moving ChannelPhase logic from ChannelManager into Channel, it is useful to error when a Channel is not in the expected state. Add a ChannelError::SendError variant for this purpose, which results in sending an error message without closing the channel.

Now that ChannelPhase is encapsulated in Channel, phase transitions can be moved from ChannelManager to Channel. Update the funding_signed phase transition accordingly. This allows for simpler logic in ChannelManager since the channel does not need to removed and then readded into the channel_by_id map.

Now that ChannelPhase is encapsulated in Channel, phase transitions can be moved from ChannelManager to Channel. Update the tx_complete phase transition accordingly. This allows for simpler logic in ChannelManager since the channel does not need to removed and then re-added into the channel_by_id map.

jkczyz · 2025-01-24T23:22:25Z

Rebased.

TheBlueMatt

Thanks

TheBlueMatt · 2025-01-27T21:06:59Z

lightning/src/ln/channel.rs

 /// its variants containing an appropriate channel struct.
-pub(super) enum Channel<SP: Deref> where SP::Target: SignerProvider {
+enum ChannelPhase<SP: Deref> where SP::Target: SignerProvider {
+	Undefined,


Bleh, I almost prefer unsafe { mem::uninitialized }, oh well.

TheBlueMatt · 2025-01-27T21:10:23Z

lightning/src/ln/channelmanager.rs

+			hash_map::Entry::Occupied(mut chan_entry) => {
+				let chan = chan_entry.get_mut();
+				match chan
+					.funding_signed(&msg, best_block, &self.signer_provider, &self.logger)


We need to standardize whether wrapping the logger happens in channelmanager or channel and make sure we're consistent...at some point

TheBlueMatt · 2025-01-27T21:15:06Z

lightning/src/ln/channelmanager.rs

+						// updates should be made on it. Previously, full_stack_target
+						// found an (unreachable) panic when the monitor update contained
+						// within `shutdown_finish` was applied.
+						chan.unset_funding_info();


We removed debug_assert!(matches!(e, ChannelError::Close(_)) which I think is actually important here since we don't re-add the channel.

This may be ChannelError::SendError if the channel wasn't in the expected phase.

Err, right, duh, we're no longer removing-and-adding so this is fine.

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from effabbb to 9773f1a Compare January 17, 2025 23:21

optout21 reviewed Jan 18, 2025

View reviewed changes

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

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

jkczyz added weekly goal Someone wants to land this this week and removed weekly goal Someone wants to land this this week labels Jan 21, 2025

jkczyz marked this pull request as ready for review January 21, 2025 19:49

jkczyz commented Jan 21, 2025

View reviewed changes

jkczyz added the weekly goal Someone wants to land this this week label Jan 21, 2025

wpaulino reviewed Jan 22, 2025

View reviewed changes

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

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

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from 9773f1a to f3cd992 Compare January 22, 2025 17:59

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from f3cd992 to e84d9d1 Compare January 23, 2025 18:31

wpaulino reviewed Jan 23, 2025

View reviewed changes

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

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from e84d9d1 to f7f5567 Compare January 23, 2025 20:13

wpaulino reviewed Jan 23, 2025

View reviewed changes

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

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from f7f5567 to 4a30d10 Compare January 23, 2025 22:05

carlaKC reviewed Jan 24, 2025

View reviewed changes

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

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

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from 4a30d10 to 22436bb Compare January 24, 2025 16:37

optout21 previously approved these changes Jan 24, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from 22436bb to 6639f0c Compare January 24, 2025 23:05

jkczyz added 8 commits January 24, 2025 17:08

Rename convert_chan_phase_err

efa1b35

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Rename break_chan_phase_entry

92bdf28

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Rename try_chan_phase_entry

a73c1f4

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Rename remove_channel_phase

38114d7

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Rename chan_phase_entry to chan_entry

7e95cba

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Rename chan_phase as appropriate

17b4972

Now that ChannelPhase has been renamed, drop phase from related identifiers.

Remove explicit use of Channel variants from tests

7c92614

The old ChannelPhase variants will be used internally in Channel, so they should no longer be used elsewhere.

Rename phase and qualify chan, as needed

ae651ea

Now that ChannelPhase has been renamed, drop phase from related identifiers. Also, qualify uses of chan to avoid overloading the identifier.

jkczyz added 5 commits January 24, 2025 17:19

Add ChannelPhase::Undefined variant

f8975d9

When attempting a ChannelPhase transition, the variant-specific channel struct needs to be taken by self in order to move its ChannelContext into the struct for the new phase. Add a variant for an intermediate state, allowing such actions.

jkczyz dismissed optout21’s stale review via b75ca39 January 24, 2025 23:21

jkczyz force-pushed the 2025-01-refactor-channel-phase branch from 6639f0c to b75ca39 Compare January 24, 2025 23:21

jkczyz mentioned this pull request Jan 27, 2025

Further decouple ChannelManager from Channel state somewhat #3539

Merged

wpaulino approved these changes Jan 27, 2025

View reviewed changes

TheBlueMatt approved these changes Jan 27, 2025

View reviewed changes

TheBlueMatt merged commit ed7befc into lightningdevkit:main Jan 27, 2025
24 of 25 checks passed

jkczyz mentioned this pull request Jan 29, 2025

Follow-ups to PR 3137 #3423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate `Channel` enum variants inside a struct #3550

Encapsulate `Channel` enum variants inside a struct #3550

jkczyz commented Jan 17, 2025 •

edited

Loading

optout21 left a comment

jkczyz commented Jan 21, 2025 •

edited

Loading

jkczyz Jan 21, 2025

wpaulino Jan 21, 2025

wpaulino left a comment

codecov bot commented Jan 22, 2025

wpaulino left a comment

jkczyz commented Jan 23, 2025

carlaKC left a comment

optout21 left a comment

jkczyz commented Jan 24, 2025

jkczyz commented Jan 24, 2025

TheBlueMatt left a comment

TheBlueMatt Jan 27, 2025

TheBlueMatt Jan 27, 2025

TheBlueMatt Jan 27, 2025

jkczyz Jan 27, 2025

TheBlueMatt Jan 28, 2025

Encapsulate Channel enum variants inside a struct #3550

Encapsulate Channel enum variants inside a struct #3550

Conversation

jkczyz commented Jan 17, 2025 • edited Loading

optout21 left a comment

Choose a reason for hiding this comment

jkczyz commented Jan 21, 2025 • edited Loading

jkczyz Jan 21, 2025

Choose a reason for hiding this comment

wpaulino Jan 21, 2025

Choose a reason for hiding this comment

wpaulino left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2025

Codecov Report

wpaulino left a comment

Choose a reason for hiding this comment

jkczyz commented Jan 23, 2025

carlaKC left a comment

Choose a reason for hiding this comment

optout21 left a comment

Choose a reason for hiding this comment

jkczyz commented Jan 24, 2025

jkczyz commented Jan 24, 2025

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Jan 27, 2025

Choose a reason for hiding this comment

TheBlueMatt Jan 27, 2025

Choose a reason for hiding this comment

TheBlueMatt Jan 27, 2025

Choose a reason for hiding this comment

jkczyz Jan 27, 2025

Choose a reason for hiding this comment

TheBlueMatt Jan 28, 2025

Choose a reason for hiding this comment

Encapsulate `Channel` enum variants inside a struct #3550

Encapsulate `Channel` enum variants inside a struct #3550

jkczyz commented Jan 17, 2025 •

edited

Loading

jkczyz commented Jan 21, 2025 •

edited

Loading