Skip to content

test proposer frequencies#3155

Merged
ebuchman merged 5 commits into2960_normalize_priosfrom
bucky/test-proposer-priority
Jan 18, 2019
Merged

test proposer frequencies#3155
ebuchman merged 5 commits into2960_normalize_priosfrom
bucky/test-proposer-priority

Conversation

@ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Jan 18, 2019

Add test to check that proposers are selected at the frequency we expect.

The following fixes needed to be made:

  • take absolute value of the priorities diff
  • normalize repeatedly until the diff is less than the threshold

@ebuchman ebuchman requested a review from melekes as a code owner January 18, 2019 19:02
@codecov-io
Copy link

codecov-io commented Jan 18, 2019

Codecov Report

Merging #3155 into 2960_normalize_prios will increase coverage by 0.03%.
The diff coverage is n/a.

@@                   Coverage Diff                    @@
##           2960_normalize_prios    #3155      +/-   ##
========================================================
+ Coverage                 62.78%   62.82%   +0.03%     
========================================================
  Files                       212      212              
  Lines                     17468    17468              
========================================================
+ Hits                      10968    10974       +6     
+ Misses                     5572     5570       -2     
+ Partials                    928      924       -4
Impacted Files Coverage Δ
rpc/client/httpclient.go 68.66% <0%> (-0.93%) ⬇️
consensus/reactor.go 65.52% <0%> (-0.48%) ⬇️
blockchain/pool.go 79.52% <0%> (+1.7%) ⬆️
privval/server.go 89.23% <0%> (+3.07%) ⬆️
libs/events/events.go 98.05% <0%> (+4.85%) ⬆️

@ebuchman ebuchman changed the title WIP: test proposer frequencies test proposer frequencies Jan 18, 2019
// i.e. threshold should always be > 0
if diff > threshold && threshold > 0 {

for diff > threshold && threshold > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is guaranteed to terminate, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so as the rescaling / normalisation should guarantee that after a finite (a few) number of iterations we reach diff <= thresold. I'd assume in the case of non-random prios, this should happen after one iter even almost always.

@ebuchman ebuchman merged commit 47c0a17 into 2960_normalize_prios Jan 18, 2019
@ebuchman ebuchman deleted the bucky/test-proposer-priority branch January 18, 2019 20:37
for j := 0; j < N; j++ {
votePower := int64(cmn.RandInt() % maxPower)
privVal := types.NewMockPV()
pubKey := privVal.GetPubKey()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (as this is only a test): but why do we use the MockPV here to generate a pub key while below we use ed25519.GenPrivKey().PubKey()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason!

}
diff := max - min
return diff
return int64(math.Abs(float64(diff)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. Thanks @ebuchman!

@liamsi
Copy link
Contributor

liamsi commented Jan 18, 2019

Thanks so much @ebuchman!

{[]int64{5, 100}},
{[]int64{50, 50}},
{[]int64{50, 100}},
{[]int64{1, 1000}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to have a test with voting power close to MaxTotalVotingPower/2 or alike. Unfortunately, it would run forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants