Merge pull request #2373 from OffchainLabs/fix-challenges

Make the test signer easier to use in parallel tests

arbnode/dataposter/dataposter_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"math/big"
-	"net/http"
 	"testing"
 	"time"
 
@@ -14,6 +13,7 @@ import (
 	"github.com/ethereum/go-ethereum/accounts/abi/bind/backends"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/params"
 	"github.com/ethereum/go-ethereum/rpc"
 	"github.com/google/go-cmp/cmp"
@@ -58,14 +58,14 @@ func TestParseReplacementTimes(t *testing.T) {
 	}
 }
 
-func signerTestCfg(addr common.Address) (*ExternalSignerCfg, error) {
+func signerTestCfg(addr common.Address, url string) (*ExternalSignerCfg, error) {
 	cp, err := externalsignertest.CertPaths()
 	if err != nil {
 		return nil, fmt.Errorf("getting certificates path: %w", err)
 	}
 	return &ExternalSignerCfg{
 		Address:          common.Bytes2Hex(addr.Bytes()),
-		URL:              externalsignertest.SignerURL,
+		URL:              url,
 		Method:           externalsignertest.SignerMethod,
 		RootCA:           cp.ServerCert,
 		ClientCert:       cp.ClientCert,
@@ -106,15 +106,14 @@ var (
 )
 
 func TestExternalSigner(t *testing.T) {
-	httpSrv, srv := externalsignertest.NewServer(t)
-	cert, key := "./testdata/localhost.crt", "./testdata/localhost.key"
+	srv := externalsignertest.NewServer(t)
 	go func() {
-		if err := httpSrv.ListenAndServeTLS(cert, key); err != nil && err != http.ErrServerClosed {
-			t.Errorf("ListenAndServeTLS() unexpected error:  %v", err)
+		if err := srv.Start(); err != nil {
+			log.Error("Failed to start external signer server:", err)
 			return
 		}
 	}()
-	signerCfg, err := signerTestCfg(srv.Address)
+	signerCfg, err := signerTestCfg(srv.Address, srv.URL())
 	if err != nil {
 		t.Fatalf("Error getting signer test config: %v", err)
 	}

arbnode/dataposter/externalsignertest/externalsignertest.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"math/big"
+	"net"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -20,15 +22,13 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/rpc"
 	"github.com/offchainlabs/nitro/arbnode/dataposter/externalsigner"
+	"github.com/offchainlabs/nitro/util/testhelpers"
 )
 
 var (
 	dataPosterPath = "arbnode/dataposter"
 	selfPath       = filepath.Join(dataPosterPath, "externalsignertest")
-
-	SignerPort   = 1234
-	SignerURL    = fmt.Sprintf("https://localhost:%v", SignerPort)
-	SignerMethod = "test_signTransaction"
+	SignerMethod   = "test_signTransaction"
 )
 
 type CertAbsPaths struct {
@@ -38,6 +38,12 @@ type CertAbsPaths struct {
 	ClientKey  string
 }
 
+type SignerServer struct {
+	*http.Server
+	*SignerAPI
+	listener net.Listener
+}
+
 func basePath() (string, error) {
 	_, file, _, ok := runtime.Caller(1)
 	if !ok {
@@ -71,7 +77,7 @@ func CertPaths() (*CertAbsPaths, error) {
 	}, nil
 }
 
-func NewServer(t *testing.T) (*http.Server, *SignerAPI) {
+func NewServer(t *testing.T) *SignerServer {
 	rpcServer := rpc.NewServer()
 	signer, address, err := setupAccount("/tmp/keystore")
 	if err != nil {
@@ -94,8 +100,13 @@ func NewServer(t *testing.T) (*http.Server, *SignerAPI) {
 	pool := x509.NewCertPool()
 	pool.AppendCertsFromPEM(clientCert)
 
+	ln, err := testhelpers.FreeTCPPortListener()
+	if err != nil {
+		t.Fatalf("Error getting a listener on a free TCP port: %v", err)
+	}
+
 	httpServer := &http.Server{
-		Addr:              fmt.Sprintf(":%d", SignerPort),
+		Addr:              ln.Addr().String(),
 		Handler:           rpcServer,
 		ReadTimeout:       30 * time.Second,
 		ReadHeaderTimeout: 30 * time.Second,
@@ -109,12 +120,36 @@ func NewServer(t *testing.T) (*http.Server, *SignerAPI) {
 	}
 
 	t.Cleanup(func() {
-		if err := httpServer.Close(); err != nil {
+		if err := httpServer.Close(); err != nil && !errors.Is(err, http.ErrServerClosed) {
 			t.Fatalf("Error shutting down http server: %v", err)
 		}
+		// Explicitly close the listner in case the server was never started.
+		if err := ln.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
+			t.Fatalf("Error closing listener: %v", err)
+		}
 	})
 
-	return httpServer, s
+	return &SignerServer{httpServer, s, ln}
+}
+
+// URL returns the URL of the signer server.
+//
+// Note: The server must return "localhost" for the hostname part of
+// the URL to match the expectations from the TLS certificate.
+func (s *SignerServer) URL() string {
+	port := strings.Split(s.Addr, ":")[1]
+	return fmt.Sprintf("https://localhost:%s", port)
+}
+
+func (s *SignerServer) Start() error {
+	cp, err := CertPaths()
+	if err != nil {
+		return err
+	}
+	if err := s.ServeTLS(s.listener, cp.ServerCert, cp.ServerKey); err != nil && !errors.Is(err, http.ErrServerClosed) {
+		return err
+	}
+	return nil
 }
 
 // setupAccount creates a new account in a given directory, unlocks it, creates

system_tests/batch_poster_test.go

@@ -8,7 +8,6 @@ import (
 	"crypto/rand"
 	"fmt"
 	"math/big"
-	"net/http"
 	"strings"
 	"testing"
 	"time"
@@ -62,14 +61,14 @@ func addNewBatchPoster(ctx context.Context, t *testing.T, builder *NodeBuilder,
 	}
 }
 
-func externalSignerTestCfg(addr common.Address) (*dataposter.ExternalSignerCfg, error) {
+func externalSignerTestCfg(addr common.Address, url string) (*dataposter.ExternalSignerCfg, error) {
 	cp, err := externalsignertest.CertPaths()
 	if err != nil {
 		return nil, fmt.Errorf("getting certificates path: %w", err)
 	}
 	return &dataposter.ExternalSignerCfg{
 		Address:          common.Bytes2Hex(addr.Bytes()),
-		URL:              externalsignertest.SignerURL,
+		URL:              url,
 		Method:           externalsignertest.SignerMethod,
 		RootCA:           cp.ServerCert,
 		ClientCert:       cp.ClientCert,
@@ -80,24 +79,13 @@ func externalSignerTestCfg(addr common.Address) (*dataposter.ExternalSignerCfg,
 func testBatchPosterParallel(t *testing.T, useRedis bool) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
-	httpSrv, srv := externalsignertest.NewServer(t)
-	cp, err := externalsignertest.CertPaths()
-	if err != nil {
-		t.Fatalf("Error getting cert paths: %v", err)
-	}
-	t.Cleanup(func() {
-		if err := httpSrv.Shutdown(ctx); err != nil {
-			t.Fatalf("Error shutting down http server: %v", err)
-		}
-	})
+	srv := externalsignertest.NewServer(t)
 	go func() {
-		log.Debug("Server is listening on port 1234...")
-		if err := httpSrv.ListenAndServeTLS(cp.ServerCert, cp.ServerKey); err != nil && err != http.ErrServerClosed {
-			log.Debug("ListenAndServeTLS() failed", "error", err)
+		if err := srv.Start(); err != nil {
+			log.Error("Failed to start external signer server:", err)
 			return
 		}
 	}()
-
 	var redisUrl string
 	if useRedis {
 		redisUrl = redisutil.CreateTestRedis(ctx, t)
@@ -114,7 +102,7 @@ func testBatchPosterParallel(t *testing.T, useRedis bool) {
 	builder := NewNodeBuilder(ctx).DefaultConfig(t, true)
 	builder.nodeConfig.BatchPoster.Enable = false
 	builder.nodeConfig.BatchPoster.RedisUrl = redisUrl
-	signerCfg, err := externalSignerTestCfg(srv.Address)
+	signerCfg, err := externalSignerTestCfg(srv.Address, srv.URL())
 	if err != nil {
 		t.Fatalf("Error getting external signer config: %v", err)
 	}

system_tests/staker_test.go

@@ -12,7 +12,6 @@ import (
 	"errors"
 	"fmt"
 	"math/big"
-	"net/http"
 	"strings"
 	"testing"
 	"time"
@@ -61,20 +60,10 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool)
 	t.Parallel()
 	ctx, cancelCtx := context.WithCancel(context.Background())
 	defer cancelCtx()
-	httpSrv, srv := externalsignertest.NewServer(t)
-	cp, err := externalsignertest.CertPaths()
-	if err != nil {
-		t.Fatalf("Error getting cert paths: %v", err)
-	}
-	t.Cleanup(func() {
-		if err := httpSrv.Shutdown(ctx); err != nil {
-			t.Fatalf("Error shutting down http server: %v", err)
-		}
-	})
+	srv := externalsignertest.NewServer(t)
 	go func() {
-		log.Debug("Server is listening on port 1234...")
-		if err := httpSrv.ListenAndServeTLS(cp.ServerCert, cp.ServerKey); err != nil && err != http.ErrServerClosed {
-			log.Debug("ListenAndServeTLS() failed", "error", err)
+		if err := srv.Start(); err != nil {
+			log.Error("Failed to start external signer server:", err)
 			return
 		}
 	}()
@@ -234,7 +223,7 @@ func stakerTestImpl(t *testing.T, faultyStaker bool, honestStakerInactive bool)
 	}
 	Require(t, err)
 	cfg := arbnode.ConfigDefaultL1NonSequencerTest()
-	signerCfg, err := externalSignerTestCfg(srv.Address)
+	signerCfg, err := externalSignerTestCfg(srv.Address, srv.URL())
 	if err != nil {
 		t.Fatalf("Error getting external signer config: %v", err)
 	}

util/testhelpers/port.go

@@ -0,0 +1,17 @@
+package testhelpers
+
+import (
+	"net"
+)
+
+// FreeTCPPortListener returns a listener listening on an unused local port.
+//
+// This is useful for tests that need to bind to a port without risking a conflict.
+func FreeTCPPortListener() (net.Listener, error) {
+	// This works because the kernel will assign an unused port when ":0" is opened.
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		return nil, err
+	}
+	return l, nil
+}

util/testhelpers/port_test.go

@@ -0,0 +1,23 @@
+package testhelpers
+
+import (
+	"net"
+	"testing"
+)
+
+func TestFreeTCPPortListener(t *testing.T) {
+	aListener, err := FreeTCPPortListener()
+	if err != nil {
+		t.Fatal(err)
+	}
+	bListener, err := FreeTCPPortListener()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if aListener.Addr().(*net.TCPAddr).Port == bListener.Addr().(*net.TCPAddr).Port {
+		t.Errorf("FreeTCPPortListener() got same port: %v, %v", aListener, bListener)
+	}
+	if aListener.Addr().(*net.TCPAddr).Port == 0 || bListener.Addr().(*net.TCPAddr).Port == 0 {
+		t.Errorf("FreeTCPPortListener() got port 0")
+	}
+}