Reduce block code

[dhardy]

• Added a CHANGELOG.md entry

Summary

This is just a cut-down version of the block code: we don't need most of it.

Motivation

I think this hits most of your goals outside of le / utils, @newpavlov? No rewrite required.

Details

We don't need BlockRng64, so long as we're okay with abandoning the half-sample code (next_u32). I think that's fine.

We don't need to implement RngCore / CryptoRng on BlockRng either.

It turns out that we do need to keep CryptoGenerator if we want to allow ReseedingRng to implement CryptoRng in a generic fashion. I suppose otherwise we could implement CryptoRng only on ThreadRng (which we already do).

We also need to implement SeedableRng on the core, otherwise ReseedingRng doesn't work.

Unsolved here: fill_bytes is generic over W: Observable which is a private trait (essentially @newpavlov's Word trait with less features). So I think it's either that trait or fill_bytes_from_u32 and fill_bytes_from_u64 which isn't exactly neat design.

Also adds trait le::Word (a renamed sealed version of Observable). It looks like we can't reasonably avoid having a Word trait.

[dhardy]

Rand compatibility: rust-random/rand#1690

[dhardy]

List of changes:

• Remove outdated documentation.
• Improve doc example in block
• Make BlockRng functionality generic; most functionality requires only W: Clone though next_u64_from_u32 requires W = u32 and fill_bytes requires W: Word.
• Do not implement RngCore on BlockRng; use inherent methods instead. This is required by the above since what was next_u32 now returns W and since next_u64_from_u32 has an additional (effective) W = u32 bound.
• Do not implement CryptoRng on BlockRng (required by above).
• Do not implement SeedableRng for BlockRng. Motivation: this is never used since BlockRng is always wrapped.
• Remove BlockRng64 code. Only it's next_u32 method differs in functionality from what is now available on BlockRng, and I believe this is an acceptable loss in functionality. It is not implementable on BlockRng since it requires an additional half_used field (and changes to the behaviour of other methods).
• Rename trait Observable → Word and make a public sealed trait.

Questions:

• Should all BlockRng functionality have a W: Word bound? Either way should be fine, but std often prefers to use the least bounds possible.
• Should we expand Word and replace le methods as in Replace block module with helper functions #24? If so, this would not be implemented in this PR.

[newpavlov]

As I wrote before, I don't think we need the Generator trait. I also really don't like the manual wrapping of BlockRng in implementation crates. It just results in a bunch of annoying boilerplate. Additionally, it's not clear how you intend to allow serialization/deserialization for Generator-based RNGs.

Finally, the separate index field is redundant and only results in bloating structure size of buffered RNGs.

[dhardy]

As I wrote before, I don't think we need the Generator trait.

Already discussed elsewhere.

I also really don't like the manual wrapping of BlockRng in implementation crates. It just results in a bunch of annoying boilerplate.

It's no different than your BlockBuffer though:

    fn next_u32(&mut self) -> u32 {
        self.0.next_word()
    }

vs

    fn next_u32(&mut self) -> u32 {
        self.buffer.next_word(|block| self.core.next_block(block))
    }

Additionally, it's not clear how you intend to allow serialization/deserialization for Generator-based RNGs.

#30.

Finally, the separate index field is redundant and only results in bloating structure size of buffered RNGs.

I believe I already said that this should be compatible with this approach, but I'm leaving it for a future PR to keep my PRs small.

[newpavlov]

It's no different than your BlockBuffer though:

In that case I wouldn't get the code size reduction in the implementation PRs. The difference is that existence of BlockBuffer encourages implementation crates to expose the block RNG types, while I believe it should be done only in special circumstances (like chacha20).

#30

It is significantly worse than simply deriving serde traits for the core type and adding a with shim for the block buffer type.

[dhardy]

As I wrote before, I don't think we need the Generator trait.

Well, it looks like we do (#35). Assuming that we want to wrap the buffer and constrain it to [W; N], I don't see another way of supporting Zeroize in dependent crates (without depending on it in this crate).

The difference is that existence of BlockBuffer encourages implementation crates to expose the block RNG types, while I believe it should be done only in special circumstances (like chacha20).

True. ReseedingRng uses this. I don't know if Generator cores have any other uses, but I don't think exposing them is necessarily a bad thing. It's also optional (aside from where required for thread_rng).

I'm still not 100% sure on whether we should include CryptoGenerator (it's used by ReseedingRng but arguably not enormously useful), but I'll leave it for now.

[newpavlov]

Well, it looks like we do

No, we don't. We can use zeroize_flat_type on the chacha20 side if we are to promise the necessary pre-conditions in BlockBuffer docs (it can be as simple as a guarantee that it's a wrapper around [W; N]). Frankly, I don't like your solution, it feels really hacky and backwards.

[dhardy]

Okay, so there is an unsafe solution using BlockBuffer (with #[repr(transparent)]).

Frankly, I don't like your solution, it feels really hacky and backwards.

At this point I'm not sure how to take this criticism. Tell you what, I'll implement the rest of my planned changes then we can compare again. It would be useful to have a third-party opinion.