From 7621d7f9020c8a9fc2fa4692d07056cfacc8c69e Mon Sep 17 00:00:00 2001 From: eugene Date: Tue, 10 Aug 2021 16:56:45 -0400 Subject: [PATCH 1/5] lnwallet: add IsChannelClean method and related tests Adds a method to the LightningChannel struct called IsChannelClean that returns a boolean telling the caller whether the channel state is clean or not. Clean in this case means there are no lingering updates to be signed for, no HTLC's active on either sides commitment transaction, and no pending commitments on either side. This can be used for dynamic commitments or during a strict cooperative close process that ensures atomicity of the channel. --- lnwallet/channel.go | 44 ++++++++++++ lnwallet/channel_test.go | 147 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 56991bd3f..9143238b0 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4470,6 +4470,50 @@ func (lc *LightningChannel) ReceiveNewCommitment(commitSig lnwire.Sig, return nil } +// IsChannelClean returns true if neither side has pending commitments, neither +// side has HTLC's, and all updates are locked in irrevocably. Internally, it +// utilizes the oweCommitment function by calling it for local and remote +// evaluation. We check if we have a pending commitment for our local state +// since this function may be called by sub-systems that are not the link (e.g. +// the rpcserver), and the ReceiveNewCommitment & RevokeCurrentCommitment calls +// are not atomic, even though link processing ensures no updates can happen in +// between. +func (lc *LightningChannel) IsChannelClean() bool { + lc.RLock() + defer lc.RUnlock() + + // Check whether we have a pending commitment for our local state. + if lc.localCommitChain.hasUnackedCommitment() { + return false + } + + // Check whether our counterparty has a pending commitment for their + // state. + if lc.remoteCommitChain.hasUnackedCommitment() { + return false + } + + // We call ActiveHtlcs to ensure there are no HTLCs on either + // commitment. + if len(lc.channelState.ActiveHtlcs()) != 0 { + return false + } + + // Now check that both local and remote commitments are signing the + // same updates. + if lc.oweCommitment(true) { + return false + } + + if lc.oweCommitment(false) { + return false + } + + // If we reached this point, the channel has no HTLCs and both + // commitments sign the same updates. + return true +} + // OweCommitment returns a boolean value reflecting whether we need to send // out a commitment signature because there are outstanding local updates and/or // updates in the local commit tx that aren't reflected in the remote commit tx diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 6f190ade7..c201cc787 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9669,3 +9669,150 @@ func TestMayAddOutgoingHtlcZeroValue(t *testing.T) { require.NoError(t, aliceChannel.MayAddOutgoingHtlc()) require.NoError(t, bobChannel.MayAddOutgoingHtlc()) } + +// TestIsChannelClean tests that IsChannelClean returns the expected values +// in different channel states. +func TestIsChannelClean(t *testing.T) { + t.Parallel() + + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + channeldb.ZeroHtlcTxFeeBit, + ) + require.NoError(t, err) + defer cleanUp() + + // Channel state should be clean at the start of the test. + assertCleanOrDirty(true, aliceChannel, bobChannel, t) + + // Assert that neither side considers the channel clean when alice + // sends an htlc. + // ---add---> + htlc, preimage := createHTLC(0, lnwire.MilliSatoshi(5000000)) + _, err = aliceChannel.AddHTLC(htlc, nil) + require.NoError(t, err) + _, err = bobChannel.ReceiveHTLC(htlc) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // Assert that the channel remains dirty until the HTLC is completely + // removed from both commitments. + + // ---sig---> + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() + require.NoError(t, err) + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---rev--- + bobRevocation, _, err := bobChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---sig--- + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() + require.NoError(t, err) + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // ---rev---> + aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <--settle-- + err = bobChannel.SettleHTLC(preimage, 0, nil, nil, nil) + require.NoError(t, err) + err = aliceChannel.ReceiveHTLCSettle(preimage, 0) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---sig--- + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() + require.NoError(t, err) + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // ---rev---> + aliceRevocation, _, err = aliceChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // ---sig---> + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() + require.NoError(t, err) + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---rev--- + bobRevocation, _, err = bobChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + require.NoError(t, err) + assertCleanOrDirty(true, aliceChannel, bobChannel, t) + + // Now we check that update_fee is handled and state is dirty until it + // is completely locked in. + // ---fee---> + fee := chainfee.SatPerKWeight(333) + err = aliceChannel.UpdateFee(fee) + require.NoError(t, err) + err = bobChannel.ReceiveUpdateFee(fee) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // ---sig---> + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() + require.NoError(t, err) + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---rev--- + bobRevocation, _, err = bobChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // <---sig--- + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() + require.NoError(t, err) + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + require.NoError(t, err) + assertCleanOrDirty(false, aliceChannel, bobChannel, t) + + // The state should finally be clean after alice sends her revocation. + // ---rev---> + aliceRevocation, _, err = aliceChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + require.NoError(t, err) + assertCleanOrDirty(true, aliceChannel, bobChannel, t) +} + +// assertCleanOrDirty is a helper function that asserts that both channels are +// clean if clean is true, and dirty if clean is false. +func assertCleanOrDirty(clean bool, alice, bob *LightningChannel, + t *testing.T) { + + t.Helper() + + if clean { + require.True(t, alice.IsChannelClean()) + require.True(t, bob.IsChannelClean()) + return + } + + require.False(t, alice.IsChannelClean()) + require.False(t, bob.IsChannelClean()) +} From b2e90480edf5d845f01fa3848a78f7aa4bde4a2e Mon Sep 17 00:00:00 2001 From: eugene Date: Tue, 10 Aug 2021 16:59:17 -0400 Subject: [PATCH 2/5] htlcswitch: extend ChannelLink interface with ShutdownIfChannelClean This allows a caller to ensure to optimistically shut down the link if the channel is clean. If the channel is not clean, an error is returned and the link continues functioning as normal. The caller should also call RemoveLink to ensure that the link isn't seen as usable within the switch. --- htlcswitch/interfaces.go | 5 +++ htlcswitch/link.go | 61 +++++++++++++++++++++++++--- htlcswitch/link_test.go | 85 +++++++++++++++++++++++++++++++++++++++ htlcswitch/linkfailure.go | 3 ++ htlcswitch/mock.go | 1 + 5 files changed, 149 insertions(+), 6 deletions(-) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index d2d93232d..b899e6a90 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -88,6 +88,11 @@ type ChannelUpdateHandler interface { // MayAddOutgoingHtlc returns an error if we may not add an outgoing // htlc to the channel. MayAddOutgoingHtlc() error + + // ShutdownIfChannelClean shuts the link down if the channel state is + // clean. This can be used with dynamic commitment negotiation or coop + // close negotiation which require a clean channel state. + ShutdownIfChannelClean() error } // ChannelLink is an interface which represents the subsystem for managing the diff --git a/htlcswitch/link.go b/htlcswitch/link.go index eed8ba316..2760c42bc 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -301,6 +301,13 @@ type localUpdateAddMsg struct { err chan error } +// shutdownReq contains an error channel that will be used by the channelLink +// to send an error if shutdown failed. If shutdown succeeded, the channel will +// be closed. +type shutdownReq struct { + err chan error +} + // channelLink is the service which drives a channel's commitment update // state-machine. In the event that an HTLC needs to be propagated to another // link, the forward handler from config is used which sends HTLC to the @@ -369,6 +376,10 @@ type channelLink struct { // sub-systems with the latest set of active HTLC's on our channel. htlcUpdates chan *contractcourt.ContractUpdate + // shutdownRequest is a channel that the channelLink will listen on to + // service shutdown requests from ShutdownIfChannelClean calls. + shutdownRequest chan *shutdownReq + // updateFeeTimer is the timer responsible for updating the link's // commitment fee every time it fires. updateFeeTimer *time.Timer @@ -414,12 +425,13 @@ func NewChannelLink(cfg ChannelLinkConfig, channel: channel, shortChanID: channel.ShortChanID(), // TODO(roasbeef): just do reserve here? - htlcUpdates: make(chan *contractcourt.ContractUpdate), - hodlMap: make(map[channeldb.CircuitKey]hodlHtlc), - hodlQueue: queue.NewConcurrentQueue(10), - log: build.NewPrefixLog(logPrefix, log), - quit: make(chan struct{}), - localUpdateAdd: make(chan *localUpdateAddMsg), + htlcUpdates: make(chan *contractcourt.ContractUpdate), + shutdownRequest: make(chan *shutdownReq), + hodlMap: make(map[channeldb.CircuitKey]hodlHtlc), + hodlQueue: queue.NewConcurrentQueue(10), + log: build.NewPrefixLog(logPrefix, log), + quit: make(chan struct{}), + localUpdateAdd: make(chan *localUpdateAddMsg), } } @@ -1176,6 +1188,20 @@ func (l *channelLink) htlcManager() { return } + case req := <-l.shutdownRequest: + // If the channel is clean, we send nil on the err chan + // and return to prevent the htlcManager goroutine from + // processing any more updates. The full link shutdown + // will be triggered by RemoveLink in the peer. + if l.channel.IsChannelClean() { + req.err <- nil + return + } + + // Otherwise, the channel has lingering updates, send + // an error and continue. + req.err <- ErrLinkFailedShutdown + case <-l.quit: return } @@ -2434,6 +2460,29 @@ func (l *channelLink) HandleChannelUpdate(message lnwire.Message) { l.mailBox.AddMessage(message) } +// ShutdownIfChannelClean triggers a link shutdown if the channel is in a clean +// state and errors if the channel has lingering updates. +// +// NOTE: Part of the ChannelUpdateHandler interface. +func (l *channelLink) ShutdownIfChannelClean() error { + errChan := make(chan error, 1) + + select { + case l.shutdownRequest <- &shutdownReq{ + err: errChan, + }: + case <-l.quit: + return ErrLinkShuttingDown + } + + select { + case err := <-errChan: + return err + case <-l.quit: + return ErrLinkShuttingDown + } +} + // updateChannelFee updates the commitment fee-per-kw on this channel by // committing to an update_fee message. func (l *channelLink) updateChannelFee(feePerKw chainfee.SatPerKWeight) error { diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index e685a3e21..d228b9509 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -6532,6 +6532,91 @@ func TestPendingCommitTicker(t *testing.T) { } } +// TestShutdownIfChannelClean tests that a link will exit the htlcManager loop +// if and only if the underlying channel state is clean. +func TestShutdownIfChannelClean(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, batchTicker, start, cleanUp, _, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + require.NoError(t, err) + + var ( + coreLink = aliceLink.(*channelLink) + aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs + ) + + shutdownAssert := func(expectedErr error) { + err = aliceLink.ShutdownIfChannelClean() + if expectedErr != nil { + require.Error(t, err, expectedErr) + } else { + require.NoError(t, err) + } + } + + err = start() + require.NoError(t, err) + defer cleanUp() + + ctx := linkTestContext{ + t: t, + aliceLink: aliceLink, + bobChannel: bobChannel, + aliceMsgs: aliceMsgs, + } + + // First send an HTLC from Bob to Alice and assert that the link can't + // be shutdown while the update is outstanding. + htlc := generateHtlc(t, coreLink, 0) + + // <---add----- + ctx.sendHtlcBobToAlice(htlc) + // <---sig----- + ctx.sendCommitSigBobToAlice(1) + // ----rev----> + ctx.receiveRevAndAckAliceToBob() + shutdownAssert(ErrLinkFailedShutdown) + + // ----sig----> + ctx.receiveCommitSigAliceToBob(1) + shutdownAssert(ErrLinkFailedShutdown) + + // <---rev----- + ctx.sendRevAndAckBobToAlice() + shutdownAssert(ErrLinkFailedShutdown) + + // ---settle--> + ctx.receiveSettleAliceToBob() + shutdownAssert(ErrLinkFailedShutdown) + + // ----sig----> + ctx.receiveCommitSigAliceToBob(0) + shutdownAssert(ErrLinkFailedShutdown) + + // <---rev----- + ctx.sendRevAndAckBobToAlice() + shutdownAssert(ErrLinkFailedShutdown) + + // <---sig----- + ctx.sendCommitSigBobToAlice(0) + shutdownAssert(ErrLinkFailedShutdown) + + // ----rev----> + ctx.receiveRevAndAckAliceToBob() + shutdownAssert(nil) + + // Now that the link has exited the htlcManager loop, attempt to + // trigger the batch ticker. It should not be possible. + select { + case batchTicker <- time.Now(): + t.Fatalf("expected batch ticker to be inactive") + case <-time.After(5 * time.Second): + } +} + // assertFailureCode asserts that an error is of type ClearTextError and that // the failure code is as expected. func assertFailureCode(t *testing.T, err error, code lnwire.FailCode) { diff --git a/htlcswitch/linkfailure.go b/htlcswitch/linkfailure.go index 32aa83ade..2f4be531e 100644 --- a/htlcswitch/linkfailure.go +++ b/htlcswitch/linkfailure.go @@ -5,6 +5,9 @@ import "github.com/go-errors/errors" var ( // ErrLinkShuttingDown signals that the link is shutting down. ErrLinkShuttingDown = errors.New("link shutting down") + + // ErrLinkFailedShutdown signals that a requested shutdown failed. + ErrLinkFailedShutdown = errors.New("link failed to shutdown") ) // errorCode encodes the possible types of errors that will make us fail the diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index fb97c54e9..fd2964465 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -757,6 +757,7 @@ func (f *mockChannelLink) ChannelPoint() *wire.OutPoint { return func (f *mockChannelLink) Stop() {} func (f *mockChannelLink) EligibleToForward() bool { return f.eligible } func (f *mockChannelLink) MayAddOutgoingHtlc() error { return nil } +func (f *mockChannelLink) ShutdownIfChannelClean() error { return nil } func (f *mockChannelLink) setLiveShortChanID(sid lnwire.ShortChannelID) { f.shortChanID = sid } func (f *mockChannelLink) UpdateShortChanID() (lnwire.ShortChannelID, error) { f.eligible = true From 608d11dcbc6b0cfe4441951aeaf1a5655c2bbb06 Mon Sep 17 00:00:00 2001 From: eugene Date: Thu, 12 Aug 2021 12:34:30 -0400 Subject: [PATCH 3/5] peer: add mockUpdateHandler for use in mockMessageSwitch --- peer/brontide_test.go | 10 +++++----- peer/test_utils.go | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/peer/brontide_test.go b/peer/brontide_test.go index 2141a945f..588090878 100644 --- a/peer/brontide_test.go +++ b/peer/brontide_test.go @@ -40,7 +40,7 @@ func TestPeerChannelClosureAcceptFeeResponder(t *testing.T) { broadcastTxChan := make(chan *wire.MsgTx) alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, + notifier, broadcastTxChan, noUpdate, nil, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -143,7 +143,7 @@ func TestPeerChannelClosureAcceptFeeInitiator(t *testing.T) { broadcastTxChan := make(chan *wire.MsgTx) alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, + notifier, broadcastTxChan, noUpdate, nil, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -265,7 +265,7 @@ func TestPeerChannelClosureFeeNegotiationsResponder(t *testing.T) { broadcastTxChan := make(chan *wire.MsgTx) alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, + notifier, broadcastTxChan, noUpdate, nil, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -459,7 +459,7 @@ func TestPeerChannelClosureFeeNegotiationsInitiator(t *testing.T) { broadcastTxChan := make(chan *wire.MsgTx) alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, + notifier, broadcastTxChan, noUpdate, nil, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -795,7 +795,7 @@ func TestCustomShutdownScript(t *testing.T) { // Open a channel. alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, test.update, + notifier, broadcastTxChan, test.update, nil, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) diff --git a/peer/test_utils.go b/peer/test_utils.go index b6cf16b06..1d63a67bf 100644 --- a/peer/test_utils.go +++ b/peer/test_utils.go @@ -54,7 +54,8 @@ var noUpdate = func(a, b *channeldb.OpenChannel) {} // an updateChan function which can be used to modify the default values on // the channel states for each peer. func createTestPeer(notifier chainntnfs.ChainNotifier, - publTx chan *wire.MsgTx, updateChan func(a, b *channeldb.OpenChannel)) ( + publTx chan *wire.MsgTx, updateChan func(a, b *channeldb.OpenChannel), + mockSwitch *mockMessageSwitch) ( *Brontide, *lnwallet.LightningChannel, func(), error) { aliceKeyPriv, aliceKeyPub := btcec.PrivKeyFromBytes( @@ -306,7 +307,11 @@ func createTestPeer(notifier chainntnfs.ChainNotifier, }, } - htlcSwitch := &mockMessageSwitch{} + // If mockSwitch is not set by the caller, set it to the default as the + // caller does not need to control it. + if mockSwitch == nil { + mockSwitch = &mockMessageSwitch{} + } nodeSignerAlice := netann.NewNodeSigner(aliceKeySigner) @@ -349,7 +354,7 @@ func createTestPeer(notifier chainntnfs.ChainNotifier, PubKeyBytes: pubKey, ErrorBuffer: errBuffer, ChainIO: chainIO, - Switch: htlcSwitch, + Switch: mockSwitch, ChanActiveTimeout: chanActiveTimeout, InterceptSwitch: htlcswitch.NewInterceptableSwitch(nil), @@ -404,6 +409,37 @@ func (m *mockMessageSwitch) GetLinksByInterface(pub [33]byte) ( return nil, nil } +// mockUpdateHandler is a mock implementation of the ChannelUpdateHandler +// interface. It is used in mockMessageSwitch's GetLinksByInterface method. +type mockUpdateHandler struct { + cid lnwire.ChannelID +} + +// newMockUpdateHandler creates a new mockUpdateHandler. +func newMockUpdateHandler(cid lnwire.ChannelID) *mockUpdateHandler { + return &mockUpdateHandler{ + cid: cid, + } +} + +// HandleChannelUpdate currently does nothing. +func (m *mockUpdateHandler) HandleChannelUpdate(msg lnwire.Message) {} + +// ChanID returns the mockUpdateHandler's cid. +func (m *mockUpdateHandler) ChanID() lnwire.ChannelID { return m.cid } + +// Bandwidth currently returns a dummy value. +func (m *mockUpdateHandler) Bandwidth() lnwire.MilliSatoshi { return 0 } + +// EligibleToForward currently returns a dummy value. +func (m *mockUpdateHandler) EligibleToForward() bool { return false } + +// MayAddOutgoingHtlc currently returns nil. +func (m *mockUpdateHandler) MayAddOutgoingHtlc() error { return nil } + +// ShutdownIfChannelClean currently returns nil. +func (m *mockUpdateHandler) ShutdownIfChannelClean() error { return nil } + type mockMessageConn struct { t *testing.T From aeaa009e92b61d1d934813066a976c3393a2cca7 Mon Sep 17 00:00:00 2001 From: eugene Date: Tue, 10 Aug 2021 17:00:51 -0400 Subject: [PATCH 4/5] peer+chancloser: tryLinkShutdown during cooperative close process Adds a new Brontide struct method tryLinkShutdown that attempts to fetch the target link and calls ShutdownIfChannelClean on it. This allows the coop close process to guarantee atomicity of the underlying channel state. Also removes the UnregisterChannel method from the chancloser's config as the link is shut down before the chancloser is created. --- lnwallet/chancloser/chancloser.go | 11 ---- peer/brontide.go | 97 ++++++++++++++++++++++--------- peer/brontide_test.go | 43 +++++++++++--- peer/test_utils.go | 8 ++- 4 files changed, 110 insertions(+), 49 deletions(-) diff --git a/lnwallet/chancloser/chancloser.go b/lnwallet/chancloser/chancloser.go index 0fe643bcf..2eaa83288 100644 --- a/lnwallet/chancloser/chancloser.go +++ b/lnwallet/chancloser/chancloser.go @@ -79,11 +79,6 @@ type ChanCloseCfg struct { // Channel is the channel that should be closed. Channel *lnwallet.LightningChannel - // UnregisterChannel is a function closure that allows the ChanCloser to - // unregister a channel. Once this has been done, no further HTLC's should - // be routed through the channel. - UnregisterChannel func(lnwire.ChannelID) - // BroadcastTx broadcasts the passed transaction to the network. BroadcastTx func(*wire.MsgTx, string) error @@ -212,8 +207,6 @@ func (c *ChanCloser) initChanShutdown() (*lnwire.Shutdown, error) { // closing script. shutdown := lnwire.NewShutdown(c.cid, c.localDeliveryScript) - // TODO(roasbeef): err if channel has htlc's? - // Before closing, we'll attempt to send a disable update for the channel. // We do so before closing the channel as otherwise the current edge policy // won't be retrievable from the graph. @@ -222,10 +215,6 @@ func (c *ChanCloser) initChanShutdown() (*lnwire.Shutdown, error) { c.chanPoint, err) } - // Before returning the shutdown message, we'll unregister the channel to - // ensure that it isn't seen as usable within the system. - c.cfg.UnregisterChannel(c.cid) - // Before continuing, mark the channel as cooperatively closed with a nil // txn. Even though we haven't negotiated the final txn, this guarantees // that our listchannels rpc will be externally consistent, and reflect diff --git a/peer/brontide.go b/peer/brontide.go index 695a09a87..c3c3dd754 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -1184,11 +1184,9 @@ func waitUntilLinkActive(p *Brontide, // The link may already be active by this point, and we may have missed the // ActiveLinkEvent. Check if the link exists. - links, _ := p.cfg.Switch.GetLinksByInterface(p.cfg.PubKeyBytes) - for _, link := range links { - if link.ChanID() == cid { - return link - } + link := p.fetchLinkFromKeyAndCid(cid) + if link != nil { + return link } // If the link is nil, we must wait for it to be active. @@ -1216,16 +1214,7 @@ func waitUntilLinkActive(p *Brontide, // The link shouldn't be nil as we received an // ActiveLinkEvent. If it is nil, we return nil and the // calling function should catch it. - links, _ = p.cfg.Switch.GetLinksByInterface( - p.cfg.PubKeyBytes, - ) - for _, link := range links { - if link.ChanID() == cid { - return link - } - } - - return nil + return p.fetchLinkFromKeyAndCid(cid) case <-p.quit: return nil @@ -2408,12 +2397,11 @@ func (p *Brontide) fetchActiveChanCloser(chanID lnwire.ChannelID) ( // cooperative channel closure. chanCloser, ok := p.activeChanCloses[chanID] if !ok { - // If we need to create a chan closer for the first time, then - // we'll check to ensure that the channel is even in the proper - // state to allow a co-op channel closure. - if len(channel.ActiveHtlcs()) != 0 { - return nil, fmt.Errorf("cannot co-op close " + - "channel w/ active htlcs") + // Optimistically try a link shutdown, erroring out if it + // failed. + if err := p.tryLinkShutdown(chanID); err != nil { + peerLog.Errorf("failed link shutdown: %v", err) + return nil, err } // We'll create a valid closing state machine in order to @@ -2453,9 +2441,8 @@ func (p *Brontide) fetchActiveChanCloser(chanID lnwire.ChannelID) ( chanCloser = chancloser.NewChanCloser( chancloser.ChanCloseCfg{ - Channel: channel, - UnregisterChannel: p.cfg.Switch.RemoveLink, - BroadcastTx: p.cfg.Wallet.PublishTransaction, + Channel: channel, + BroadcastTx: p.cfg.Wallet.PublishTransaction, DisableChannel: func(chanPoint wire.OutPoint) error { return p.cfg.ChanStatusMgr.RequestDisable(chanPoint, false) }, @@ -2569,11 +2556,18 @@ func (p *Brontide) handleLocalCloseReq(req *htlcswitch.ChanClose) { return } + // Optimistically try a link shutdown, erroring out if it + // failed. + if err := p.tryLinkShutdown(chanID); err != nil { + peerLog.Errorf("failed link shutdown: %v", err) + req.Err <- err + return + } + chanCloser := chancloser.NewChanCloser( chancloser.ChanCloseCfg{ - Channel: channel, - UnregisterChannel: p.cfg.Switch.RemoveLink, - BroadcastTx: p.cfg.Wallet.PublishTransaction, + Channel: channel, + BroadcastTx: p.cfg.Wallet.PublishTransaction, DisableChannel: func(chanPoint wire.OutPoint) error { return p.cfg.ChanStatusMgr.RequestDisable(chanPoint, false) }, @@ -2700,6 +2694,55 @@ func (p *Brontide) handleLinkFailure(failure linkFailureReport) { } } +// tryLinkShutdown attempts to fetch a target link from the switch, calls +// ShutdownIfChannelClean to optimistically trigger a link shutdown, and +// removes the link from the switch. It returns an error if any step failed. +func (p *Brontide) tryLinkShutdown(cid lnwire.ChannelID) error { + // Fetch the appropriate link and call ShutdownIfChannelClean to ensure + // no other updates can occur. + chanLink := p.fetchLinkFromKeyAndCid(cid) + + // If the link happens to be nil, return ErrChannelNotFound so we can + // ignore the close message. + if chanLink == nil { + return ErrChannelNotFound + } + + // Else, the link exists, so attempt to trigger shutdown. If this + // fails, we'll send an error message to the remote peer. + if err := chanLink.ShutdownIfChannelClean(); err != nil { + return err + } + + // Next, we remove the link from the switch to shut down all of the + // link's goroutines and remove it from the switch's internal maps. We + // don't call WipeChannel as the channel must still be in the + // activeChannels map to process coop close messages. + p.cfg.Switch.RemoveLink(cid) + + return nil +} + +// fetchLinkFromKeyAndCid fetches a link from the switch via the remote's +// public key and the channel id. +func (p *Brontide) fetchLinkFromKeyAndCid( + cid lnwire.ChannelID) htlcswitch.ChannelUpdateHandler { + + var chanLink htlcswitch.ChannelUpdateHandler + + // We don't need to check the error here, and can instead just loop + // over the slice and return nil. + links, _ := p.cfg.Switch.GetLinksByInterface(p.cfg.PubKeyBytes) + for _, link := range links { + if link.ChanID() == cid { + chanLink = link + break + } + } + + return chanLink +} + // finalizeChanClosure performs the final clean up steps once the cooperative // closure transaction has been fully broadcast. The finalized closing state // machine should be passed in. Once the transaction has been sufficiently diff --git a/peer/brontide_test.go b/peer/brontide_test.go index 588090878..01a5f0c89 100644 --- a/peer/brontide_test.go +++ b/peer/brontide_test.go @@ -39,8 +39,10 @@ func TestPeerChannelClosureAcceptFeeResponder(t *testing.T) { } broadcastTxChan := make(chan *wire.MsgTx) + mockSwitch := &mockMessageSwitch{} + alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, nil, + notifier, broadcastTxChan, noUpdate, mockSwitch, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -49,6 +51,9 @@ func TestPeerChannelClosureAcceptFeeResponder(t *testing.T) { chanID := lnwire.NewChanIDFromOutPoint(bobChan.ChannelPoint()) + mockLink := newMockUpdateHandler(chanID) + mockSwitch.links = append(mockSwitch.links, mockLink) + // We send a shutdown request to Alice. She will now be the responding // node in this shutdown procedure. We first expect Alice to answer // this shutdown request with a Shutdown message. @@ -142,14 +147,20 @@ func TestPeerChannelClosureAcceptFeeInitiator(t *testing.T) { } broadcastTxChan := make(chan *wire.MsgTx) + mockSwitch := &mockMessageSwitch{} + alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, nil, + notifier, broadcastTxChan, noUpdate, mockSwitch, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) } defer cleanUp() + chanID := lnwire.NewChanIDFromOutPoint(bobChan.ChannelPoint()) + mockLink := newMockUpdateHandler(chanID) + mockSwitch.links = append(mockSwitch.links, mockLink) + // We make Alice send a shutdown request. updateChan := make(chan interface{}, 1) errChan := make(chan error, 1) @@ -179,7 +190,6 @@ func TestPeerChannelClosureAcceptFeeInitiator(t *testing.T) { aliceDeliveryScript := shutdownMsg.Address // Bob will respond with his own Shutdown message. - chanID := shutdownMsg.ChannelID alicePeer.chanCloseMsgs <- &closeMsg{ cid: chanID, msg: lnwire.NewShutdown(chanID, @@ -264,8 +274,10 @@ func TestPeerChannelClosureFeeNegotiationsResponder(t *testing.T) { } broadcastTxChan := make(chan *wire.MsgTx) + mockSwitch := &mockMessageSwitch{} + alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, nil, + notifier, broadcastTxChan, noUpdate, mockSwitch, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -274,6 +286,9 @@ func TestPeerChannelClosureFeeNegotiationsResponder(t *testing.T) { chanID := lnwire.NewChanIDFromOutPoint(bobChan.ChannelPoint()) + mockLink := newMockUpdateHandler(chanID) + mockSwitch.links = append(mockSwitch.links, mockLink) + // Bob sends a shutdown request to Alice. She will now be the responding // node in this shutdown procedure. We first expect Alice to answer this // Shutdown request with a Shutdown message. @@ -458,14 +473,20 @@ func TestPeerChannelClosureFeeNegotiationsInitiator(t *testing.T) { } broadcastTxChan := make(chan *wire.MsgTx) + mockSwitch := &mockMessageSwitch{} + alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, noUpdate, nil, + notifier, broadcastTxChan, noUpdate, mockSwitch, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) } defer cleanUp() + chanID := lnwire.NewChanIDFromOutPoint(bobChan.ChannelPoint()) + mockLink := newMockUpdateHandler(chanID) + mockSwitch.links = append(mockSwitch.links, mockLink) + // We make the initiator send a shutdown request. updateChan := make(chan interface{}, 1) errChan := make(chan error, 1) @@ -496,7 +517,6 @@ func TestPeerChannelClosureFeeNegotiationsInitiator(t *testing.T) { aliceDeliveryScript := shutdownMsg.Address // Bob will answer the Shutdown message with his own Shutdown. - chanID := lnwire.NewChanIDFromOutPoint(bobChan.ChannelPoint()) respShutdown := lnwire.NewShutdown(chanID, dummyDeliveryScript) alicePeer.chanCloseMsgs <- &closeMsg{ cid: chanID, @@ -793,20 +813,27 @@ func TestCustomShutdownScript(t *testing.T) { } broadcastTxChan := make(chan *wire.MsgTx) + mockSwitch := &mockMessageSwitch{} + // Open a channel. alicePeer, bobChan, cleanUp, err := createTestPeer( - notifier, broadcastTxChan, test.update, nil, + notifier, broadcastTxChan, test.update, + mockSwitch, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) } defer cleanUp() + chanPoint := bobChan.ChannelPoint() + chanID := lnwire.NewChanIDFromOutPoint(chanPoint) + mockLink := newMockUpdateHandler(chanID) + mockSwitch.links = append(mockSwitch.links, mockLink) + // Request initiator to cooperatively close the channel, with // a specified delivery address. updateChan := make(chan interface{}, 1) errChan := make(chan error, 1) - chanPoint := bobChan.ChannelPoint() closeCommand := htlcswitch.ChanClose{ CloseType: htlcswitch.CloseRegular, ChanPoint: chanPoint, diff --git a/peer/test_utils.go b/peer/test_utils.go index 1d63a67bf..3ce1cbe03 100644 --- a/peer/test_utils.go +++ b/peer/test_utils.go @@ -380,7 +380,9 @@ func createTestPeer(notifier chainntnfs.ChainNotifier, // mockMessageSwitch is a mock implementation of the messageSwitch interface // used for testing without relying on a *htlcswitch.Switch in unit tests. -type mockMessageSwitch struct{} +type mockMessageSwitch struct { + links []htlcswitch.ChannelUpdateHandler +} // BestHeight currently returns a dummy value. func (m *mockMessageSwitch) BestHeight() uint32 { @@ -402,11 +404,11 @@ func (m *mockMessageSwitch) CreateAndAddLink(cfg htlcswitch.ChannelLinkConfig, return nil } -// GetLinksByInterface currently returns dummy values. +// GetLinksByInterface returns the active links. func (m *mockMessageSwitch) GetLinksByInterface(pub [33]byte) ( []htlcswitch.ChannelUpdateHandler, error) { - return nil, nil + return m.links, nil } // mockUpdateHandler is a mock implementation of the ChannelUpdateHandler From 868958d2bca223ef2e843ef82b97c0b19e13c070 Mon Sep 17 00:00:00 2001 From: eugene Date: Wed, 11 Aug 2021 11:59:56 -0400 Subject: [PATCH 5/5] docs: update release notes for 0.14 --- docs/release-notes/release-notes-0.14.0.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.0.md b/docs/release-notes/release-notes-0.14.0.md index 92f4b0be7..5987450c3 100644 --- a/docs/release-notes/release-notes-0.14.0.md +++ b/docs/release-notes/release-notes-0.14.0.md @@ -112,6 +112,8 @@ you. * Locally force closed channels are now [kept in the channel.backup file until their time lock has fully matured](https://github.com/lightningnetwork/lnd/pull/5528). +* [Cooperative closes optimistically shutdown the associated `link` before closing the channel.](https://github.com/lightningnetwork/lnd/pull/5618) + ## Build System * [A new pre-submit check has been