Refactoring Remote signers by jleni · Pull Request #3370 · tendermint/tendermint

jleni · 2019-03-01T08:39:50Z

This PR is related to #3107 and a continuation of #3351

It is important to emphasise that in the privval original design, client/server and listening/dialing roles are inverted and do not follow a conventional interaction.

Given two hosts A and B:

Host A is listener/client
Host B is dialer/server (contains the secret key)
When A requires a signature, it needs to wait for B to dial in before it can issue a request.
A only accepts a single connection and any failure leads to dropping the connection and waiting for B to reconnect.

The original rationale behind this design was based on security.

Host B only allows outbound connections to a list of whitelisted hosts.
It is not possible to reach B unless B dials in. There are no listening/open ports in B.

This PR results in the following changes:

Refactors ping/heartbeat to avoid previously existing race conditions.
Separates transport (dialer/listener) from signing (client/server) concerns to simplify workflow.
Unifies and abstracts away the differences between unix and tcp sockets.
A single signer endpoint implementation unifies connection handling code (read/write/close/connection obj)
The signer request handler (server side) is customizable to increase testability.
Updates and extends unit tests

A high level overview of the classes is as follows:

Transport (endpoints): The following classes take care of establishing a connection

SignerDialerEndpoint
SignerListeningEndpoint
SignerEndpoint groups common functionality (read/write/timeouts/etc.)

Signing (client/server): The following classes take care of exchanging request/responses

SignerClient
SignerServer

This PR also closes #3601

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

privval/signer_listener_endpoint.go

privval/signer_dialer_endpoint.go

tools/tm-signer-harness/internal/test_harness_test.go

codecov-io · 2019-03-19T10:37:12Z

Codecov Report

Merging #3370 into master will increase coverage by 0.09%.
The diff coverage is 77.28%.

@@            Coverage Diff             @@
##           master    #3370      +/-   ##
==========================================
+ Coverage   65.97%   66.07%   +0.09%     
==========================================
  Files         234      237       +3     
  Lines       20244    20343      +99     
==========================================
+ Hits        13356    13441      +85     
- Misses       5816     5832      +16     
+ Partials     1072     1070       -2

Impacted Files	Coverage Δ
types/priv_validator.go	`54.54% <ø> (ø)`	⬆️
privval/socket_listeners.go	`86.2% <ø> (ø)`	⬆️
privval/errors.go	`0% <0%> (ø)`	⬆️
privval/signer_server.go	`100% <100%> (ø)`
privval/messages.go	`100% <100%> (ø)`	⬆️
privval/signer_dialer_endpoint.go	`100% <100%> (ø)`
privval/utils.go	`33.33% <16.66%> (-66.67%)`	⬇️
privval/signer_client.go	`48.33% <48.33%> (ø)`
tools/tm-signer-harness/internal/test_harness.go	`62.96% <68.42%> (-0.25%)`	⬇️
node/node.go	`62.29% <77.77%> (-0.46%)`	⬇️
... and 18 more

jleni · 2019-03-19T11:08:16Z

This fail is unrelated but probably interesting. Maybe we should open another issue for this:
https://circleci.com/gh/tendermint/tendermint/50723#tests/containers/3

=== RUN   TestTransportMultiplexValidateNodeInfo
panic: send on closed channel

goroutine 2011 [running]:
github.com/tendermint/tendermint/p2p.TestTransportMultiplexAcceptNonBlocking.func1(0xc0002d2b40, 0xc000a3c000, 0xc000a3c0c0, 0xc000a3c060)
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:256 +0x614
created by github.com/tendermint/tendermint/p2p.TestTransportMultiplexAcceptNonBlocking
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:231 +0x273
FAIL	github.com/tendermint/tendermint/p2p	2.775s
Exited with code 1

jleni · 2019-04-01T11:04:13Z

@melekes @liamsi any news about this?
maybe we can discuss if we merge this or include other elements as what Tony was proposing.

jleni · 2019-04-01T11:05:23Z

I think it makes sense that we define a way to beta test changes like this. Is there a testnet planned for this purpose?

liamsi · 2019-04-02T12:31:56Z

ref: #3486

liamsi · 2019-04-02T12:33:11Z

@melekes @liamsi any news about this?
maybe we can discuss if we merge this or include other elements as what Tony was proposing.

Let us review it another time. I think we should merge this. But I'm not sure I do remember which changes Tony was proposing. Let's jump on call?

jleni · 2019-04-02T12:40:10Z

Here is the link to Tony's proposal

allow full node startup without a connection for improved failover
#3455

liamsi · 2019-04-02T12:47:00Z

Ah Ok this one. Hmm. I'd prefer to keep this separate and implement Tony's proposal in a follow-up PR. Let's chat about this tomorrow.

.gitignore

node/node.go

privval/signer_client.go

privval/signer_client_test.go

privval/signer_listener_endpoint.go

privval/signer_dialer_endpoint.go

privval/signer_client.go

CHANGELOG_PENDING.md

node/node.go

melekes · 2019-07-26T07:25:38Z

privval/signer_client_test.go

+
+func TestSignerClose(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {


let's drop func() {}() wrappers if they don't serve any specific purpose (async code is generally harder to support / test / etc.)

privval/signer_listener_endpoint.go

privval/signer_requestHandler.go

privval/socket_dialers_test.go

privval/utils.go

types/priv_validator.go

privval/signer_listener_endpoint.go

privval/socket_dialers_test.go

privval/errors.go

privval/signer_dialer_endpoint.go

privval/signer_server.go

cmd/priv_val_server/main.go

privval/signer_listener_endpoint_test.go

privval/signer_client_test.go

privval/signer_dialer_endpoint.go

privval/signer_listener_endpoint.go

privval/signer_listener_endpoint_test.go

privval/signer_server.go

privval/signer_dialer_endpoint.go

melekes · 2019-07-29T08:01:16Z

privval/signer_listener_endpoint.go

+	pingTimer     *time.Ticker

-	pingTimer *time.Ticker
+	instanceMtx sync.Mutex // Ensures instance public methods access, i.e. SendRequest


I think we can just say

not concurrency safe

in the godoc comment and remove the instanceMtx

This mutex is actually very important so external calls to SendRequest never mix with internal SendRequest(&PingRequest)

melekes · 2019-08-05T14:00:45Z

panic: Fail in goroutine after TestTransportHandshake has completed

goroutine 2777 [running]:
testing.(*common).Fail(0xc00054c200)
	/usr/local/go/src/testing/testing.go:565 +0x1a4
testing.(*common).Error(0xc00054c200, 0xc0001bcfa8, 0x1, 0x1)
	/usr/local/go/src/testing/testing.go:654 +0x8b
github.com/tendermint/tendermint/p2p.TestTransportHandshake.func1.2(0xc00054c200, 0xfaa060, 0xc0004ae110)
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:540 +0x17f
created by github.com/tendermint/tendermint/p2p.TestTransportHandshake.func1
	/go/src/github.com/tendermint/tendermint/p2p/transport_test.go:531 +0x296
FAIL	github.com/tendermint/tendermint/p2p	7.962s

not sure if it's related

melekes · 2019-08-05T15:05:21Z

CHANGELOG_PENDING.md


 ### IMPROVEMENTS:

+- [privval] \#3370 Refactors and simplifies validator/kms connection handling. Please refer to thttps://github.com/tendermint/tendermint/pull/3370#issue-257360971


Suggested change

- [privval] \#3370 Refactors and simplifies validator/kms connection handling. Please refer to thttps://github.com/tendermint/tendermint/pull/3370#issue-257360971

- [privval] \#3370 Refactors and simplifies validator/kms connection handling. Please refer to [this comment](https://github.com/tendermint/tendermint/pull/3370#issue-257360971) for details.

liamsi reviewed Mar 18, 2019

View reviewed changes

privval/signer_listener_endpoint.go Outdated Show resolved Hide resolved

liamsi reviewed Mar 18, 2019

View reviewed changes

privval/signer_dialer_endpoint.go Outdated Show resolved Hide resolved

liamsi reviewed Mar 18, 2019

View reviewed changes

tools/tm-signer-harness/internal/test_harness_test.go Outdated Show resolved Hide resolved

jleni marked this pull request as ready for review March 19, 2019 11:31

jleni requested review from ebuchman and melekes as code owners March 19, 2019 11:31

jleni mentioned this pull request Mar 20, 2019

privval: allow full node startup without a connection for improved failover #3455

Closed

xla changed the title ~~WIP: Refactoring Remote signers~~ [WIP] Refactoring Remote signers Apr 2, 2019

melekes mentioned this pull request Apr 2, 2019

privval: Consumes out of order message if previous sign request timed out #3486

Closed

melekes reviewed Apr 2, 2019

View reviewed changes

jleni added 12 commits April 3, 2019 14:15

refactoring - work in progress

c879c6e

reworking unit tests

0750113

Encapsulating and fixing unit tests

d9b3001

Improve tests

91e00b3

Clean up

588e7f3

Fix/improve unit tests

c734898

clean up tests

ad7eb8f

Improving service endpoint

a66063f

fixing unit test

33b8c63

fix linter issues

bce20db

avoid invalid cache values (improve later?)

fb31eef

complete implementation

defade8

melekes reviewed Jul 26, 2019

View reviewed changes

Addressing review comments

10510de

golangcibot reviewed Jul 26, 2019

View reviewed changes

privval/signer_listener_endpoint.go Show resolved Hide resolved

simplifying connection workflow

62b99b0

golangcibot reviewed Jul 26, 2019

View reviewed changes

privval/socket_dialers_test.go Outdated Show resolved Hide resolved

jleni added 4 commits July 27, 2019 00:16

fix linter/go import issue

df6d867

using base service quit

5318872

updating comment

b78054e

Simplifying design + adjusting names

f40ab3d

golangcibot reviewed Jul 27, 2019

View reviewed changes

privval/errors.go Outdated Show resolved Hide resolved

privval/signer_dialer_endpoint.go Outdated Show resolved Hide resolved

privval/signer_server.go Outdated Show resolved Hide resolved

cmd/priv_val_server/main.go Outdated Show resolved Hide resolved

fixing linter issues

aae6553

golangcibot reviewed Jul 27, 2019

View reviewed changes

privval/signer_listener_endpoint_test.go Outdated Show resolved Hide resolved

refactoring test harness + fixes

7e7e214

melekes reviewed Jul 28, 2019

View reviewed changes

Addressing review comments

622c149

golangcibot reviewed Jul 29, 2019

View reviewed changes

privval/signer_dialer_endpoint.go Outdated Show resolved Hide resolved

jleni added 2 commits July 29, 2019 08:50

cleaning up

e596e50

adding additional error check

d4530a4

melekes reviewed Jul 29, 2019

View reviewed changes

melekes approved these changes Jul 29, 2019

View reviewed changes

tac0turtle added ready-for-review and removed WIP T:breaking Type: Breaking Change labels Jul 29, 2019

Merge branch 'master' into jleni/privval

0b3ea79

melekes reviewed Aug 5, 2019

View reviewed changes

melekes merged commit f7f034a into tendermint:master Aug 5, 2019

ebuchman mentioned this pull request Aug 23, 2019

Tendermint disconnects tmkms on signing error #3844

Closed

liamsi mentioned this pull request Mar 16, 2021

Fix nondeterministic test failures celestiaorg/celestia-core#216

Closed

aeddi mentioned this pull request Jan 20, 2025

feat: init gnokms tool with gnokey backend gnolang/gno#3554

Merged

5 tasks


		### IMPROVEMENTS:

		- [privval] \#3370 Refactors and simplifies validator/kms connection handling. Please refer to thttps://github.com/tendermint/tendermint/pull/3370#issue-257360971

Conversation

jleni commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jleni commented Mar 19, 2019

Uh oh!

jleni commented Apr 1, 2019

Uh oh!

jleni commented Apr 1, 2019

Uh oh!

liamsi commented Apr 2, 2019

Uh oh!

liamsi commented Apr 2, 2019

Uh oh!

jleni commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamsi commented Apr 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

jleni Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

melekes Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

melekes commented Aug 5, 2019

Uh oh!

melekes Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

jleni commented Mar 1, 2019 •

edited

Loading

codecov-io commented Mar 19, 2019 •

edited

Loading

jleni commented Apr 2, 2019 •

edited

Loading