rand_core: introduce Seed trait

[newpavlov]

This resolves the issue with longer arrays, makes the code slightly more concise, and allows to use u32/u64/u128 as seeds.

Discussed in #1643 and #1666.

[newpavlov]

Currently it breaks chacha20, but it should be easy enough to fix.

[newpavlov]

I wonder if we should also implement this trait for [u32/u64/u128; N] types. For example, for PCG32 [u64; 2] is a "natural" seed, while for ChaCha20 it would be [u32; 8].

[dhardy]

I was assuming this was the purpose of Seed. Also why you deleted the "Default for large arrays" doc.

[newpavlov]

My main purpose was to support non-u8 seeds, support for large arrays is a great side benefit.

[newpavlov]

Ideally, we would use impl FnOnce(&mut [u8; { Self::SIZE }]), but it does not work on the current stable Rust.

[dhardy]

This should be named try_from_fill(buf: impl ...).

But I'm wondering why we actually want it. The only advantage I can see over a simple fn from_bytes(bytes: [u8; Self::SIZE]) is that it does not rely on the optimiser to elide a move.

[newpavlov]

As I wrote in the comment above, [u8; Self::SIZE] does not work because of the const generics limitations.

[dhardy]

Ah.. this limitation is not a side-effect of using a closure.

We could use fn from_bytes(bytes: &[u8]) -> Self; but we'd lose assertion of the size.

[dhardy]

So, Seed is a reinvention of rand::Fill?

I'm not really convinced we need this. If someone wants to use a u128 as a seed in the existing version, they only need to call x.to_le_bytes().

[newpavlov]

It looks similar to Fill, but it's purpose is different. It's used to clean up the unwieldy Clone + Default + AsRef<[u8]> + AsMut<[u8]> bound and to directly support "natural" seed types instead of plain byte arrays. The advantage of properly supporting large arrays can not be overlooked as well.

[dhardy]

unwieldy Clone + Default + AsRef<[u8]> + AsMut<[u8]> bound

This isn't really unwieldy; it just means "like [u8; N]".

to directly support "natural" seed types

This could easily be misused. Xoshiro128PlusPlus::from_seed(0) has special handling but from_seed(1) does not; users should call seed_from_u64(1) instead.

The advantage of properly supporting large arrays can not be overlooked as well.

We should get a proper fix from Rust soon: rust-lang/rust#61415 which won't force usage of alternative types like [u32; 16]. It's also not a big issue for us since [u8; 32] is already supported and is normally big enough.

[newpavlov]

This isn't really unwieldy; it just means "like [u8; N]".

And it allows for unrelated types like Vec<u8> to be used for this associated type. I think a litmus test should be whether the API could get included into core or not. I think the proposed Seed trait has much higher chances than the current bound.

It may have been a different story if we were able to use const SEED_SIZE: usize;, but alas.

This could easily be misused.

How is it different from using [0u8; N] or any other "bad" seed? On the other hand, it allows users to use hard-coded random seeds without overhead of running PCG and swapping byte orders (applies only to BE, so not important, but still).

We should get a proper fix from Rust soon

Well, "soon" here may mean year or more until stabilization.

[newpavlov]

It looks like the main difference in our opinions is whether we should allow non-u8 seeds. I think that many PRNG algorithms are defined in terms of such seeds and it's worth to expose it in our API. And you think that it's not worth the trouble in practice.

I guess we should see what others think about it.

[dhardy]

It looks like the main difference in our opinions is whether we should allow non-u8 seeds.

Yes, PRNGs normally use something like [u32; N] or u128 internally. But crypto RNGs normally define seeding from bytes and non-crypto PRNGs usually either don't define a seeding routine or do define (or suggest) one as an extra step, as is the case of Xoshiro generators using SplitMix64.

I've seen people use bad seeds before. [u8; N] is at least a little bit hard to mis-use (besides [0; _] which we already have special handling for). Not supporting Xoshiro256PlusPlus::from_seed(1) is a feature in my opinion.

[dhardy]

There's a second reason I'm against this: it is a significant re-working of what has been a stable part of rand_core since v0.1 seven and a half years ago (excepting the addition of an AsRef<[u8]> bound in v0.9 this year which caused negligible disruption).

Mostly I see this PR as unnecessary churn (plus some additional complexity). I would prefer to wait until we have const generics then replace type Seed with [u8; BYTE_LEN] and const BYTE_LEN: usize.

My main purpose was to support non-u8 seeds

to directly support "natural" seed types

Is this important?

It would enable Xoshiro128PlusPlus to use its native [u32; 4] type and Lcg64Xsh32 aka Pcg32 to use [u64; 2]. In the latter case this closely corresponds to the state and increment internal values, but the type already has an inherent constructor for this: fn new(state: u64, stream: u64).

The advantage of properly supporting large arrays can not be overlooked as well.

This could be a significant advantage, except that nothing really requires larger keys than [u8; 32] for which this isn't a problem.

And it allows for unrelated types like Vec<u8> to be used for this associated type.

This PR would not permit usage of Vec<u8> since no third-party crate could implement Seed for Vec<_>. The same does not apply to third-party types (e.g. a wrapper around Vec<_>). So neither approach actually fixes the size of seed used.

Not supporting Xoshiro256PlusPlus::from_seed(1) is a feature in my opinion.

This comment was a mistake in my part: since the RNG fixes the seed type (and would likely use [u32; 4], mirroring the internal state type), it is not necessarily an issue (we can assume that RNG designers will make sensible choices here).

---

In summary: technically (I don't think) either approach is significantly better than the other. The churn and the implied need for an extended stabilisation period are however significant reasons not to do this.

[newpavlov]

This PR would not permit usage of Vec since no third-party crate could implement Seed for Vec<_>.

I meant it as a disadvantage of the current API, since Vec<u8> satisfies the bound. With the Seed trait we would control which std types can be used as seeds and not.

u32/u64 seeds may be slightly more performant in cases where PRNG is re-initialized frequently (proper alignment, no need for byte swaps), but I agree with the assessment that either approach is significantly better than the other.

Since no one else has voiced support for this approach, I am going to close this PR.