-
Notifications
You must be signed in to change notification settings - Fork 432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port new SeedableRng trait #233
Conversation
I will add a couple of comments first |
1a8e1f3
to
7c4d8a2
Compare
/// println!("{}", rng.gen::<f64>()); | ||
/// rng.reseed(&[5, 6, 7, 8]); | ||
/// println!("{}", rng.gen::<f64>()); | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the examples for now. When we split this off into the rand_core
crate, we can't run tests here anyway because there are no PRNG's available?
src/lib.rs
Outdated
pub struct StdRng { | ||
rng: IsaacWordRng, | ||
} | ||
pub struct StdRng(IsaacWordRng); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should not have changed StdRng
in this PR, but it seemed like a minor change and made a direct copy of the rest possible.
let mut state = seed; | ||
let mut seed = <StdRng as SeedableRng>::Seed::default(); | ||
for x in seed.iter_mut() { | ||
// PCG algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need PCG here, simply using the bytes is enough (for ISAAC of HC-128). But it is temporary anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know my implementation was overkill for the requirements! We don't know the length of the seed in general, though I guess in this case we do.
@@ -1071,23 +1110,6 @@ mod test { | |||
} | |||
} | |||
|
|||
pub fn iter_eq<I, J>(i: I, j: J) -> bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was only used by test_weak_rng
|
||
#[test] | ||
#[cfg(feature="std")] | ||
fn test_weak_rng() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't do much, and I didn't want to update it...
It is comparing two RNGs seeded by the same results of weak_rng
. It is just about impossible for them to be different.
@@ -84,54 +80,11 @@ impl<R: Rng, Rsdr: Reseeder<R>> Rng for ReseedingRng<R, Rsdr> { | |||
} | |||
} | |||
|
|||
impl<S, R: SeedableRng<S>, Rsdr: Reseeder<R> + Default> | |||
SeedableRng<(Rsdr, S)> for ReseedingRng<R, Rsdr> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With an [u8]
seed it is not possible to provide a reseeder. I just removed these function, we have a new
function anyway. I don't know if it makes sense to add them back, with ReseedWithNew
as default reseeder. That would only make it available with the std
feature. I did not add from_reseeder
as an alternative, I don't really like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this as long as we maintain equivalent functionality in StdRng
for now (we can also think about changing that later). It doesn't make much sense having reproducibility functionality in reseeding RNGs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely follow what you mean, StdRng
does not depend on ReesedingRng
?
I am trying to think of a way to add it back, because I think from_rng
can be a convenient way to initialize ReesedingRng
. The reseeder
is going to use NewSeeded
usually anyway. But that is turning into quite some changes, better for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, thread_rng
reseeds periodically; as has been discussed the utility of this is questionable, especially since OsRng
will not return week numbers on almost any OS.
I don't see any other use for ReseedingRng
, so we could just remove it, other than a version specific to the requirements of thread_rng
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some use in keeping ReseedingRng
. It can provide a little extra sort of security. And we probably want to use it for the proposed fork protection. I think I see some possible use around cloning PRNG's, where one gets reseeded on first use after a clone. If we get the overhead down to ~~0%, I see no reason not to use it sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; I'm just saying don't worry about breaking the API here.
/// Something that can be used to reseed an RNG via `ReseedingRng`. | ||
/// | ||
/// # Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also lost this example 😞. Not that it made any sense, but at least it showed how to use the API. Hopefully I/we can add something back in the future.
@@ -115,6 +115,13 @@ impl ::std::error::Error for TimerError { | |||
} | |||
} | |||
|
|||
impl From<TimerError> for Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Became necessary because of NewSeeded
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like something I forgot.
src/lib.rs
Outdated
@@ -904,7 +930,7 @@ impl SeedableRng for StdRng { | |||
/// This will seed the generator with randomness from thread_rng. | |||
#[cfg(feature="std")] | |||
pub fn weak_rng() -> XorShiftRng { | |||
thread_rng().gen() | |||
XorShiftRng::new().unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not great to unwrap here, but hopefully we can replace weak_rng
with a SmallRng
wrapper type soon, and let user code use it with NewSeeded
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thread_rng
is the better way to seed new weak_rng
s anyway? It should perform much better if multiple weak RNGs are created, so use from_rng
? Unfortunately you still need some error handling to retry a few times in case XorshiftRng::from_rng
fails with seed 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure it is better. If a library already makes use of thread_rng
it will be faster. But if it never uses thread_rng
the first use of weak_rng
will be slower, because thread_rng
is far more costly to initialize.
I don't care much either way, this mirrors NewSeeded
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the first use; I think it would be nice to make weak_rng
fairly fast average-case.
/// Controls how the thread-local RNG is reseeded. | ||
#[cfg(feature="std")] | ||
#[derive(Debug)] | ||
struct ThreadRngReseeder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary with ReseedWithNew
.
Also I moved around the rest op the ThreadRng
code a bit, partly to make the copy easier, partly a little cleanup.
99c7029
to
eca2565
Compare
src/lib.rs
Outdated
/// Seeding from a cryptographic generator should be fine. On the other | ||
/// hand, seeding a simple numerical generator from another of the same | ||
/// type sometimes has serious side effects such as effectively cloning the | ||
/// generator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very happy with this text; it's vague. It might be worth mentioning XorShiftRng
and simple counting RNGs; I don't know.
I think it might also be worth pointing out that the default implementation uses from_seed
internally and should always be sufficient, but PRNGs may have more optimal implementations. Also that there is no reproducibility requirement here. Maybe better to remove the text about this from the trait documentation and put it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it, but completely agree 😄. Will try to write something else.
src/lib.rs
Outdated
/// hand, seeding a simple numerical generator from another of the same | ||
/// type sometimes has serious side effects such as effectively cloning the | ||
/// generator. | ||
fn from_rng<R: Rng>(mut rng: R) -> Result<Self, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with dynamic dispatch? If so, great. (I.e. X::from_rng(make_rng() as &mut Rng)
.)
src/lib.rs
Outdated
@@ -801,10 +830,11 @@ pub struct Closed01<F>(pub F); | |||
|
|||
/// The standard RNG. This is designed to be efficient on the current | |||
/// platform. | |||
/// | |||
/// The underlying algorithm is not fixed, thus values from this generator | |||
/// cannot be guaranteed to be reproducible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We understand what we mean by reproducible now, but quite a few people clearly have not; I don't think we should use the term in documentation without qualification. Try:
Reproducibility of output from this generator is not required, thus future library versions may use a different internal generator with different output. Further, this particular generator is not portable and produces different output on 32- and 64-bit platforms. If you require reproducible output, use a named RNG, e.g. ChaChaRng
.
let mut state = seed; | ||
let mut seed = <StdRng as SeedableRng>::Seed::default(); | ||
for x in seed.iter_mut() { | ||
// PCG algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know my implementation was overkill for the requirements! We don't know the length of the seed in general, though I guess in this case we do.
seed[4], seed[5], seed[6], seed[7], // seed | ||
0, 0, 0, 0], // counter | ||
index: STATE_WORDS, // generate on first use | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused about why this differed; that's partly why I asked you to make this PR.
let ptr = seed_u32.as_mut_ptr() as *mut u8; | ||
|
||
let slice = slice::from_raw_parts_mut(ptr, 4 * 4); | ||
rng.try_fill_bytes(slice)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another unsafe pointer block. I wonder, should we add something like fn le::as_byte_arr(x: &[u32; 4]) -> &[u8; 16]
? I guess no because that function can't do le
conversion, so naming is wrong. I'm not sure how to get the genericity right either except with a macro like array_ref
(but as u8
).
Leave it for now, but to think about.
@@ -246,6 +246,12 @@ impl<T:Rand> Rand for Option<T> { | |||
} | |||
} | |||
|
|||
impl<T: SeedableRng> Rand for T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this will get removed soon enough anyway.
@@ -84,54 +80,11 @@ impl<R: Rng, Rsdr: Reseeder<R>> Rng for ReseedingRng<R, Rsdr> { | |||
} | |||
} | |||
|
|||
impl<S, R: SeedableRng<S>, Rsdr: Reseeder<R> + Default> | |||
SeedableRng<(Rsdr, S)> for ReseedingRng<R, Rsdr> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this as long as we maintain equivalent functionality in StdRng
for now (we can also think about changing that later). It doesn't make much sense having reproducibility functionality in reseeding RNGs anyway.
@@ -115,6 +115,13 @@ impl ::std::error::Error for TimerError { | |||
} | |||
} | |||
|
|||
impl From<TimerError> for Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like something I forgot.
src/lib.rs
Outdated
@@ -904,7 +930,7 @@ impl SeedableRng for StdRng { | |||
/// This will seed the generator with randomness from thread_rng. | |||
#[cfg(feature="std")] | |||
pub fn weak_rng() -> XorShiftRng { | |||
thread_rng().gen() | |||
XorShiftRng::new().unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thread_rng
is the better way to seed new weak_rng
s anyway? It should perform much better if multiple weak RNGs are created, so use from_rng
? Unfortunately you still need some error handling to retry a few times in case XorshiftRng::from_rng
fails with seed 0.
I see that servo uses Maybe we should update |
eca2565
to
45f87af
Compare
I have included the changes to Also tried rewriting the documentation in |
😄 They have some logging... I hope they can drop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one bit of documentation to clarify and one question about from_rng
, but I'm also wondering again about NewSeeded
:-/
Unfortunately we can't just leave it out because of ReseedingRng
. What do you think about adding an EntropyRng
with the fallback logic (OsRng
/ JitterRng
/ user-defined RNG) internally? Sorry I see you just created #235.
src/lib.rs
Outdated
/// | ||
/// # Example | ||
/// It is recommended to seed PRNG's with a seed of more than circa | ||
/// 100 bits, which means an array of `[u8; 12]` or greater. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For cryptography it should be double that; don't mislead people here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eSTREAM considered 128 bits acceptable. But you have a good point, I only thought of period sizes etc. Will update.
src/lib.rs
Outdated
|
||
#[cfg(feature="std")] | ||
impl<R: SeedableRng> NewSeeded for R { | ||
fn new() -> Result<Self, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not quite sure about this; e.g. we could provide fn entropy_source() -> EntropyRng
to allow X::from_rng(entropy_source())?
.
fn from_seed(seed: Self::Seed) -> Self { | ||
let mut seed_u32 = [0u32; 8]; | ||
le::read_u32_into(&seed, &mut seed_u32); | ||
let mut seed_extended = [w(0); RAND_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Sorry, it didn't occur to me that the arrays have different lengths!
rng.try_fill_bytes(slice)?; | ||
} | ||
for i in seed.iter_mut() { | ||
*i = w(i.0.to_le()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right on both accounts, though maybe I was wrong to argue for the Endian-fixing, but it doesn't matter much.
But, since this sets the whole of mem
, why not call init(seed, 0)
?
45f87af
to
b21d1a9
Compare
Updated the documentation. |
Actually, I think it's more about how much entropy the source RNG has than anything else; e.g. a simple counting RNG should be just about good enough provided it had at least 128-bits of fresh entropy and wasn't re-used. So okay, I suppose we go with |
I don't know completely follow what you reply to... Entropy is still a bit of a fuzzy term for me. I think of it as bits freshly generated out of thin air. But you are using it right, as the number of unpredictable, incompressible, unbiased bits. Cryptographic RNG's don't require their seed to be 'freshly generated out of thin air'. Only that there is no calculable relation between the bits in a seed: if you now 127 bits of a 128 bit seed, there is still no way to predict the last bit. Is this something we should somehow document? Or where you talking to filling the state table of ISAAC with for example a counting RNG? |
For this purpose, available entropy is the most important thing I think. If you know 127 of 128 bits, and you know the algorithm (Open Source), then what the algorithm does is unimportant because there are only 2 possible states. In fact I doubt it even matters much what the seeding algorithm does; e.g. if you just repeated the same 128-bit number to fill I think "entropy" has to be taken in context; for an attacker it just means what is unknown, not what is truly random (but e.g. if ChaCha is used to expand a 64-bit seed to 256-bits, it's measured as 64-bits of entropy). (Of course there's also "security by obscurity", but generally I believe it's recommended to assume the attacker knows the algorithms.) |
This sounds like dangerous thinking. Even if there are 128 bits provided at the start, because the array is filled with a pattern it is not designed for it will still be much easier than a brute force of 2^128 tries. ISAAC and HC-128 rely on indirect memory accessing. Filling the state by repeating the same 128 bits would give you only 16 possible location in the table to access. Only after generating a couple of kilobytes I think it will even out somewhat. I did a simple unscientific test. I replaced the init function with: fn init(mut mem: [w32; RAND_SIZE], rounds: u32) -> IsaacRng {
let mut mem = [w(0); RAND_SIZE];
for i in (0..RAND_SIZE/4).map(|i| i * 4) {
mem[i] = w(0);
mem[i+1] = w(180616861);
mem[i+2] = w(85785785);
mem[i+3] = w(2789804721);
}
let mut rng = IsaacRng {
rsl: [0; RAND_SIZE],
mem: mem,
a: w(0),
b: w(0),
c: w(0),
index: 0,
};
// Prepare the first set of results
rng.isaac();
rng
} The three 'random' numbers are only generated by me mistreating my keyboard. You can say it only uses something like 80 bits of entropy. But it makes it easy to see the effects. The first 256 results (after sorting them) look like this:
|
Okay, you've convinced me that that is not a good idea. I realised of course that the algorithm might not interact with the repeated sequence ideally, but didn't realise it would be that bad. |
f97e698
to
3d4e686
Compare
I have modified |
Renamed Is there something else I need to do to get this mergable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the doc, yes, I'm happy.
I fixed up the doc commented on myself: https://github.com/dhardy/rand/tree/seedable_rng
src/lib.rs
Outdated
/// Each pseudo-random number generator (PRNG) should implement this. | ||
pub trait SeedableRng: Sized { | ||
/// Seed type, which is restricted to `u8` arrays with a length of | ||
/// 4, 8, 12, 16, 24 and 32. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text is out of date
src/lib.rs
Outdated
@@ -277,6 +276,7 @@ use prng::Isaac64Rng as IsaacWordRng; | |||
|
|||
use distributions::{Range, IndependentSample}; | |||
use distributions::range::SampleRange; | |||
#[cfg(feature="std")]use reseeding::{ReseedingRng, ReseedWithNew}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be pedantic, but can you add a space before use
?
src/lib.rs
Outdated
/// # Example | ||
/// This is the recommended way to initialize PRNGs. See the `NewRng` | ||
/// trait that provides a convenient `new` method using `from_rng` and a | ||
/// good entropy source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the second sentence:
The
NewRng
trait provides a convenientnew
method based onfrom_rng
.
And can you link to NewRng
, e.g. like this:
[`Open01`]: struct.Open01.html
src/lib.rs
Outdated
/// PRNG. Otherwise small PRNG's could show statistical bias in the first | ||
/// couple of results, and possibly not use their entire period well. | ||
/// Cryptographic PRNG's can be less secure or even insecure when they are | ||
/// seeded from a non-cryptographic PRNG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended important to use a good source of randomness to initialize the
PRNG. Cryptographic PRNG may be rendered insecure when seeded from a non-cryptographic PRNG or with insufficient entropy. Some non-cryptographic PRNGs may show statistical bias in their first results and may even have abnormally short periods if their seed numbers are not well distributed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 better to use some stronger words here. I don't believe the period of small RNG's with a fixed period is effected. And for small RNG's without a fixed period, short cycles are possible but not really related to the 'randomness' of the seed.
src/lib.rs
Outdated
fn from_seed(seed: Seed) -> Self; | ||
/// Examples of good RNG's for seeding are entropy sources like `OsRng` and | ||
/// `JitterRng`. Also cryptographically secure PRNG's (like `thread_rng`) | ||
/// can be used without hesitation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to seed from a strong external entropy source like OsRng
or from a cryptographic PRNG.
src/lib.rs
Outdated
} | ||
|
||
|
||
/// Seeding mechanism for PRNGs, providing a `new` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A convenient way to seed new algorithmic generators, otherwise known as pseudo-random number generators (PRNGs).
(Replace with PRNG on next line)
Thank you for looking again at the documentation. I mostly followed your suggestions, but not everywhere. Feel free to say when it should change some more. |
Sorry, didn't see this line before making the changes. I can't see an extra commit though? |
Darn, I pushed but didn't create the commit! dhardy@1302d4e |
Maybe try a merge, looks like our edits don't fully overlap. |
faa78de
to
34239fa
Compare
I did things manually. I only didn't link |
🎉 |
I am not sure how this PR was merged. Master now also contains the changes from your |
I merged your branch into mine, then master into that, then that back into master. |
No description provided.