SmallRng uses wrong seed_from_u64 implementation

[ironhaven]

Currently SmallRng implements SeedableRng but it does not specify a seed_from_u64 method so SmallRng uses the default (pcg32) trait implementation. Because of this the Xorshiro's seed_from_u64 method using Splitmix64 is effectively dead code. Splitmix64 is recommended for seeding Xorshiro by opinionated people so this is definitely a bug.

The documentation for seed_from_u64 says changing the algorithm is a "value-breaking change" but on the other hand the current implementation is a unintended bug so might not be protected from breaking changes.

Should this wait for the next breaking version or be merged immediately?

[dhardy]

Splitmix64 is recommended for seeding Xorshiro by opinionated people so this is definitely a bug.

There is discussion on this in #1038. I'm not really convinced it's worth introducing a breaking change for this. In any case, we make zero guarantees about compatibility of SmallRng with other Xoshiro implementations; use the rand_xoshiro crate if you want that.

Should this wait for the next breaking version or be merged immediately?

This is a value-breaking change. We haven't merged breaking changes yet but probably will soon.

[vks]

I agree it's a bug, because SmallRng::seed_from_u64 was supposed to be using SplitMix (see #1038).

[dhardy]

@vks do you think it is reasonable to increase the tolerance of your normal distribution sparkline test here?

[vks]

@dhardy Sure, it seems the test failed only 0.4 % of the tolerance. I should probably calculate the probability of failing with the current tolerance to determine a better threshold.

[vks]

This still needs an update to the changelog and a fix for the tolerance of the test.

[ironhaven]

I modified the test by feeding all the numbers in the range 0..100 to the seed_from_u64 and these seeds failed at least one expected error bucket.

1 2 3 8 10 12 14 15 25 38 44 46 48 49 54 63 66 73 81 82 87 93

This means that 22/100 random seeds failed the unit test with error more than 3 standard deviations from expected.
If I allow 4 standard deviations, only a single seed fails 87
I get similar results if I use from_entropy or switch back to the pcg32 implementation.

The other solution would ditch the truly obvious seed of 1 and pick a fair random number like 4 to move on.

[vks]

This means that 22/100 random seeds failed the unit test with error more than 3 standard deviations from expected.

Thanks for investigating this! That's a much higher failure rate than what I would fundamentally expect for the 3 standard deviations threshold. I'll need to take a closer look at the error distribution.

[vks]

This only needs updating the changelog for 0.9, documenting the value-breaking change to seed_from_u64.

[dhardy]

I check all merged PRs when writing changelogs anyway (we always miss some), so I can fix that then).