From cb5fc716594bd40364bf758a86af1b7e54650f08 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 19 May 2023 17:16:25 -0700 Subject: [PATCH 1/2] htlcswitch: add new LinkFailureAction enum In this commit, we add a new LinkFailureAction enum to take over the old force close bool. Force closing isn't the only thing we might want to do when we decide to fail the link, so this is a prep refactoring for an upcoming change. --- htlcswitch/link.go | 18 +++++++++--------- htlcswitch/link_test.go | 9 +++++---- htlcswitch/linkfailure.go | 18 +++++++++++++++--- peer/brontide.go | 9 ++++----- 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 5886baaa0..feb0a7a8b 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1036,8 +1036,8 @@ func (l *channelLink) htlcManager() { // storing the transaction in the db. l.fail( LinkFailureError{ - code: ErrSyncError, - ForceClose: true, + code: ErrSyncError, + FailureAction: LinkFailureForceClose, // nolint:lll }, "unable to synchronize channel "+ "states: %v", err, @@ -1077,8 +1077,8 @@ func (l *channelLink) htlcManager() { l.fail( LinkFailureError{ - code: ErrRecoveryError, - ForceClose: false, + code: ErrRecoveryError, + FailureAction: LinkFailureForceNone, }, "unable to synchronize channel "+ "states: %v", err, @@ -1782,8 +1782,8 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { if err := l.channel.ReceiveHTLCSettle(pre, idx); err != nil { l.fail( LinkFailureError{ - code: ErrInvalidUpdate, - ForceClose: true, + code: ErrInvalidUpdate, + FailureAction: LinkFailureForceClose, }, "unable to handle upstream settle HTLC: %v", err, ) @@ -1947,9 +1947,9 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { } l.fail( LinkFailureError{ - code: ErrInvalidCommitment, - ForceClose: true, - SendData: sendData, + code: ErrInvalidCommitment, + FailureAction: LinkFailureForceClose, + SendData: sendData, }, "ChannelPoint(%v): unable to accept new "+ "commitment: %v", diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 72afa2ebb..c461ab33e 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -5457,8 +5457,9 @@ func TestChannelLinkFail(t *testing.T) { // If we expect the link to force close the channel in this // case, check that it happens. If not, make sure it does not // happen. - require.Equal( - t, test.shouldForceClose, linkErr.ForceClose, test.name, + isForceCloseErr := (linkErr.FailureAction == LinkFailureForceClose) + require.True( + t, test.shouldForceClose == isForceCloseErr, test.name, ) require.Equal( t, test.permanentFailure, linkErr.PermanentFailure, @@ -6523,7 +6524,7 @@ func TestPipelineSettle(t *testing.T) { // ForceClose should be false. select { case linkErr := <-linkErrors: - require.False(t, linkErr.ForceClose) + require.False(t, linkErr.FailureAction == LinkFailureForceClose) case <-forwardChan: t.Fatal("packet was erroneously forwarded") } @@ -6559,7 +6560,7 @@ func TestPipelineSettle(t *testing.T) { // ForceClose should be false. select { case linkErr := <-linkErrors: - require.False(t, linkErr.ForceClose) + require.False(t, linkErr.FailureAction == LinkFailureForceClose) case <-forwardChan: t.Fatal("packet was erroneously forwarded") } diff --git a/htlcswitch/linkfailure.go b/htlcswitch/linkfailure.go index 1f454a7bb..58da01189 100644 --- a/htlcswitch/linkfailure.go +++ b/htlcswitch/linkfailure.go @@ -53,6 +53,19 @@ const ( ErrCircuitError ) +// LinkFailureAction is an enum-like type that describes the action that should +// be taken in response to a link failure. +type LinkFailureAction uint8 + +const ( + // LinkFailureForceNone indicates no action is to be taken. + LinkFailureForceNone LinkFailureAction = iota + + // LinkFailureForceClose indicates that the channel should be force + // closed. + LinkFailureForceClose +) + // LinkFailureError encapsulates an error that will make us fail the current // link. It contains the necessary information needed to determine if we should // force close the channel in the process, and if any error data should be sent @@ -61,9 +74,8 @@ type LinkFailureError struct { // code is the type of error this LinkFailureError encapsulates. code errorCode - // ForceClose indicates whether we should force close the channel - // because of this error. - ForceClose bool + // FailureAction describes what we should do to fail the channel. + FailureAction LinkFailureAction // PermanentFailure indicates whether this failure is permanent, and // the channel should not be attempted loaded again. diff --git a/peer/brontide.go b/peer/brontide.go index 08cad15de..964054a6e 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -3094,11 +3094,10 @@ func (p *Brontide) handleLinkFailure(failure linkFailureReport) { // being applied. p.WipeChannel(&failure.chanPoint) - // If the error encountered was severe enough, we'll now force close the - // channel to prevent reading it to the switch in the future. - if failure.linkErr.ForceClose { - p.log.Warnf("Force closing link(%v)", - failure.shortChanID) + // If the error encountered was severe enough, we'll now force close + // the channel to prevent reading it to the switch in the future. + if failure.linkErr.FailureAction == htlcswitch.LinkFailureForceClose { + p.log.Warnf("Force closing link(%v)", failure.shortChanID) closeTx, err := p.cfg.ChainArb.ForceCloseContract( failure.chanPoint, From 4d633f04e31c566c74ef2d3c000b37258b263851 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 19 May 2023 17:18:00 -0700 Subject: [PATCH 2/2] htlcswitch: add new LinkFailureDisconnect action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we add a new LinkFailureDisconnect action that'll be used if we detect that the remote party hasn't sent a revoke and ack when it actually should. Before this commit, we would log our action, tear down the link, but then not actually force a connection recycle, as we assumed that if the TCP connection was actually stale, then the read/write timeout would expire. In practice this doesn't always seem to be the case, so we make a strong action here to actually force a disconnection in hopes that either side will reconnect and keep the good times rollin' 🕺. --- docs/release-notes/release-notes-0.16.3.md | 15 ++++++++++++--- htlcswitch/link.go | 11 ++++++++--- htlcswitch/link_test.go | 14 ++++++++------ htlcswitch/linkfailure.go | 5 +++++ peer/brontide.go | 7 +++++++ 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/docs/release-notes/release-notes-0.16.3.md b/docs/release-notes/release-notes-0.16.3.md index b7b5ff5ba..b5ca8ab71 100644 --- a/docs/release-notes/release-notes-0.16.3.md +++ b/docs/release-notes/release-notes-0.16.3.md @@ -1,10 +1,10 @@ # Release Notes -## Mempool +## Mempool Optimizations * Optimized [mempool -management](https://github.com/lightningnetwork/lnd/pull/7681) to lower the CPU -usage. + management](https://github.com/lightningnetwork/lnd/pull/7681) to lower the + CPU usage. ## Misc @@ -12,7 +12,16 @@ usage. all macaroon DB root keys on `ChangePassword`/`GenerateNewRootKey` respectively. +## Channel Link Bug Fix + +* If we detect the remote link is inactive, [we'll now tear down the + connection](https://github.com/lightningnetwork/lnd/pull/7711) in addition to + stopping the link's statemachine. If we're persistently connected with the + peer, then this'll force a reconnect, which may restart things and help avoid + certain force close scenarios. + # Contributors (Alphabetical Order) * Elle Mouton +* Olaoluwa Osuntokun * Yong Yu diff --git a/htlcswitch/link.go b/htlcswitch/link.go index feb0a7a8b..7ef6d9a00 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1037,7 +1037,7 @@ func (l *channelLink) htlcManager() { l.fail( LinkFailureError{ code: ErrSyncError, - FailureAction: LinkFailureForceClose, // nolint:lll + FailureAction: LinkFailureForceClose, //nolint:lll }, "unable to synchronize channel "+ "states: %v", err, @@ -1239,8 +1239,13 @@ func (l *channelLink) htlcManager() { } case <-l.cfg.PendingCommitTicker.Ticks(): - l.fail(LinkFailureError{code: ErrRemoteUnresponsive}, - "unable to complete dance") + l.fail( + LinkFailureError{ + code: ErrRemoteUnresponsive, + FailureAction: LinkFailureDisconnect, + }, + "unable to complete dance", + ) return // A message from the switch was just received. This indicates diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index c461ab33e..e744c75fc 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -5457,7 +5457,8 @@ func TestChannelLinkFail(t *testing.T) { // If we expect the link to force close the channel in this // case, check that it happens. If not, make sure it does not // happen. - isForceCloseErr := (linkErr.FailureAction == LinkFailureForceClose) + isForceCloseErr := (linkErr.FailureAction == + LinkFailureForceClose) require.True( t, test.shouldForceClose == isForceCloseErr, test.name, ) @@ -6343,11 +6344,12 @@ func TestPendingCommitTicker(t *testing.T) { // Assert that we get the expected link failure from Alice. select { case linkErr := <-linkErrs: - if linkErr.code != ErrRemoteUnresponsive { - t.Fatalf("error code mismatch, "+ - "want: ErrRemoteUnresponsive, got: %v", - linkErr.code) - } + require.Equal( + t, linkErr.code, ErrRemoteUnresponsive, + fmt.Sprintf("error code mismatch, want: "+ + "ErrRemoteUnresponsive, got: %v", linkErr.code), + ) + require.Equal(t, linkErr.FailureAction, LinkFailureDisconnect) case <-time.After(time.Second): t.Fatalf("did not receive failure") diff --git a/htlcswitch/linkfailure.go b/htlcswitch/linkfailure.go index 58da01189..f04a41603 100644 --- a/htlcswitch/linkfailure.go +++ b/htlcswitch/linkfailure.go @@ -64,6 +64,11 @@ const ( // LinkFailureForceClose indicates that the channel should be force // closed. LinkFailureForceClose + + // LinkFailureDisconnect indicates that we should disconnect in an + // attempt to recycle the connection. This can be useful if we think a + // TCP connection or state machine is stalled. + LinkFailureDisconnect ) // LinkFailureError encapsulates an error that will make us fail the current diff --git a/peer/brontide.go b/peer/brontide.go index 964054a6e..1b1534b2d 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -3142,6 +3142,13 @@ func (p *Brontide) handleLinkFailure(failure linkFailureReport) { "remote peer: %v", err) } } + + // If the failure action is disconnect, then we'll execute that now. If + // we had to send an error above, it was a sync call, so we expect the + // message to be flushed on the wire by now. + if failure.linkErr.FailureAction == htlcswitch.LinkFailureDisconnect { + p.Disconnect(fmt.Errorf("link requested disconnect")) + } } // tryLinkShutdown attempts to fetch a target link from the switch, calls