Merge pull request #101 from libp2p/feat/remove-discovery

feat: remove relay discovery and unspecified relay dialing

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,37 +44,50 @@ 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
 }
 
 // RelayOpts are options for configuring the relay transport.
-type RelayOpt int
+type RelayOpt func(*Relay) error
 
 var (
 	// OptActive configures the relay transport to actively establish
 	// outbound connections on behalf of clients. You probably don't want to
 	// enable this unless you know what you're doing.
-	OptActive = RelayOpt(0)
+	OptActive RelayOpt = func(r *Relay) error {
+		r.active = true
+		return nil
+	}
 	// OptHop configures the relay transport to accept requests to relay
 	// traffic on behalf of third-parties. Unless OptActive is specified,
 	// 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 = RelayOpt(2)
+	OptHop = func(r *Relay) error {
+		r.hop = true
+		return nil
+	}
+	// 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 = func(r *Relay) error {
+		log.Errorf(
+			"circuit.OptDiscovery is now a no-op: %s",
+			"dialing peers with a random relay is no longer supported",
+		)
+		return nil
+	}
 )
 
 type RelayError struct {
@@ -94,28 +106,16 @@ 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 {
-		switch opt {
-		case OptActive:
-			r.active = true
-		case OptHop:
-			r.hop = true
-		case OptDiscovery:
-			r.discovery = true
-		default:
-			return nil, fmt.Errorf("unrecognized option: %d", opt)
+		if err := opt(r); err != nil {
+			return nil, err
 		}
 	}
 
 	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)