Merge pull request #5020 from ethereum-optimism/aj/test-discovery-fix

op-node/p2p: Fix flaky TestDiscovery

go.mod

@@ -32,6 +32,7 @@ require (
 	github.com/urfave/cli v1.22.9
 	github.com/urfave/cli/v2 v2.17.2-0.20221006022127-8f469abc00aa
 	golang.org/x/crypto v0.6.0
+	golang.org/x/exp v0.0.0-20230213192124-5e25df0256eb
 	golang.org/x/term v0.5.0
 )
 
@@ -174,7 +175,6 @@ require (
 	go.uber.org/fx v1.19.1 // indirect
 	go.uber.org/multierr v1.9.0 // indirect
 	go.uber.org/zap v1.24.0 // indirect
-	golang.org/x/exp v0.0.0-20230213192124-5e25df0256eb // indirect
 	golang.org/x/mod v0.8.0 // indirect
 	golang.org/x/net v0.7.0 // indirect
 	golang.org/x/sync v0.1.0 // indirect

op-node/p2p/host_test.go

@@ -19,6 +19,7 @@ import (
 	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
 	tswarm "github.com/libp2p/go-libp2p/p2p/net/swarm/testing"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/slices"
 
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/p2p/enode"
@@ -314,19 +315,22 @@ func TestDiscovery(t *testing.T) {
 
 	// B and C don't know each other yet, but both have A as a bootnode.
 	// It should only be a matter of time for them to connect, if they discover each other via A.
-	var firstPeersOfB []peer.ID
-	for i := 0; i < 2; i++ {
+	timeout := time.After(time.Second * 10)
+	var peersOfB []peer.ID
+	// B should be connected to the bootnode (A) it used (it's a valid optimism node to connect to here)
+	// C should also be connected, although this one might take more time to discover
+	for !slices.Contains(peersOfB, hostA.ID()) || !slices.Contains(peersOfB, hostC.ID()) {
 		select {
-		case <-time.After(time.Second * 30):
-			t.Fatal("failed to get connection to B in time")
+		case <-timeout:
+			var peers []string
+			for _, id := range peersOfB {
+				peers = append(peers, id.String())
+			}
+			t.Fatalf("timeout reached - expected host A: %v and host C: %v to be in %v", hostA.ID().String(), hostC.ID().String(), peers)
 		case c := <-connsB:
-			firstPeersOfB = append(firstPeersOfB, c.RemotePeer())
+			peersOfB = append(peersOfB, c.RemotePeer())
 		}
 	}
-	// B should be connected to the bootnode it used (it's a valid optimism node to connect to here)
-	require.Contains(t, firstPeersOfB, hostA.ID())
-	// C should be connected, although this one might take more time to discover
-	require.Contains(t, firstPeersOfB, hostC.ID())
 }
 
 // Most tests should use mocknets instead of using the actual local host network