WIP: Priv Validator Refactor by ebuchman · Pull Request #1044 · tendermint/tendermint

ebuchman · 2017-12-31T02:59:24Z

Hard to see the changes because I busted up the file into its own package, but it should be much easier to read through now.

@silasdavis @wrl @ethanfrey @adrianbrink @melekes

adrianbrink · 2017-12-31T11:47:26Z

This cleans it up a lot. :-)

What is the purpose of PrivValidatorStore? Also, shouldn't CarefulSigner be rather a struct that implements PrivValidator?

Couldn't we remove the Signer interface and just let the client implement PrivValidator. This would mean that we'd have a NanoPrivValidator struct.

I am wondering though how it would work with moving the signing logic into a Thales HSM.

adrianbrink · 2017-12-31T11:53:56Z

I'm wary though how people would use different signers. Ideally wanting to use a NanoLedger or Thales shouldn't require you to fork Tendermint Core. Either we have a finite list of supported signers hardcoded in Core and then try to figure out which one to use through a config option or Core expects a signer to be available behind some socket so that way in the config you can specify a socket address and Tendermint Core will just send the bytes to sign that way.

silasdavis · 2017-12-31T14:52:26Z

I'm wary though how people would use different signers.
@adrianbrink what are you wary about in particular? Note that we can pass in a PrivValidator so we wouldn't need fork Tendermint core to pass in an alternative signer. Nothing to stop you providing configurable default signers, but the flexibility to provide a signer is useful.

I'm not quite following the full idea with CarefulSigner, but I don't see any implementation of GetCarefulSigner so assume that is coming.

ebuchman · 2017-12-31T16:21:44Z

What is the purpose of PrivValidatorStore

A place to store the actual priv key, so eg. it can be encrypted on disk. This might go away though.

Also, shouldn't CarefulSigner be rather a struct that implements PrivValidator
I'm not quite following the full idea with CarefulSigner

CarefulSigner is the thing that prevents double signing. In this issue, Silas mentioned a Verifier interface - that's what this is. It decouples signing from the verification. For instance, you could use a ledger for just the signing, but still use our LastSignedInfo to prevent double signing. Or you could use the ledger to implement both.

Couldn't we remove the Signer interface and just let the client implement PrivValidator

Possibly, but at least for now that would spoil CarefulSigner

I am wondering though how it would work with moving the signing logic into a Thales HSM.

The HSM would implement Signer and CarefulSigner

Either we have a finite list of supported signers hardcoded in Core and then try to figure out which one to use through a config option or Core expects a signer to be available behind some socket so that way in the config you can specify a socket address and Tendermint Core will just send the bytes to sign that way.

Both of these sounds pretty good. Let's start with a list of supported things (unencrypted, encrypted, ledger, thales) and then open it up to a socket.

melekes · 2017-12-31T15:43:25Z

state/execution.go

+		power := v.Power
 		if power < 0 {
-			return fmt.Errorf("Power (%d) overflows int64", v.Power)
+			return fmt.Errorf("Power (%d) must be positive int64", v.Power)


v.Power -> power

melekes · 2017-12-31T15:46:56Z

types/priv_validator/priv_validator.go

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


cmn.FileExists

melekes · 2017-12-31T15:49:10Z

types/priv_validator/priv_validator_test.go

-	privVal.Save()
-	addr := privVal.GetAddress()
+	privVal.CarefulSigner.(*LastSignedInfo).LastHeight = height
+	privVal.CarefulSigner.(*LastSignedInfo).saveFn(privVal.CarefulSigner)


this is harder to read&understand comparing to privVal.Save()

melekes · 2017-12-31T16:26:01Z

types/priv_validator/signer.go

+
+//-----------------------------------------------------------------
+
+// DefaultSigner implements Signer.


how about a check: var _ Signer = (*DefaultSigner)(nil)

melekes · 2017-12-31T16:28:39Z

types/priv_validator/types.go

+		return stepPrecommit
+	default:
+		cmn.PanicSanity("Unknown vote type")
+		return 0


unreachable code

melekes · 2017-12-31T16:36:54Z

types/validator.go

 	return val, privVal
 }
+
+type Signer interface {


2 Signer interfaces. Wont this be confusing?

yes this one is just for testing tho. problem of circular imports ...

I'll change it to TestSigner

adrianbrink · 2017-12-31T18:21:03Z

How about calling it Verifier instead of CarefulSigner. I have a commit with these changes but you are committing faster than me :-) @ebuchman

ebuchman · 2017-12-31T19:57:45Z

It's not just a Verifier though - at the least its a VerifyAndSigner. And it's also something that's expected to have mutable internal state if its going to keep track of things to not double sign. So it seemed CarefulSigner was a good fit for a name.

The CarefulSigner interface isn't actually being used in any useful way right now, but I think it's useful to have, especially for cases where the LastSignedInfo is on disk but the Signer is an HSM.

The goal is for the DefaultPrivValidator to just use Signer and CarefulSigner interfaces directly, but we need to be able to unmarshall into those, so I'm waiting on changes to go-wire to make that work nicely. In the meantime, it should be a very small amount of code to add a new priv validator - the only thing is you have to handle your own marshal/unmarshal for saving/loading from disk. Once the new wire changes are in, everyone can just use the DefaultPrivValidator, and provide the Signer and CarefulSigner they like :)

codecov-io · 2017-12-31T20:17:42Z

Codecov Report

Merging #1044 into develop will decrease coverage by 0.13%.
The diff coverage is 59.04%.

@@             Coverage Diff             @@
##           develop    #1044      +/-   ##
===========================================
- Coverage    59.66%   59.52%   -0.14%     
===========================================
  Files          116      119       +3     
  Lines        10667    10772     +105     
===========================================
+ Hits          6364     6412      +48     
- Misses        3725     3779      +54     
- Partials       578      581       +3

ebuchman · 2018-01-05T00:52:05Z

Ok - I went through this again and simplified.

Please take a look at the ADR and the new simplified code.

Dropped the CarefulSigner and just put those methods on the LastSignedInfo.

Shouldn't be any lost functionality really.

Tendermint will jsut support PrivValidatorUnencrypted and PrivValidatorSocket.

Folks will still be able to override in-process if they want with their own PrivValidator, and should be able to easily use the LastSignedInfo.

melekes · 2018-01-08T14:01:00Z

state/execution.go

+		power := v.Power
 		if power < 0 {
-			return fmt.Errorf("Power (%d) overflows int64", v.Power)
+			return fmt.Errorf("Power (%d) must be positive int64", power)


let's add address here too: "Power (%d) of validator %X must be positive int64", power, address

melekes · 2018-01-08T14:02:20Z

types/evidence_test.go

 	vote1 := makeVote(val, chainID, 0, 10, 2, 1, blockID)
 	badVote := makeVote(val, chainID, 0, 10, 2, 1, blockID)
-	badVote.Signature = val2.PrivKey.Sign(SignBytes(chainID, badVote))
+	badVote.Signature, _ = val2.Sign(SignBytes(chainID, badVote))


it's better not ignore errors. why not do require.NoError(t, err)

melekes · 2018-01-08T15:05:33Z

types/priv_validator/sign_info.go

+	"github.com/tendermint/tendermint/types"
+)
+
+// TODO: type ?


step is exposed in LastSignedInfo, so don't think this is a good idea because of that

melekes · 2018-01-08T15:05:41Z

types/priv_validator/sign_info.go

+
+// TODO: type ?
+const (
+	stepNone      int8 = 0 // Used to distinguish the initial state


melekes · 2018-01-08T15:10:54Z

types/priv_validator/sign_info.go

+func (info *LastSignedInfo) Reset() {
+	info.Height = 0
+	info.Round = 0
+	info.Step = 0


ebuchman · 2018-02-09T22:04:03Z

Closing in favor of fresh new PR without the integrations #1203

ebuchman added 3 commits December 30, 2017 19:56

dont add validators with 0 power

54117bd

wip: priv_val refactor

a84c9df

types/priv_validator

39bf68a

ebuchman requested a review from melekes as a code owner December 31, 2017 02:59

builds

8cfb032

some cleanup - works with running node

6c84f96

melekes reviewed Dec 31, 2017

View reviewed changes

ebuchman added 2 commits December 31, 2017 12:37

fix tests

957bb6b

drop Last prefix. fix p2p tests

d755579

metalinter fix

ba996df

ebuchman added 2 commits January 4, 2018 14:28

fix timestamp for signing things that only differ by timestamp

6e72fcb

priv_val: drop CarefulSigner and simplify

4e88c8b

ebuchman mentioned this pull request Jan 5, 2018

Priv val via sockets #1060

Closed

melekes reviewed Jan 8, 2018

View reviewed changes

This was referenced Jan 21, 2018

Detached Signing Validator Design #819

Closed

Active/active validator setup #870

Closed

ebuchman mentioned this pull request Feb 9, 2018

types/priv_validator package #1203

Merged

ebuchman closed this Feb 9, 2018

xla deleted the priv_val branch September 18, 2018 20:28


		//-----------------------------------------------------------------

		// DefaultSigner implements Signer.

Conversation

ebuchman commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrianbrink commented Dec 31, 2017

Uh oh!

adrianbrink commented Dec 31, 2017

Uh oh!

silasdavis commented Dec 31, 2017

Uh oh!

ebuchman commented Dec 31, 2017

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!

adrianbrink commented Dec 31, 2017

Uh oh!

ebuchman commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ebuchman commented Jan 5, 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!

ebuchman commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ebuchman commented Dec 31, 2017 •

edited

Loading

ebuchman commented Dec 31, 2017 •

edited

Loading

codecov-io commented Dec 31, 2017 •

edited

Loading