rcmgr: Backwards compatibility if you wrap default impl (#2805)

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()
+}

p2p/host/resource-manager/rcmgr.go

@@ -316,17 +316,16 @@ 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{})
 }
 
 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)

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