Remove CryptoGenerator and reduce BlockRng to BlockBuffer

[dhardy]

• Added a CHANGELOG.md entry

Motivation

rust-random/rand#1722 makes this useless.

Other

Any suggestions for a new name for trait Generator? As-is it matches its most important generate method, so the name doesn't seem too bad.

[newpavlov]

Any suggestions for a new name for trait Generator?

BlockRng would be the most consistent name with BlockRng::generate_block (or next_block) method. Instead of the wrapper I would prefer to have a separate BlockBuffer struct.

[dhardy]

The names are consistent.

What is the motivation for making BlockBuffer separate? It makes little sense to me (without removing the trait as in the more recent #24 design).

[newpavlov]

Separation of concerns. IMO struct Rng { core: RngCore, buffer: BlockBuffer<..> } is more transparent than struct Rng(BlockRng(RngCore));. And zeroization support would look less confusing. It's easy to understand what happens in:

self.core.zeroize();
self.buffer = Default::default();
zeroize::optimization_barrier(&self.buffer)

While the wrapper would result in a less straightforward code.

The same applies to serialization as well.

[dhardy]

Separation of concerns

If they were truly separate components I'd agree, but the only pub methods not needing both parameters are word_offset and remaining_results.

I see you succeeded in convincing @tarcieri to make optimization_barrier public in RustCrypto/utils#1261, though it's not in a released version yet.

[dhardy]

@newpavlov I made significant additions to this PR; please re-review.

[newpavlov]

Word is already bounded by Default via the Sealed trait.

And I think we can simplify the bound to just G::Output: Default, no? UPD: No, we use Word functionality in the implementation.

[newpavlov]

Is this method used in practice? I think we had a discussion about this and came to conclusion that it's better to use serialization/deserialization over [W; N] instead of &[W].

[dhardy]

Yes, it is used: https://github.com/rust-random/rngs/blob/master/rand_isaac/src/isaac.rs#L171

And no, that is the conclusion that you came to. My conclusion was that where fixed-size serialization is preferred it is still possible with this approach (since N is known), while the reverse is not with your approach (and variable length serialization is preferable if the output format happens to be something like JSON).

[newpavlov]

Sure, whatever...

But at least add methods for fixed-sized (de)serialization. It would mean that your struct has two ways of handling it, but so be it.

[newpavlov]

Update changelog.

[newpavlov]

Use R instead of G like in all other bounds?

[newpavlov]

Maybe rename the method to next_block for consistency with the Rng methods?

[newpavlov]

It would be nice to explicitly document that BlockBuffer::default() initializes all bits of the state to make it friendlier to the optimization_barrier-based zeroization.

[dhardy]

Regarding the renaming, the largest problem is actually that we use 'core' to refer to the BlockRng (e.g. in the doc example and as a parameter to methods like next_word). Why 'core'? This is the core (random) generator.

So, BlockRng? The old struct BlockRng referred to a block-based random-number generator. So the name was fine.

BlockBuffer is okay but ultimately a poor name; it buffers results not blocks. ResultsBuffer or just Buffer might work.

The new trait BlockRng is actually a bad name: implementations are (random) (data) block generators, not number generators (no one needs 512-bit numbers).

The old name block::Generator is about as close to "random block generator" as one can get without an unwieldy name I think (unless we want to call it Rbg).

---

Since the main argument for separation of the core and the buffer seems to be zeroization and alternatives need specific documentation, why don't we just add a method like fn clear?

---

To avoid complicating history of a single PR too much I will close this one and replace it.