[3/4] - lnwallet/chancloser: add new protofsm based RBF chan closer #8512
Conversation
Roasbeef
added
channel closing
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
|
Hi @Roasbeef can you please explain the scenario where you're expecting the state to transition back from Given that the channel flushing has already happened before the state moved to |
|
@saubyk good question, so I need to update the diagram slightly, but you're right in that at that point the channel is already flushed. With the code that's checked in, there's a special fast path that'll transition from Here's the location of one of the fast path transitions: So in that case we sent shutdown, received it, then rather than wait in the Also just to clarify, the way the protocol works is that when either side wants to do a new RBF iteration (RBF their closing txn), they send the I kept things like this rather than introducing some new states to keep the amount of total states to a minimum, and re-use more of the existing transition logic. As an example, if they send a |
Roasbeef
force-pushed
the
peer-msg-router
branch
from
b973aae
to
b8a229f
Compare
Roasbeef
force-pushed
the
protofsm
branch
2 times, most recently
from
fa12732
to
578288e
Compare
Roasbeef
changed the title
Roasbeef
changed the title
There was a problem hiding this comment.
First pass. This seems to implement the CCV2 protocol as I understand it, without issue. I did find the code hard to review though due to having to track the semantics of protofsm. I believe aspects of protofsm can be simplified specifically in how it pertains to emitting events.
In my ideal world we would make simplifying protofsm a prerequisite for merging this, but as I understand it, the deadline trumps fixing protofsm? I'd like others' input here. What I like here is that all of the state transitions are very clearly pure and I don't have any difficulty tracking whether or not the implementation of the states/state transitions is valid. However, some of the other operational semantics in how it ferries events out of it into the surrounding context, specifically as it pertains to internal events, can be complex.
|
|
||
| c.EarlyRemoteOffer = fn.Some(*msg) | ||
|
|
||
| // TODO(roasbeef): unit test! |
There was a problem hiding this comment.
Still needs to be done. Marking to remember.
| func validateShutdownScript(upfrontScript, peerScript lnwire.DeliveryAddress, | ||
| netParams *chaincfg.Params) error { |
There was a problem hiding this comment.
Was this signature change unrelated? I'm having a hard time figuring out how it relates to the rest of this commit.
There was a problem hiding this comment.
Looking back, the change was to remove the usage of Disconnect. As it should be the job of the higher layer to decide if we need to disconnect or not, we already return a discernible error.
| // then also the flushing event. | ||
| return &CloseStateTransition{ | ||
| NextState: &ShutdownPending{ | ||
| prevState: c, |
There was a problem hiding this comment.
Is it possible for us to memleak with this type of a construction? I see us building up an ever larger execution trace, here. I think that since this is the closing state machine, we will drive towards final termination so it's not particularly concerning, but I wonder what happens for much longer lived state machines.
There was a problem hiding this comment.
Good question, I think assuming the root ref to the state machine (which in the next PR lives in a map in the peer) is eligible for collection, then the entire state machine would be.
TBH, I don't think prevState is even used anywhere. The original idea was so the state new the traversal path it took to arrive at the current location, but it isn't used at all anymore. The main idea was to use the struct to add some information re what the state transition graph looks like. I tried with some type param stuff, but it got hairy and made things more difficult to move.
Will consider just moving to comment template.
| // TODO(roasbeef): assert same one set based on type, will be | ||
| // invalid otherwise anyway? |
There was a problem hiding this comment.
yeah the sig won't validate if it isn't the same one we did.
| // variant of the co-op close tx. | ||
| // | ||
| // TODO(roasbeef): db will only store one instance -- which is ok? | ||
| err = env.ChanObserver.MarkCoopBroadcasted(closeTx, true) |
There was a problem hiding this comment.
What does true here signify?
There was a problem hiding this comment.
If it was locally initiated or not.
| default: | ||
|
|
||
| return &CloseStateTransition{ | ||
| NextState: c, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Why not error here like we do elsewhere to mark invalid state transitions? I think we should stay consistent about whether we silently drop the transition or we return an error. I think mixing approaches will lead to confusion.
There was a problem hiding this comment.
Further, this seems to imply that a new Shutdown here won't kick the process off again.
| if !r.isExpectedChanID(msg.ChannelID) { | ||
| return fn.None[ProtocolEvent]() | ||
| } |
There was a problem hiding this comment.
invert these inquiries and consolidate all of the None cases.
| // TODO(roasbeef): not working? need custom checker? | ||
| /*functionalOpt := mock.FunctionalOptions( | ||
| lnwallet.WithCustomSequence(sequence), | ||
| )*/ |
There was a problem hiding this comment.
Heh this was that bug fix for taproot chans. With the latest version though, we'll use lock time instead.
In this commit, we add the initial set of states for the new protofsm based rbf chan closer. A diagram outlining the new states and their transitions can be found here: https://gist.github.com/Roasbeef/acc4ff51b9dff127230228a05553cdfe. Unlike the existing co-op close process, this co-op close can be restarted at anytime if either side sends a shutdown message. From there, we'll each obtain a new RBF'd version that can be re-broadcasted. This commit creates the set of states, along with the environment that our state machine will use to drive itself forward.
In this commit, we add the ability to specify a custom sequence for a co-op close tx. This'll come in handy later as the new co-op close process allows a party to set a custom sequence.
In this commit, we add the state transitions for the new protofsm based RBF chan closer. The underlying protocol is a new asymmetric co-op close process, wherein either side can initiate a chan closer, and use their settled funds to pay for fees within the channel.
This'll allow us to treat the state machine as a MsgEndpoint, and have the readHandler in the peer automatically send new messages to it.
|
@yyforyongyu: review reminder |
Overview
In this PR, we add a new RBF based co-op channel close state machine. This uses the new
protofsmpackage to define the states and transitions for the state machine. The new state machine is then also updated to become apeer.MsgEndpointwhich allows us to use the newpeer.MsgRouterto handle all message dispatch into the state machine. A series of new wrapper structs (taking care to always accept interfaces to decouple from the concrete types) are created to be able to initialize the environment needed by the new state machine.This PR doesn't yet include integration within the peer struct. That will come in a follow up PR as we need some additional changes to be able to handle the two possible co-op types that will exist.
The diff might look somewhat large, but over half of it us unit tests, and a large portion of the non-test diff is the new state and event declarations.
State Machine Flow
The new state machine can be modeled with the following diagram:
--- title: Co-Op Close V2 --- stateDiagram-v2 state CoopCloseV2 { [*] --> ChannelActive ChannelActive --> ShutdownPending: send_shutdown ChannelActive --> ShutdownPending: shutdown_received ShutdownPending --> ChannelFlushing: shutdown_received ShutdownPending --> ChannelFlushing: shutdown_complete ChannelFlushing --> ClosingNegotiation: channel_flushed state ClosingNegotiation { direction TB [*] --> LocalCloseStart [*] --> RemoteCloseStart LocalCloseStart --> LocalOfferSent: send_offer LocalOfferSent --> ClosePending: local_sig_received RemoteCloseStart --> ClosePending: offer_received } ClosingNegotiation --> ShutdownPending: send_shutdown ClosingNegotiation --> ShutdownPending: shutdown_received ClosingNegotiation --> CloseFin: txn_confirmation }Remaining TODOs
Fixes #7091