Encapsulate `Channel` enum variants inside a struct by jkczyz · Pull Request #3550 · lightningdevkit/rust-lightning

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)

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

+ 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

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.

wpaulino

LGTM after squash

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!

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

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

+ 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

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

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

+ 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

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.

wpaulino

LGTM after squash

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!

optout21

LGTM, OK to squash

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