From 95436c5dfa2f35de9a0f9c5f6d297d7c9d79f0e1 Mon Sep 17 00:00:00 2001 From: JoeK Date: Wed, 28 Oct 2020 20:05:28 +0000 Subject: [PATCH 1/5] privval: increase ping timeout to 5s Partially closes #5550 Co-authored-by: JoeK --- CHANGELOG_PENDING.md | 3 ++- privval/signer_listener_endpoint.go | 4 ++++ privval/socket_listeners.go | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 6454e913a63..8ae019b02aa 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -24,8 +24,9 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### IMPROVEMENTS -- [privval] \#5603 Add `--key` to `init`, `gen_validator`, `testnet` & `unsafe_reset_priv_validator` for use in generating `secp256k1` keys. +- [privval] \#5603 Add `--key` to `init`, `gen_validator`, `testnet` & `unsafe_reset_priv_validator` for use in generating `secp256k1` keys. ### BUG FIXES - [types] \#5523 Change json naming of `PartSetHeader` within `BlockID` from `parts` to `part_set_header` (@marbar3778) +- [privval] \#5638 Increase ping timeout to 5s (@JoeKash) diff --git a/privval/signer_listener_endpoint.go b/privval/signer_listener_endpoint.go index 1f05995fac4..c222076b7e3 100644 --- a/privval/signer_listener_endpoint.go +++ b/privval/signer_listener_endpoint.go @@ -103,6 +103,9 @@ func (sl *SignerListenerEndpoint) SendRequest(request privvalproto.Message) (*pr return nil, err } + // Reset pingTimer to avoid sending unnecessary pings. + sl.pingTimer.Reset(defaultPingPeriodMilliseconds * time.Millisecond) + return &res, nil } @@ -117,6 +120,7 @@ func (sl *SignerListenerEndpoint) ensureConnection(maxWait time.Duration) error } // block until connected or timeout + sl.Logger.Info("SignerListener: Blocking for connection") sl.triggerConnect() err := sl.WaitConnection(sl.connectionAvailableCh, maxWait) if err != nil { diff --git a/privval/socket_listeners.go b/privval/socket_listeners.go index 908e05e2ebd..23df2a3143a 100644 --- a/privval/socket_listeners.go +++ b/privval/socket_listeners.go @@ -10,7 +10,7 @@ import ( const ( defaultTimeoutAcceptSeconds = 3 - defaultPingPeriodMilliseconds = 100 + defaultPingPeriodMilliseconds = 5000 ) // timeoutError can be used to check if an error returned from the netp package From a6b3d9cb1f9a1b419464985db48e4d88c498756d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Nov 2020 14:37:17 +0400 Subject: [PATCH 2/5] privval: calculate ping interval based on read/write timeout --- privval/signer_dialer_endpoint.go | 20 ++++++------ privval/signer_endpoint.go | 2 +- privval/signer_listener_endpoint.go | 40 ++++++++++++++++++------ privval/signer_listener_endpoint_test.go | 6 +++- privval/socket_listeners.go | 1 - 5 files changed, 47 insertions(+), 22 deletions(-) diff --git a/privval/signer_dialer_endpoint.go b/privval/signer_dialer_endpoint.go index bd98314b600..93d26b04392 100644 --- a/privval/signer_dialer_endpoint.go +++ b/privval/signer_dialer_endpoint.go @@ -15,24 +15,26 @@ const ( // SignerServiceEndpointOption sets an optional parameter on the SignerDialerEndpoint. type SignerServiceEndpointOption func(*SignerDialerEndpoint) -// SignerDialerEndpointTimeoutReadWrite sets the read and write timeout for connections -// from external signing processes. +// SignerDialerEndpointTimeoutReadWrite sets the read and write timeout for +// connections from client processes. func SignerDialerEndpointTimeoutReadWrite(timeout time.Duration) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.timeoutReadWrite = timeout } } -// SignerDialerEndpointConnRetries sets the amount of attempted retries to acceptNewConnection. +// SignerDialerEndpointConnRetries sets the amount of attempted retries to +// acceptNewConnection. func SignerDialerEndpointConnRetries(retries int) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.maxConnRetries = retries } } -// SignerDialerEndpointRetryWaitInterval sets the retry wait interval to a custom value +// SignerDialerEndpointRetryWaitInterval sets the retry wait interval to a +// custom value. func SignerDialerEndpointRetryWaitInterval(interval time.Duration) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.retryWait = interval } } -// SignerDialerEndpoint dials using its dialer and responds to any -// signature requests using its privVal. +// SignerDialerEndpoint dials using its dialer and responds to any signature +// requests using its privVal. type SignerDialerEndpoint struct { signerEndpoint @@ -57,13 +59,13 @@ func NewSignerDialerEndpoint( maxConnRetries: defaultMaxDialRetries, } + sd.BaseService = *service.NewBaseService(logger, "SignerDialerEndpoint", sd) + sd.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second + for _, optionFunc := range options { optionFunc(sd) } - sd.BaseService = *service.NewBaseService(logger, "SignerDialerEndpoint", sd) - sd.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second - return sd } diff --git a/privval/signer_endpoint.go b/privval/signer_endpoint.go index 8ea5e2f8aab..eb2ed442fbe 100644 --- a/privval/signer_endpoint.go +++ b/privval/signer_endpoint.go @@ -12,7 +12,7 @@ import ( ) const ( - defaultTimeoutReadWriteSeconds = 3 + defaultTimeoutReadWriteSeconds = 5 ) type signerEndpoint struct { diff --git a/privval/signer_listener_endpoint.go b/privval/signer_listener_endpoint.go index c222076b7e3..fefa683172c 100644 --- a/privval/signer_listener_endpoint.go +++ b/privval/signer_listener_endpoint.go @@ -11,11 +11,22 @@ import ( privvalproto "github.com/tendermint/tendermint/proto/tendermint/privval" ) -// SignerValidatorEndpointOption sets an optional parameter on the SocketVal. -type SignerValidatorEndpointOption func(*SignerListenerEndpoint) +// SignerListenerEndpointOption sets an optional parameter on the SignerListenerEndpoint. +type SignerListenerEndpointOption func(*SignerListenerEndpoint) + +// SignerListenerEndpointTimeoutReadWrite sets the read and write timeout for +// connections from external signing processes. +// +// Default: 5s +func SignerListenerEndpointTimeoutReadWrite(timeout time.Duration) SignerListenerEndpointOption { + return func(sl *SignerListenerEndpoint) { sl.signerEndpoint.timeoutReadWrite = timeout } +} -// SignerListenerEndpoint listens for an external process to dial in -// and keeps the connection alive by dropping and reconnecting +// SignerListenerEndpoint listens for an external process to dial in and keeps +// the connection alive by dropping and reconnecting. +// +// The process will send pings every ~3s (read/write timeout * 2/3) to keep the +// connection alive. type SignerListenerEndpoint struct { signerEndpoint @@ -25,6 +36,7 @@ type SignerListenerEndpoint struct { timeoutAccept time.Duration pingTimer *time.Ticker + pingInterval time.Duration instanceMtx tmsync.Mutex // Ensures instance public methods access, i.e. SendRequest } @@ -33,15 +45,21 @@ type SignerListenerEndpoint struct { func NewSignerListenerEndpoint( logger log.Logger, listener net.Listener, + options ...SignerListenerEndpointOption, ) *SignerListenerEndpoint { - sc := &SignerListenerEndpoint{ + sl := &SignerListenerEndpoint{ listener: listener, timeoutAccept: defaultTimeoutAcceptSeconds * time.Second, } - sc.BaseService = *service.NewBaseService(logger, "SignerListenerEndpoint", sc) - sc.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second - return sc + sl.BaseService = *service.NewBaseService(logger, "SignerListenerEndpoint", sl) + sl.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second + + for _, optionFunc := range options { + optionFunc(sl) + } + + return sl } // OnStart implements service.Service. @@ -49,7 +67,9 @@ func (sl *SignerListenerEndpoint) OnStart() error { sl.connectRequestCh = make(chan struct{}) sl.connectionAvailableCh = make(chan net.Conn) - sl.pingTimer = time.NewTicker(defaultPingPeriodMilliseconds * time.Millisecond) + // NOTE: ping timeout must be less than read/write timeout + sl.pingInterval = time.Duration(sl.signerEndpoint.timeoutReadWrite.Milliseconds()*2/3) * time.Millisecond + sl.pingTimer = time.NewTicker(sl.pingInterval) go sl.serviceLoop() go sl.pingLoop() @@ -104,7 +124,7 @@ func (sl *SignerListenerEndpoint) SendRequest(request privvalproto.Message) (*pr } // Reset pingTimer to avoid sending unnecessary pings. - sl.pingTimer.Reset(defaultPingPeriodMilliseconds * time.Millisecond) + sl.pingTimer.Reset(sl.pingInterval) return &res, nil } diff --git a/privval/signer_listener_endpoint_test.go b/privval/signer_listener_endpoint_test.go index 02b19eff3d1..cbd45e6ceed 100644 --- a/privval/signer_listener_endpoint_test.go +++ b/privval/signer_listener_endpoint_test.go @@ -168,7 +168,11 @@ func newSignerListenerEndpoint(logger log.Logger, addr string, timeoutReadWrite listener = tcpLn } - return NewSignerListenerEndpoint(logger, listener) + return NewSignerListenerEndpoint( + logger, + listener, + SignerListenerEndpointTimeoutReadWrite(testTimeoutReadWrite), + ) } func startListenerEndpointAsync(t *testing.T, sle *SignerListenerEndpoint, endpointIsOpenCh chan struct{}) { diff --git a/privval/socket_listeners.go b/privval/socket_listeners.go index 23df2a3143a..b17562427e2 100644 --- a/privval/socket_listeners.go +++ b/privval/socket_listeners.go @@ -10,7 +10,6 @@ import ( const ( defaultTimeoutAcceptSeconds = 3 - defaultPingPeriodMilliseconds = 5000 ) // timeoutError can be used to check if an error returned from the netp package From 8802cc438ce77eb6d411dbc37a592690e546e0e3 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Nov 2020 14:39:23 +0400 Subject: [PATCH 3/5] update changelog --- CHANGELOG_PENDING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 8ae019b02aa..f22eae1d49f 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -29,4 +29,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### BUG FIXES - [types] \#5523 Change json naming of `PartSetHeader` within `BlockID` from `parts` to `part_set_header` (@marbar3778) -- [privval] \#5638 Increase ping timeout to 5s (@JoeKash) +- [privval] \#5638 Increase read/write timeout to 5s and calculate ping interval based on it (@JoeKash) From 1972448829430f0d4df2255a715edf171d0ac9a6 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Nov 2020 14:43:37 +0400 Subject: [PATCH 4/5] remove reset --- privval/signer_listener_endpoint.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/privval/signer_listener_endpoint.go b/privval/signer_listener_endpoint.go index fefa683172c..be5acb27896 100644 --- a/privval/signer_listener_endpoint.go +++ b/privval/signer_listener_endpoint.go @@ -123,9 +123,6 @@ func (sl *SignerListenerEndpoint) SendRequest(request privvalproto.Message) (*pr return nil, err } - // Reset pingTimer to avoid sending unnecessary pings. - sl.pingTimer.Reset(sl.pingInterval) - return &res, nil } From a9d26320b110916a0184ac8555203dfee8caafc4 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Nov 2020 14:51:37 +0400 Subject: [PATCH 5/5] format file --- privval/socket_listeners.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/privval/socket_listeners.go b/privval/socket_listeners.go index b17562427e2..ad49adac43b 100644 --- a/privval/socket_listeners.go +++ b/privval/socket_listeners.go @@ -9,7 +9,7 @@ import ( ) const ( - defaultTimeoutAcceptSeconds = 3 + defaultTimeoutAcceptSeconds = 3 ) // timeoutError can be used to check if an error returned from the netp package