Refactor ChannelState to decouple state flags from states #2691

wpaulino · 2023-10-26T23:10:58Z

Previously, our ChannelState contained bits for both states and flags. To make matters worse, some of the flags could apply to multiple states. This led to its API being very cumbersome, having to apply masks in most scenarios to check for certain states. As LDK grows and more features are added requiring more states/flags, the need for a simpler API arises.

This refactor aims to improve this by decoupling the state flags from the ChannelState enum. Each state that requires flags will now have its own flags type, to ensure flags can only be applied to their intended state. All of this is done while maintaining backwards and forwards compatibility.

While conceptualizing this design, I also explored two other options:

Using boolean flags to denote the different flags on a specific state: this did not mesh well with our serialization concerns, and having to check a set of booleans in specific scenarios doesn't really scale.
No enforcement of flags only being applied to their intended state: this results in a smaller diff with less boilerplate code, but I think the cost here is worth it to ensure correctness, especially as LDK matures and more features are added.

codecov-commenter · 2023-10-26T23:24:01Z

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (9942994) 88.64% compared to head (6492cc9) 88.49%.
Report is 20 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channel.rs	78.67%	49 Missing and 9 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2691      +/-   ##
==========================================
- Coverage   88.64%   88.49%   -0.15%     
==========================================
  Files         115      115              
  Lines       90446    90786     +340     
  Branches    90446    90786     +340     
==========================================
+ Hits        80173    80345     +172     
- Misses       7874     8022     +148     
- Partials     2399     2419      +20

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

dunxen

Concept ACK this implementation.

We're going to be touch a lot more of the state in splicing so it makes sense to have the extra type safety.

dunxen · 2023-10-31T07:33:16Z

lightning/src/ln/channel.rs

+	FundingCreated,
+	/// We've received/sent `funding_created` and `funding_signed` and are thus now waiting on the
+	/// funding transaction to confirm.
+	FundingSent(FundingSentFlags),


Can we now rename this to something more general like AwaitingConfirmation so that it can also apply to V2 channel establishment?

Yeah probably something like AwaitingChannelReady/FundingSigned since it also applies to zero conf.

Yeah that's better :)

dunxen · 2023-10-31T07:46:47Z

lightning/src/ln/channel.rs

+	/// We have sent `funding_created` and are awaiting a `funding_signed` to advance to
+	/// `FundingSent`. Note that this is nonsense for an inbound channel as we immediately generate
+	/// `funding_signed` upon receipt of `funding_created`, so simply skip this state.
+	FundingCreated,


I guess this could be used for both inbound and outbound channels in V2 establishment when the one party has sent the initial commitment_signed and not received the counterparty`s yet. I don't know what a good general name for that would be but happy to just use this state.

FundingNegotiated?

TheBlueMatt · 2023-11-14T23:16:50Z

lightning/src/ln/channel.rs

+
+define_state_flags!(
+	"Flags that only apply to [`ChannelState::FundingSent`].",
+	FUNDED_STATE, FundingSentFlags, [


I'm confused here, we can be PEER_DISCONNECTED and even MONITOR_UPDATE_IN_PROGRESS in FundingSent.

It's a bit subtle, but using the FUNDED_STATE arm of the macro includes all FundedStateFlags (PeerDisconnected, MonitorUpdateInProgress, LocalShutdownSent, RemoteShutdownSent) as well.

TheBlueMatt · 2023-12-05T22:35:06Z

Needs rebase.

Previously, our `ChannelState` contained bits for both states and flags. To make matters worse, some of the flags could apply to multiple states. This led to its API being very cumbersome, having to apply masks in most scenarios to check for certain states. As LDK grows and more features are added requiring more states/flags, the need for a simpler API arises. This refactor aims to improve this by decoupling the state flags from the `ChannelState` enum. Each state that requires flags will now have its own flags type, to ensure flags can only be applied to their intended state. All of this is done while maintaining backwards and forwards compatibility.

wpaulino · 2023-12-05T23:39:55Z

Rebased and also added a fixup for the flag renames @dunxen pointed out.

TheBlueMatt · 2023-12-06T19:07:18Z

IMO the second commit shouldn't be a fixup, renaming the flags should be a separate commit.

dunxen

LGTM. I don't believe there are any other additions required by V2 establishment, but after rebase I'll know for sure. Would belong in another PR anyway.

`FundingCreated` and `FundingSent` were mostly named after the respective `funding_created` and `funding_sent` wire messages. They include the signature for the initial commitment transaction when opening a channel. With dual funding, these messages are no longer used, and instead we rely on the existing `commitment_signed` to exchange those signatures.

TheBlueMatt

LGTM. With the new design I think we can/should push for more compile-time checking as well as more assertions of valid state, but it doesn't have to happen here.

TheBlueMatt · 2023-12-07T21:22:35Z

lightning/src/ln/channel.rs

+
+		impl core::ops::Not for $flag_type {
+			type Output = Self;
+			fn not(self) -> Self::Output { Self(!self.0) }


Do we want an invariant that only valid flags are set?

You can only set valid flags already though?

This method returns a flags object with a pile of invalid flags set.

I don't think this is safe because sometimes we use Not to obtain a mask, and we wouldn't want to clear out the other bits of the thing we're masking.

I mean maybe we should update the callsites to not depend on disallowed bits being set?

That's what I did in #2780.

lightning/src/ln/channel.rs

wpaulino added this to the 0.0.119 milestone Oct 26, 2023

dunxen self-assigned this Oct 27, 2023

wpaulino force-pushed the refactor-channel-state branch from aefe8cf to df1dd80 Compare October 27, 2023 15:58

dunxen reviewed Oct 31, 2023

View reviewed changes

wpaulino added the Seeking Code Review label Nov 3, 2023

TheBlueMatt reviewed Nov 14, 2023

View reviewed changes

wpaulino force-pushed the refactor-channel-state branch from df1dd80 to 312965b Compare November 27, 2023 20:36

wpaulino force-pushed the refactor-channel-state branch from 312965b to 158c14c Compare December 5, 2023 23:39

dunxen approved these changes Dec 7, 2023

View reviewed changes

wpaulino force-pushed the refactor-channel-state branch from 158c14c to 6492cc9 Compare December 7, 2023 20:39

TheBlueMatt approved these changes Dec 7, 2023

View reviewed changes

TheBlueMatt merged commit 6199406 into lightningdevkit:main Dec 7, 2023

wpaulino deleted the refactor-channel-state branch December 8, 2023 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ChannelState to decouple state flags from states #2691

Refactor ChannelState to decouple state flags from states #2691

wpaulino commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 •

edited

Loading

dunxen left a comment

dunxen Oct 31, 2023

wpaulino Oct 31, 2023

dunxen Nov 14, 2023

dunxen Oct 31, 2023

wpaulino Oct 31, 2023

TheBlueMatt Nov 14, 2023

wpaulino Nov 15, 2023

TheBlueMatt commented Dec 5, 2023

wpaulino commented Dec 5, 2023

TheBlueMatt commented Dec 6, 2023

dunxen left a comment

TheBlueMatt left a comment

TheBlueMatt Dec 7, 2023

wpaulino Dec 8, 2023

TheBlueMatt Dec 8, 2023

wpaulino Dec 8, 2023

TheBlueMatt Dec 11, 2023

wpaulino Dec 11, 2023

Refactor ChannelState to decouple state flags from states #2691

Refactor ChannelState to decouple state flags from states #2691

Conversation

wpaulino commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 • edited Loading

Codecov Report

dunxen 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

TheBlueMatt commented Dec 5, 2023

wpaulino commented Dec 5, 2023

TheBlueMatt commented Dec 6, 2023

dunxen left a comment

Choose a reason for hiding this comment

TheBlueMatt 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

codecov-commenter commented Oct 26, 2023 •

edited

Loading