chacha20: RNG refactor

[newpavlov]

Removes the StreamId type and uses u64 instead. Replaces Seed wit type alias for [u8; 32], removing various other bespoke conversions from other types.

[tarcieri]

Why the name change here?

[newpavlov]

So it would not collide with KeyIvInit::new. Since the method is now public it could be a problem.

[tarcieri]

It seems weird for it to have two constructor methods that take different argument types and have different names but fundamentally do the same thing

[dhardy]

init usually means to do initial set-up of an existing item, not construct a new one.

Perhaps this should be new_rng or new_legacy?

[tarcieri]

I wouldn't be too worried about the name if it weren't a public method. I think I preferred how it was private and feature-gated before.

[dhardy]

It doesn't need to be public for rand because that uses SeedableRng, and I think there isn't a reason it needs to pub for cipher usage either?

[newpavlov]

Note that I made the method public and renamed it because the SeedableRng implementation was removed for ChaChaCore.

[tarcieri]

This PR could use a better description, like mentioning it's removing Seed and StreamId

[tarcieri]

I pushed a commit on top of this here: #531

It changes pub fn init to fn new_internal, restoring the previous feature gating, and adds a Seed type alias for [u8; 32] which is conditionally re-exported when the rng feature is enabled.

[newpavlov]

I merged #531 into this branch. As I commented above, the init method (renamed by you to new_internal) was intentionally made public. So we either should return the SeedableRng impl for ChaChaCore or keep the method public (but then new_internal would not be the best name).

Personally, I think the inherent method should be sufficient.

[tarcieri]

Making it private was pretty much the main motivation of #531.

I don't understand the motivation for making it public. What problem is that solving that was previously solved by the SeedableRng impls? Who are you expecting to call that and why? Why can't they use the SeedableRng impls on the *Rng types?

[newpavlov]

IMO it simplifies the rng module and public API surface of the crate. But if you prefer to keep the SeedableRng impl, I will return it.

[dhardy]

What was actually merged here? The diff shown by GH looks mostly the same as #531. Oh, #531 got rebased since I reviewed it.

[dhardy]

I don't understand the motivation for making it public. What problem is that solving that was previously solved by the SeedableRng impls? Who are you expecting to call that and why? Why can't they use the SeedableRng impls on the *Rng types?

See this review comment.

It doesn't really matter to me whether ChaChaCore has pub fn new_internal(..) -> Self or impl SeedableRng, though the former requires some code tweaks in rand.

BTW my review notes on the documentation in #531 seem to have been ignored.