[core] Fix proposals shuffling implementation #7921
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robin-near
force-pushed
the
shuffle
branch
2 times, most recently
from
d473ee6
to
be5d9ff
Compare
matklad
approved these changes
Oct 25, 2022
chain/epoch-manager/src/proposals.rs
Outdated
Comment on lines
271
to
289
| let mut rng: Hc128Rng = SeedableRng::from_seed(rng_seed); | ||
| dup_proposals.shuffle(&mut rng); | ||
| for i in (1..dup_proposals.len()).rev() { | ||
| dup_proposals.swap(i, gen_index_old(&mut rng, (i + 1) as u64) as usize); | ||
| } | ||
| } | ||
|
|
||
| fn gen_index_old(rng: &mut Hc128Rng, bound: u64) -> u64 { | ||
| // This is a simplified copy of the rand gen_index implementation to ensure that | ||
| // upgrades to the rand library will not cause a change in the shuffling behavior. | ||
| let zone = u64::MAX - (u64::MAX - bound + 1) % (bound); | ||
| loop { | ||
| let v = rng.next_u64(); | ||
| let mul = (v as u128) * (bound as u128); | ||
| let (hi, lo) = ((mul >> 64) as u64, mul as u64); | ||
| if lo < zone { | ||
| return hi; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If we are feeling enterpisey, I'd probably extracted this code into a separate module/type EpochManagerRng to make sure its public API doesn't expose rand. Right now this code has an implicit invariant of "don't use any rand methods besides from_seed and next_u64", and I'd love for that invariant to have teeth.
There was a problem hiding this comment.
Sorry not exactly sure what you mean - the module is already named old_* and these two functions are not public.
There was a problem hiding this comment.
Ah, sorry, I missed two points:
- that this is already a separate module
- that we no longer relying on randomness here for the latest protocol
I was fearing that we still use rand, and need to guard aginst future code changes.
mm-near
approved these changes
Oct 25, 2022
nikurt
pushed a commit
that referenced
this pull request
Nov 7, 2022
nikurt
pushed a commit
that referenced
this pull request
Nov 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The SliceRandom::shuffle implementation changed from rand 0.6.5 to 0.7.0 in a subtle way: rust-random/rand#809 because the underlying gen_index implementation changed. (It appears that @matklad 's concern on #7717 turns out to be quite real!)
In order to reproduce the old behavior and to make sure this never changes again, this PR replicates the gen_index implementation (it's pretty straight-forward) from 0.6.5. The test case is updated (the old test case was unfortunately incorrect from #7717). Also, @mm-near 's #7909 now passes with this change.