refactor(consensus): optimize initialize priv-validator#512
refactor(consensus): optimize initialize priv-validator#512
Conversation
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Please update PR description.
Please see linter issues in internal/consensus/invalid_test.go
| case *tmcons.Vote: | ||
| r.state.mtx.RLock() | ||
| isValidator := r.state.Validators.HasProTxHash(r.state.privValidatorProTxHash) | ||
| isValidator := r.state.Validators.HasProTxHash(r.state.privValidator.ProTxHash) |
There was a problem hiding this comment.
did you test this on a full node that is not a validator? I guess we can get panic here
There was a problem hiding this comment.
since fullnode never vote, it should work
internal/consensus/state.go
Outdated
| privValidator types.PrivValidator // for signing votes | ||
| privValidatorType types.PrivValidatorType | ||
| config *config.ConsensusConfig | ||
| privValidator *privValidator |
There was a problem hiding this comment.
Instead of using pointer here, we can add IsZero() func to privValidator.
| func (cs *State) isProposer() bool { | ||
| proTxHash := cs.privValidatorProTxHash | ||
| return proTxHash != nil && bytes.Equal(cs.Validators.GetProposer().ProTxHash.Bytes(), proTxHash.Bytes()) | ||
| return cs.privValidator.IsProTxHashEqual(cs.Validators.GetProposer().ProTxHash) |
There was a problem hiding this comment.
is it possible for cs.privValidator to be nil? (full node, light node, ...)
There was a problem hiding this comment.
isProposer is called in enterPropose method, this functionality is available for validator only
| ProTxHash types.ProTxHash | ||
| } | ||
|
|
||
| func (pv *privValidator) IsProTxHashEqual(proTxHash types.ProTxHash) bool { |
There was a problem hiding this comment.
I don't understand why you created this privValidator struct.
Can't we just implement it in types.PrivValidator, instead of creating this wrapper struct?
There was a problem hiding this comment.
To add this method to priv-validator, we have to modify PrivValidator interface and support this method at each implementation. I compared the effort between this structure and other modifications and decided to stay with this.
shotonoff
left a comment
shotonoff
left a comment
There was a problem hiding this comment.
please have a look at the answers
| case *tmcons.Vote: | ||
| r.state.mtx.RLock() | ||
| isValidator := r.state.Validators.HasProTxHash(r.state.privValidatorProTxHash) | ||
| isValidator := r.state.Validators.HasProTxHash(r.state.privValidator.ProTxHash) |
There was a problem hiding this comment.
since fullnode never vote, it should work
| func (cs *State) isProposer() bool { | ||
| proTxHash := cs.privValidatorProTxHash | ||
| return proTxHash != nil && bytes.Equal(cs.Validators.GetProposer().ProTxHash.Bytes(), proTxHash.Bytes()) | ||
| return cs.privValidator.IsProTxHashEqual(cs.Validators.GetProposer().ProTxHash) |
There was a problem hiding this comment.
isProposer is called in enterPropose method, this functionality is available for validator only
lklimek
changed the title
2 participants
Issue being fixed or feature implemented
Some of the logic corresponding to the private-validator in
consensus.Stateis redundant and cannot be usedWhat was done?
This PR includes some changes of related to usage of private-validator in consensus.State.
updatePrivValidatorProTxHashmethod, because pro-tx-hash can't be changedState.SetPrivValidatormethodHow Has This Been Tested?
Unit and E2E tests
Breaking Changes
N/A
Checklist:
For repository code-owners and collaborators only