Endianness is inconsistent between `rand` 0.4.0 and `rand` 0.7.0

[dingxiangfei2009]

Related to #875

During the tests, we discovered inconsistency in endianness when comparing the outputs from rand 0.4.0 and rand 0.7.0.
As an extension to #875, tests are run to check if next_u64 is consistent across versions. These tests shows that the byte order of the output has switched.

For instance, on rand 0.4.0, running the following code

let mut rng = rand::ChaChaRng::new_unseeded();
let lower = 88293;
let higher = 9300932;
rng.set_counter(lower, higher);
assert_eq_bin!(4060232610, rng.next_u32());
assert_eq_bin!(2786236710, rng.next_u32());
assert_eq_bin!(3748877652, rng.next_u32());
assert_eq_bin!(3324792667, rng.next_u32());
println!("{:b}", rng.next_u64());

gives

10100111101100110100101110001011
00000000011011000000000000111011

With rand 0.7.0, running the following

let mut rng = ChaChaRng::from_seed([0u8; 32]);
let lower = 88293;
let higher = 9300932;
rng.set_word_pos(lower << 4);
rng.set_stream(higher);
assert_eq!(4060232610, rng.next_u32());
assert_eq!(2786236710, rng.next_u32());
assert_eq!(3748877652, rng.next_u32());
assert_eq!(3324792667, rng.next_u32());
println!("{:b}", rng.next_u64());

gives us

00000000011011000000000000111011
10100111101100110100101110001011

So byte orders are reversed.

I also reviewed the wordings of RngCore trait, and the required methods have no guarantees on the byte order of next_* methods.

Maybe we should enforce some form of endianness requirement?

[newpavlov]

I think rand v0.4 had an endianness bug, since #874 now includes a dedicated big-endian CI job and it passes without issues (yes, rand_chacha tests include next_u32 tests). Although since MIPS can use both endians, to be safe we should check if CI job is indeed works correctly, e.g. by testing code like this:

let buf: [u8; 4] = [1, 2, 3, 4];
assert_eq!(u32::from_ne_bytes(buf), 0x0403_0201);

[dhardy]

I believe we didn't even check endian-correctness back with 0.4, but do now. So I think this is a non-issue?

[newpavlov]

@dhardy
Can you check the CI job using the code snippet just to be safe?

Otherwise I think it's also documentation issue, since AFAIK right now we indeed do not have any guarantees/requirements regarding reproducibility of results across BE/LE machines.

[dingxiangfei2009]

So next_* enforces LE?

[dhardy]

Endianness portability is an implicit guarantee in the book, though maybe it should be explicit. It's also mentioned in the RngCore doc. The fact that this differs from 0.4 is probably not mentioned simply because it was never guaranteed before this (it is actually mentioned in the 0.5 migration guide).

The MIPS tester does indeed use BE; it's picked up endianness bugs before.

So next_* enforces LE?

Basically, we recommend usage of LE everywhere, though the only requirement on RNGs is portability, thus BE RNGs are also possible. I believe all our RNGs are LE however.

I don't believe there is any issue here, except perhaps unclear documentation.

[newpavlov]

There is also an issue of code like this:

let mut rng = FooRng::seed_from_u64(0);
rng.fill_bytes(&mut buf[..1]);
let b = rng.next_u32();

AFAIK right now we do not specify how PRNGs should behave in such situations. For performance reasons you want to align output, i.e. if internally RNG outputs 32-bit integers, then in the first call it will be more efficient to discard 24 bits than buffering them for future use. But doing so may be counter-intuitive if we view PRNG as a stream of bytes.

I think interaction between fill_bytes, next_u32 and next_u64 should be covered as well. It can be as simple as "implementation dependent".

[dhardy]

@newpavlov do we need to specify how PRNGs should behave? My view is that we should require PRNGs output given a fixed seed be portable, but go no further. This is IIRC basically what we do now.

[newpavlov]

@dhardy I am not objecting to that, I just argue that we probably should specify somehow in documentation that implementations may skip bytes for performance reasons.

[dhardy]

Read this doc then tell me it isn't already there. Seriously, we've covered this topic maybe half a dozen times now.

Of course, doc improvements are welcome, but I think this may be the case of an important detail being lost in a sea of information 😬

[newpavlov]

Ah, yes. I forgot about this part:

many implementations of fill_bytes consume a whole number of u32 or u64 values and drop any remaining unused bytes.

I would maybe add that the same can happen for next_u32 if PRNG internally outputs 64-bit numbers.

[dhardy]

Feel free. Our only 64-bit block RNG (Isaac64) actually keeps the next u32 in this case, though things like Pcg64 don't.

[dhardy]

I believe we can close this? If there are outstanding issues I'll re-open.