feat: remove relay discovery and unspecified relay dialing

This was a misfeature that looks great in demos but isn't useful in practice.

First, random relays we discover are unlikely to support _active_ relaying. This
means that picking a random relay is usually going to fail because the random
relay won't already be connected to the target.

Second, there are two use-cases for relaying:

1. Connecting to undialable nodes.
2. Relaying all outbound traffic.

For the first case, we only want to use the relays specified by the target
peers. For the second case, we'd need to modify either libp2p's dialer logic (in
the swarm) to prefix all addresses with the specified relay.

The logic _here_ covers neither use-case.

p2p/protocol/internal/circuitv1-deprecated/dial.go

@@ -3,7 +3,6 @@ package relay
 import (
 	"context"
 	"fmt"
-	"math/rand"
 
 	"github.com/libp2p/go-libp2p-core/peer"
 	"github.com/libp2p/go-libp2p-core/transport"
@@ -30,6 +29,13 @@ func (r *Relay) Dial(ctx context.Context, a ma.Multiaddr, p peer.ID) (*Conn, err
 		return nil, fmt.Errorf("%s is not a relay address", a)
 	}
 
+	if relayaddr == nil {
+		return nil, fmt.Errorf(
+			"can't dial a p2p-circuit without specifying a relay: %s",
+			a,
+		)
+	}
+
 	// Strip the /p2p-circuit prefix from the destaddr.
 	_, destaddr = ma.SplitFirst(destaddr)
 
@@ -38,11 +44,6 @@ func (r *Relay) Dial(ctx context.Context, a ma.Multiaddr, p peer.ID) (*Conn, err
 		dinfo.Addrs = append(dinfo.Addrs, destaddr)
 	}
 
-	if relayaddr == nil {
-		// unspecific relay address, try dialing using known hop relays
-		return r.tryDialRelays(ctx, *dinfo)
-	}
-
 	var rinfo *peer.AddrInfo
 	rinfo, err := peer.AddrInfoFromP2pAddr(relayaddr)
 	if err != nil {
@@ -51,36 +52,3 @@ func (r *Relay) Dial(ctx context.Context, a ma.Multiaddr, p peer.ID) (*Conn, err
 
 	return r.DialPeer(ctx, *rinfo, *dinfo)
 }
-
-func (r *Relay) tryDialRelays(ctx context.Context, dinfo peer.AddrInfo) (*Conn, error) {
-	var relays []peer.ID
-	r.mx.Lock()
-	for p := range r.relays {
-		relays = append(relays, p)
-	}
-	r.mx.Unlock()
-
-	// shuffle list of relays, avoid overloading a specific relay
-	for i := range relays {
-		j := rand.Intn(i + 1)
-		relays[i], relays[j] = relays[j], relays[i]
-	}
-
-	for _, relay := range relays {
-		if len(r.host.Network().ConnsToPeer(relay)) == 0 {
-			continue
-		}
-
-		rctx, cancel := context.WithTimeout(ctx, HopConnectTimeout)
-		c, err := r.DialPeer(rctx, peer.AddrInfo{ID: relay}, dinfo)
-		cancel()
-
-		if err == nil {
-			return c, nil
-		}
-
-		log.Debugf("error opening relay connection through %s: %s", dinfo.ID, err.Error())
-	}
-
-	return nil, fmt.Errorf("Failed to dial through %d known relay hosts", len(relays))
-}

p2p/protocol/internal/circuitv1-deprecated/relay.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"sync"
 	"sync/atomic"
 	"time"
 
@@ -45,15 +44,11 @@ type Relay struct {
 	ctx      context.Context
 	self     peer.ID
 
-	active    bool
-	hop       bool
-	discovery bool
+	active bool
+	hop    bool
 
 	incoming chan *Conn
 
-	relays map[peer.ID]struct{}
-	mx     sync.Mutex
-
 	// atomic counters
 	streamCount  int32
 	liveHopCount int32
@@ -72,9 +67,14 @@ var (
 	// this will only relay traffic between peers already connected to this
 	// node.
 	OptHop = RelayOpt(1)
-	// OptDiscovery configures this relay transport to discover new relays
-	// by probing every new peer. You almost _certainly_ don't want to
-	// enable this.
+	// OptDiscovery is a no-op. It was introduced as a way to probe new
+	// peers to see if they were willing to act as a relays. However, in
+	// practice, it's useless. While it does test to see if these peers are
+	// relays, it doesn't (and can't), check to see if these peers are
+	// _active_ relays (i.e., will actively dial the target peer).
+	//
+	// This option may be re-enabled in the future but for now you shouldn't
+	// use it.
 	OptDiscovery = RelayOpt(2)
 )
 
@@ -94,7 +94,6 @@ func NewRelay(ctx context.Context, h host.Host, upgrader *tptu.Upgrader, opts ..
 		ctx:      ctx,
 		self:     h.ID(),
 		incoming: make(chan *Conn),
-		relays:   make(map[peer.ID]struct{}),
 	}
 
 	for _, opt := range opts {
@@ -104,18 +103,17 @@ func NewRelay(ctx context.Context, h host.Host, upgrader *tptu.Upgrader, opts ..
 		case OptHop:
 			r.hop = true
 		case OptDiscovery:
-			r.discovery = true
+			log.Errorf(
+				"circuit.OptDiscovery is now a no-op: %s",
+				"dialing peers with a random relay is no longer supported",
+			)
 		default:
 			return nil, fmt.Errorf("unrecognized option: %d", opt)
 		}
 	}
 
 	h.SetStreamHandler(ProtoID, r.handleNewStream)
 
-	if r.discovery {
-		h.Network().Notify(r.notifiee())
-	}
-
 	return r, nil
 }
 

p2p/protocol/internal/circuitv1-deprecated/relay_test.go

@@ -69,7 +69,7 @@ func TestBasicRelay(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1], OptHop)
 
@@ -143,7 +143,7 @@ func TestRelayReset(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1], OptHop)
 
@@ -201,7 +201,7 @@ func TestBasicRelayDial(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	_ = newTestRelay(t, ctx, hosts[1], OptHop)
 	r3 := newTestRelay(t, ctx, hosts[2])
@@ -263,13 +263,12 @@ func TestBasicRelayDial(t *testing.T) {
 	}
 }
 
-func TestUnspecificRelayDial(t *testing.T) {
+func TestUnspecificRelayDialFails(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
 
 	hosts := getNetHosts(t, ctx, 3)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1], OptHop)
 
@@ -281,36 +280,22 @@ func TestUnspecificRelayDial(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 
 	var (
-		conn1, conn2 net.Conn
-		done         = make(chan struct{})
+		done = make(chan struct{})
 	)
 
 	defer func() {
+		cancel()
 		<-done
-		if conn1 != nil {
-			conn1.Close()
-		}
-		if conn2 != nil {
-			conn2.Close()
-		}
 	}()
 
-	msg := []byte("relay works!")
 	go func() {
 		defer close(done)
 		list := r3.Listener()
 
 		var err error
-		conn1, err = list.Accept()
-		if err != nil {
-			t.Error(err)
-			return
-		}
-
-		_, err = conn1.Write(msg)
-		if err != nil {
-			t.Error(err)
-			return
+		_, err = list.Accept()
+		if err == nil {
+			t.Error("should not have received relay connection")
 		}
 	}()
 
@@ -320,19 +305,9 @@ func TestUnspecificRelayDial(t *testing.T) {
 	defer rcancel()
 
 	var err error
-	conn2, err = r1.Dial(rctx, addr, hosts[2].ID())
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	data := make([]byte, len(msg))
-	_, err = io.ReadFull(conn2, data)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if !bytes.Equal(data, msg) {
-		t.Fatal("message was incorrect:", string(data))
+	_, err = r1.Dial(rctx, addr, hosts[2].ID())
+	if err == nil {
+		t.Fatal("expected dial with unspecified relay address to fail, even if we're connected to a relay")
 	}
 }
 
@@ -347,7 +322,7 @@ func TestRelayThroughNonHop(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1])
 
@@ -384,7 +359,7 @@ func TestRelayNoDestConnection(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1], OptHop)
 
@@ -419,7 +394,7 @@ func TestActiveRelay(t *testing.T) {
 
 	time.Sleep(10 * time.Millisecond)
 
-	r1 := newTestRelay(t, ctx, hosts[0], OptDiscovery)
+	r1 := newTestRelay(t, ctx, hosts[0])
 
 	newTestRelay(t, ctx, hosts[1], OptHop, OptActive)
 

p2p/protocol/internal/circuitv1-deprecated/transport_test.go

@@ -26,7 +26,7 @@ var msg = []byte("relay works!")
 func testSetupRelay(t *testing.T, ctx context.Context) []host.Host {
 	hosts := getNetHosts(t, ctx, 3)
 
-	err := AddRelayTransport(ctx, hosts[0], swarmt.GenUpgrader(hosts[0].Network().(*swarm.Swarm)), OptDiscovery)
+	err := AddRelayTransport(ctx, hosts[0], swarmt.GenUpgrader(hosts[0].Network().(*swarm.Swarm)))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -121,7 +121,7 @@ func TestSpecificRelayTransportDial(t *testing.T) {
 	}
 }
 
-func TestUnspecificRelayTransportDial(t *testing.T) {
+func TestUnspecificRelayTransportDialFails(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -137,17 +137,9 @@ func TestUnspecificRelayTransportDial(t *testing.T) {
 
 	hosts[0].Peerstore().AddAddrs(hosts[2].ID(), []ma.Multiaddr{addr}, peerstore.TempAddrTTL)
 
-	s, err := hosts[0].NewStream(rctx, hosts[2].ID(), TestProto)
-	if err != nil {
-		t.Fatal(err)
+	_, err = hosts[0].NewStream(rctx, hosts[2].ID(), TestProto)
+	if err == nil {
+		t.Fatal("dial to unspecified address should have failed")
 	}
 
-	data, err := ioutil.ReadAll(s)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if !bytes.Equal(data, msg) {
-		t.Fatal("message was incorrect:", string(data))
-	}
 }