Add ValidatorPubkeyTypes as a consensus param by ValarDragon · Pull Request #2636 · tendermint/tendermint

ValarDragon · 2018-10-14T05:00:47Z

Updated all relevant documentation in docs - updated all I'm aware of.
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

codecov-io · 2018-10-14T05:06:38Z

Codecov Report

Merging #2636 into develop will decrease coverage by 0.18%.
The diff coverage is 60%.

@@             Coverage Diff             @@
##           develop    #2636      +/-   ##
===========================================
- Coverage    61.72%   61.54%   -0.19%     
===========================================
  Files          207      207              
  Lines        16942    16939       -3     
===========================================
- Hits         10458    10425      -33     
- Misses        5622     5649      +27     
- Partials       862      865       +3

Impacted Files	Coverage Δ
evidence/pool.go	`56.14% <0%> (ø)`	⬆️
state/store.go	`70.07% <0%> (ø)`	⬆️
evidence/reactor.go	`65.93% <100%> (ø)`	⬆️
state/execution.go	`75.67% <100%> (ø)`	⬆️
state/validation.go	`65.62% <100%> (ø)`	⬆️
consensus/state.go	`76.17% <0%> (-2.82%)`	⬇️
consensus/reactor.go	`70.96% <0%> (-1.04%)`	⬇️
tools/tm-monitor/monitor/network.go	`88.5% <0%> (-0.75%)`	⬇️
node/node.go	`65.24% <0%> (-0.31%)`	⬇️
p2p/node_info.go	`89.23% <0%> (ø)`	⬆️
... and 2 more

ValarDragon · 2018-10-14T05:08:51Z

Should ValidatorPubkeyTypes instead be PubkeyTypes as a sub-component of ValidatorParams?

melekes

🍰 🌮 🍉

melekes · 2018-10-14T17:58:55Z

types/params.go

 	}

+	if len(params.ValidatorPubkeyTypes) == 0 {
+		return cmn.NewError("len(ValidatorPubkeyTypes) must be greater than 0.")


no . at the end please

ebuchman

So this is just to add it to the ConsensusParams. Enforcement will come in a second PR? Or do you want to add that here.

ebuchman · 2018-10-16T15:54:26Z

types/params.go

 type ConsensusParams struct {
-	BlockSize      `json:"block_size_params"`
-	EvidenceParams `json:"evidence_params"`
+	BlockSize            `json:"block_size_params"`


Thinking about this, we can probably drop the Params and _params suffixes from these fields.

Also I wonder if the ConsensusParams is a good place for Version to live? Then we don't need to add anything to EndBlock for the app to update the versions, its just part of the ConsensusParams.

As for this PR, maybe we should have a new struct to hold the pubkey types? Could be called Signatures or Cryptography or Validators or something ?

This makes a lot of sense for app version imo.

Re new struct, sure I think Validators makes sense! I do think having the params suffix is nice for disambiguation when your looking at types.

Cool. Let's go with Validators.

Yeh, Params makes sense for the types, but it's redundant for the field names. I guess here it's all embedded structs, but maybe it shouldn't be, and then we can have proper field names and struct type name distinctions.

ebuchman · 2018-10-16T15:55:11Z

types/params.go

 }

+// DefaultValidatorPubkeyTypes Params returns a default set of pubkey types to
+// be accepted for validators.


Comment should tell us what the default is

ebuchman · 2018-10-16T15:56:39Z

types/params.go

 	if params2.EvidenceParams != nil {
 		res.EvidenceParams.MaxAge = params2.EvidenceParams.MaxAge
 	}
+	if params2.ValidatorPubkeyTypes != nil {


Works for structs but not sure if we might just end up with a non-nil empty list here?

ValarDragon · 2018-10-16T16:00:14Z

My plan was to add enforcement in a second PR. (Primarily because I was unsure what function should be aware of the consensus params when getting the validator set updates)

…aram

ValarDragon · 2018-10-23T19:21:36Z

Is this good to merge? I'd like to get it in soon before we make another .proto change and I have to fix the merge conflict

ebuchman

I'm not sure if we want to use the Amino routes here. Technically ABCI knows nothing about amino routes, and the pubkey types just have two string types now, "ed25519" and "secp256k1". We should be consistent here - either we add the amino routes to the base pubkey types, or we use the simple names in the validator pubkey types...

Otherwise, this looks great. Thanks Dev.

ebuchman · 2018-10-25T02:29:01Z

types/params.go

 			params.EvidenceParams.MaxAge)
 	}

+	if len(params.Validator.ValidatorPubkeyTypes) == 0 {


Should we also validate that the types given are known?

ebuchman · 2018-10-25T02:30:16Z

types/params.go

 }

+func (params *ConsensusParams) Equals(params2 *ConsensusParams) bool {
+	if params.BlockSize == params2.BlockSize &&


We can just return this conjunction instead of using the conditional

ValarDragon · 2018-10-25T05:23:04Z

I'm not sure if we want to use the Amino routes here. Technically ABCI knows nothing about amino routes, and the pubkey types just have two string types now,

I'll change it here, but I don't really get why ABCI wouldn't just use the amino routes / why we don't make the amino routes match whats used in ABCI. (I actually think the amino routes should be whats currently used in ABCI. The tendermint prefix in amino routes will unnecessarily break compatibility with other potential consensus algorithms or make other consensus algorithms crypto more confusing)
I extracted this to a separate issue.

ebuchman · 2018-10-30T14:32:05Z

The redundant field names components (#2698) were driving me nuts so I fixed it :P

melekes

🍐

melekes · 2018-10-30T15:19:03Z

types/protobuf.go

 	ABCIPubKeyTypeSecp256k1 = "secp256k1"
 )

+// TODO: Make non-global by allowing for registration of more pubkey types


is there an associated issue?

melekes · 2018-10-30T15:20:12Z

types/params.go

+		stringSliceEqual(params.Validator.PubKeyTypes, params2.Validator.PubKeyTypes)
+}
+
+func stringSliceEqual(a, b []string) bool {


move to cmn#string.go?

ebuchman · 2018-10-30T15:36:00Z

types/params.go

+		res.Evidence.MaxAge = params2.Evidence.MaxAge
+	}
+	if params2.Validator != nil {
+		res.Validator.PubKeyTypes = params2.Validator.PubKeyTypes


Should we copy here?

Also I wonder if it makes sense to use Amino naming in the types#ConsensusParams but ABCI naming in the abci#ConsensusParams. Is that crazy? Or maybe we put an empty PubKey of the respective type in the types#ConsensusParams instead of strings.

Good idea, it probably is best to copy.

@ebuchman

* Enforce validators can only use the correct pubkey type * adapt to variable renames * Address comments from #2636 * separate updating and validation logic * update spec * Add test case for TestStringSliceEqual, clarify slice copying code * Address @ebuchman's comments * Split up testing validator update execution, and its validation

Add ValidatorPubkeyTypes as a consensus param

5e68d1c

Ref #2414

ValarDragon requested review from ebuchman, melekes and xla as code owners October 14, 2018 05:00

update spec

49cfa31

ValarDragon requested a review from zramsay as a code owner October 14, 2018 05:02

melekes approved these changes Oct 14, 2018

View reviewed changes

address anton's comment

cdeb0d0

ebuchman reviewed Oct 16, 2018

View reviewed changes

This was referenced Oct 16, 2018

Validators can run with Secp256k1 pubkeys #2414

Closed

Add Version to ConsensusParams #2671

Closed

ValarDragon added 3 commits October 21, 2018 01:58

Switch to Validator and Validator Params

ba52834

Merge branch 'develop' into dev/add_validator_pubkey_type_consensus_p…

d883524

…aram

Correct changelog entry

a5cc8ac

ebuchman reviewed Oct 25, 2018

View reviewed changes

ebuchman mentioned this pull request Oct 25, 2018

Deduplicate components of field names #2698

Closed

ValarDragon mentioned this pull request Oct 25, 2018

Make cryptography amino routes match ABCI routes #2701

Closed

Address bucky's comments!

241bf3d

ValarDragon force-pushed the dev/add_validator_pubkey_type_consensus_param branch from 96b7d2c to 241bf3d Compare October 27, 2018 04:01

ValarDragon added 2 commits October 26, 2018 21:04

forgot to update changelog

e85a96c

fix typo

d3381af

fix Params naming

27cd7c8

melekes approved these changes Oct 30, 2018

View reviewed changes

ebuchman approved these changes Oct 30, 2018

View reviewed changes

ebuchman merged commit 56d7160 into develop Oct 30, 2018

ebuchman mentioned this pull request Oct 30, 2018

Enforce that validators use pubkey types from validator params #2714

Closed

4 tasks

ValarDragon mentioned this pull request Nov 1, 2018

Enforce validators can only use the correct pubkey type #2739

Merged

6 tasks

ValarDragon added a commit that referenced this pull request Nov 1, 2018

Address comments from #2636

2622b0c

ebuchman deleted the dev/add_validator_pubkey_type_consensus_param branch November 7, 2018 06:51

Conversation

ValarDragon commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ValarDragon commented Oct 14, 2018

Uh oh!

melekes left a comment

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!

ebuchman Oct 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon commented Oct 16, 2018

Uh oh!

ValarDragon commented Oct 23, 2018

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!

ValarDragon commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebuchman commented Oct 30, 2018

Uh oh!

melekes 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ValarDragon commented Oct 14, 2018 •

edited

Loading

codecov-io commented Oct 14, 2018 •

edited

Loading

ebuchman Oct 16, 2018 •

edited

Loading

ValarDragon commented Oct 25, 2018 •

edited

Loading