What common traits should RNG implementations implement?

[pitdicker]

Summary

• All RNGs should implement Debug but such that no internal details are output
• All RNGs should implement Clone if, and only if, the clone will have identical output to the original
• RNGs should never implement Copy
• No guidance regarding other traits, e.g. Eq and PartialEq
• Serialisation traits: see Should crypto RNGs implement serialization? #31

---

All Rng implementations should implement Debug and Clone. For pseudo-random number generators it can also be useful to implement Eq and PartialEq. Traits that should not be implemented or derived are Copy, Default and Display.

Debug

Many RNG implementations go through trouble to make the internal state not recoverable from the output, or at least not trivially so. For cryptographic RNGs their security depends on this. Then it seems strange to me if the internal state could show up in debug logs etc.

So for PRNGs we should recomment to add a custom Debug implementation that only displays the struct name, like IsaacRng currently does.

Wrappers should derive Debug normally.

Clone, Eq an PartialEq, but not Copy

An older RFC (never accepted, but with good some comments) argued RNG implementations should not implement Copy unless the copy generates independent/uncorrelated sequences. So OsRng could implement it, but PRNGs should not. It is otherwise to easy to end up with:

let mut a = ChaChaRng::new();
let mut b = a;
assert!(a.next_u32() == b.next_u32());

For consistency I would suggest RNGs to never implement copy. This makes it easy to swap out implementations. For example to replace OsRng with ChaChaRng in user code.

Clone can be useful in the rare situations where you need two identical RNGs (I found them useful when testing). Together with Eq it can be useful to store the state of an RNG with clone, and restore or compare an RNG to a previous state.

No Default

RNGs should be properly initialized, and not be created with a default fixed value.

I think some guideline like this should be part of the documentation, and possibly of the RFC.

[pitdicker]

Note ChaChaRng and IsaacRng currently implement Copy, while XorShiftRng does not.

[dhardy]

I agree that RNGs should definitely not implement Copy.

PRNGs should also implement SeedFromRng (or whatever variation on this we end up with), and probably also SeedableRng (this doc is out of date).

[dhardy]

Also see:

• should RNGs really be clone?
• Copy impl for random number generators

I also wonder about removing Clone, but it's also sometimes useful and much less dangerous than Copy.

[pitdicker]

Ah, good links!

One of the suggestions in them is to add clone-like functionality, but not implement the actual trait. I wonder if that would still be suggested with the Libz Blitz initiative an API guidelines... Now there is more of a push for interoperability and implementing common traits.

Copy is easy to misuse, but Clone already makes you question "should I really be doing this?"

[dhardy]

Regarding Eq and PartialEq is there any good rationale for expecting these? I see little genuine uses. Normally unit tests just compare a section of output.

I also imagine they're not trivial to implement correctly. E.g. our recent discussion about whether IsaacRng::init should call issac() at the end shows there can be multiple equivalent states.

Also, regarding Eq especially, there may be states which shouldn't be equal but may end up generating the same output. E.g. a PRNG with a byte-buffer for output might choose to implement next_u64 by throwing away any remaining bytes before the correct alignment boundary for u64 (I'm not aware of any implementations doing this, but it's something which might make sense). If you took one of these, cloned it, and read one byte from one, they shouldn't compare equal (because if you read one byte from each it would differ), but they would still output the same results from next_u64.

[pitdicker]

As @dhardy noted in #16 (comment) it might be expected that after let a = b.clone(); the following is true: a.next_u32() == b.next_u32(). Then there are generators for which it does not make sense to implement Clone, like OsRng and ReadRng.

[dhardy]

I added a summary at the top. I think we're happy with this?

[pitdicker]

Good idea to add labels "apparently solved". I would like to think a bit more about Eq and PartialEq.

[dhardy]

Can we also recommend Serde support behind a feature gate? See rust-random#189.

Note that there are currently two issues likely to prevent serde_derive working on PRNGs and manual implementation is annoyingly verbose, so implementing Serde support shouldn't be a hard requirement.

[pitdicker]

I am not sure it is such a good idea, for the same reasons RNGs shouldn't implement Debug: The security of cryptographic RNGs depends on not knowing their internal state. Then does it make sense to be able to serialize that state?

rust-random#84 (comment) mentions a use case: games want to be able to restore to some specific state, and only knowing the initial seed is not enough.

I think we can offer something better, by having seekable RNGs like PCG, ChaCha, and with some effort Xorshift(*). Now only the initial seed is needed and a counter, both which can be recorded with a wrapper.

[dhardy]

There are two problems there: (1) the counter needs to be extracted from somewhere, making implementation harder unless all RNGs now have to count output, and (2) not all RNGs are seekable (or can be made seekable without literally repeating each step).

One could compromise and not implement this for CSPRNGs, but really the API shouldn't be responsible for securing details (e.g. a user could simply transmute to a POD struct type mimicking the internal structure if he really wanted to read the contents).

[pitdicker]

(sorry for taking your comment apart)

There are two problems there: (1) the counter needs to be extracted from somewhere, making implementation harder unless all RNGs now have to count output

Hm, my idea to work with a wrapper with a counter doesn't work.
For example, should it be:

impl Rng for RngWithCounter {
    // RNG outputs 32 bits natively
    fn next_u32(&mut self) -> u32 {
        self.counter += 1;
        self.rng.next_u32()
    }
    fn next_u64(&mut self) -> u64 {
        self.counter += 2;
        self.rng.next_u64()
    }
    /*/
}

Or maybe:

impl Rng for RngWithCounter {
    // RNG outputs 64 bits natively
    fn next_u32(&mut self) -> u32 {
        self.counter += 1;
        self.rng.next_u32()
    }
    fn next_u64(&mut self) -> u64 {
        self.counter += 1;
        self.rng.next_u64()
    }
    /*/
}

Or some other, more complex scheme?
So a wrapper does not work. And including a counter with each rng is not good either.

(2) not all RNGs are seekable (or can be made seekable without literally repeating each step)

Yes, true. But (as far as I know) all common non-cryptographic ones can be (LSFR and LCG families).

a user could simply transmute to a POD struct type mimicking the internal structure if he really wanted to read the contents

Using unsafe and transmute is a pretty good red flag though :-)

the API shouldn't be responsible for securing details

Not sure. I don't think we should commit to security too much. jitterentropy was using protected memory mechanisms, and overwriting used random numbers still in a buffer. That all goes a little too far for me.

But not making it easy for safe Rust code to accidentally reveal an RNGs state seems like a sensible precaution to me.

With this post on the PCG blog one could argue not only CSPRNGs should not reveal their state, but normal PRNGs maybe neither.

Not sure what to do it. Actually I don't care all that strong about it...
Maybe it should be possible, but just with a little speed-bump?

[dhardy]

I'm going to mark this as "apparently solved" again. The serialization stuff has been moved and there seems to be little interest in (partial) Eq either way (I can't think of any uses beyond trivial unit tests, or how it could be a real security issue or otherwise a problem).

[dhardy]

Added to RFC: rust-lang/rfcs@89ea3a9

[dhardy]

Do we need to write some documentation / a guide before closing this? Possibly a small addition to the RngCore doc would suffice.