move some timeout options to Params so we can change params, but also…

… modify in test environments in a sane way

share/p2p/discovery/discovery.go

@@ -28,18 +28,11 @@ const (
 	// findPeersTimeout limits the FindPeers operation in time
 	findPeersTimeout = time.Minute
 
-	// advertiseRetryTimeout defines time interval between advertise attempts.
-	advertiseRetryTimeout = time.Second * 1
-
 	// logInterval defines the time interval at which a warning message will be logged
 	// if the desired number of nodes is not detected.
 	logInterval = 5 * time.Minute
 )
 
-// discoveryRetryTimeout defines time interval between discovery attempts
-// this is set independently for tests in discover_test.go
-var DiscoveryRetryTimeout = advertiseRetryTimeout * 60
-
 // Discovery combines advertise and discover services and allows to store discovered nodes.
 // TODO: The code here gets horribly hairy, so we should refactor this at some point
 type Discovery struct {
@@ -183,7 +176,7 @@ func (d *Discovery) Advertise(ctx context.Context) {
 			// we don't want retry indefinitely in busy loop
 			// internal discovery mechanism may need some time before attempts
 			select {
-			case <-time.After(advertiseRetryTimeout):
+			case <-time.After(d.params.AdvertiseRetryTimeout):
 				if !timer.Stop() {
 					<-timer.C
 				}
@@ -210,7 +203,7 @@ func (d *Discovery) Advertise(ctx context.Context) {
 // It initiates peer discovery upon request and restarts the process until the soft limit is
 // reached.
 func (d *Discovery) discoveryLoop(ctx context.Context) {
-	t := time.NewTicker(DiscoveryRetryTimeout)
+	t := time.NewTicker(d.params.DiscoveryRetryTimeout)
 	defer t.Stop()
 
 	warnTicker := time.NewTicker(logInterval)

share/p2p/discovery/discovery_test.go

@@ -26,8 +26,6 @@ const (
 func TestDiscovery(t *testing.T) {
 	const nodes = 10 // higher number brings higher coverage
 
-	DiscoveryRetryTimeout = time.Millisecond * 100 // defined in discovery.go
-
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*20)
 	t.Cleanup(cancel)
 
@@ -43,7 +41,7 @@ func TestDiscovery(t *testing.T) {
 	}
 
 	host, routingDisc := tn.peer()
-	params := DefaultParameters()
+	params := TestParameters()
 	params.PeersLimit = nodes
 
 	// start discovery listener service for peerA
@@ -103,7 +101,7 @@ func TestDiscoveryTagged(t *testing.T) {
 	// sub will discover both peers, but on different tags
 	sub, routingDisc := tn.peer()
 
-	params := DefaultParameters()
+	params := TestParameters()
 
 	// create 2 discovery services for sub, each with a different tag
 	done1 := make(chan struct{})

share/p2p/discovery/options.go

@@ -7,6 +7,11 @@ import (
 	"github.com/libp2p/go-libp2p/core/peer"
 )
 
+const (
+	defaultAdvertiseRetryTimeout = time.Second
+	defaultDiscoveryRetryTimeout = defaultAdvertiseRetryTimeout * 60
+)
+
 // Parameters is the set of Parameters that must be configured for the Discovery module
 type Parameters struct {
 	// PeersLimit defines the soft limit of FNs to connect to via discovery.
@@ -15,6 +20,13 @@ type Parameters struct {
 	// AdvertiseInterval is a interval between advertising sessions.
 	// NOTE: only full and bridge can advertise themselves.
 	AdvertiseInterval time.Duration
+
+	// advertiseRetryTimeout defines time interval between advertise attempts.
+	AdvertiseRetryTimeout time.Duration
+
+	// DiscoveryRetryTimeout defines time interval between discovery attempts
+	// this is set independently for tests in discover_test.go
+	DiscoveryRetryTimeout time.Duration
 }
 
 // options is the set of options that can be configured for the Discovery module
@@ -33,13 +45,33 @@ type Option func(*options)
 // for the Discovery module
 func DefaultParameters() *Parameters {
 	return &Parameters{
-		PeersLimit:        5,
-		AdvertiseInterval: time.Hour,
+		PeersLimit:            5,
+		AdvertiseInterval:     time.Hour,
+		AdvertiseRetryTimeout: defaultAdvertiseRetryTimeout,
+		DiscoveryRetryTimeout: defaultDiscoveryRetryTimeout,
 	}
 }
 
+// TestParameters returns the default Parameters' configuration values
+// for the Discovery module, with some changes for configuration
+// during tests
+func TestParameters() *Parameters {
+	p := DefaultParameters()
+	p.AdvertiseInterval = time.Second * 1
+	p.DiscoveryRetryTimeout = time.Millisecond * 50
+	return p
+}
+
 // Validate validates the values in Parameters
 func (p *Parameters) Validate() error {
+	if p.AdvertiseRetryTimeout <= 0 {
+		return fmt.Errorf("discovery: advertise retry timeout cannot be zero or negative")
+	}
+
+	if p.DiscoveryRetryTimeout <= 0 {
+		return fmt.Errorf("discovery: discovery retry timeout cannot be zero or negative")
+	}
+
 	if p.PeersLimit <= 0 {
 		return fmt.Errorf("discovery: peers limit cannot be zero or negative")
 	}

share/p2p/discovery/options_test.go

@@ -0,0 +1,78 @@
+package discovery
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidate(t *testing.T) {
+	tests := []struct {
+		name    string
+		params  Parameters
+		wantErr bool
+	}{
+		{
+			name: "valid parameters",
+			params: Parameters{
+				PeersLimit:            5,
+				AdvertiseInterval:     time.Hour,
+				AdvertiseRetryTimeout: time.Second,
+				DiscoveryRetryTimeout: time.Minute,
+			},
+			wantErr: false,
+		},
+		{
+			name: "negative PeersLimit",
+			params: Parameters{
+				PeersLimit:            0,
+				AdvertiseInterval:     time.Hour,
+				AdvertiseRetryTimeout: time.Second,
+				DiscoveryRetryTimeout: time.Minute,
+			},
+			wantErr: true,
+		},
+		{
+			name: "negative AdvertiseInterval",
+			params: Parameters{
+				PeersLimit:            5,
+				AdvertiseInterval:     -time.Hour,
+				AdvertiseRetryTimeout: time.Second,
+				DiscoveryRetryTimeout: time.Minute,
+			},
+			wantErr: true,
+		},
+		{
+			name: "negative AdvertiseRetryTimeout",
+			params: Parameters{
+				PeersLimit:            5,
+				AdvertiseInterval:     time.Hour,
+				AdvertiseRetryTimeout: -time.Second,
+				DiscoveryRetryTimeout: time.Minute,
+			},
+			wantErr: true,
+		},
+		{
+			name: "negative DiscoveryRetryTimeout",
+			params: Parameters{
+				PeersLimit:            5,
+				AdvertiseInterval:     time.Hour,
+				AdvertiseRetryTimeout: time.Second,
+				DiscoveryRetryTimeout: -time.Minute,
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.params.Validate()
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

share/p2p/peers/manager_test.go

@@ -395,11 +395,9 @@ func TestIntegration(t *testing.T) {
 		bnRouter, err := dht.New(ctx, bnHost, opts...)
 		require.NoError(t, err)
 
-		// hack
-		discovery.DiscoveryRetryTimeout = time.Millisecond * 100
-
 		params := discovery.DefaultParameters()
 		params.AdvertiseInterval = time.Second
+		params.DiscoveryRetryTimeout = time.Millisecond * 100
 
 		bnDisc, err := discovery.NewDiscovery(
 			params,
@@ -433,7 +431,7 @@ func TestIntegration(t *testing.T) {
 		}
 
 		// set up discovery for full node with hook to peer manager and check discovered peer
-		params = discovery.DefaultParameters()
+		params = discovery.TestParameters()
 		params.AdvertiseInterval = time.Second
 		params.PeersLimit = 10