e2e: Migrate duplicate node id test from kurtosis

Author: maru-ava

.github/workflows/test.e2e.persistent.yml

@@ -28,4 +28,4 @@ jobs:
         run: ./scripts/build.sh -r
       - name: Run e2e tests with persistent network
         shell: bash
-        run: ./scripts/tests.e2e.persistent.sh ./build/avalanchego
+        run: E2E_SERIAL=1 ./scripts/tests.e2e.persistent.sh ./build/avalanchego

.github/workflows/test.e2e.yml

@@ -28,4 +28,4 @@ jobs:
         run: ./scripts/build.sh -r
       - name: Run e2e tests
         shell: bash
-        run: scripts/tests.e2e.sh ./build/avalanchego
+        run: E2E_SERIAL=1 ./scripts/tests.e2e.sh ./build/avalanchego

scripts/tests.e2e.sh

@@ -44,10 +44,27 @@ else
 fi
 
 #################################
-# - Execute in parallel (-p) with the ginkgo cli to minimize execution time.
-#   The test binary by itself isn't capable of running specs in parallel.
+# Determine ginkgo args
+GINKGO_ARGS=""
+if [[ -n "${E2E_SERIAL:-}" ]]; then
+  # Specs will be executed serially. This supports running e2e tests in CI
+  # where parallel execution of tests that start new nodes beyond the
+  # initial set of validators could overload the free tier CI workers.
+  # Forcing serial execution in this test script instead of marking
+  # resource-hungry tests as serial supports executing the test suite faster
+  # on powerful development workstations.
+  echo "tests will be executed serially to minimize resource requirements"
+else
+  # Enable parallel execution of specs defined in the test binary, which
+  # requires invoking the binary via the ginkgo cli. The test binary by
+  # itself isn't capable of executing specs in parallel.
+  echo "tests will be executed in parallel"
+  GINKGO_ARGS="-p"
+fi
+
+#################################
 # - Execute in random order to identify unwanted dependency
-ginkgo -p -v --randomize-all ./tests/e2e/e2e.test -- ${E2E_ARGS} \
+ginkgo ${GINKGO_ARGS} -v --randomize-all ./tests/e2e/e2e.test -- ${E2E_ARGS} \
 && EXIT_CODE=$? || EXIT_CODE=$?
 
 if [[ ${EXIT_CODE} -gt 0 ]]; then

tests/e2e/e2e.go

@@ -5,12 +5,16 @@
 package e2e
 
 import (
+	"context"
 	"math/rand"
+	"os"
 	"time"
 
 	ginkgo "github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/require"
 
+	cfg "github.com/ava-labs/avalanchego/config"
+	"github.com/ava-labs/avalanchego/tests"
 	"github.com/ava-labs/avalanchego/tests/fixture"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet/local"
@@ -25,6 +29,10 @@ const (
 	// Enough for test/custom networks.
 	DefaultConfirmTxTimeout = 20 * time.Second
 
+	// This interval should represent the upper bound of the time
+	// required to start a new node on a local test network.
+	DefaultNodeStartTimeout = 20 * time.Second
+
 	// A long default timeout used to timeout failed operations but
 	// unlikely to induce flaking due to unexpected resource
 	// contention.
@@ -69,3 +77,34 @@ func (te *TestEnvironment) AllocateFundedKeys(count int) []*secp256k1.PrivateKey
 func (te *TestEnvironment) AllocateFundedKey() *secp256k1.PrivateKey {
 	return te.AllocateFundedKeys(1)[0]
 }
+
+// Add a temporary node that is only intended to be used by a single
+// test. Its ID and URI are not intended to be returned from The
+// Network instance to avoid accessibility from other tests.
+func AddTemporaryNode(network testnet.Network, flags testnet.FlagsMap) testnet.Node {
+	require := require.New(ginkgo.GinkgoT())
+
+	// A temporary location ensures the node won't be accessible from
+	// the Network instance.
+	nodeDir, err := os.MkdirTemp("", "")
+	require.NoError(err)
+
+	flags[cfg.DataDirKey] = nodeDir
+	node, err := network.AddNode(ginkgo.GinkgoWriter, flags)
+	require.NoError(err)
+
+	// Ensure node is stopped and its configuration removed
+	ginkgo.DeferCleanup(func() {
+		tests.Outf("Shutting down temporary node %s\n", node.GetID())
+		require.NoError(node.Remove())
+	})
+
+	return node
+}
+
+// Wait for the given node to report healthy.
+func WaitForHealthy(node testnet.Node) {
+	ctx, cancel := context.WithTimeout(context.Background(), DefaultTimeout)
+	defer cancel()
+	require.NoError(ginkgo.GinkgoT(), node.WaitForHealthy(ctx))
+}

tests/e2e/e2e_test.go

@@ -23,6 +23,7 @@ import (
 
 	// ensure test packages are scanned by ginkgo
 	_ "github.com/ava-labs/avalanchego/tests/e2e/banff"
+	_ "github.com/ava-labs/avalanchego/tests/e2e/faultinjection"
 	_ "github.com/ava-labs/avalanchego/tests/e2e/p"
 	_ "github.com/ava-labs/avalanchego/tests/e2e/static-handlers"
 	_ "github.com/ava-labs/avalanchego/tests/e2e/x/transfer"

tests/e2e/faultinjection/duplicate_node_id.go

@@ -0,0 +1,90 @@
+// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package faultinjection
+
+import (
+	"context"
+
+	ginkgo "github.com/onsi/ginkgo/v2"
+	"github.com/stretchr/testify/require"
+
+	"github.com/ava-labs/avalanchego/api/info"
+	cfg "github.com/ava-labs/avalanchego/config"
+	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/tests/e2e"
+	"github.com/ava-labs/avalanchego/tests/fixture/testnet"
+	"github.com/ava-labs/avalanchego/utils/set"
+)
+
+var _ = ginkgo.Describe("Duplicate node handling", func() {
+	require := require.New(ginkgo.GinkgoT())
+
+	ginkgo.It("should ensure that a given Node ID (i.e. staking keypair) can be used at most once on a network", func() {
+		network := e2e.Env.GetNetwork()
+		nodes := network.GetNodes()
+
+		ginkgo.By("creating new node")
+		node1 := e2e.AddTemporaryNode(network, testnet.FlagsMap{})
+		e2e.WaitForHealthy(node1)
+
+		ginkgo.By("checking that the new node is connected to its peers")
+		checkConnectedPeers(nodes, node1)
+
+		ginkgo.By("creating a second new node with the same staking keypair as the first new node")
+		node1Flags := node1.GetConfig().Flags
+		node2Flags := testnet.FlagsMap{
+			cfg.StakingTLSKeyContentKey: node1Flags[cfg.StakingTLSKeyContentKey],
+			cfg.StakingCertContentKey:   node1Flags[cfg.StakingCertContentKey],
+		}
+		node2 := e2e.AddTemporaryNode(network, node2Flags)
+
+		ginkgo.By("checking that the second new node fails to become healthy within timeout")
+		ctx, cancel := context.WithTimeout(context.Background(), e2e.DefaultNodeStartTimeout)
+		defer cancel()
+		err := node2.WaitForHealthy(ctx)
+		require.ErrorIs(err, context.DeadlineExceeded)
+
+		ginkgo.By("stopping the first new node")
+		require.NoError(node1.Stop())
+
+		ginkgo.By("checking that the second new node becomes healthy within timeout")
+		e2e.WaitForHealthy(node2)
+
+		ginkgo.By("checking that the second new node is connected to its peers")
+		checkConnectedPeers(nodes, node2)
+	})
+})
+
+// Check that a new node is connected to existing nodes and vice versa
+func checkConnectedPeers(existingNodes []testnet.Node, newNode testnet.Node) {
+	require := require.New(ginkgo.GinkgoT())
+
+	// Collect the node ids of the new node's peers
+	infoClient := info.NewClient(newNode.GetProcessContext().URI)
+	peers, err := infoClient.Peers(context.Background())
+	require.NoError(err)
+	peerIDs := set.NewSet[ids.NodeID](len(existingNodes))
+	for _, peer := range peers {
+		peerIDs.Add(peer.ID)
+	}
+
+	newNodeID := newNode.GetID().String()
+	for _, existingNode := range existingNodes {
+		// Check that the existing node is a peer of the new node
+		require.True(peerIDs.Contains(existingNode.GetID()))
+
+		// Check that the new node is a peer
+		infoClient := info.NewClient(existingNode.GetProcessContext().URI)
+		peers, err := infoClient.Peers(context.Background())
+		require.NoError(err)
+		isPeer := false
+		for _, peer := range peers {
+			if peer.ID.String() == newNodeID {
+				isPeer = true
+				break
+			}
+		}
+		require.True(isPeer)
+	}
+}

tests/fixture/testnet/interfaces.go

@@ -4,6 +4,9 @@
 package testnet
 
 import (
+	"context"
+	"io"
+
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/node"
 )
@@ -12,11 +15,16 @@ import (
 type Network interface {
 	GetConfig() NetworkConfig
 	GetNodes() []Node
+	AddNode(w io.Writer, flags FlagsMap) (Node, error)
 }
 
 // Defines node capabilities supportable regardless of how a network is orchestrated.
 type Node interface {
 	GetID() ids.NodeID
 	GetConfig() NodeConfig
 	GetProcessContext() node.NodeProcessContext
+	IsHealthy(ctx context.Context) (bool, error)
+	WaitForHealthy(ctx context.Context) error
+	Stop() error
+	Remove() error
 }

tests/fixture/testnet/local/network.go

@@ -34,7 +34,8 @@ var (
 	errInvalidKeyCount         = errors.New("failed to populate local network config: non-zero key count is only valid for a network without keys")
 	errLocalNetworkDirNotSet   = errors.New("local network directory not set - has Create() been called?")
 	errInvalidNetworkDir       = errors.New("failed to write local network: invalid network directory")
-	errNotHealthyBeforeTimeout = errors.New("failed to see all nodes healthy before timeout")
+	errNotHealthyBeforeTimeout = fmt.Errorf("failed to see all nodes healthy before timeout: %w", context.DeadlineExceeded)
+	errMissingBootstrapNodes   = errors.New("failed to add node due to missing bootstrap nodes")
 )
 
 // Default root dir for storing networks and their configuration.
@@ -101,6 +102,22 @@ func (ln *LocalNetwork) GetNodes() []testnet.Node {
 	return nodes
 }
 
+// Adds a backend-agnostic node to the network
+func (ln *LocalNetwork) AddNode(w io.Writer, flags testnet.FlagsMap) (testnet.Node, error) {
+	if flags == nil {
+		flags = testnet.FlagsMap{}
+	}
+	node, err := ln.AddLocalNode(w, &LocalNode{
+		NodeConfig: testnet.NodeConfig{
+			Flags: flags,
+		},
+	})
+	if err != nil {
+		return nil, err
+	}
+	return node, nil
+}
+
 // Starts a new network stored under the provided root dir. Required
 // configuration will be defaulted if not provided.
 func StartNetwork(
@@ -655,3 +672,51 @@ func (ln *LocalNetwork) ReadAll() error {
 	}
 	return ln.ReadNodes()
 }
+
+func (ln *LocalNetwork) AddLocalNode(w io.Writer, node *LocalNode) (*LocalNode, error) {
+	// Assume network configuration has been written to disk and is current in memory
+
+	if node == nil {
+		node = NewLocalNode("")
+	}
+	if err := ln.PopulateNodeConfig(node); err != nil {
+		return nil, err
+	}
+
+	// Use dynamic port allocation. Starting a network with static ports may be
+	// faster by allowing bootstrap IPs to be determined statically, but
+	// bootstrap IPs are already available when adding a node to an existing
+	// network.
+	var httpPort uint16 = 0
+	var stakingPort uint16 = 0
+
+	// Collect staking addresses of running nodes for use in bootstraping the new node
+	if err := ln.ReadNodes(); err != nil {
+		return nil, fmt.Errorf("failed to read local network nodes: %w", err)
+	}
+	bootstrapIPs := []string{}
+	bootstrapIDs := []string{}
+	for _, node := range ln.Nodes {
+		if len(node.StakingAddress) == 0 {
+			// Node is not running
+			continue
+		}
+
+		bootstrapIPs = append(bootstrapIPs, node.StakingAddress)
+		bootstrapIDs = append(bootstrapIDs, node.NodeID.String())
+	}
+	if len(bootstrapIDs) == 0 {
+		return nil, errMissingBootstrapNodes
+	}
+
+	node.SetNetworkingConfigDefaults(httpPort, stakingPort, bootstrapIDs, bootstrapIPs)
+
+	if err := node.WriteConfig(); err != nil {
+		return nil, err
+	}
+	if err := node.Start(w, ln.ExecPath); err != nil {
+		return nil, err
+	}
+
+	return node, nil
+}