Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnwallet: update RBF state machine w/ latest spec guidelines #9568

Merged
159 changes: 152 additions & 7 deletions lnwallet/chancloser/rbf_coop_states.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ var (
// ErrCloserAndClosee is returned when we expect a sig covering both
// outputs, it isn't present.
ErrCloserAndClosee = fmt.Errorf("expected CloserAndClosee sig")

// ErrWrongLocalScript is returned when the remote party sends a
// ClosingComplete message that doesn't carry our last local script
// sent.
ErrWrongLocalScript = fmt.Errorf("wrong local script")
)

// ProtocolEvent is a special interface used to create the equivalent of a
Expand Down Expand Up @@ -382,7 +387,7 @@ type AsymmetricPeerState interface {
type ProtocolStates interface {
ChannelActive | ShutdownPending | ChannelFlushing | ClosingNegotiation |
LocalCloseStart | LocalOfferSent | RemoteCloseStart |
ClosePending | CloseFin
ClosePending | CloseFin | CloseErr
}

// ChannelActive is the base state for the channel closer state machine. In
Expand Down Expand Up @@ -449,6 +454,11 @@ type ShutdownPending struct {
// IdealFeeRate is the ideal fee rate we'd like to use for the closing
// attempt.
IdealFeeRate fn.Option[chainfee.SatPerVByte]

// EarlyRemoteOffer is the offer we received from the remote party
// before we received their shutdown message. We'll stash it to process
// later.
EarlyRemoteOffer fn.Option[OfferReceivedEvent]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a violation of the spec? Why should we be permissive in accepting something that we should explicitly reject?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale comes from the Robustness Principle: https://en.wikipedia.org/wiki/Robustness_principle. I can remove it ofc, but handling the early send case (they send their first offer before they recv our shutdown reply) can help us save an otherwise aborted flow.

Eclair had this behavior early on when we did interop. I think we need more interop hours/flows to conclude if it's safe to remove or not.

As mentioned, this was a flake in the itests (not added in this PR) that was due to some async send logic (when a send is conditional on some predicate) in the state machine executor. There're a few other options there including:

  1. Modify the state machine executor to do a sync test of the predicate, doing a blocking send if it's already true.
  2. Modify this state machine further to add an intermediate WaitingForSend state. Once we issue the send event, we shift into this state, then process another event once the send has finalized.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the messages were single-threaded we could not worry about this early stashing logic

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the lift like here? I really think it's a good idea to have it be single-threaded as I personally find it easier to reason about

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the transport layer the messages order are always guaranteed right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the lift like here? I really think it's a good idea to have it be single-threaded as I personally find it easier to reason about

They are single threaded. The exception is when you don't want a message to go out unconditionally, instead you want to wait for a condition to be upheld.

I've added a commit to do the first option.

I think it makes sense to still leave this in place, we've seen it pop up once during interop, and we still have 2 implementations to go before we finalize interop with all the major implementations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok I misunderstood how this worked.

}

// String returns the name of the state for ShutdownPending.
Expand Down Expand Up @@ -523,6 +533,13 @@ type ClosingNegotiation struct {
// the ShouldRouteTo method to determine which state route incoming
// events to.
PeerState lntypes.Dual[AsymmetricPeerState]

// CloseChannelTerms is the terms we'll use to close the channel. We
// hold a value here which is pointed to by the various
// AsymmetricPeerState instances. This allows us to update this value if
// the remote peer sends a new address, with each of the state noting
// the new value via a pointer.
*CloseChannelTerms
}

// String returns the name of the state for ClosingNegotiation.
Expand All @@ -542,6 +559,56 @@ func (c *ClosingNegotiation) IsTerminal() bool {
// protocolSealed indicates that this struct is a ProtocolEvent instance.
func (c *ClosingNegotiation) protocolStateSealed() {}

// ErrState can be used to introspect into a benign error related to a state
// transition.
type ErrState interface {
sealed()

error

// Err returns an error for the ErrState.
Err() error
}

// ErrStateCantPayForFee is sent when the local party attempts a fee update
// that they can't actually party for.
type ErrStateCantPayForFee struct {
localBalance btcutil.Amount

attemptedFee btcutil.Amount
}

// NewErrStateCantPayForFee returns a new NewErrStateCantPayForFee error.
func NewErrStateCantPayForFee(localBalance, attemptedFee btcutil.Amount,
) *ErrStateCantPayForFee {

return &ErrStateCantPayForFee{
localBalance: localBalance,
attemptedFee: attemptedFee,
}
}

// sealed makes this a sealed interface.
func (e *ErrStateCantPayForFee) sealed() {
}

// Err returns an error for the ErrState.
func (e *ErrStateCantPayForFee) Err() error {
return fmt.Errorf("cannot pay for fee of %v, only have %v local "+
"balance", e.attemptedFee, e.localBalance)
}

// Error returns the error string for the ErrState.
func (e *ErrStateCantPayForFee) Error() string {
return e.Err().Error()
}

// String returns the string for the ErrStateCantPayForFee.
func (e *ErrStateCantPayForFee) String() string {
return fmt.Sprintf("ErrStateCantPayForFee(local_balance=%v, "+
"attempted_fee=%v)", e.localBalance, e.attemptedFee)
}

// CloseChannelTerms is a set of terms that we'll use to close the channel. This
// includes the balances of the channel, and the scripts we'll use to send each
// party's funds to.
Expand All @@ -553,11 +620,11 @@ type CloseChannelTerms struct {

// DeriveCloseTxOuts takes the close terms, and returns the local and remote tx
// out for the close transaction. If an output is dust, then it'll be nil.
//
// TODO(roasbeef): add func for w/e heuristic to not manifest own output?
func (c *CloseChannelTerms) DeriveCloseTxOuts() (*wire.TxOut, *wire.TxOut) {
//nolint:ll
deriveTxOut := func(balance btcutil.Amount, pkScript []byte) *wire.TxOut {
// We'll base the existence of the output on our normal dust
// check.
dustLimit := lnwallet.DustLimitForSize(len(pkScript))
if balance >= dustLimit {
return &wire.TxOut{
Expand Down Expand Up @@ -618,7 +685,7 @@ func (c *CloseChannelTerms) RemoteCanPayFees(absoluteFee btcutil.Amount) bool {
// input events:
// - SendOfferEvent
type LocalCloseStart struct {
CloseChannelTerms
*CloseChannelTerms
}

// String returns the name of the state for LocalCloseStart, including proposed
Expand Down Expand Up @@ -658,11 +725,14 @@ func (l *LocalCloseStart) protocolStateSealed() {}
// input events:
// - LocalSigReceived
type LocalOfferSent struct {
CloseChannelTerms
*CloseChannelTerms

// ProposedFee is the fee we proposed to the remote party.
ProposedFee btcutil.Amount

// ProposedFeeRate is the fee rate we proposed to the remote party.
ProposedFeeRate chainfee.SatPerVByte

// LocalSig is the signature we sent to the remote party.
LocalSig lnwire.Sig
}
Expand Down Expand Up @@ -706,11 +776,27 @@ func (l *LocalOfferSent) IsTerminal() bool {
type ClosePending struct {
// CloseTx is the pending close transaction.
CloseTx *wire.MsgTx

*CloseChannelTerms

// FeeRate is the fee rate of the closing transaction.
FeeRate chainfee.SatPerVByte

// Party indicates which party is at this state. This is used to
// implement the state transition properly, based on ShouldRouteTo.
Party lntypes.ChannelParty
}

// String returns the name of the state for ClosePending.
func (c *ClosePending) String() string {
return fmt.Sprintf("ClosePending(txid=%v)", c.CloseTx.TxHash())
return fmt.Sprintf("ClosePending(txid=%v, party=%v, fee_rate=%v)",
c.CloseTx.TxHash(), c.Party, c.FeeRate)
}

// isType returns true if the value is of type T.
func isType[T any](value any) bool {
_, ok := value.(T)
return ok
}

// ShouldRouteTo returns true if the target state should process the target
Expand All @@ -720,6 +806,17 @@ func (c *ClosePending) ShouldRouteTo(event ProtocolEvent) bool {
case *SpendEvent:
return true
default:
switch {
case c.Party == lntypes.Local && isType[*SendOfferEvent](event):
return true

case c.Party == lntypes.Remote && isType[*OfferReceivedEvent](
event,
):

return true
}

return false
}
}
Expand Down Expand Up @@ -759,7 +856,7 @@ func (c *CloseFin) IsTerminal() bool {
// - fromState: ChannelFlushing
// - toState: ClosePending
type RemoteCloseStart struct {
CloseChannelTerms
*CloseChannelTerms
}

// String returns the name of the state for RemoteCloseStart.
Expand All @@ -786,6 +883,54 @@ func (l *RemoteCloseStart) IsTerminal() bool {
return false
}

// CloseErr is an error state in the protocol. We enter this state when a
// protocol constraint is violated, or an upfront sanity check fails.
type CloseErr struct {
ErrState

*CloseChannelTerms

// Party indicates which party is at this state. This is used to
// implement the state transition properly, based on ShouldRouteTo.
Party lntypes.ChannelParty
}

// String returns the name of the state for CloseErr, including error and party
// details.
func (c *CloseErr) String() string {
return fmt.Sprintf("CloseErr(Party: %v, Error: %v)", c.Party, c.Err())
}

// ShouldRouteTo returns true if the target state should process the target
// event.
func (c *CloseErr) ShouldRouteTo(event ProtocolEvent) bool {
switch event.(type) {
case *SpendEvent:
return true
default:
switch {
case c.Party == lntypes.Local && isType[*SendOfferEvent](event):
return true

case c.Party == lntypes.Remote && isType[*OfferReceivedEvent](
event,
):

return true
}

return false
}
}

// protocolStateSealed indicates that this struct is a ProtocolEvent instance.
func (c *CloseErr) protocolStateSealed() {}

// IsTerminal returns true if the target state is a terminal state.
func (c *CloseErr) IsTerminal() bool {
return true
}

// RbfChanCloser is a state machine that handles the RBF-enabled cooperative
// channel close protocol.
type RbfChanCloser = protofsm.StateMachine[ProtocolEvent, *Environment]
Expand Down
Loading
Loading