Simplify trait BlockRngCore and rename to Generator

[dhardy]

• Added a CHANGELOG.md entry

Summary

Removes assoc. type BlockRngCore::Item and removes bounds on Results.

Renames to Generator.

BlockRng{,64} is now more restrictive: e.g. it can't support Result = Vec<u32>. I think we could support the same bounds as previously (AsRef<[u32]> + Default), but we shouldn't. Anyway, #24 makes this irrelevant.

Motivation

#24 and the rngs PR made me realise that we do want a "generator" trait, especially for ReseedingRng. Hence e.g. Hc128Core should remain public. This design is derived from the current BlockRngCore, but significantly simpler.

This PR solves two existing issues:

• BlockRngCore::Results supports variable-sized buffer types like Vec<u32> which it really shouldn't.
• BlockRngCore::Results requires a Default bound (for construction), but [u32; 64] doesn't (yet) support this, requiring annoying workarounds in rand_chacha and chacha20.

Missing parts

Documentation

Obviously documentation needs some work. Better to wait until after #24 though (which will need to be rebased over this — shouldn't be too hard).

Provided impls

I'd also love to provide impls like this:

impl<G: Generator<Output = u32>> RngCore for G {
    fn next_u32(&mut self) -> u32 {
        let mut x = 0;
        self.generate(&mut x);
        x
    }

    fn next_u64(&mut self) -> u64 {
        le::next_u64_via_u32(self)
    }

    fn fill_bytes(&mut self, dst: &mut [u8]) {
        le::fill_bytes_via_next(self, dst)
    }
}

Sadly there's "one little issue": conflicting trait implementations. (Do I want to go on a rant about how Specialization/other-solutions-to-conflicting-trait-impls has been Rust's biggest missing feature for over a decade? Yes, but also no. Currently we can't even do this.)

So we work around that by having RNG crates impl both traits. (We could provide a set of macros to help, but probably doc-examples that can be copied are the better solution: there won't be much code anyway provided we keep the existing helper methods.)

Of course, RNG crates could still implement only the RngCore trait.

Questions

Should word-generators implement the Generator trait? In a way it's a nice interface, though we don't have any immediate applications. Perhaps combined with specialization it could lead to some optimizations (e.g. rust-random/rand#1286 is an example where the sample-size arguably should depend on the generator).

Drawbacks

rand_core has a lot of traits:

• RngCore, CryptoRng
• Generator, CryptoGenerator
• TryRngCore, TryCryptoRng
• SeedableRng

Is this too many?

[newpavlov]

If ReseedingRng is the only motivation for this PR, then, as I wrote in #24, I don't think we should bother with the block trait, especially considering that it would block us from removing the serde feature.

[dhardy]

This doesn't stop us removing the serde feature. We could rebase #24 on top of this; the only real differences are that the Generator trait would remain and crates like rand_hc would continue to expose both a "core" and an RNG.

[dhardy]

I'm inclined to go this path (this PR and #24 but keeping the Generator / CryptoGenerator traits).

The thing I like least is that Generator is implementable for word generators like Xoshiro but useless — "when" Rust supports something like specialization, specialization on the type of Generator::Result may actually be useful, but until then I think it's useless.

[dhardy]

Should this be renamed? Most of the std::ops traits have an Output associated type, so that may be the more logical name.

[newpavlov]

We could rebase #24 on top of this

I don't think it makes sense to have both the block traits and #24. We should either use the block traits and a generic block wrapper around them to implement RngCore (and thus keep the serde feature), or replace them with utility functions and user-defined buffer field.

My initial intent for introducing the block trait was simplification of implementation code by using type aliases (i.e. type MyRng = BlockRng<MyRngCore>;), but in practice all implementation crates (at least in this org) use BlockRng<MyRngCore> as a private field and re-implement the trait by manual delegation. Granted, the type alias approach has issues, so I understand why it's not used (and why we migrated to buffering macros in RustCrypto).

The block trait code results in a significant amount of useless boilerplate, both in rand_core and in implementation crates. Note that #24 and rust-random/rngs#78 significantly reduce the number of LoC, despite the former adding a lot of new docs.

Since you haven't provided any additional motivation for this PR except SeedableRng, I am against it. I will prepare a migration PR for rand to demonstrate that #24 works fine for SeedableRng.

[dhardy]

Your arguments seem to be against keeping the BlockRng code. I modified that here since I wanted to push a clean PR (tests fine and can be reviewed independently) and it wasn't hard, but I wasn't intent on keeping the block code.

Since you haven't provided any additional motivation for this PR except SeedableRng, I am against it.

Motivation roughly coincides with direct utility of generator "cores". (There might be other motivations but I believe they are grossly limited by the lack of specialization, for now at least.) A search for ChaCha{8,12,20}Core shows a few custom uses of ReseedingRng, though not a huge number.

I will prepare a migration PR for rand to demonstrate that #24 works fine for SeedableRng.

Oh, it will work. I wonder whether there will be a noticeable performance hit; possibly not much since branch predictors are quite good. But if I remember correctly the current design was benchmark driven.

[newpavlov]

See rust-random/rand#1686

[dhardy]

Code updated (mostly just putting Generator back in block since it's not much use to anything else).

Motivation updated: this solves two issues.

I don't see any real downsides with this (see rust-random/rand#1690) so would like to merge.

[dhardy]

This PR is a straight upgrade in my opinion (nothing undecided), so I'm merging simply to have less in-progress stuff.

[baloo]

chacha20, a downstream implementation of BlockRngCore, can't force the Output to be Zeroize'd anymore.
https://github.com/RustCrypto/stream-ciphers/blob/0713373c1be8abee6fc94873060aee292f0ded2d/chacha20/src/rng.rs#L151

as the BlockRng impl has some strict opinion on the shape of Output:

rand_core/src/block.rs

Line 166 in 5d19070

impl<const N: usize, G: Generator<Output = [u32; N]>> RngCore for BlockRng<G> {