BREAKING: Write random_mod in terms of new random_bits
#1026
Conversation
This file contains hidden or 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
In order to use `random_bits_core` in `random_mod_core`, we need to make it produce `R::Error`; then we call `map_err` in `RandomBits`, rather than in `random_bits_core`, to get a `RandomBitsError`.
## Breaking changes - `Encoding::Repr` can no longer be assumed to implement `Copy`, so consumers of `Encoding::Repr` will need to explicitly call `clone`. - The numbers produced by both `random_bits` and `random_mod` will generally be different, and calling these functions will leave the RNG in a different state, than before. ## Summary This essentially applies RustCrypto#285 to `random_bits` as well as `random_mod`. Both functions behave the same way now, with the only difference being that `random_mod` adds rejection sampling; otherwise both will produce the same numbers over the same entropy stream. Questions of platform dependence are now easy; we do not define these algorithms in terms of machine words but in terms of bytes. Randomly sampled `Uint`s are constructed little-endian over the entropy stream. This leverages the existing work making `Uint` implement `Encoding`. It additionally needs `Encoding` on `BoxedUint` to make `RandomMod` and `RandomBits` work there; this is implemented, but requires dropping the `Copy` constraint on `Encoding::Repr`. This change also uses a variable- rather than constant-time comparison for the rejection loop. The only real reason to do this is the linked [compiler bug][0], but the only downside is that this makes `random_mod` leak somewhat more timing data than it otherwise would. However, `random_mod` inherently must leak some timing data (in the form of rejections), and so should arguably just be named `random_mod_vartime` in the first place. [0]: rust-lang/rust#149522
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1026 +/- ##
==========================================
- Coverage 79.87% 79.82% -0.05%
==========================================
Files 163 163
Lines 17737 17752 +15
==========================================
+ Hits 14167 14171 +4
- Misses 3570 3581 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
That's probably a good change if we want to be able to impl |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Breaking changes
Encoding::Repris no longer required to implementCopy, so consumers ofEncoding::Reprwill need to explicitly callclone.The numbers produced by both
random_bitsandrandom_modwill generally be different, and calling these functions will leave the RNG in a different state, than before.Summary
This essentially applies #285 to
random_bitsas well asrandom_mod. Both functions behave the same way now, with the only difference being thatrandom_modadds rejection sampling; otherwise both will produce the same numbers over the same entropy stream.Questions of platform dependence are now easy; we do not define these algorithms in terms of sequences of machine words but of bytes. Randomly sampled
Uints are now always constructed little-endian over the entropy stream. This does not preclude future machine-specific optimizations, but given how perilous the landscape has been (e.g. #1018), I’ve elected to err in the direction of parsimony rather than performance for this change.This leverages the existing work making
UintimplementEncoding. It additionally needsEncodingonBoxedUintto makeRandomModandRandomBitswork there; this is implemented, but requires dropping theCopyconstraint onEncoding::Repr.This change also uses a variable- rather than constant-time comparison for the rejection loop. The only real reason to do this is the linked compiler bug, but the only downside is that this makes
random_modleak somewhat more timing data than it otherwise would. However,random_modinherently must leak some timing data (in the form of rejections), and so should arguably just be namedrandom_mod_vartimein the first place.Fixes #1009