[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 using PR comments)
Other keywords and placeholders
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.
|
Updated to point to master. |
Roasbeef
force-pushed
the
rbf-coop-fsm
branch
from
f8b9c69 to
c2154b6
Compare
|
|
||
| // genChannelType generates various channel types, ensuring good coverage of | ||
| // different channel configurations including anchor outputs and other features | ||
| func genChannelType(t *rapid.T) channeldb.ChannelType { |
There was a problem hiding this comment.
From the rapid go page:
Compared to [testing.F.Fuzz](https://pkg.go.dev/testing#F.Fuzz), rapid shines in generating complex
structured data, including state machine tests, but lacks coverage-guided feedback and mutations. Note
that with [MakeFuzz](https://pkg.go.dev/pgregory.net/rapid#MakeFuzz), any rapid test can be used as a
fuzz target for the standard fuzzer.
It would be good to convert all of our rapid tests to fuzz tests. Rapid would be used to generate the initial data (I'm guessing it would be better than go's fuzzing engine for that) and then the fuzzer would mutate off of that.
|
@Roasbeef, remember to re-request review from reviewers when ready |
Roasbeef
force-pushed
the
rbf-coop-fsm
branch
from
53232fc to
3c6f2cb
Compare
|
Haven't looked too much into 4/4 yet, so I'm not sure if these items are addressed there. I do think the following should be addressed either here or in the next PR (especially points 2 & 3):
Besides that the PR looks good and can approve once CI passes |
Gotcha, the diagram in the OP should still be up to date. |
Looks like that's trying to handle a case where Alice has sent a shutdown with one script, but then wants to switch to another. I don't quite get that example though, Alice sent the script using the shutdown so she much send another one (after the round) if she wants to swap her script. Will reply on the spec. EDIT: commented about that here https://github.com/lightning/bolts/pull/1205/files#r1871743799 |
So the Not super familiar with it yet, but can look into adapting it, may not play will with the way |
Roasbeef
force-pushed
the
rbf-coop-fsm
branch
2 times, most recently
from
8013872 to
98c4d02
Compare
Roasbeef
force-pushed
the
rbf-coop-fsm
branch
from
98c4d02 to
fb1e4b3
Compare
We'll have the empty slice tuple represent the None case 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.
This preps us for an upcoming change to the rbf coop state machine where either party can pay for the channel fees. We also add a new test to make sure the new function adheres to some key properties.
In this commit, we update the core coop close logic with the new custom payer param. We also expand the existing unit tests to ensure that the fee is deducted from the proper party.
In this commit, we enable a custom payer for the rbf coop close. This allows us to ensure that the party that started one side of the close flow pays the fees.
Roasbeef
force-pushed
the
rbf-coop-fsm
branch
from
fb1e4b3 to
3c5f96d
Compare
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