Repository: libp2p/go-libp2p
Pull request #2171 (2023-03-04 11:42:07)

relay: fix deadlock when closing
fixes: https://github.com/libp2p/go-libp2p/issues/2167


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4d53dcd4..012815d3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,10 @@
 
 # [v0.27.0](https://github.com/libp2p/go-libp2p/releases/tag/v0.27.0) (unreleased)
 
+### Breaking Changes
+
+* The `LocalPrivateKey` method was removed from the `network.Conn` interface.
+
 # [v0.26.1](https://github.com/libp2p/go-libp2p/releases/tag/v0.26.1)
 
 This patch release fixes two bugs:
diff --git a/core/network/conn.go b/core/network/conn.go
index 56031ab0..97168915 100644
--- a/core/network/conn.go
+++ b/core/network/conn.go
@@ -53,9 +53,6 @@ type ConnSecurity interface {
 	// LocalPeer returns our peer ID
 	LocalPeer() peer.ID
 
-	// LocalPrivateKey returns our private key
-	LocalPrivateKey() ic.PrivKey
-
 	// RemotePeer returns the peer ID of the remote peer.
 	RemotePeer() peer.ID
 
diff --git a/core/sec/insecure/insecure.go b/core/sec/insecure/insecure.go
index 9b3664e2..9ed20f09 100644
--- a/core/sec/insecure/insecure.go
+++ b/core/sec/insecure/insecure.go
@@ -40,9 +40,7 @@ type Transport struct {
 
 var _ sec.SecureTransport = &Transport{}
 
-// NewWithIdentity constructs a new insecure transport. The provided private key
-// is stored and returned from LocalPrivateKey to satisfy the
-// SecureTransport interface, and the public key is sent to
+// NewWithIdentity constructs a new insecure transport. The public key is sent to
 // remote peers. No security is provided.
 func NewWithIdentity(protocolID protocol.ID, id peer.ID, key ci.PrivKey) *Transport {
 	return &Transport{
@@ -57,12 +55,6 @@ func (t *Transport) LocalPeer() peer.ID {
 	return t.id
 }
 
-// LocalPrivateKey returns the local private key.
-// This key is used only for identity generation and provides no security.
-func (t *Transport) LocalPrivateKey() ci.PrivKey {
-	return t.key
-}
-
 // SecureInbound *pretends to secure* an inbound connection to the given peer.
 // It sends the local peer's ID and public key, and receives the same from the remote peer.
 // No validation is performed as to the authenticity or ownership of the provided public key,
@@ -70,19 +62,18 @@ func (t *Transport) LocalPrivateKey() ci.PrivKey {
 //
 // SecureInbound may fail if the remote peer sends an ID and public key that are inconsistent
 // with each other, or if a network error occurs during the ID exchange.
-func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn, p peer.ID) (sec.SecureConn, error) {
+func (t *Transport) SecureInbound(_ context.Context, insecure net.Conn, p peer.ID) (sec.SecureConn, error) {
 	conn := &Conn{
-		Conn:         insecure,
-		local:        t.id,
-		localPrivKey: t.key,
+		Conn:        insecure,
+		local:       t.id,
+		localPubKey: t.key.GetPublic(),
 	}
 
-	err := conn.runHandshakeSync()
-	if err != nil {
+	if err := conn.runHandshakeSync(); err != nil {
 		return nil, err
 	}
 
-	if t.key != nil && p != "" && p != conn.remote {
+	if p != "" && p != conn.remote {
 		return nil, fmt.Errorf("remote peer sent unexpected peer ID. expected=%s received=%s", p, conn.remote)
 	}
 
@@ -97,19 +88,18 @@ func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn, p peer
 // SecureOutbound may fail if the remote peer sends an ID and public key that are inconsistent
 // with each other, or if the ID sent by the remote peer does not match the one dialed. It may
 // also fail if a network error occurs during the ID exchange.
-func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (sec.SecureConn, error) {
+func (t *Transport) SecureOutbound(_ context.Context, insecure net.Conn, p peer.ID) (sec.SecureConn, error) {
 	conn := &Conn{
-		Conn:         insecure,
-		local:        t.id,
-		localPrivKey: t.key,
+		Conn:        insecure,
+		local:       t.id,
+		localPubKey: t.key.GetPublic(),
 	}
 
-	err := conn.runHandshakeSync()
-	if err != nil {
+	if err := conn.runHandshakeSync(); err != nil {
 		return nil, err
 	}
 
-	if t.key != nil && p != conn.remote {
+	if p != conn.remote {
 		return nil, fmt.Errorf("remote peer sent unexpected peer ID. expected=%s received=%s",
 			p, conn.remote)
 	}
@@ -117,19 +107,14 @@ func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p pee
 	return conn, nil
 }
 
-func (t *Transport) ID() protocol.ID {
-	return t.protocolID
-}
+func (t *Transport) ID() protocol.ID { return t.protocolID }
 
 // Conn is the connection type returned by the insecure transport.
 type Conn struct {
 	net.Conn
 
-	local  peer.ID
-	remote peer.ID
-
-	localPrivKey ci.PrivKey
-	remotePubKey ci.PubKey
+	local, remote             peer.ID
+	localPubKey, remotePubKey ci.PubKey
 }
 
 func makeExchangeMessage(pubkey ci.PubKey) (*pb.Exchange, error) {
@@ -150,12 +135,12 @@ func makeExchangeMessage(pubkey ci.PubKey) (*pb.Exchange, error) {
 
 func (ic *Conn) runHandshakeSync() error {
 	// If we were initialized without keys, behave as in plaintext/1.0.0 (do nothing)
-	if ic.localPrivKey == nil {
+	if ic.localPubKey == nil {
 		return nil
 	}
 
 	// Generate an Exchange message
-	msg, err := makeExchangeMessage(ic.localPrivKey.GetPublic())
+	msg, err := makeExchangeMessage(ic.localPubKey)
 	if err != nil {
 		return err
 	}
@@ -239,11 +224,6 @@ func (ic *Conn) RemotePublicKey() ci.PubKey {
 	return ic.remotePubKey
 }
 
-// LocalPrivateKey returns the private key for the local peer.
-func (ic *Conn) LocalPrivateKey() ci.PrivKey {
-	return ic.localPrivKey
-}
-
 // ConnState returns the security connection's state information.
 func (ic *Conn) ConnState() network.ConnectionState {
 	return network.ConnectionState{}
diff --git a/p2p/net/mock/mock_conn.go b/p2p/net/mock/mock_conn.go
index b6fe8f2d..970fc2fc 100644
--- a/p2p/net/mock/mock_conn.go
+++ b/p2p/net/mock/mock_conn.go
@@ -158,11 +158,6 @@ func (c *conn) LocalPeer() peer.ID {
 	return c.local
 }
 
-// LocalPrivateKey is the private key of the peer on our side.
-func (c *conn) LocalPrivateKey() ic.PrivKey {
-	return c.localPrivKey
-}
-
 // RemoteMultiaddr is the Multiaddr on the remote side
 func (c *conn) RemoteMultiaddr() ma.Multiaddr {
 	return c.remoteAddr
diff --git a/p2p/net/swarm/swarm_conn.go b/p2p/net/swarm/swarm_conn.go
index 406b100a..8cdabc41 100644
--- a/p2p/net/swarm/swarm_conn.go
+++ b/p2p/net/swarm/swarm_conn.go
@@ -168,11 +168,6 @@ func (c *Conn) RemotePeer() peer.ID {
 	return c.conn.RemotePeer()
 }
 
-// LocalPrivateKey is the public key of the peer on this side
-func (c *Conn) LocalPrivateKey() ic.PrivKey {
-	return c.conn.LocalPrivateKey()
-}
-
 // RemotePublicKey is the public key of the peer on the remote side
 func (c *Conn) RemotePublicKey() ic.PubKey {
 	return c.conn.RemotePublicKey()
diff --git a/p2p/protocol/circuitv2/relay/relay.go b/p2p/protocol/circuitv2/relay/relay.go
index 5a5e97bd..77c592e8 100644
--- a/p2p/protocol/circuitv2/relay/relay.go
+++ b/p2p/protocol/circuitv2/relay/relay.go
@@ -51,6 +51,7 @@ type Relay struct {
 	constraints *constraints
 	scope       network.ResourceScopeSpan
 	notifiee    network.Notifiee
+	wg          sync.WaitGroup
 
 	mx     sync.Mutex
 	rsvp   map[peer.ID]time.Time
@@ -98,6 +99,8 @@ func New(h host.Host, opts ...Option) (*Relay, error) {
 	h.SetStreamHandler(proto.ProtoIDv2Hop, r.handleStream)
 	r.notifiee = &network.NotifyBundle{DisconnectedF: r.disconnected}
 	h.Network().Notify(r.notifiee)
+
+	r.wg.Add(1)
 	go r.background()
 
 	return r, nil
@@ -105,17 +108,18 @@ func New(h host.Host, opts ...Option) (*Relay, error) {
 
 func (r *Relay) Close() error {
 	r.mx.Lock()
-	defer r.mx.Unlock()
 	if !r.closed {
 		r.closed = true
+		r.mx.Unlock()
+
 		r.host.RemoveStreamHandler(proto.ProtoIDv2Hop)
 		r.host.Network().StopNotify(r.notifiee)
 		r.scope.Done()
 		r.cancel()
-		for p := range r.rsvp {
-			r.host.ConnManager().UntagPeer(p, "relay-reservation")
-		}
+		r.wg.Wait()
+		return nil
 	}
+	r.mx.Unlock()
 	return nil
 }
 
@@ -564,6 +568,8 @@ func (r *Relay) background() {
 		case <-ticker.C:
 			r.gc()
 		case <-r.ctx.Done():
+			r.gc()
+			r.wg.Done()
 			return
 		}
 	}
@@ -576,7 +582,7 @@ func (r *Relay) gc() {
 	now := time.Now()
 
 	for p, expire := range r.rsvp {
-		if expire.Before(now) {
+		if r.closed || expire.Before(now) {
 			delete(r.rsvp, p)
 			r.host.ConnManager().UntagPeer(p, "relay-reservation")
 		}
diff --git a/p2p/security/noise/session.go b/p2p/security/noise/session.go
index 2f6c23c0..fa32ab8f 100644
--- a/p2p/security/noise/session.go
+++ b/p2p/security/noise/session.go
@@ -95,10 +95,6 @@ func (s *secureSession) LocalPeer() peer.ID {
 	return s.localID
 }
 
-func (s *secureSession) LocalPrivateKey() crypto.PrivKey {
-	return s.localKey
-}
-
 func (s *secureSession) LocalPublicKey() crypto.PubKey {
 	return s.localKey.GetPublic()
 }
diff --git a/p2p/security/tls/conn.go b/p2p/security/tls/conn.go
index 3ebc7aef..143da392 100644
--- a/p2p/security/tls/conn.go
+++ b/p2p/security/tls/conn.go
@@ -12,9 +12,7 @@ import (
 type conn struct {
 	*tls.Conn
 
-	localPeer peer.ID
-	privKey   ci.PrivKey
-
+	localPeer       peer.ID
 	remotePeer      peer.ID
 	remotePubKey    ci.PubKey
 	connectionState network.ConnectionState
@@ -26,10 +24,6 @@ func (c *conn) LocalPeer() peer.ID {
 	return c.localPeer
 }
 
-func (c *conn) LocalPrivateKey() ci.PrivKey {
-	return c.privKey
-}
-
 func (c *conn) RemotePeer() peer.ID {
 	return c.remotePeer
 }
diff --git a/p2p/security/tls/transport.go b/p2p/security/tls/transport.go
index b8c1dcfb..7c28efe3 100644
--- a/p2p/security/tls/transport.go
+++ b/p2p/security/tls/transport.go
@@ -168,7 +168,6 @@ func (t *Transport) setupConn(tlsConn *tls.Conn, remotePubKey ci.PubKey) (sec.Se
 	return &conn{
 		Conn:         tlsConn,
 		localPeer:    t.localPeer,
-		privKey:      t.privKey,
 		remotePeer:   remotePeerID,
 		remotePubKey: remotePubKey,
 		connectionState: network.ConnectionState{
diff --git a/p2p/transport/quic/conn.go b/p2p/transport/quic/conn.go
index 1154cf72..a2da81eb 100644
--- a/p2p/transport/quic/conn.go
+++ b/p2p/transport/quic/conn.go
@@ -18,7 +18,6 @@ type conn struct {
 	scope     network.ConnManagementScope
 
 	localPeer      peer.ID
-	privKey        ic.PrivKey
 	localMultiaddr ma.Multiaddr
 
 	remotePeerID    peer.ID
@@ -66,9 +65,6 @@ func (c *conn) AcceptStream() (network.MuxedStream, error) {
 // LocalPeer returns our peer ID
 func (c *conn) LocalPeer() peer.ID { return c.localPeer }
 
-// LocalPrivateKey returns our private key
-func (c *conn) LocalPrivateKey() ic.PrivKey { return c.privKey }
-
 // RemotePeer returns the peer ID of the remote peer.
 func (c *conn) RemotePeer() peer.ID { return c.remotePeerID }
 
diff --git a/p2p/transport/quic/listener.go b/p2p/transport/quic/listener.go
index 017189cb..73bb5026 100644
--- a/p2p/transport/quic/listener.go
+++ b/p2p/transport/quic/listener.go
@@ -133,7 +133,6 @@ func (l *listener) setupConnWithScope(qconn quic.Connection, connScope network.C
 		scope:           connScope,
 		localPeer:       l.localPeer,
 		localMultiaddr:  localMultiaddr,
-		privKey:         l.privKey,
 		remoteMultiaddr: remoteMultiaddr,
 		remotePeerID:    remotePeerID,
 		remotePubKey:    remotePubKey,
diff --git a/p2p/transport/quic/transport.go b/p2p/transport/quic/transport.go
index 5ccebf93..2d68b660 100644
--- a/p2p/transport/quic/transport.go
+++ b/p2p/transport/quic/transport.go
@@ -153,7 +153,6 @@ func (t *transport) dialWithScope(ctx context.Context, raddr ma.Multiaddr, p pee
 		quicConn:        pconn,
 		transport:       t,
 		scope:           scope,
-		privKey:         t.privKey,
 		localPeer:       t.localPeer,
 		localMultiaddr:  localMultiaddr,
 		remotePubKey:    remotePubKey,


Test patch:
diff --git a/core/sec/insecure/insecure_test.go b/core/sec/insecure/insecure_test.go
index 8663f1d9..f6ceba41 100644
--- a/core/sec/insecure/insecure_test.go
+++ b/core/sec/insecure/insecure_test.go
@@ -103,6 +103,7 @@ func connect(t *testing.T, clientTpt, serverTpt *Transport, clientExpectsID, ser
 
 // Check the peer IDs
 func testIDs(t *testing.T, clientTpt, serverTpt *Transport, clientConn, serverConn sec.SecureConn) {
+	t.Helper()
 	require.Equal(t, clientConn.LocalPeer(), clientTpt.LocalPeer(), "Client Local Peer ID mismatch.")
 	require.Equal(t, clientConn.RemotePeer(), serverTpt.LocalPeer(), "Client Remote Peer ID mismatch.")
 	require.Equal(t, clientConn.LocalPeer(), serverConn.RemotePeer(), "Server Local Peer ID mismatch.")
@@ -110,9 +111,9 @@ func testIDs(t *testing.T, clientTpt, serverTpt *Transport, clientConn, serverCo
 
 // Check the keys
 func testKeys(t *testing.T, clientTpt, serverTpt *Transport, clientConn, serverConn sec.SecureConn) {
-	sk := serverConn.LocalPrivateKey()
-	require.True(t, sk.Equals(serverTpt.LocalPrivateKey()), "private key mismatch")
-	require.True(t, sk.GetPublic().Equals(clientConn.RemotePublicKey()), "public key mismatch")
+	t.Helper()
+	require.True(t, clientConn.RemotePublicKey().Equals(serverTpt.key.GetPublic()), "client conn key mismatch")
+	require.True(t, serverConn.RemotePublicKey().Equals(clientTpt.key.GetPublic()), "server conn key mismatch")
 }
 
 // Check sending and receiving messages
diff --git a/p2p/host/relaysvc/relay_test.go b/p2p/host/relaysvc/relay_test.go
index d995cac2..4117e119 100644
--- a/p2p/host/relaysvc/relay_test.go
+++ b/p2p/host/relaysvc/relay_test.go
@@ -34,7 +34,7 @@ func TestReachabilityChangeEvent(t *testing.T) {
 	require.Eventually(
 		t,
 		func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil },
-		1*time.Second,
+		3*time.Second,
 		100*time.Millisecond,
 		"relay should be nil on private reachability")
 
@@ -45,7 +45,7 @@ func TestReachabilityChangeEvent(t *testing.T) {
 	require.Eventually(
 		t,
 		func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil },
-		1*time.Second,
+		3*time.Second,
 		100*time.Millisecond,
 		"relay should be nil on unknown reachability")
 
diff --git a/p2p/net/connmgr/connmgr_test.go b/p2p/net/connmgr/connmgr_test.go
index 7468f671..69f82a39 100644
--- a/p2p/net/connmgr/connmgr_test.go
+++ b/p2p/net/connmgr/connmgr_test.go
@@ -796,7 +796,6 @@ type mockConn struct {
 
 func (m mockConn) Close() error                                          { panic("implement me") }
 func (m mockConn) LocalPeer() peer.ID                                    { panic("implement me") }
-func (m mockConn) LocalPrivateKey() crypto.PrivKey                       { panic("implement me") }
 func (m mockConn) RemotePeer() peer.ID                                   { panic("implement me") }
 func (m mockConn) RemotePublicKey() crypto.PubKey                        { panic("implement me") }
 func (m mockConn) LocalMultiaddr() ma.Multiaddr                          { panic("implement me") }
diff --git a/p2p/security/noise/transport_test.go b/p2p/security/noise/transport_test.go
index ada0cbfe..42477f83 100644
--- a/p2p/security/noise/transport_test.go
+++ b/p2p/security/noise/transport_test.go
@@ -159,15 +159,16 @@ func TestKeys(t *testing.T) {
 	defer initConn.Close()
 	defer respConn.Close()
 
-	sk := respConn.LocalPrivateKey()
-	pk := sk.GetPublic()
-
-	if !sk.Equals(respTransport.privateKey) {
-		t.Error("Private key Mismatch.")
+	pk1 := respConn.RemotePublicKey()
+	pk2 := initTransport.privateKey.GetPublic()
+	if !pk1.Equals(pk2) {
+		t.Errorf("Public key mismatch. expected %x got %x", pk1, pk2)
 	}
 
-	if !pk.Equals(initConn.RemotePublicKey()) {
-		t.Errorf("Public key mismatch. expected %x got %x", pk, initConn.RemotePublicKey())
+	pk3 := initConn.RemotePublicKey()
+	pk4 := respTransport.privateKey.GetPublic()
+	if !pk3.Equals(pk4) {
+		t.Errorf("Public key mismatch. expected %x got %x", pk3, pk4)
 	}
 }
 
diff --git a/p2p/security/tls/transport_test.go b/p2p/security/tls/transport_test.go
index a30c742a..2d3c2d97 100644
--- a/p2p/security/tls/transport_test.go
+++ b/p2p/security/tls/transport_test.go
@@ -111,8 +111,6 @@ func TestHandshakeSucceeds(t *testing.T) {
 
 		require.Equal(t, clientConn.LocalPeer(), clientID)
 		require.Equal(t, serverConn.LocalPeer(), serverID)
-		require.True(t, clientConn.LocalPrivateKey().Equals(clientKey), "client private key mismatch")
-		require.True(t, serverConn.LocalPrivateKey().Equals(serverKey), "server private key mismatch")
 		require.Equal(t, clientConn.RemotePeer(), serverID)
 		require.Equal(t, serverConn.RemotePeer(), clientID)
 		require.True(t, clientConn.RemotePublicKey().Equals(serverKey.GetPublic()), "server public key mismatch")
@@ -249,8 +247,6 @@ func TestHandshakeWithNextProtoSucceeds(t *testing.T) {
 
 		require.Equal(t, clientConn.LocalPeer(), clientID)
 		require.Equal(t, serverConn.LocalPeer(), serverID)
-		require.True(t, clientConn.LocalPrivateKey().Equals(clientKey), "client private key mismatch")
-		require.True(t, serverConn.LocalPrivateKey().Equals(serverKey), "server private key mismatch")
 		require.Equal(t, clientConn.RemotePeer(), serverID)
 		require.Equal(t, serverConn.RemotePeer(), clientID)
 		require.True(t, clientConn.RemotePublicKey().Equals(serverKey.GetPublic()), "server public key mismatch")
diff --git a/p2p/transport/quic/conn_test.go b/p2p/transport/quic/conn_test.go
index 69294587..189a38df 100644
--- a/p2p/transport/quic/conn_test.go
+++ b/p2p/transport/quic/conn_test.go
@@ -103,12 +103,10 @@ func testHandshake(t *testing.T, tc *connTestCase) {
 		defer serverConn.Close()
 
 		require.Equal(t, conn.LocalPeer(), clientID)
-		require.True(t, conn.LocalPrivateKey().Equals(clientKey), "local private key doesn't match")
 		require.Equal(t, conn.RemotePeer(), serverID)
 		require.True(t, conn.RemotePublicKey().Equals(serverKey.GetPublic()), "remote public key doesn't match")
 
 		require.Equal(t, serverConn.LocalPeer(), serverID)
-		require.True(t, serverConn.LocalPrivateKey().Equals(serverKey), "local private key doesn't match")
 		require.Equal(t, serverConn.RemotePeer(), clientID)
 		require.True(t, serverConn.RemotePublicKey().Equals(clientKey.GetPublic()), "remote public key doesn't match")
 	}