From 3329308bce06723103a597b602953cb9e4fe558e Mon Sep 17 00:00:00 2001 From: ziggie Date: Sun, 22 Jun 2025 10:45:56 +0200 Subject: [PATCH 1/4] multi: fix deadlock in p2p race condition In case we cannot guarantee that the peers has started up completely we launch the disconnect method asynchronously. --- peer/brontide.go | 7 ++++- server.go | 71 +++++++++++++++++++++++++++++++----------------- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/peer/brontide.go b/peer/brontide.go index 5d00e14ab..213f28c76 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -1591,6 +1591,10 @@ func (p *Brontide) WaitForDisconnect(ready chan struct{}) { // Disconnect terminates the connection with the remote peer. Additionally, a // signal is sent to the server and htlcSwitch indicating the resources // allocated to the peer can now be cleaned up. +// +// NOTE: Be aware that this method will block if the peer is still starting up. +// Therefore consider starting it in a goroutine if you cannot guarantee that +// the peer has finished starting up before calling this method. func (p *Brontide) Disconnect(reason error) { if !atomic.CompareAndSwapInt32(&p.disconnect, 0, 1) { return @@ -1603,7 +1607,8 @@ func (p *Brontide) Disconnect(reason error) { // started, otherwise we will skip reading it as this chan won't be // closed, hence blocks forever. if atomic.LoadInt32(&p.started) == 1 { - p.log.Debugf("Started, waiting on startReady signal") + p.log.Debugf("Peer hasn't finished starting up yet, waiting " + + "on startReady signal before closing connection") select { case <-p.startReady: diff --git a/server.go b/server.go index d8d1f4422..ac38449c2 100644 --- a/server.go +++ b/server.go @@ -4941,27 +4941,22 @@ func (s *server) connectToPersistentPeer(pubKeyStr string) { // removePeer removes the passed peer from the server's state of all active // peers. +// +// NOTE: Server mutex must be held when calling this function. func (s *server) removePeer(p *peer.Brontide) { if p == nil { return } - srvrLog.Debugf("removing peer %v", p) + srvrLog.Debugf("Removing peer %v", p) - // As the peer is now finished, ensure that the TCP connection is - // closed and all of its related goroutines have exited. - p.Disconnect(fmt.Errorf("server: disconnecting peer %v", p)) - - // If this peer had an active persistent connection request, remove it. - if p.ConnReq() != nil { - s.connMgr.Remove(p.ConnReq().ID()) - } - - // Ignore deleting peers if we're shutting down. + // Exit early if we have already been instructed to shutdown, the peers + // will be disconnected in the server shutdown process. if s.Stopped() { return } + // Capture the peer's public key and string representation. pKey := p.PubKey() pubSer := pKey[:] pubStr := string(pubSer) @@ -4974,22 +4969,45 @@ func (s *server) removePeer(p *peer.Brontide) { delete(s.outboundPeers, pubStr) } - // Remove the peer's access permission from the access manager. - peerPubStr := string(p.IdentityKey().SerializeCompressed()) - s.peerAccessMan.removePeerAccess(peerPubStr) + // When removing the peer we make sure to disconnect it asynchronously + // to avoid blocking the main server goroutine because it is holding the + // server's mutex. Disconnecting the peer might block and wait until the + // peer has fully started up. This can happen if an inbound and outbound + // race condition occurs. + s.wg.Add(1) + go func() { + defer s.wg.Done() - // Copy the peer's error buffer across to the server if it has any items - // in it so that we can restore peer errors across connections. - if p.ErrorBuffer().Total() > 0 { - s.peerErrors[pubStr] = p.ErrorBuffer() - } + p.Disconnect(fmt.Errorf("server: disconnecting peer %v", p)) - // Inform the peer notifier of a peer offline event so that it can be - // reported to clients listening for peer events. - var pubKey [33]byte - copy(pubKey[:], pubSer) + // If this peer had an active persistent connection request, + // remove it. + if p.ConnReq() != nil { + s.connMgr.Remove(p.ConnReq().ID()) + } - s.peerNotifier.NotifyPeerOffline(pubKey) + // Remove the peer's access permission from the access manager. + peerPubStr := string(p.IdentityKey().SerializeCompressed()) + s.peerAccessMan.removePeerAccess(peerPubStr) + + // Copy the peer's error buffer across to the server if it has + // any items in it so that we can restore peer errors across + // connections. We need to look up the error after the peer has + // been disconnected because we write the error in the + // `Disconnect` method. + s.mu.Lock() + if p.ErrorBuffer().Total() > 0 { + s.peerErrors[pubStr] = p.ErrorBuffer() + } + s.mu.Unlock() + + // Inform the peer notifier of a peer offline event so that it + // can be reported to clients listening for peer events. + var pubKey [33]byte + copy(pubKey[:], pubSer) + + s.peerNotifier.NotifyPeerOffline(pubKey) + }() } // ConnectToPeer requests that the server connect to a Lightning Network peer @@ -5130,7 +5148,10 @@ func (s *server) DisconnectPeer(pubKey *btcec.PublicKey) error { // Remove the peer by calling Disconnect. Previously this was done with // removePeer, which bypassed the peerTerminationWatcher. - peer.Disconnect(fmt.Errorf("server: DisconnectPeer called")) + // + // NOTE: We call it in a goroutine to avoid blocking the main server + // goroutine because we might hold the server's mutex. + go peer.Disconnect(fmt.Errorf("server: DisconnectPeer called")) return nil } From 0151993b8846c68764586ef3c63ed4afe6337e88 Mon Sep 17 00:00:00 2001 From: ziggie Date: Sun, 22 Jun 2025 10:51:06 +0200 Subject: [PATCH 2/4] docs: add release-notes --- docs/release-notes/release-notes-0.19.2.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.2.md b/docs/release-notes/release-notes-0.19.2.md index fcfa09621..5c36460b3 100644 --- a/docs/release-notes/release-notes-0.19.2.md +++ b/docs/release-notes/release-notes-0.19.2.md @@ -29,6 +29,10 @@ - [Fixed](https://github.com/lightningnetwork/lnd/pull/9962) a case where the node may panic if it's running in the remote signer mode. +- [Fixed](https://github.com/lightningnetwork/lnd/pull/9978) a deadlock which + can happen when the peer start-up has not yet completed but a another p2p + connection attempt tries to disconnect the peer. + # New Features ## Functional Enhancements From 409561ac62d7eed1b79891545c21641bc19b7aca Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 24 Jun 2025 15:59:15 +0200 Subject: [PATCH 3/4] lnd: rename removePeer to removePeerUnsafe rename `removePeer` to highlight that the mutex has to be acquired before calling it. --- server.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server.go b/server.go index ac38449c2..18e23b224 100644 --- a/server.go +++ b/server.go @@ -4091,7 +4091,7 @@ func (s *server) InboundPeerConnected(conn net.Conn) { // Remove the current peer from the server's internal state and // signal that the peer termination watcher does not need to // execute for this peer. - s.removePeer(connectedPeer) + s.removePeerUnsafe(connectedPeer) s.ignorePeerTermination[connectedPeer] = struct{}{} s.scheduledPeerConnection[pubStr] = func() { s.peerConnected(conn, nil, true) @@ -4208,7 +4208,7 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn) // Remove the current peer from the server's internal state and // signal that the peer termination watcher does not need to // execute for this peer. - s.removePeer(connectedPeer) + s.removePeerUnsafe(connectedPeer) s.ignorePeerTermination[connectedPeer] = struct{}{} s.scheduledPeerConnection[pubStr] = func() { s.peerConnected(conn, connReq, false) @@ -4730,7 +4730,7 @@ func (s *server) peerTerminationWatcher(p *peer.Brontide, ready chan struct{}) { // First, cleanup any remaining state the server has regarding the peer // in question. - s.removePeer(p) + s.removePeerUnsafe(p) // Next, check to see if this is a persistent peer or not. if _, ok := s.persistentPeers[pubStr]; !ok { @@ -4939,11 +4939,11 @@ func (s *server) connectToPersistentPeer(pubKeyStr string) { }() } -// removePeer removes the passed peer from the server's state of all active -// peers. +// removePeerUnsafe removes the passed peer from the server's state of all +// active peers. // // NOTE: Server mutex must be held when calling this function. -func (s *server) removePeer(p *peer.Brontide) { +func (s *server) removePeerUnsafe(p *peer.Brontide) { if p == nil { return } @@ -5147,7 +5147,7 @@ func (s *server) DisconnectPeer(pubKey *btcec.PublicKey) error { delete(s.persistentPeersBackoff, pubStr) // Remove the peer by calling Disconnect. Previously this was done with - // removePeer, which bypassed the peerTerminationWatcher. + // removePeerUnsafe, which bypassed the peerTerminationWatcher. // // NOTE: We call it in a goroutine to avoid blocking the main server // goroutine because we might hold the server's mutex. From 24e3958a0bd9739a607f00efff8c7e23ffc275a6 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 24 Jun 2025 16:52:55 +0200 Subject: [PATCH 4/4] lnd: improve code comment --- rpcserver.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 124f7e9dd..687cfc201 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1870,11 +1870,13 @@ func (r *rpcServer) DisconnectPeer(ctx context.Context, // In order to avoid erroneously disconnecting from a peer that we have // an active channel with, if we have any channels active with this - // peer, then we'll disallow disconnecting from them. + // peer, then we'll disallow disconnecting from them in certain + // situations. if len(nodeChannels) != 0 { - // If we are not in a dev environment or the configed dev value - // `unsafedisconnect` is false, we return an error since there - // are active channels. + // If the configured dev value `unsafedisconnect` is false, we + // return an error since there are active channels. For + // production environments, we allow disconnecting from a peer + // even if there are channels active with them. if !r.cfg.Dev.GetUnsafeDisconnect() { return nil, fmt.Errorf("cannot disconnect from "+ "peer(%x), still has %d active channels",