From c69dbb25cec3afc2982ffda2e27a0f91fba7b9e2 Mon Sep 17 00:00:00 2001
From: Thane Thomson <connect@thanethomson.com>
Date: Thu, 17 Jan 2019 15:10:56 +0200
Subject: [PATCH] Consolidates deadline tests for privval Unix/TCP (#3143)

* Consolidates deadline tests for privval Unix/TCP

Following on from #3115 and #3132, this converts fundamental timeout
errors from the client's `acceptConnection()` method so that these can
be detected by the test for the TCP connection.

Timeout deadlines are now tested for both TCP and Unix domain socket
connections.

There is also no need for the additional secret connection code: the
connection will time out at the `acceptConnection()` phase for TCP
connections, and will time out when attempting to obtain the
`RemoteSigner`'s public key for Unix domain socket connections.

* Removes extraneous logging

* Adds IsConnTimeout helper function

This commit adds a helper function to detect whether an error is either
a fundamental networking timeout error, or an `ErrConnTimeout` error
specific to the `RemoteSigner` class.

* Adds a test for the IsConnTimeout() helper function

* Separates tests logically for IsConnTimeout
---
 privval/client_test.go        | 53 ++++++++++++++++-------------------
 privval/remote_signer.go      | 15 ++++++++++
 privval/remote_signer_test.go | 22 +++++++++++++++
 3 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/privval/client_test.go b/privval/client_test.go
index 3c327064..72682a8d 100644
--- a/privval/client_test.go
+++ b/privval/client_test.go
@@ -13,7 +13,6 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/libs/log"
 
-	p2pconn "github.com/tendermint/tendermint/p2p/conn"
 	"github.com/tendermint/tendermint/types"
 )
 
@@ -186,40 +185,36 @@ func TestSocketPVVoteKeepalive(t *testing.T) {
 	}
 }
 
-// TestSocketPVDeadlineTCPOnly is not relevant to Unix domain sockets, since the
-// OS knows instantaneously the state of both sides of the connection.
-func TestSocketPVDeadlineTCPOnly(t *testing.T) {
-	var (
-		addr            = testFreeTCPAddr(t)
-		listenc         = make(chan struct{})
-		thisConnTimeout = 100 * time.Millisecond
-		sc              = newSocketVal(log.TestingLogger(), addr, thisConnTimeout)
-	)
+func TestSocketPVDeadline(t *testing.T) {
+	for _, tc := range socketTestCases(t) {
+		func() {
+			var (
+				listenc         = make(chan struct{})
+				thisConnTimeout = 100 * time.Millisecond
+				sc              = newSocketVal(log.TestingLogger(), tc.addr, thisConnTimeout)
+			)
 
-	go func(sc *SocketVal) {
-		defer close(listenc)
+			go func(sc *SocketVal) {
+				defer close(listenc)
 
-		assert.Equal(t, sc.Start().(cmn.Error).Data(), ErrConnTimeout)
+				// Note: the TCP connection times out at the accept() phase,
+				// whereas the Unix domain sockets connection times out while
+				// attempting to fetch the remote signer's public key.
+				assert.True(t, IsConnTimeout(sc.Start()))
 
-		assert.False(t, sc.IsRunning())
-	}(sc)
+				assert.False(t, sc.IsRunning())
+			}(sc)
 
-	for {
-		conn, err := cmn.Connect(addr)
-		if err != nil {
-			continue
-		}
+			for {
+				_, err := cmn.Connect(tc.addr)
+				if err == nil {
+					break
+				}
+			}
 
-		_, err = p2pconn.MakeSecretConnection(
-			conn,
-			ed25519.GenPrivKey(),
-		)
-		if err == nil {
-			break
-		}
+			<-listenc
+		}()
 	}
-
-	<-listenc
 }
 
 func TestRemoteSignVoteErrors(t *testing.T) {
diff --git a/privval/remote_signer.go b/privval/remote_signer.go
index d928b198..1371e333 100644
--- a/privval/remote_signer.go
+++ b/privval/remote_signer.go
@@ -258,3 +258,18 @@ func handleRequest(req RemoteSignerMsg, chainID string, privVal types.PrivValida
 
 	return res, err
 }
+
+// IsConnTimeout returns a boolean indicating whether the error is known to
+// report that a connection timeout occurred. This detects both fundamental
+// network timeouts, as well as ErrConnTimeout errors.
+func IsConnTimeout(err error) bool {
+	if cmnErr, ok := err.(cmn.Error); ok {
+		if cmnErr.Data() == ErrConnTimeout {
+			return true
+		}
+	}
+	if _, ok := err.(timeoutError); ok {
+		return true
+	}
+	return false
+}
diff --git a/privval/remote_signer_test.go b/privval/remote_signer_test.go
index 8927e224..cb2a600d 100644
--- a/privval/remote_signer_test.go
+++ b/privval/remote_signer_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tendermint/tendermint/crypto/ed25519"
@@ -66,3 +67,24 @@ func TestRemoteSignerRetryTCPOnly(t *testing.T) {
 		t.Error("expected remote to observe connection attempts")
 	}
 }
+
+func TestIsConnTimeoutForFundamentalTimeouts(t *testing.T) {
+	// Generate a networking timeout
+	dialer := DialTCPFn(testFreeTCPAddr(t), time.Millisecond, ed25519.GenPrivKey())
+	_, err := dialer()
+	assert.Error(t, err)
+	assert.True(t, IsConnTimeout(err))
+}
+
+func TestIsConnTimeoutForWrappedConnTimeouts(t *testing.T) {
+	dialer := DialTCPFn(testFreeTCPAddr(t), time.Millisecond, ed25519.GenPrivKey())
+	_, err := dialer()
+	assert.Error(t, err)
+	err = cmn.ErrorWrap(ErrConnTimeout, err.Error())
+	assert.True(t, IsConnTimeout(err))
+}
+
+func TestIsConnTimeoutForNonTimeoutErrors(t *testing.T) {
+	assert.False(t, IsConnTimeout(cmn.ErrorWrap(ErrDialRetryMax, "max retries exceeded")))
+	assert.False(t, IsConnTimeout(errors.New("completely irrelevant error")))
+}