Repository: libp2p/go-libp2p
Pull request #2805 (2024-05-21 18:58:06)

rcmgr: Backwards compatibility if you wrap default impl
I tried to introduce a new method to the ResourceManager without breaking the API. That was a mistake, I should have broken the API because the new method was effectively required.

Users who wrapped the default implementation of the ResourceManager wouldn't get the new method. The circuitv2 transport would not see the new method, and then call the fallback method, which would fail in the default implementation. This is a subtle footgun that could have been avoided by either changing the api or changing the default implementation's fallback method to know not to expect an IP address when doing a circuitv2 connection. This fix does the latter for now to not break the API. Another PR I'll introduce soon will break the API and I'll try to merge that before the next release.



Patch:
diff --git a/p2p/host/resource-manager/rcmgr.go b/p2p/host/resource-manager/rcmgr.go
index 739657c2..dbde4779 100644
--- a/p2p/host/resource-manager/rcmgr.go
+++ b/p2p/host/resource-manager/rcmgr.go
@@ -316,9 +316,7 @@ func (r *resourceManager) nextStreamId() int64 {
 	return r.streamId
 }
 
-// OpenConnectionNoIP is like OpenConnection, but does not use IP information.
-// Used when we still want to limit the connection by other scopes, but don't
-// have IP information like when we are relaying.
+// OpenConnectionNoIP is deprecated and will be removed in the next release
 func (r *resourceManager) OpenConnectionNoIP(dir network.Direction, usefd bool, endpoint multiaddr.Multiaddr) (network.ConnManagementScope, error) {
 	return r.openConnection(dir, usefd, endpoint, netip.Addr{})
 }
@@ -326,7 +324,8 @@ func (r *resourceManager) OpenConnectionNoIP(dir network.Direction, usefd bool,
 func (r *resourceManager) OpenConnection(dir network.Direction, usefd bool, endpoint multiaddr.Multiaddr) (network.ConnManagementScope, error) {
 	ip, err := manet.ToIP(endpoint)
 	if err != nil {
-		return nil, err
+		// No IP address
+		return r.openConnection(dir, usefd, endpoint, netip.Addr{})
 	}
 
 	ipAddr, ok := netip.AddrFromSlice(ip)
diff --git a/p2p/protocol/circuitv2/client/transport.go b/p2p/protocol/circuitv2/client/transport.go
index 135fdcf0..2c9e49f5 100644
--- a/p2p/protocol/circuitv2/client/transport.go
+++ b/p2p/protocol/circuitv2/client/transport.go
@@ -48,23 +48,8 @@ func AddTransport(h host.Host, upgrader transport.Upgrader) error {
 var _ transport.Transport = (*Client)(nil)
 var _ io.Closer = (*Client)(nil)
 
-// If the resource manager supports OpenConnectionNoIP, we'll use it to open connections.
-// That's because the swarm is already limiting by IP address at the swarm
-// level, and here we want to limit by everything but the IP.
-// Some ResourceManager implementations may not care about IP addresses, so we
-// do our own interface check to see if they provide this option.
-type rcmgrOpenConnectionNoIPer interface {
-	OpenConnectionNoIP(network.Direction, bool, ma.Multiaddr) (network.ConnManagementScope, error)
-}
-
 func (c *Client) Dial(ctx context.Context, a ma.Multiaddr, p peer.ID) (transport.CapableConn, error) {
-	var connScope network.ConnManagementScope
-	var err error
-	if rcmgr, ok := c.host.Network().ResourceManager().(rcmgrOpenConnectionNoIPer); ok {
-		connScope, err = rcmgr.OpenConnectionNoIP(network.DirOutbound, false, a)
-	} else {
-		connScope, err = c.host.Network().ResourceManager().OpenConnection(network.DirOutbound, false, a)
-	}
+	connScope, err := c.host.Network().ResourceManager().OpenConnection(network.DirOutbound, false, a)
 
 	if err != nil {
 		return nil, err


Test patch:
diff --git a/libp2p_test.go b/libp2p_test.go
index 343681be..462cf374 100644
--- a/libp2p_test.go
+++ b/libp2p_test.go
@@ -7,14 +7,20 @@ import (
 	"fmt"
 	"regexp"
 	"testing"
+	"time"
 
 	"github.com/libp2p/go-libp2p/core/connmgr"
 	"github.com/libp2p/go-libp2p/core/crypto"
+	"github.com/libp2p/go-libp2p/core/event"
 	"github.com/libp2p/go-libp2p/core/host"
+	"github.com/libp2p/go-libp2p/core/network"
 	"github.com/libp2p/go-libp2p/core/peer"
+	"github.com/libp2p/go-libp2p/core/peerstore"
 	"github.com/libp2p/go-libp2p/core/routing"
 	"github.com/libp2p/go-libp2p/core/transport"
+	rcmgr "github.com/libp2p/go-libp2p/p2p/host/resource-manager"
 	"github.com/libp2p/go-libp2p/p2p/net/swarm"
+	"github.com/libp2p/go-libp2p/p2p/protocol/ping"
 	"github.com/libp2p/go-libp2p/p2p/security/noise"
 	tls "github.com/libp2p/go-libp2p/p2p/security/tls"
 	quic "github.com/libp2p/go-libp2p/p2p/transport/quic"
@@ -393,3 +399,48 @@ func TestMain(m *testing.M) {
 		goleak.IgnoreAnyFunction("github.com/jackpal/go-nat-pmp.(*Client).GetExternalAddress"),
 	)
 }
+
+func TestDialCircuitAddrWithWrappedResourceManager(t *testing.T) {
+	relay, err := New(EnableRelayService())
+	require.NoError(t, err)
+	defer relay.Close()
+
+	// Fake that the relay is publicly reachable
+	emitterForRelay, err := relay.EventBus().Emitter(&event.EvtLocalReachabilityChanged{})
+	require.NoError(t, err)
+	defer emitterForRelay.Close()
+	emitterForRelay.Emit(event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic})
+
+	peerBehindRelay, err := New(EnableAutoRelayWithStaticRelays([]peer.AddrInfo{{ID: relay.ID(), Addrs: relay.Addrs()}}))
+	require.NoError(t, err)
+	defer peerBehindRelay.Close()
+	// Emit an event to tell this peer it is private
+	emitterForPeerBehindRelay, err := peerBehindRelay.EventBus().Emitter(&event.EvtLocalReachabilityChanged{})
+	require.NoError(t, err)
+	defer emitterForPeerBehindRelay.Close()
+	emitterForPeerBehindRelay.Emit(event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPrivate})
+
+	// Use a wrapped resource manager to test that the circuit dialing works
+	// with it. Look at the PR introducing this test for context
+	type wrappedRcmgr struct{ network.ResourceManager }
+	mgr, err := rcmgr.NewResourceManager(rcmgr.NewFixedLimiter(rcmgr.DefaultLimits.AutoScale()))
+	require.NoError(t, err)
+	wmgr := wrappedRcmgr{mgr}
+	h, err := New(ResourceManager(wmgr))
+	require.NoError(t, err)
+	defer h.Close()
+
+	h.Peerstore().AddAddrs(relay.ID(), relay.Addrs(), 10*time.Minute)
+	h.Peerstore().AddAddr(peerBehindRelay.ID(),
+		ma.StringCast(
+			fmt.Sprintf("/p2p/%s/p2p-circuit", relay.ID()),
+		),
+		peerstore.TempAddrTTL,
+	)
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	res := <-ping.Ping(ctx, h, peerBehindRelay.ID())
+	require.NoError(t, res.Error)
+	defer cancel()
+}