fix: remove proposer from ValidatorSet #56
Conversation
egonspace
changed the base branch from
feature/introduce_vrf_based_proposer_election
to
develop
egonspace
added
C: enhancement
…/remove_proposer_from_validator_set
|
PTAL |
consensus/common_test.go
Outdated
| @@ -47,7 +47,7 @@ type cleanupFunc func() | |||
| // genesis, chain_id, priv_val | |||
| var config *cfg.Config // NOTE: must be reset for each _test.go file | |||
| var consensusReplayConfig *cfg.Config | |||
| var ensureTimeout = time.Millisecond * 100 | |||
| var ensureTimeout = time.Millisecond * 1000 | |||
There was a problem hiding this comment.
Why do you change this timeout value?
| @@ -257,7 +257,7 @@ func MakeGenesisState(genDoc *types.GenesisDoc) (State, error) { | |||
|
|
|||
| NextValidators: nextValidatorSet, | |||
| Validators: validatorSet, | |||
| LastValidators: types.NewRandomValidatorSet(nil, types.MakeRoundHash(genDoc.Hash(), 1, 0)), | |||
| LastValidators: types.NewValidatorSet(nil), | |||
There was a problem hiding this comment.
Why don't you add Proposer to the State of the state module, such as the State of the consensus module?
There was a problem hiding this comment.
We can add LastProposer to State if we need it, but for now, we don't need it.
There was a problem hiding this comment.
OK, I understand. Thank you. :)
| @@ -37,7 +37,7 @@ func TestApplyBlock(t *testing.T) { | |||
| blockExec := sm.NewBlockExecutor(stateDB, log.TestingLogger(), proxyApp.Consensus(), | |||
| mock.Mempool{}, sm.MockEvidencePool{}) | |||
|
|
|||
| block := makeBlockWithPrivVal(state, privVals[state.Validators.Proposer.Address.String()], 1) | |||
| block := makeBlockWithPrivVal(state, privVals[state.Validators.Validators[0].Address.String()], 1) | |||
There was a problem hiding this comment.
Can you tell me what your intention is in replacing Proposer with Validator[0] in this test case?
There was a problem hiding this comment.
ValidatorSet.Proposer is not available any more. In this test, validatorSet has only one validator, so Validators[0] is the proposer in this case.
If we must get the proposer among several Validators, we should use SelectProposer() function instead of ValidatorSet.Proposer
There was a problem hiding this comment.
For tests with more than one Validator, I modified it to use SelectProposer().
There was a problem hiding this comment.
I see, the reason why Validator[0] can be swapped with Proposer is because there is only one validator in ValidatorSet.
| @@ -73,6 +73,7 @@ type RoundState struct { | |||
| // Subjective time when +2/3 precommits for Block at Round were found | |||
| CommitTime time.Time `json:"commit_time"` | |||
| Validators *types.ValidatorSet `json:"validators"` | |||
| Proposer *types.Validator `json:"proposer"` | |||
There was a problem hiding this comment.
It needs to output a string for the new field with StringIndented().
There was a problem hiding this comment.
It's good point. Thank you for that.
Propose: remove Proposer from ValidatorSet.
Motive for this fix
State.Validators, State.NextValidators, State.LastValidators are all ValidatorSet type.
But ValidatorSet.Proposer can be set by calling SelectProposer(lastProof, height, round) and
height, round are active runtime information of ConsensusState.
Therefore, it is not appropriate for these information to be inside the State, which contains committed block information.
I think Proposer should be only in ConsensusState.
This fix is a proposal for this.
And #55 does not verify the proposer of proposal block, so this PR contains that verification.
I haven't executed the tests yet, but if this proposal is accepted to some degree, I'll take them.
Closes: #XXX
Description
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer