Introduce VRF-based Proposer Election #55

torao · 2020-04-08T02:10:01Z

Closes: #48

Description

This PR changes the Proposer selection by the current round-robin PoS to the one based on VRF.

when a round is changed
when a genesis block is created
when replaying blocks

NOTE: With this change in Proposer selection, some of the test cases that were supposed to be deterministic Proposers in the previous round-robin PoS are skipped. These may proceed in a separate PR as they take time to investigate to apply to non-deterministic Proposer selection.

For contributor use:

Wrote tests
Updated CHANGELOG_PENDING.md
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments
Re-reviewed Files changed in the Github PR explorer

zemyblue · 2020-04-09T06:19:50Z

state/state.go

-	hash := tmhash.New()
-	hash.Write(state.LastProofHash)
-	return hash.Sum(b), nil
+	return MakeRoundHash(state.LastProofHash, state.LastBlockHeight, round), nil


How about removing error return?

Yes, I'll remove the error as it doesn't make sense.

zemyblue · 2020-04-09T06:35:35Z

state/state.go

-		validatorSet = types.NewValidatorSet(validators)
-		nextValidatorSet = types.NewValidatorSet(validators).CopyIncrementProposerPriority(1)
+		validatorSet = types.NewRandomValidatorSet(validators, MakeRoundHash(genDoc.Hash(), 0, 0))
+		nextValidatorSet = types.NewRandomValidatorSet(validators, MakeRoundHash(genDoc.Hash(), 0, 1))


I think nextValidatorSet should be samplated from next block height, not next round.
Because the validator set is changed each height.

Certainly, this field is used to set and validate the NextValidatorHash value of the block. In the comments, it is described as a validator for the next block.

NextValidatorsHash tmbytes.HexBytes `json:"next_validators_hash"` // validators for the next block

In our version, we cannot select the next Proposer without the cooperation of the first Proposer at the time of the genesis block. Does the transaction processing begin at the next block of genesis (i.e., height 2)? I haven't confirmed it yet, but this code in Tendermint currently assumes that validators remain unchanged even if the height is incremented, so it may be true.

With such an assumption, I'll increase the height here, not the round.

types/validator_set.go

zemyblue · 2020-04-09T06:48:00Z

types/validator_set.go

@@ -295,6 +337,7 @@ func (vals *ValidatorSet) GetProposer() (proposer *Validator) {
 	}
 	if vals.Proposer == nil {
 		vals.Proposer = vals.findProposer()


How about removing this code of line.
Because the proposer is elected by ResetProposerAtRandom using random sampling.

Yes, it's maybe possible. Now that I've checked, it seems that findProposer() and the functions called by it are immutable, and don't have any side effects. So I'll remove this step.

zemyblue · 2020-04-09T06:55:40Z

types/validator_set.go

@@ -295,6 +337,7 @@ func (vals *ValidatorSet) GetProposer() (proposer *Validator) {
 	}
 	if vals.Proposer == nil {
 		vals.Proposer = vals.findProposer()
+		vals.ResetProposerAtRandom([]byte{})


It seems a little dangerous code.
If proposer is empty, appropriate proposer should be set. But this elect the proposer as using default empty seed

I think it is better to return nil if the vals.Proposer is empty.

In the current Tendermint code, we'll often find codes where the Proposer changes made without heights or rounds like here. I struggled with how can I rewrite such code, but I'll set nil so that an explicit error will occur if we use it incorrectly.

egonspace · 2020-04-13T00:55:04Z

consensus/common_test.go

@@ -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


Tendermint test cases seem to use hundreds of milliseconds for timeout. Did you change it because there was some problem?

When running tests on CircleCI, a test was failing consecutively due to timeouts.

--- FAIL: TestMempoolNoProgressUntilTxsAvailable (0.28s)
panic: Timeout expired while waiting for new activity on the channel [recovered]
panic: Timeout expired while waiting for new activity on the channel

I modified the timeout and confirmed that it was due to a CI environment-specific performance issue. I'm not sure if this 100msec means a response time limit as a Tendermint specification, but I'll bring it back to 100msec.

egonspace · 2020-04-13T01:03:43Z

consensus/state.go

@@ -928,6 +931,9 @@ func (cs *State) enterPropose(height int64, round int) {
 	}
 	logger.Debug("This node is a validator")

+	// Select the current height and round Proposer
+	cs.Validators.SelectProposerWithRound(cs.state.LastProofHash, height, round)


I think SelectProposerWithRound should be exactly the same as the existing code's IncidenceProposerPriority location. Shouldn't we call SelectProposerWithRound here?

I am also curious.
SelectProposerWithRound is also called on enterNewRound
What is the reason why it is called twice?

Whereas the previous IncrementProposerPriority had to be handled carefully because it causes a state change, but SelectProposer doesn't need to be called as so carefully because it's an idempotent, i.e., the same result for the same parameter. For this reason, I've taken a slightly conservative policy and placed them where rounds may be progressing or regressing in this PR.

However, in enterProposal(), it was seen that the height and round could be entered differently from ConsensusState, so I put this here, but I'm not sure if this is really necessary here. `IncrementProposerPriority' isn't performed here, so it's may better to remove it and see how it goes. I'll fix this.

egonspace · 2020-04-13T01:06:53Z

state/state.go

@@ -247,8 +240,8 @@ func MakeGenesisState(genDoc *types.GenesisDoc) (State, error) {
 		for i, val := range genDoc.Validators {
 			validators[i] = types.NewValidator(val.PubKey, val.Power)
 		}
-		validatorSet = types.NewValidatorSet(validators)
-		nextValidatorSet = types.NewValidatorSet(validators).CopyIncrementProposerPriority(1)
+		validatorSet = types.NewRandomValidatorSet(validators, types.MakeRoundHash(genDoc.Hash(), 1, 0))


I have told you this code, but I will think more about whether it is right to make it like this on the genesisState.

I think we need to separate about default valudatorSet and selected validatorSet of our algorithm.
And I think This validatorSet of this state is the all validator set not sampling

And we also need to decide which validatorSet we store in the block.
I think it it better to store sampling validatorSet in the block, and use separated property with all validatorSet and sampling validatorSet in the State of state and consensus module.
How about this?

As in the code diff above, in the current Tendermint code, the genesis block and the 2nd block appear to use the same validators. I think it'd enough when we could investigate more detail about it in the future by adding the ability to randomly select a ValidatorSet.

egonspace · 2020-04-13T01:21:32Z

types/validator_set.go

@@ -76,6 +85,7 @@ func (vals *ValidatorSet) IsNilOrEmpty() bool {
 func (vals *ValidatorSet) CopyIncrementProposerPriority(times int) *ValidatorSet {


Since there are only tests that use CopyIncrementProcessPriority now, it would be better to remove this function or move it to the test.

Indeed, CopyIncrementProposerPriority() is no longer used in the production code, so I'll replace it with an alternative code and remove it.

egonspace · 2020-04-13T01:44:48Z

Is it necessary to leave a code calling the IncisionProposerPriority()? I think we can replace the call for IncrementProposerPriority() with a code that calls SelectProposerWithRound().

zemyblue · 2020-04-13T03:00:55Z

types/genesis.go

@@ -59,7 +59,7 @@ func (genDoc *GenesisDoc) ValidatorHash() []byte {
 	for i, v := range genDoc.Validators {
 		vals[i] = NewValidator(v.PubKey, v.Power)
 	}
-	vset := NewValidatorSet(vals)
+	vset := NewRandomValidatorSet(vals, []byte{})


How about replacing []byte{} to genDoc.Hash()?
the hash of genesis is the hash of GenesisDoc.

Yes, it may not affect the next step of acquiring for vset.Hash() but your mention is a formal genesis structure. I'll fix it.

zemyblue · 2020-04-13T04:37:13Z

@torao san,
How about save all valistors and sampling validators into the types.ValidatorSet like below?
And change the function logics accordingly.
Because, to separate all validator and sampling validators

AS-IS

https://github.com/line/tendermint/blob/66dc287b945f94d536df4c9c8a9471b90cbf94ca/types/validator_set.go#L44-L51

TO-BE

type ValidatorSet struct {
	// NOTE: persisted via reflect, must be exported.
	AllValidators []*Validator `json:"validators"`
        Validators []*Validator
	Proposer   *Validator   `json:"proposer"`

	// cached (unexported)
	totalVotingPower int64
}

torao · 2020-04-13T07:52:48Z

@egonspace Now IncrementsProposerPriority() doesn't affect our process because we are using Proposer selection based on VotingPower. But I keep it for when we consider the selection of a Proposer based on ProposerPriority. In this PR, I'll write a TODO to describe why this function is left.

@zemyblue Now I'm not sure that the ValidatorSet passed from ABCI is an all validator in a P2P network. I think we could discuss in a future change rather than in this PR about the advantage of having all validators in a ValidatorSet.

zemyblue

LGTM

Fix skipped tests of #55 (1)

torao added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Apr 8, 2020

torao added this to the Evolve Leader Election into VRF milestone Apr 8, 2020

torao self-assigned this Apr 8, 2020

torao force-pushed the feature/introduce_vrf_based_proposer_election branch 3 times, most recently from 14e2321 to 59ad1b1 Compare April 8, 2020 02:25

add VRF-based proposer election

75976ac

torao force-pushed the feature/introduce_vrf_based_proposer_election branch 2 times, most recently from 127f830 to dbaaa2d Compare April 8, 2020 12:36

skip failure test-case

01ca490

torao force-pushed the feature/introduce_vrf_based_proposer_election branch from dbaaa2d to 01ca490 Compare April 9, 2020 03:37

zemyblue reviewed Apr 9, 2020

View reviewed changes

apply validator_set_test.go to VRF

7bc2106

torao force-pushed the feature/introduce_vrf_based_proposer_election branch from 06ece55 to d19a0b1 Compare April 10, 2020 12:26

fix the points made in the PR

66dc287

torao force-pushed the feature/introduce_vrf_based_proposer_election branch from d19a0b1 to 66dc287 Compare April 10, 2020 12:52

torao requested review from egonspace and zemyblue April 10, 2020 13:03

torao changed the title ~~WIP: Introduce VRF-based Proposer Election~~ Introduce VRF-based Proposer Election Apr 10, 2020

egonspace reviewed Apr 13, 2020

View reviewed changes

zemyblue reviewed Apr 13, 2020

View reviewed changes

egonspace mentioned this pull request Apr 13, 2020

fix: remove proposer from ValidatorSet #56

Merged

5 tasks

apply fix commented points on PR

747e2f9

torao force-pushed the feature/introduce_vrf_based_proposer_election branch from ad24a5e to 747e2f9 Compare April 13, 2020 09:06

zemyblue approved these changes Apr 13, 2020

View reviewed changes

egonspace approved these changes Apr 13, 2020

View reviewed changes

torao merged commit 37f0c66 into develop Apr 14, 2020

zemyblue mentioned this pull request Apr 14, 2020

Fix skipped tests of #55 (1) #57

Merged

5 tasks

zemyblue added a commit that referenced this pull request Apr 17, 2020

Merge pull request #57 from zemyblue/feature/fix_reactor_test

5b42a63

Fix skipped tests of #55 (1)

zemyblue deleted the feature/introduce_vrf_based_proposer_election branch April 20, 2020 04:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce VRF-based Proposer Election #55

Introduce VRF-based Proposer Election #55

torao commented Apr 8, 2020 •

edited

Loading

zemyblue Apr 9, 2020

torao Apr 10, 2020

zemyblue Apr 9, 2020

torao Apr 10, 2020

zemyblue Apr 9, 2020

torao Apr 10, 2020

zemyblue Apr 9, 2020

torao Apr 10, 2020

egonspace Apr 13, 2020

torao Apr 13, 2020

egonspace Apr 13, 2020

zemyblue Apr 13, 2020

torao Apr 13, 2020

egonspace Apr 13, 2020

zemyblue Apr 13, 2020

torao Apr 13, 2020

egonspace Apr 13, 2020

torao Apr 13, 2020

egonspace commented Apr 13, 2020

zemyblue Apr 13, 2020

torao Apr 13, 2020

zemyblue commented Apr 13, 2020 •

edited

Loading

torao commented Apr 13, 2020

zemyblue left a comment

		@@ -76,6 +85,7 @@ func (vals *ValidatorSet) IsNilOrEmpty() bool {
		func (vals ValidatorSet) CopyIncrementProposerPriority(times int) ValidatorSet {

Introduce VRF-based Proposer Election #55

Introduce VRF-based Proposer Election #55

Conversation

torao commented Apr 8, 2020 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egonspace commented Apr 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zemyblue commented Apr 13, 2020 • edited Loading

AS-IS

TO-BE

torao commented Apr 13, 2020

zemyblue left a comment

Choose a reason for hiding this comment

torao commented Apr 8, 2020 •

edited

Loading

zemyblue commented Apr 13, 2020 •

edited

Loading