test: Write a test case to verify a new voter sampling #129

kukugi · 2020-10-23T04:41:43Z

Description

This PR is for adding a test case to verify #124.
Verify a new function to elect voters and verify a functions using for sampling

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

torao

check these!

torao · 2020-10-23T05:13:42Z

types/voter_set.go

@@ -784,6 +784,9 @@ func sortVoters(candidates []*voter) []*voter {
 	sort.Slice(temp, func(i, j int) bool {
 		bigA := new(big.Int).Mul(big.NewInt(temp[i].val.VotingPower), big.NewInt(temp[j].val.StakingPower))
 		bigB := new(big.Int).Mul(big.NewInt(temp[j].val.VotingPower), big.NewInt(temp[i].val.StakingPower))


Is it correct to multiply temp[i].val.VotingPower by temp[j].val.StakingPower instead of temp[i]?

it is for comparing votingPower(i) / stakingPower(i) and votingPower(j) / stakingPower(j). so compare that way

Yes it will be right. This code is for comparing two fractions using the fact a/b > c/d iff a*d > b*c.

torao · 2020-10-23T05:17:37Z

types/voter_set.go

@@ -784,6 +784,9 @@ func sortVoters(candidates []*voter) []*voter {
 	sort.Slice(temp, func(i, j int) bool {
 		bigA := new(big.Int).Mul(big.NewInt(temp[i].val.VotingPower), big.NewInt(temp[j].val.StakingPower))
 		bigB := new(big.Int).Mul(big.NewInt(temp[j].val.VotingPower), big.NewInt(temp[i].val.StakingPower))
+		if bigA.Cmp(bigB) == 0 {
+			return bytes.Compare(temp[i].val.Address, temp[j].val.Address) == -1
+		}
 		return bigA.Cmp(bigB) == 1


Could you use switch, or use a variable that stores the result instead of executing the same function twice? This loop for sorting will cause a large number of iterations like O(N log N).

ok, it would be better 👍

egonspace · 2020-10-23T05:42:41Z

types/voter_set_test.go

@@ -719,6 +643,19 @@ func sameVoters(c1 []*Validator, c2 []*Validator) bool {
 }

 func TestElectVotersNonDup(t *testing.T) {
+	for n := 100; n <= 100000; n *= 10 {


Where is n used at?

egonspace · 2020-10-23T05:44:04Z

types/voter_set_test.go

@@ -719,6 +643,19 @@ func sameVoters(c1 []*Validator, c2 []*Validator) bool {
 }

 func TestElectVotersNonDup(t *testing.T) {
+	for n := 100; n <= 100000; n *= 10 {
+		validators := newValidatorSet(100, func(i int) int64 {
+			return int64(100 * (i + 1))


How about to use random staking power?

egonspace · 2020-10-23T05:52:20Z

types/voter_set_test.go

+	shuffled := validators.Copy()
+	for i := range shuffled.Validators {
+		r := rand.Intn(len(shuffled.Validators))
+		shuffled.Validators[i], shuffled.Validators[r] = shuffled.Validators[r], shuffled.Validators[i]


Oh, I didn't know swapping was possible without temp variables. Good!

egonspace · 2020-10-23T06:09:40Z

types/voter_set_test.go

+
+		winners := electVotersNonDup(validators.Copy(), 0, 30)
+
+		assert.True(t, isByzantine(winners, validators.totalStakingPower, 30))


Although the code was written earlier, the 356 line of isByzantine seems to be wrong.
Should it be topFVotersVotingPower >= totalVotingPower/3 not totalPriority?

And use following to calculate tolerableByzantinePower.

tolerableByzantinePower := totalPriority * tolerableByzantinePercent / 100 // ceiling tolerableByzantinePower if totalPriority*tolerableByzantinePercent%100 > 0 { tolerableByzantinePower++ }

like electVotersNonDup.

And is assert.True right? Shouldn't it be byzantine?

egonspace · 2020-10-23T06:16:05Z

types/voter_set_test.go

+		return 1
+	})
+
+	voters1 := electVotersNonDup(validators.Copy(), 1234, 20)


This cannot be proved by a single case. We'd better test for many cases using random.

egonspace · 2020-10-23T06:39:38Z

types/voter_set_test.go

+		voters := newVotersWithRandomVotingPowerDescending(100000, 100, 1000)
+		totalStakingPower := int64(1000 * len(voters))
+
+		tolerableByzantinePower := totalStakingPower * tolerableByzantinePercent / 100


How about to make a separate function for calculating tolerableByzantinePower? It's used at isByzantine and electVotersNonDup, too.

change a staking power to voting power in condition to determine if is voters byzantine change a type of winpoint to big int. beacause, it is using for sampling only

egonspace · 2020-10-26T00:39:23Z

types/voter_set_test.go

-	voters2 := electVotersNonDup(validators.Copy(), 4321, 20)
+	voters := electVotersNonDup(validators.Copy(), 0, 30)
+	for n := 1; n <= 100; n++ {
+		otherVoters := electVotersNonDup(validators.Copy(), rand.Uint64(), 30)


rand.Uint64() may generate 0.

egonspace · 2020-10-26T00:40:48Z

types/voter_set_test.go


-		assert.False(t, isByzantine(winners, validators.totalStakingPower, 20))
+		if len(winners) < n {


Why is this condition needed?

egonspace · 2020-10-26T00:48:50Z

types/voter_set_test.go

@@ -643,15 +646,16 @@ func sameVoters(c1 []*Validator, c2 []*Validator) bool {
 }

 func TestElectVotersNonDup(t *testing.T) {
-	for n := 100; n <= 10000; n *= 10 {
+	for n := 100; n < 10000; n *= 10 {


The loop only repeats twice with 100, 1000 of n.
How about to use for n := 100; n <= 1000; n += 100?
And we must test electVotersNonDup as modifying tolerableByzantinePercent parameter too. I think we'd better take this test out as a separate test.
One more, let's use random for the seed.

github-actions · 2020-10-27T12:28:08Z

types/voter_set_test.go

+func TestElectVotersNonDupVoterCountHardCode(t *testing.T) {
+	validatorSet := newValidatorSet(100, func(i int) int64 { return int64(i) })
+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 141 characters (lll)

github-actions · 2020-10-27T12:28:09Z

types/voter_set_test.go

+	validatorSet := newValidatorSet(100, func(i int) int64 { return int64(i) })
+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:28:09Z

types/voter_set_test.go

+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{20, 20, 20, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:28:09Z

types/voter_set_test.go

+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{20, 20, 20, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{30, 30, 30, 30, 30, 30, 30, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:28:09Z

types/voter_set_test.go

+		{70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 91, 100, 100, 100, 100, 100, 100, 100},
+		{100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 168 characters (lll)

github-actions · 2020-10-27T12:33:34Z

types/voter_set_test.go

+func TestElectVotersNonDupVoterCountHardCode(t *testing.T) {
+	validatorSet := newValidatorSet(100, func(i int) int64 { return int64(i) })
+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 141 characters (lll)

github-actions · 2020-10-27T12:33:34Z

types/voter_set_test.go

+	validatorSet := newValidatorSet(100, func(i int) int64 { return int64(i) })
+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:33:34Z

types/voter_set_test.go

+	expected := [][]int{
+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{20, 20, 20, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:33:34Z

types/voter_set_test.go

+		{6, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{10, 12, 15, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{20, 20, 20, 21, 21, 26, 29, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{30, 30, 30, 30, 30, 30, 30, 34, 36, 39, 41, 44, 48, 54, 54, 57, 65, 65, 69, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 142 characters (lll)

github-actions · 2020-10-27T12:33:34Z

types/voter_set_test.go

+		{70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 70, 71, 76, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 80, 82, 84, 87, 91, 100, 100, 100, 100, 100, 100, 100},
+		{90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 90, 91, 100, 100, 100, 100, 100, 100, 100},
+		{100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100},


🚫 [golangci] _{reported by reviewdog 🐶}
line is 168 characters (lll)

egonspace · 2020-10-27T12:43:29Z

There are a lot of corrections.

added minVoters parameter to electVotersNonDup
modify sort functions using in-place
fix winPoint calculation (multiply at first and then divide)
if there is no sampling satisfying the finality, then return all validators having voting power same to staking power
countVoters -> getTolerableByzantinePower (used big.Int considering overflow) and test case for it
remove useless function
added more test cases

kukugi

LGTM 👍

kukugi · 2020-10-28T06:56:37Z

types/voter_set_test.go

+			}
+			break
+		}
+		assert.True(t, isByzantineTolerable(winners, 30))


One of the same isByzantineTolerable function calls can be replaced with a variable that stores last function call

Oh, it's for debugging. I remove it.

torao

OK with a few trivial comments

torao · 2020-10-28T09:54:36Z

types/voter_set_test.go

+}
+
+func TestElectVotersNonDupByzantineTolerable(t *testing.T) {
+	rand.Seed(time.Now().UnixNano())


If it should be tested under a random situation, it's better to make sure to log the seed value using t.Logf() or use a fixed-value as the seed, so that it can certainly be reproduced if a very rare test-failure occurs.

torao · 2020-10-28T09:55:31Z

types/voter_set_test.go

+}
+
+func TestElectVotersNonDupMinVoters(t *testing.T) {
+	rand.Seed(time.Now().UnixNano())


test: Write a test case to verify a new voter sampling

ab45ea1

kukugi requested review from torao and egonspace October 23, 2020 05:03

torao reviewed Oct 23, 2020

View reviewed changes

egonspace reviewed Oct 23, 2020

View reviewed changes

test: Write a test case to verify a new voter sampling

be3651a

kukugi force-pushed the test/sampling branch from 0fa9730 to be3651a Compare October 23, 2020 07:00

Change a condition when determining if is voters byzantine

6463e5f

change a staking power to voting power in condition to determine if is voters byzantine change a type of winpoint to big int. beacause, it is using for sampling only

egonspace reviewed Oct 26, 2020

View reviewed changes

egonspace mentioned this pull request Oct 26, 2020

Consensus by the currently elected voter does not satisfy finality. #115

Closed

4 tasks

kukugi and others added 2 commits October 26, 2020 11:23

test: Use a seed to generate random value in test

b31c6da

fix: overflow, integer division truncated, more test cases

fbb1a18

github-actions bot reviewed Oct 27, 2020

View reviewed changes

Woosang Son added 2 commits October 27, 2020 21:31

fix: remove sample code

55a485d

fix: fmt error

11980dc

github-actions bot reviewed Oct 27, 2020

View reviewed changes

Woosang Son added 2 commits October 28, 2020 08:01

feat: add test sample, verifying sort order

e041312

fix: priv test failure

e83038b

kukugi commented Oct 28, 2020

View reviewed changes

fix: remove debugging code

45c1c22

egonspace requested review from Kynea0b and zemyblue October 28, 2020 08:03

torao approved these changes Oct 28, 2020

View reviewed changes

fix: apply comment; log seed for random

ae5b44b

kukugi self-assigned this Oct 30, 2020

egonspace approved these changes Nov 9, 2020

View reviewed changes

egonspace merged commit 48a925f into develop Nov 12, 2020

egonspace deleted the test/sampling branch December 10, 2020 05:04

syleeeee unassigned kukugi Jun 7, 2021

tnasu mentioned this pull request Jul 27, 2022

Swap StakingPower and VotingPower, and modify from StakingPower to VotingWeight #449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Write a test case to verify a new voter sampling #129

test: Write a test case to verify a new voter sampling #129

kukugi commented Oct 23, 2020 •

edited

Loading

torao left a comment

torao Oct 23, 2020

kukugi Oct 23, 2020

egonspace Oct 23, 2020

torao Oct 23, 2020

kukugi Oct 23, 2020

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020

egonspace Oct 23, 2020

egonspace Oct 23, 2020

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 26, 2020

egonspace Oct 26, 2020

egonspace Oct 26, 2020 •

edited

Loading

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

github-actions bot Oct 27, 2020

egonspace commented Oct 27, 2020 •

edited

Loading

kukugi left a comment

kukugi Oct 28, 2020

egonspace Oct 28, 2020

torao left a comment

torao Oct 28, 2020

torao Oct 28, 2020


		winners := electVotersNonDup(validators.Copy(), 0, 30)

		assert.True(t, isByzantine(winners, validators.totalStakingPower, 30))


		assert.False(t, isByzantine(winners, validators.totalStakingPower, 20))
		if len(winners) < n {

test: Write a test case to verify a new voter sampling #129

test: Write a test case to verify a new voter sampling #129

Conversation

kukugi commented Oct 23, 2020 • edited Loading

Description

torao left a 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 Oct 23, 2020 • edited Loading

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 Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

egonspace Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egonspace Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egonspace Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

github-actions bot Oct 27, 2020

Choose a reason for hiding this comment

egonspace commented Oct 27, 2020 • edited Loading

kukugi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kukugi commented Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 23, 2020 •

edited

Loading

egonspace Oct 26, 2020 •

edited

Loading

egonspace commented Oct 27, 2020 •

edited

Loading