Clean up and split up is_pre_funded_state
#4021
Conversation
`Channel::is_pre_funded_state` is used to mean several different things. In this case, its used to skip all the `shutdown` logic as the funding transaction can't possibly have been broadcasted so there's really no ned to try to sign a transaction spending it. Here, we really want to capture any channel in `NegotiatedFunding` or any V1 channel in `FundingNegotiated` or, finally, any V2 channel in `FundingNegotiated` where we haven't yet sent our signatures (which is not captured in `is_pre_funded_state`). Instead of a new helper, we just check the states directly in `shutdown` handling.
`Channel::is_pre_funded_state` is used to mean several different things. In this case its used to decide if we should provide a `ChannelMonitorUpdate` marking a channel as closed when we go to force-close it. Here, we want to capture exactly when the original `ChannelMonitor` is first created, but were doing so indirectly by looking at the channel's state. Worse, `is_pre_funded_state` got updated to be false whenever there is an interctive signing session, which isn't correct for this use - we may have an interactive signing session but have already persisted the original `ChannelMonitor` when we received the first `commitment_signed`. Instead, we just move to examining `cur_counterparty_commitment_transaction_number` which is decrementing for the first time at exactly the time we create the original `ChannelMonitor`, so it provides a much simpler test. Fixes lightningdevkit#3880
`Channel::is_pre_funded_state` is used to mean several different things. In the past few commits we stopped using it for a few conflicting uses, but here we break out the remaining uses and rename the remnants for clarity. `is_funding_broadcast` was using `is_pre_funded_state` and was then later used to decide if the `Channel` could be written to disk (because it can be resumed on restart), if we should broadcast a force-close transaction, and when to emit a `ChannelPending` event. These were also somewhat divergent - we shouldn't generate a `ChannelReady` event or broadcast a force-closing transaction until we've actually broadcasted but want to write the `Channel` to disk once we enter funding signature exchange for dual-funded open. Thus, the ability to write a `Channel` to disk is provided by a new `can_resume_on_restart` method. Then, `is_funding_broadcast` is updated to only consider funding broadcasted after we provide our funding signatures (i.e. the funding *could* have been broadcasted). This is still a bit early to generate a `ChannelPending` event (as the funding may not actually have been broadcasted yet), but its better than it was. Finally, the remaining `is_pre_funded_state` is renamed `can_resume_on_reconnect`, which has slightly different semantics than on-restart channels in batch opens.
|
👋 Thanks for assigning @wpaulino as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
==========================================
- Coverage 88.84% 88.77% -0.07%
==========================================
Files 175 175
Lines 127760 127862 +102
Branches 127760 127862 +102
==========================================
+ Hits 113510 113514 +4
- Misses 11679 11795 +116
+ Partials 2571 2553 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -9107,13 +9107,20 @@ where | |||
| "Peer sent shutdown when we needed a channel_reestablish".to_owned(), | |||
| )); | |||
| } | |||
| if self.context.channel_state.is_pre_funded_state() { | |||
| let mut not_broadcasted = | |||
| matches!(self.context.channel_state, ChannelState::NegotiatingFunding(_)); | |||
There was a problem hiding this comment.
We'll also be in this state during splices, so it seems we need more context before attempting to consider it "not broadcasted"
There was a problem hiding this comment.
If we disconnect during this state with a splice, we need to go back to ChannelReady
There was a problem hiding this comment.
Probably that just implies that we should not convert to NegotiatingFunding during splices? We have too many things that depend on the state and it seems weird to jump back to the "pre-funding" state when we're not actually pre-funding.
There was a problem hiding this comment.
We have too many things that depend on the state
How does the same not apply to us being in FundingNegotiated?
it seems weird to jump back to the "pre-funding" state when we're not actually pre-funding
Wouldn't it be better to think about it in terms of whether the channel can be resumed after a disconnect/restart instead of being funded or not? That seems to better align with some of the changes here. I see "pre-funding" more ambiguous now that a v2 channel could be considered pre-funding both before the funding transaction is built and before signatures are exchanged for it.
There was a problem hiding this comment.
How does the same not apply to us being in FundingNegotiated?
I meant that we have a lot of things that look at state, and previously FundingNegotiated was part of the "this is pre-funding" and now its sometimes not pre-funding, which is a really weird change to me?
Wouldn't it be better to think about it in terms of whether the channel can be resumed after a disconnect/restart instead of being funded or not?
But it can be resumed, just not the splice itself. I think that's kinda my point here - it feels way more like a post-funding channel that is just paused (quiescent).
There was a problem hiding this comment.
I meant that we have a lot of things that look at state, and previously FundingNegotiated was part of the "this is pre-funding" and now its sometimes not pre-funding, which is a really weird change to me?
I'm confused then, you think it's a weird change but we're doing it anyway and somehow excluding NegotiatingFunding from following the same pattern?
But it can be resumed, just not the splice itself. I think that's kinda my point here - it feels way more like a post-funding channel that is just paused (quiescent).
Then shouldn't we be relying on whether we're post-funding based on the ChannelPhase and not the ChannelState? ChannelState predates all of this so it made sense back then but not anymore. Why should ChannelState maintain its logical ordering when ChannelPhase has kind of fulfilled that role already?
There was a problem hiding this comment.
Then shouldn't we be relying on whether we're post-funding based on the ChannelPhase and not the ChannelState? ChannelState predates all of this so it made sense back then but not anymore. Why should ChannelState maintain its logical ordering when ChannelPhase has kind of fulfilled that role already?
Last discussion I remember we kinda decided ChannelPhase doesn't work and we wanted to get rid of it. Its by no means set, but ChannelPhase has really turned out to be incredibly cumbersome and I'm kinda skeptical of trying to invest in it further. ChannelState has, at least up until the change you're describing here, been totally well-ordered with flags representing the non-ordered stuff.
@jkczyz may have an opinion here too.
There was a problem hiding this comment.
Regarding ChannelPhase, I don't think we wanted to remove it entirely but rather make it an internal detail of Channel, which it now is. The structs held by the phases are valuable to expose as they restrict which methods can be called and what state is available (e.g., splice operations and pending splices are only available on FundedChannel).
There was a problem hiding this comment.
Regarding ChannelPhase, I don't think we wanted to remove it entirely but rather make it an internal detail of Channel, which it now is.
No its not? channelmanager.rs has a ton of "convert state check" nonsense all over the place, with a lot of associated error handling that belongs in channel.rs/Channel.
The structs held by the phases are valuable to expose as they restrict which methods can be called and what state is available (e.g., splice operations and pending splices are only available on FundedChannel).
I'm really unconvinced by this now that we have it. We went down the same path with the interactive transaction builder, first making it an API that channel.rs would access directly, then realizing its actually really not adding much and wrapping it so that channel.rs can always access methods and failing them inside the interactive transaction builder, resulting in a bunch more code there without any real value (you still have to handle the error case of "we received a message for a state it cannot apply to", you just move around where that handling is, in this case to a worse place!).
A state-transition enum makes a lot of sense if there's really a lot of fields that are only exist in some states and that can result in removing a lot of unwraps to access fields that are guaranteed to be set in a state but not available before it. We don't have that with Channel, we have nearly everything in ChannelContext, and still have all the unwraps. Worse, its not like we even have different things in the scope structs - PendingV2Channel has a FundingScope just like FundedChannel (probably implying it should be in the global context), has an interactive signing session (just like FundedChannel, again maybe it should be in the global context...) and really there's barely any difference at all.
Worse still, we have two separate concepts of the "channel's state" - the ChannelState enum, which is granular and useful, and the struct-level thing which is not granular, doesn't save unwraps, and now because its duplicated state can represent a lot of invalid cases where the states conflict. Now, that's not to say that the state enum thing can't be useful - we could actually invest in it, basically removing the top level ChannelState enum, moving those to different structs in the top-level Channel enum, and moving the flags inside of them, allowing, for example, the removal of the ChannelContext in the pre-funded states and making lots of Options in the ChannelContext required. Or we could walk it back and just use the ChannelState everywhere robustly.
At the end of the day, the only unacceptable option is the state were in right now where we have two concepts of state that are very different and don't use either in a robust or reliable way.
There was a problem hiding this comment.
No its not?
channelmanager.rshas a ton of "convert state check" nonsense all over the place, with a lot of associated error handling that belongs inchannel.rs/Channel.
Right, my point was the matches on ChannelPhase were removed and in some cases pushed into Channel. In other cases, we do something like as_funded.
I'm really unconvinced by this now that we have it. We went down the same path with the interactive transaction builder, first making it an API that
channel.rswould access directly, then realizing its actually really not adding much and wrapping it so thatchannel.rscan always access methods and failing them inside the interactive transaction builder, resulting in a bunch more code there without any real value (you still have to handle the error case of "we received a message for a state it cannot apply to", you just move around where that handling is, in this case to a worse place!).
Was referring mainly to uses of as_funded, et al. There the checks when receiving a message remain in channelmanager.rs.
A state-transition enum makes a lot of sense if there's really a lot of fields that are only exist in some states and that can result in removing a lot of
unwraps to access fields that are guaranteed to be set in a state but not available before it. We don't have that withChannel, we have nearly everything inChannelContext, and still have all theunwraps. Worse, its not like we even have different things in the scope structs -PendingV2Channelhas aFundingScopejust likeFundedChannel(probably implying it should be in the global context), has an interactive signing session (just likeFundedChannel, again maybe it should be in the global context...) and really there's barely any difference at all.Worse still, we have two separate concepts of the "channel's state" - the
ChannelStateenum, which is granular and useful, and the struct-level thing which is not granular, doesn't save unwraps, and now because its duplicated state can represent a lot of invalid cases where the states conflict. Now, that's not to say that the state enum thing can't be useful - we could actually invest in it, basically removing the top levelChannelStateenum, moving those to different structs in the top-levelChannelenum, and moving the flags inside of them, allowing, for example, the removal of theChannelContextin the pre-funded states and making lots ofOptions in theChannelContextrequired. Or we could walk it back and just use theChannelStateeverywhere robustly.
FWIW, we don't have a top-level Channel enum anymore. We have a Channel struct containing a ChannelPhase enum. Totally, agree about your points about invalid cases and making fewer Options in ChannelContext.
Currently, there are a ton of places in channelmaner.rs that want ChannelContext regardless of the phase. So we'd need to tackle untangling that. Additionally, each channel sub-struct (e.g., FundedChannel) typically wants to access some field or method on ChannelContext.
At the end of the day, the only unacceptable option is the state were in right now where we have two concepts of state that are very different and don't use either in a robust or reliable way.
Yeah, I tend to agree. Let's discuss offline.
| ChannelState::NegotiatingFunding(_) => true, | ||
| ChannelState::FundingNegotiated(flags) => !flags.is_interactive_signing(), | ||
| _ => false, | ||
| ChannelState::NegotiatingFunding(_) => false, |
There was a problem hiding this comment.
Same comment here regarding splices
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Discussed this offline yesterday and I believe we concluded this is probably the right way to go (keeping the |
This turns out to have been kinda buggy, and impossible to read the code, so clean it up a bunch.