fix: Make leader election select from the whole Committee #762
Conversation
huitseeker
force-pushed
the
fix_leader_election
branch
from
2d129a7 to
9a3d231
Compare
There was a problem hiding this comment.
Great fix @huitseeker , changes make sense to me (from a non-consensus expert point of view compared to the other peer reviewers).
| let mut leader_counts_stepping_by_2 = HashMap::new(); | ||
| for i in 0..100 { | ||
| let leader = committee.leader(i); | ||
| let leader_stepping_by_2 = committee.leader(i * 2); |
There was a problem hiding this comment.
It's not immediately clear to me what we are trying to test by having the leader_stepping_by_2 here. @huitseeker could you please provide some info?
There was a problem hiding this comment.
I've added a comment: we iterate through even rounds in practice, and so I wanted to make sure we hit the whole roster (each available validator) reliably.
config/tests/config_tests.rs
Outdated
| .entry(leader_stepping_by_2) | ||
| .or_insert(0) += 1; | ||
| } | ||
| assert!(leader_counts.values().all(|v| *v > 1)); |
There was a problem hiding this comment.
Since we are weighting based on stake (where here is always 1 across the authorities) would it make sense to also check the diff between the number of times the leaders have been elected and ensure they don't diverge much? (since we use normal distribution).
There was a problem hiding this comment.
It would be an improvement, but I'm not sure how much to invest there.
We use a uniform distribution for the test_utils::committee function, which means we are doing Bernouilli sampling.
- If we want to be as general as we can, this test should generate a binomial distribution, something we can test with a goodness-of-fit test adapted for the discrete case.
- We can also test exact numbers (the test is deterministic), but this may break the second we move out of our current discipline of iterating through seeds in a very particular way.
- A good trade-off would be to use a simple inequality (e.g. Chebysheff).
I've made the tests more strict for now (solution 2. : I check each validator is hit 20 times). Please use this comment to open an issue if you think we need something better.
There was a problem hiding this comment.
Thanks for the detailed reply on this! Great to see that you did with solution (2). Definitely (3) is something we could apply here - and now that I can think off we could use in other places in the end to end tests .
huitseeker
force-pushed
the
fix_leader_election
branch
from
9a3d231 to
14b6ac9
Compare
huitseeker
force-pushed
the
fix_leader_election
branch
2 times, most recently
from
bc72c3f to
68abdfa
Compare
huitseeker
force-pushed
the
fix_leader_election
branch
from
68abdfa to
111f2e7
Compare
huitseeker
force-pushed
the
fix_leader_election
branch
from
111f2e7 to
e527abb
Compare
huitseeker
force-pushed
the
fix_leader_election
branch
from
0b304dc to
96a4b72
Compare
…#762) * fix: Make leader election select from the whole Committee Fixes MystenLabs#755 * fix: Make tests of binomial result more strict * fix: make the leader see a strict u64 (from usize) * fix: consensus behavior under tests
* fix: Make leader election select from the whole Committee Fixes #755 * fix: Make tests of binomial result more strict * fix: make the leader see a strict u64 (from usize) * fix: consensus behavior under tests
* fix: Make leader election select from the whole Committee Fixes #755 * fix: Make tests of binomial result more strict * fix: make the leader see a strict u64 (from usize) * fix: consensus behavior under tests
…/narwhal#762) * fix: Make leader election select from the whole Committee Fixes MystenLabs/narwhal#755 * fix: Make tests of binomial result more strict * fix: make the leader see a strict u64 (from usize) * fix: consensus behavior under tests
Fixes #755