types/priv_validator package by ebuchman · Pull Request #1203 · tendermint/tendermint

ebuchman · 2018-02-09T22:01:40Z

Replaces #1044
closes #673

Note this is just the new code, none of the integration (old priv val is still there).

I didn't feel too good about the integration last time, and now its riddled with conflicts, so maybe we can figure out a simple way to make it work starting a fresh. At least with this we can merge it without affecting anything.

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

melekes · 2018-02-10T07:09:33Z

types/priv_validator/json.go

+// or else generates a new one and saves it to the filePath.
+func LoadOrGenPrivValidatorJSON(filePath string) *PrivValidatorJSON {
+	var pvj *PrivValidatorJSON
+	if _, err := os.Stat(filePath); err == nil {


cmn.FileExists(filePath) ?

melekes · 2018-02-10T07:29:30Z

# github.com/tendermint/tendermint/types/priv_validator
/tmp/go-build532587797/github.com/tendermint/tendermint/types/priv_validator/_test/_obj_test/unencrypted.go:16:6: undefined: "github.com/tendermint/tendermint/types".ValidatorID

* 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

Feature/priv val sockets

ebuchman · 2018-02-19T21:07:25Z

Merged in the new priv validator socket work, but please see review from that PR: #1204 (review)

codecov-io · 2018-02-19T21:40:19Z

Codecov Report

Merging #1203 into develop will decrease coverage by 0.14%.
The diff coverage is 59.66%.

@@             Coverage Diff             @@
##           develop    #1203      +/-   ##
===========================================
- Coverage    59.66%   59.52%   -0.15%     
===========================================
  Files          121      126       +5     
  Lines        11050    11527     +477     
===========================================
+ Hits          6593     6861     +268     
- Misses        3851     4012     +161     
- Partials       606      654      +48

Following ADDR 008 the node will connect to an external process to handle signing requests. Operation of the external process is left to the user. * introduce alias for PrivValidator interface on socket client * integrate socket client in node * structure tests * remove unnecessary flag

privVal: Integrate socket client

xla · 2018-02-28T08:26:43Z

@ebuchman What's the next step here? Should we start with the actual integration by getting rid of the old priv val, flesh out cmd/priv_val_server according to ADR 008, or prepare to merge as is?

Happy to follow-up and tackle any of these.

melekes · 2018-02-28T07:52:50Z

cmd/priv_val_server/main.go

+	var (
+		chainID     = flag.String("chain-id", "mychain", "chain id")
+		listenAddr  = flag.String("laddr", ":46659", "Validator listen address (0.0.0.0:0 means any interface, any port")
+		maxConn     = flag.Int("clients", 3, "maximum of concurrent connections")


why do we want to limit that?

Originally it only accepted a single connection at a time, we had a discussion around it and decided to go with 3 for now. Usually the relation should be one-to-one, between tendermint process and external signer, as far as I know at least.

Got it. Thanks

melekes · 2018-02-28T07:58:32Z

node/node.go

+			)
+		)
+
+		if err := pvsc.Start(); err != nil {


Can't find where we stop this ...

If we defer pvsc.Stop() here it would be killed as soon as we exit the function scope. If the client is used it's vital to the node and is kind of bound to the lifecycle of the node process itself, maybe I missed a place where we have teardown when the node shuts down, that would be the place to stop it.

https://github.com/tendermint/tendermint/blob/master/node/node.go#L406

melekes · 2018-02-28T08:02:54Z

types/priv_validator/json.go

+
+func (pvj *PrivValidatorJSON) save() {
+	if pvj.filePath == "" {
+		cmn.PanicSanity("Cannot save PrivValidator: filePath not set")


Panic* methods are deprecated. Use panic instead.

melekes · 2018-02-28T08:05:43Z

types/priv_validator/sign_info.go

+	Round     int              `json:"round"`
+	Step      int8             `json:"step"`
+	Signature crypto.Signature `json:"signature,omitempty"` // so we dont lose signatures
+	SignBytes cmn.HexBytes     `json:"signbytes,omitempty"` // so we dont lose signatures


sign_bytes?

This would be inconsistent to other occurences of signbytes:

tendermint/types/priv_validator.go

Line 58 in c8a2bdf

LastSignBytes cmn.HexBytes `json:"last_signbytes,omitempty"` // so we dont lose signatures

melekes · 2018-02-28T08:06:20Z

docs/architecture/adr-008-priv-validator.md

+	Round     int              `json:"round"`
+	Step      int8             `json:"step"`
+	Signature crypto.Signature `json:"signature,omitempty"` // so we dont lose signatures
+	SignBytes data.Bytes       `json:"signbytes,omitempty"` // so we dont lose signatures


sign_bytes?

melekes · 2018-02-28T08:45:29Z

types/priv_validator/socket.go

+				return // Ignore error from listener closing.
+			}
+			pvss.Logger.Error(
+				"accpetConnections",


ebuchman · 2018-02-28T14:26:59Z

Merging for the sake of progress, will open new issue for remaining points. Thanks @xla !

ebuchman · 2018-02-28T14:35:44Z

See issue #1255 and new PR #1256 for follow-up

…not match locked block (tendermint#1203) * consensus: update state to prevote nil when proposal block does not match locked block. (tendermint#6986) * add failing test * tweak comments in failing test * failing test comment * initial attempt at removing prevote locked block logic * comment out broken function * undo reset on prevotes * fixing TestProposeValidBlock test * update test for completed POL update * comment updates * further unlock testing * update comments * Update internal/consensus/state.go * spacing nit * comment cleanup * nil check in addVote * update unlock description * update precommit on relock comment * add ensure new timeout back * rename IsZero to IsNil and replace uses of block len check with helper * add testing.T to new assertions * begin removing unlock condition * fix TestStateProposerSelection2 to precommit for nil correctly * remove erroneous sleep * update TestStatePOL comment * update relock test to be more clear * add _ into test names * rename slashing * udpate no relock function to be cleaner * do not relock on old proposal test cleanup * con state name update * remove all references to unlock * update test comments to include new * add relock test * add ensureRelock to common_test * remove all event unlock * remove unlock checks * no lint add space * lint ++ * add test for nil prevote on different proposal * fix prevote nil condition * fix defaultDoPrevote * state_test.go fixes to accomodate prevoting for nil * add failing test for POL from previous round case * update prevote logic to prevote POL from previous round * state.go comment fixes * update validatePrevotes to correctly look for nil * update new test name and comment * update POLFromPreviousRound test * fixes post merge * fix spacing * make the linter happy * change prevote log message * update prevote nil debug line * update enterPrevote comment * lint * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * add english description of alg rules * Update internal/consensus/state.go Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * comment fixes from review * fix comment * fix comment Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Fix UTs * Addressed comments * Add changelog * Update consensus/state.go Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> --------- Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

ebuchman and others added 13 commits February 9, 2018 16:38

types/priv_validator

459633f

adr-008-priv-validator

bef91ea

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:01

ebuchman changed the title ~~Feature/priv val~~ types/priv_validator package Feb 9, 2018

This was referenced Feb 9, 2018

WIP: Priv Validator Refactor #1044

Closed

Feature/priv val sockets #1204

Merged

types/priv_validator: fixes for latest p2p and cmn

8da2a6a

melekes approved these changes Feb 10, 2018

View reviewed changes

xla added 6 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

Integrate PrivValidator socket server

a14aab6

zramsay mentioned this pull request Feb 19, 2018

PrivValidator Structure #673

Closed

Merge pull request #1204 from tendermint/feature/priv_val_sockets

ffd2483

Feature/priv val sockets

xla mentioned this pull request Feb 22, 2018

privVal: Integrate socket client #1237

Merged

xla and others added 2 commits February 23, 2018 13:58

Merge pull request #1237 from tendermint/feature/priv_val_socket_client

edb871f

privVal: Integrate socket client

melekes reviewed Feb 28, 2018

View reviewed changes

ebuchman merged commit dd2d846 into develop Feb 28, 2018

ebuchman deleted the feature/priv_val branch February 28, 2018 14:27

ebuchman mentioned this pull request Feb 28, 2018

PrivValidator follow up #1255

Closed

7 tasks

xla mentioned this pull request Mar 6, 2018

Follow-ups to PrivValidator #1284

Merged

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Feb 10, 2018

Uh oh!

ebuchman commented Feb 19, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

xla commented Feb 28, 2018

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!

xla Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 commented Feb 28, 2018

Uh oh!

ebuchman commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ebuchman commented Feb 9, 2018 •

edited

Loading

codecov-io commented Feb 19, 2018 •

edited

Loading

xla Mar 6, 2018 •

edited

Loading