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

multi: implement spec-compliant coop close #6760

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions htlcswitch/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,12 @@ type ChannelLinkConfig struct {
// is called. It takes a quit chan that is used as a precaution to
// avoid deadlock.
NotifyCoopReady func(chanPoint *wire.OutPoint, quit chan struct{})

// RetransmitShutdown is a boolean that indicates whether the link
// should retransmit a Shutdown during the Reestablish flow. This
// automatically starts the link in "shutdown mode" so that new HTLCs
// are not accepted or sent.
RetransmitShutdown bool
}

// channelLink is the service which drives a channel's commitment update
Expand Down Expand Up @@ -825,6 +831,25 @@ func (l *channelLink) syncChanStates() error {
}
}

// If we have been signaled to retransmit Shutdown, we'll do so
// here.
if l.cfg.RetransmitShutdown {
// Recreate the Shutdown message.
shutdownMsg := lnwire.NewShutdown(
l.ChanID(),
l.channel.LocalUpfrontShutdownScript(),
)

// Flip the shutdownInit bool.
l.shutdownInit = true

err = l.cfg.Peer.SendMessage(false, shutdownMsg)
if err != nil {
return fmt.Errorf("unable to re-send "+
"Shutdown: %v", err)
}
}

// In any case, we'll then process their ChanSync message.
l.log.Info("received re-establishment message from remote side")

Expand Down
67 changes: 67 additions & 0 deletions htlcswitch/link_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6342,6 +6342,73 @@ func TestPendingCommitTicker(t *testing.T) {
}
}

// TestShutdownRetransmit tests that the link is able to properly retransmit a
// Shutdown message if it restarts.
func TestShutdownRetransmit(t *testing.T) {
t.Parallel()

// Create the three hop network even though we'll only be using Alice
// and Bob.
const aliceInitialBalance = btcutil.SatoshiPerBitcoin * 3
channels, _, err := createClusterChannels(
t, aliceInitialBalance, btcutil.SatoshiPerBitcoin*5,
)
require.NoError(t, err)

n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice,
channels.bobToCarol, channels.carolToBob, testStartingHeight)

// Set up the message interceptors to ensure the proper messages are
// sent and received in the correct order.
chanID := n.aliceChannelLink.ChanID()
messages := []expectedMessage{
{"alice", "bob", &lnwire.ChannelReestablish{}, false},
{"bob", "alice", &lnwire.ChannelReestablish{}, false},

{"alice", "bob", &lnwire.FundingLocked{}, false},
{"bob", "alice", &lnwire.FundingLocked{}, false},

{"alice", "bob", &lnwire.Shutdown{}, false},
{"bob", "alice", &lnwire.Shutdown{}, false},
}
n.aliceServer.intersect(createInterceptorFunc("[alice] <-- [bob]",
"alice", messages, chanID, false))
n.bobServer.intersect(createInterceptorFunc("[alice] --> [bob]", "bob",
messages, chanID, false))

closeReqChan := make(chan *ChanClose, 2)
notifyShutdown := func(req *ChanClose, quit chan struct{}) {
select {
case closeReqChan <- req:
default:
}
}

coopReadyChan := make(chan struct{}, 2)
notifyCoopReady := func(chanPoint *wire.OutPoint, quit chan struct{}) {
select {
case coopReadyChan <- struct{}{}:
default:
}
}

// Set the closure functions for Alice and Bob.
n.aliceChannelLink.cfg.NotifySendingShutdown = notifyShutdown
n.aliceChannelLink.cfg.NotifyCoopReady = notifyCoopReady
n.firstBobChannelLink.cfg.NotifySendingShutdown = notifyShutdown
n.firstBobChannelLink.cfg.NotifyCoopReady = notifyCoopReady

// Set the RetransmitShutdown config option for Alice.
n.aliceChannelLink.cfg.RetransmitShutdown = true

err = n.start()
require.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

i think we need to check l.shutdownInit == true to test if it's retransmitted.

defer n.stop()
defer func() {
_ = n.feeEstimator.Stop()
}()
}

// TestCustomShutdownScript tests that it's possible to set a custom shutdown
// script when using the NotifyShouldShutdown call.
func TestCustomShutdownScript(t *testing.T) {
Expand Down
43 changes: 42 additions & 1 deletion peer/brontide.go
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,47 @@ func (p *Brontide) addLink(chanPoint *wire.OutPoint,
return err
}

chanID := lnwire.NewChanIDFromOutPoint(chanPoint)

// If we need to retransmit Shutdown, create a ChanCloser and give it
// the Shutdown message.
retransmitShutdown := lnChan.State().HasSentShutdown()
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to retransmit shutdown when the channel has sent shutdown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will retransmit shutdown if we've ever sent shutdown in the past

Copy link
Member

Choose a reason for hiding this comment

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

Does it mean we are sending multiple shutdowns? Suppose trySendingShutdown succeeds, the node restarts, then it will send another shutdown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is ok as long as it's not in the same p2p connection

if retransmitShutdown {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do NotifySendingShutdown in link instead?

feePerKw, err := p.cfg.FeeEstimator.EstimateFeePerKW(
yyforyongyu marked this conversation as resolved.
Show resolved Hide resolved
p.cfg.CoopCloseTargetConfs,
)
if err != nil {
return err
}

// Since we've sent Shutdown, the delivery script is already
// persisted. We won't know if it's locally initiated or not.
// At this stage, this is fine so we set it to true.
chanCloser, err := p.createChanCloser(
lnChan, lnChan.LocalUpfrontShutdownScript(), feePerKw,
nil, true,
)
if err != nil {
peerLog.Errorf("unable to create chan closer: %v", err)
return err
}

p.activeChanCloses[chanID] = chanCloser

shutdownMsg := lnwire.NewShutdown(
chanID, lnChan.LocalUpfrontShutdownScript(),
)

// Give the Shutdown message to the ChanCloser without sending
// it. The link will send it during retransmission.
_, _, err = chanCloser.ProcessCloseMsg(shutdownMsg, false)
if err != nil {
peerLog.Errorf("unable to process shutdown: %v", err)
delete(p.activeChanCloses, chanID)
return err
}
}

//nolint:lll
linkCfg := htlcswitch.ChannelLinkConfig{
Peer: p,
Expand Down Expand Up @@ -1014,13 +1055,13 @@ func (p *Brontide) addLink(chanPoint *wire.OutPoint,
DeliveryAddr: deliveryAddr,
NotifySendingShutdown: p.HandleLocalCloseChanReqs,
NotifyCoopReady: p.HandleCoopReady,
RetransmitShutdown: retransmitShutdown,
}

// Before adding our new link, purge the switch of any pending or live
// links going by the same channel id. If one is found, we'll shut it
// down to ensure that the mailboxes are only ever under the control of
// one link.
chanID := lnwire.NewChanIDFromOutPoint(chanPoint)
p.cfg.Switch.RemoveLink(chanID)

// With the channel link created, we'll now notify the htlc switch so
Expand Down