Feature/priv val sockets by ebuchman · Pull Request #1204 · tendermint/tendermint

ebuchman · 2018-02-09T22:05:43Z

Note the integrations for this in node/node.go are commented out (until types/priv_validator becomes active).

* add comments to all public types * fix comments to adhere to comment standards

ebuchman · 2018-02-09T22:09:06Z

When working with the protocol between client and server I was dearly missing some form of communicating errors, but I don't know how settle we are on the current protocol.

We can modify the protocol here to support error handling. Want to propose something?

ebuchman · 2018-02-09T22:28:57Z

Also, looks like Tendermint is responsible for dialing the signer process here. I thought we wanted the signer to dial Tendermint (#1060 (comment)). Should we make the SocketServer implement PrivKey and give Tendermint that or is there some better reason this should stay as is?

codecov-io · 2018-02-09T22:56:15Z

Codecov Report

❗ No coverage uploaded for pull request base (feature/priv_val@bef91ea). Click here to learn what that means.
The diff coverage is 49.52%.

@@                 Coverage Diff                 @@
##             feature/priv_val    #1204   +/-   ##
===================================================
  Coverage                    ?   59.47%           
===================================================
  Files                       ?      126           
  Lines                       ?    11517           
  Branches                    ?        0           
===================================================
  Hits                        ?     6850           
  Misses                      ?     4019           
  Partials                    ?      648

melekes · 2018-02-10T07:37:16Z

cmd/priv_val_server/main.go

+func main() {
+	var (
+		chainID     = flag.String("chain-id", "mychain", "chain id")
+		numClients  = flag.Int("clients", 1, "number of concurrently connected clients")


how am I supposed to know the number of clients beforehand?

Or this is a maximum number of concurrent clients?

Yes, will make the description more clear.

melekes · 2018-02-10T07:43:46Z

cmd/priv_val_server/main.go

+	)
+	flag.Parse()
+
+	logger.Info("Reading args privValidatorSocketServer", "chainID", *chainID, "privPath", *privValPath)


Maybe change to fmt.Sprintf("Starting private validator socket server on %s", listenAddr), "chainID", *chainID, "privValPath", *privValPath, ?

melekes · 2018-02-10T07:44:52Z

cmd/priv_val_server/main.go

+		chainID     = flag.String("chain-id", "mychain", "chain id")
+		numClients  = flag.Int("clients", 1, "number of concurrently connected clients")
+		privValPath = flag.String("priv", "", "priv val file path")
+		socketAddr  = flag.String("socket.addr", ":46659", "socket bind addr")


Let's keep a single standard (listenAddr = flag.String("laddr")

cmd.Flags().String("p2p.laddr", config.P2P.ListenAddress, "Node listen address. (0.0.0.0:0 means any interface, any port)")

melekes · 2018-02-10T07:46:37Z

cmd/tendermint/commands/run_node.go

 	cmd.Flags().String("moniker", config.Moniker, "Node Name")

+	// priv val flags
+	cmd.Flags().String("priv_validator_addr", config.PrivValidatorAddr, "Socket address for PrivValidator")


private validator, not PrivValidator

melekes · 2018-02-10T07:53:11Z

types/priv_validator/socket.go


-	var err error
-	var conn net.Conn
+	var (


can we extract this into a separate connect function?

melekes · 2018-02-10T07:58:18Z

types/priv_validator/socket.go

 }

+// PubKey implements PrivValidator.
 func (pvsc *PrivValidatorSocketClient) PubKey() crypto.PubKey {


Should not PubKey return error too by analogy with SignProposal?

Thought so too and addressed it in 2a292ef, but would like to have a sanity check from @ebuchman on this. :>

melekes · 2018-02-10T07:59:45Z

types/priv_validator/socket.go

 			}
-			pvss.Logger.Error("Failed to accept connection: " + err.Error())
+			pvss.Logger.Error(
+				"accpetConnections",


"Failed to accept new connection"

While I see the point given the signature of Error(msg string, keyvals ...interface{}) it seems very hard to build up the context the error was coming from as it would result in:

E[02-13|17:42:57.620] Failed to accept new connection err=foo

Maybe I'm not seeing an important aspect of error logging in our codeabse, but that line has no signal in it. Happy to incorporate your comments, just wondering if it is the best way to go abouut it.

You can turn on a tracing logger by providing --trace flag https://github.com/tendermint/tendermint/blob/master/cmd/tendermint/commands/root.go#L53

In this mode logger will output full trace if error supports this feature (pkg/errors does). https://github.com/tendermint/tmlibs/blob/master/log/tracing_logger.go#L58

Thanks for pointing that out! This still doesn't really help after the fact, which makes errors emitted without tracing still not very useful.

Ok. I see your point. So the agreement always was: meaningful-message key=value. For example: "Failed to accept connection err=EOF". We're still in a process of improving the messages. Plus we started to use pkg/errors not so long ago to accumulate context.

Usually, what we do is just grep by msg ("Failed to accept connection"). Of course, if you have many places in your app where you accept a connection, then msg must be changed to something like "Failed to accept connection from X".

I am happy to hear any suggestions on how to improve the current scheme.

Very sound approach and I don't mean to start a greater discussion if we already iterating on error handling in other places.

I am happy to hear any suggestions on how to improve the current scheme.

Usually thinking about in what remote ends logs can end up in typical infrastructure I try to imagine what vital info is needed to make sense of it when consumed in some log infrastructure remote from the origin. Additionally how each log line can interpreted by itself without context of the lines before or after. As the logger is passed from the outside to the inside in the program we can enrich the keyvals with information as well which subsystems don't need to care about. A standard set of fields I try to surface, most of them which can easily be achieved with Dynamic Contextual Values from the go-kit log package:

caller: https://godoc.org/github.com/go-kit/kit/log#Caller

timestamp: https://godoc.org/github.com/go-kit/kit/log#pkg-variables

revision: compile flag: https://github.com/tendermint/tendermint/blob/master/Makefile#L10

job/system: tendermint in our case

cmd/task: nodefor the node command

service/subsystem: what we pass to cmn.NewBaseService(...) e.g.: privValidatorSocketServer

err: set if error present

melekes · 2018-02-10T08:00:30Z

types/priv_validator/socket.go

-			if !pvss.IsRunning() {
-				return // Ignore error from listener closing.
-			}
+		if err := conn.SetDeadline(time.Now().Add(time.Second)); err != nil {


deadline should be configurable or be a const, no magic numbers please :)

melekes · 2018-02-10T08:01:15Z

types/priv_validator/socket.go

-			}
+		if err := conn.SetDeadline(time.Now().Add(time.Second)); err != nil {
+			pvss.Logger.Error(
+				"acceptConnetions",


melekes · 2018-02-10T08:02:13Z

types/priv_validator/socket.go

+		req, err := readMsg(conn)
+		if err != nil {
+			if err != io.EOF {
+				pvss.Logger.Error("handleConnection", "err", err)


* break out connect functionality out of OnStart * introduce max retries

As calls to the private validator can involve side-effects like network communication it is desirable for all methods returning an error to not break the control flow of the caller. * adjust PrivValidator interface

zmanian · 2018-02-14T08:28:42Z

node/node.go


+	/* TODO
+	// Generate node PrivKey
+	privKey := crypto.GenPrivKeyEd25519()


I've been debating if this should be ephemeral.

In the ephemeral version, I imagine perhaps an ssh like confirm dialog in signatory before signatory starts accepting signing requests for a node.

if this passed in via a config file, then signatory would maintain a white list

ebuchman

I'll merge this into the other priv val branch, but we still need to fix the client vs server dichotomy (if the SocketServer is part of the Tendermint process, it shouldn't have direct access to a PrivValidator key)

ebuchman · 2018-02-19T20:43:10Z

config/config.go

 	Moniker string `mapstructure:"moniker"`

+	// TCP or UNIX socket address of the PrivValidator server
+	PrivValidatorAddr string `mapstructure:"priv_validator_addr"`


Maybe we'll open an issue to break this out into its own PrivValidatorConfig

ebuchman · 2018-02-19T20:48:22Z

types/priv_validator/socket_test.go

+	require.Nil(err)
+
+	err = pvsc.SignHeartbeat(chainID, &types.Heartbeat{})
+	require.Nil(err)


maybe we can verify the signatures too

ebuchman · 2018-02-19T20:49:18Z

types/priv_validator/socket_test.go

+	require.Nil(err)
+}
+
+func TestPrivValidatorSocketServerWithoutSecret(t *testing.T) {


maybe we can de-duplicate most of this

ebuchman · 2018-02-19T20:52:46Z

types/priv_validator/socket.go

+
+		retries--
+
+		conn, err := cmn.Connect(pvsc.SocketAddress)


do we have to cleanup this conn if we error and retry?

If it errors we don't need to worry about any cleanup, we do need to defer the closing of it if it succeeds.

If this one errors, sure nothing to clean, but what about any of the points below where we continue. Eg. if the SecretConnection fails, I think we need to close the conn

Absolutely!

ebuchman · 2018-02-19T20:59:53Z

cmd/priv_val_server/main.go

+
+	privVal := priv_val.LoadPrivValidatorJSON(*privValPath)
+
+	pvss := priv_val.NewPrivValidatorSocketServer(


So long as "client" means "one who initiates connection by dialing", I think we need the client here.

ebuchman · 2018-02-19T21:04:06Z

types/priv_validator/socket.go

+	logger log.Logger,
+	chainID, socketAddr string,
+	maxConnections int,
+	privVal PrivValidator,


if SocketServer is what runs in the Tendermint process (ie. it's listening on the socket for a connection from a priv_val_client, but it's also the one that initiates all requests), then it won't have access to a PrivValidator because it doesn't have a key for it - it only gets access to the key by sending messages over the socket

ebuchman and others added 11 commits February 9, 2018 16:52

wip: priv val via sockets

23eb84d

wip: Comment types

c27fda0

* add comments to all public types * fix comments to adhere to comment standards

wip: fix code block in ADR

d321839

wip: fix nil pointer deference

4b997c2

wip: Avoid underscore in var name

a49357b

wip: check error of wire read

ff600e9

Correct server protocol

fec5413

Use secret connection

18f7e52

Format and consolidate

32d9563

Harden tests

38d18ca

Integrate privVal client with node secret

7d71e70

ebuchman requested a review from melekes as a code owner February 9, 2018 22:05

ebuchman mentioned this pull request Feb 9, 2018

Priv val via sockets #1060

Closed

types/priv_validator: fixes for latest p2p and cmn

8da2a6a

melekes reviewed Feb 10, 2018

View reviewed changes

xla self-assigned this Feb 12, 2018

xla added 5 commits February 13, 2018 19:08

Separate connect logic

82b1a34

* break out connect functionality out of OnStart * introduce max retries

Return error for all PrivValidator methoods

2a292ef

As calls to the private validator can involve side-effects like network communication it is desirable for all methods returning an error to not break the control flow of the caller. * adjust PrivValidator interface

Apply connection deadline consistently

6c70b4c

Clean up flags

a102030

Correct config description

106d804

zmanian reviewed Feb 14, 2018

View reviewed changes

xla mentioned this pull request Feb 14, 2018

Improve logging context information #1212

Closed

Integrate PrivValidator socket server

a14aab6

ebuchman commented Feb 19, 2018

View reviewed changes

ebuchman merged commit ffd2483 into feature/priv_val Feb 19, 2018

ebuchman deleted the feature/priv_val_sockets branch February 19, 2018 21:06

ebuchman mentioned this pull request Feb 19, 2018

types/priv_validator package #1203

Merged

xla mentioned this pull request Feb 22, 2018

privVal: Integrate socket client #1237

Merged


		privVal := priv_val.LoadPrivValidatorJSON(*privValPath)

		pvss := priv_val.NewPrivValidatorSocketServer(

Conversation

ebuchman commented Feb 9, 2018

Uh oh!

ebuchman commented Feb 9, 2018

Uh oh!

ebuchman commented Feb 9, 2018

Uh oh!

codecov-io commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov-io commented Feb 9, 2018 •

edited

Loading