Block generation & buffering API design

Status quo (derived from last release): we have

pub trait BlockRngCore {
    type Item;

    type Results: AsRef<[Self::Item]> + AsMut<[Self::Item]> + Default;

    fn generate(&mut self, results: &mut Self::Results);
}

pub trait CryptoBlockRng: BlockRngCore {}

#[derive(Clone)]
pub struct BlockRng<R: BlockRngCore> {
    results: R::Results,
    index: usize,
    pub core: R,
}

impl<R: BlockRngCore<Item = u32>> RngCore for BlockRng<R> {}

#[derive(Clone)]
pub struct BlockRng64<R: BlockRngCore + ?Sized> {
    results: R::Results,
    index: usize,
    half_used: bool, // true if only half of the previous result is used
    pub core: R,
}

impl<R: BlockRngCore<Item = u64>> RngCore for BlockRng64<R> {}

Issue: the results buffer is internal, which makes it hard to do things like implement serde support externally. #30 allows this; the result is somewhat ugly but acceptable in my opinion.

Issue: BlockRng64 shouldn't need to be separate from BlockRng (if we ignore the half_used optimisation), however Rust does not (yet) support deconflicting implementations over associated types (i.e. the two impls above are considered to conflict if both apply to BlockRng).

Issue: BlockRngCore::Results supports variable-sized buffer types like Vec<u32> which it really shouldn't.

Issue: BlockRngCore::Results requires a Default bound (for construction), but [u32; 64] doesn't (yet) support this. Both chacha20 and rand_chacha crates use a custom Array type to work around this. Rust should fix this in the future: rust-lang/rust#61415.

Issue: this is quite a lot of code for what is supposed to just be an RNG-interface crate.

Attempted design:

pub trait BlockRngCore {
    type Item: Word;

    const LEN: usize;

    fn generate(&mut self, results: &mut [Self::Item; Self::LEN]);
}

Problem: Rust doesn't support generic_const_exprs on stable yet. (I don't think the design even works on unstable Rust: it complains that the results: [R::Item; R::LEN] field is an unconstrained generic constant.)

Every design which attempts to precisely specify the buffer type hits this problem: it requires that support for generic const exprs.

We also can't constrain the buffer type to Array<u32> or Array<W, _> since there is no Array trait, hence why the status-quo uses the much looser bound AsMut<[W]>.

Possible design from #26:

pub trait Generator {
    type Result;

    fn generate(&mut self, result: &mut Self::Result);
}

This is a variation on the status quo which drops bounds on the Result type, moving bounds to implementations.

This succeeds in doing something the above attempted design does not: precisely specifying the buffer type. This fixes both issues pertaining to BlockRngCore::Results above.

Possible minor simplification: remove BlockRng64. Of our RNGs, only Isaac64Rng uses it, and this RNG is rather unimportant.

It isn't quite so simple to move the BlockRng64 code to the rand_isaac crate since it uses fill_via_chunks which is internal (though that and its Observable trait could be copied too).

Attempted design from #24: replace block module with helper fns:

pub fn new_buffer<W: Word, const N: usize>() -> [W; N] { /* .. */ }

pub fn next_word_via_gen_block<W: Word, const N: usize>(
    buf: &mut [W; N],
    mut generate_block: impl FnMut(&mut [W; N]),
) -> W { /* .. */ }

pub fn next_u64_via_gen_block<const N: usize>(
    buf: &mut [u32; N],
    mut generate_block: impl FnMut(&mut [u32; N]),
) -> u64 { /* .. */ }

pub fn fill_bytes_via_gen_block<W: Word, const N: usize>(
    mut dst: &mut [u8],
    buf: &mut [W; N],
    mut generate_block: impl FnMut(&mut [W; N]),
) { /* .. */ }

Issue: lack of type-safety for the buffer.

Issue: performance issues for u32 / u64 generation with rand::rng() due to ReseedingRng lacking direct access to a block-generation fn.

I think the main question is whether we want a trait for block generation or not. Its primary use is ReseedingRng and I am not sure it's a sufficient motivation. I have a rough idea of how we can implement it efficiently (albeit slightly ugly), but unfortunately I did not have time to work on it during previous week.

The motivation is to avoid a significant penalty to rand::rng() performance (reportedly 20-50% in rust-random/rand#1686). I believe we need some type of block generation trait to avoid that.

#24 was interesting, but personally I did not like the lack of type safety around the buffer. I briefly considered adding a Buffer struct, but the result is mostly the same as BlockRng.

I'm reasonably positive about #26 as a starting point. It can be worked further (e.g. integrating the index into the buffer, supporting serialization) but I'd prefer not to push that into the same PR.

I also think it's acceptable to just drop BlockRng64 (and u64 support). It would be nice if we could simply move it into rand_isaac (the only user), but the current code uses fill_via_chunks which is internal to rand_core.

@vks

@vks @josephlr
Could you chime in? It does not seem I and @dhardy to reach any kind of agreement here. And I am getting a bit frustrated with the one-sided unapproved PR merges (why even have the merge restriction in the first place then?).

I believe that we don't need BlockRng and (Crypto)Generator traits and the simple helper BlockBuffer type is sufficient. The problem with ThreadRng performance is more or less resolved in rust-random/rand#1686 by not relying on ReseedingRng (since ChaCha already stores block counter inside itself we do not need to track size of generated data separately). The new versions of ReseedingRng is still a bit slower, but since it's no longer an important type without it being used in ThreadRng, I think it's fine (also see this comment).

@newpavlov

For context, our disagreement started with #24 which makes a lot of changes at once — largely a rewrite of the le and block code. My requests for reviews in #26 and #34 were dismissive of the general direction (and noting omissions which I had already noted potential solutions for, but didn't wish to merge in the same PR). Since responses largely ignored changes within the PR itself and we have not agreed on the central question of whether to keep the block::Generator trait I decided to move ahead with my plans so that we can compare the end result with @newpavlov's proposal (I do not wish to push all my changes into one large PR).

As noted here, the BlockBuffer design requires unsafe code in chacha20.

Otherwise, the primary differences in approach appear to be whether to use the trait block::Generator as an interface and expose "core PRNGs" like ChaCha12Core vs passing a closure to BlockBuffer methods and hiding PRNG cores (with an exception in chacha20 for use by ThreadRng).

I opened #36 which is the last of my planned changes to block, though leaves le mostly unchanged. There are a bunch of changes to le in #24 that we might also want to merge.

Shall I go ahead and merge this then rebase #24? (The conflicts are easy to resolve.)

I'd still like to evaluate the non-block-changes in #24 separately from the block changes.

I don't see the point of splitting the changes in so many PRs. Everything except the serde removal PR could've been just one PR. It would've been easier for users to inspect and result in a cleaner history. And if in the end it will be decided to follow #24, all those PRs will do nothing more than pollute the commit history. This is why I would've preferred if we first decided on the approach to follow before merging any PRs.

As noted #34 (comment), the BlockBuffer design requires unsafe code in chacha20.

It's true for the current version of zeroize, but it could be easily amended by adding a black_box-like function. This way we will be able to write:

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

pollute the commit history

Really? getrandom has many PRs just to update Cargo.lock (one of the reasons I do not want to commit that).

#24 is a lot to review at once. I don't want to try because (a) it would be hard following the discussions of review comments and (b) every time you push changes there would be a lot of redundant re-reviewing (and no, reviewing only the changes since the last commit is often not enough). Pushing your test cases into doc examples only makes this harder to review in my experience.

It's true for the current version of zeroize, but it could be easily amended by adding a black_box-like function.

zeroize::observe doesn't exist yet. I guess it would work, but would be fragile: someone who doesn't understand the buffer-zeroing code could well assume that those two lines of code can be evaluated independently (especially without unsafe). zeroize_flat_type is arguably the better way to do this, if the caller cannot see the internals of the buffer type.

getrandom has many PRs just to update Cargo.lock

These PRs can be easily skipped by users just based on the name, while your PRs layer on each other. And my point about merging unapproved PRs (which may be overwritten by #24) still stands. If you single handily committed to the Generator trait just say so, otherwise the merges were arguably premature.

#24 is a lot to review at once.

~600 LoC out of which ~500 LoC are self-contained additions (out of which ~300 LoC are new docs) is "a lot to review"? We clearly have very different scales of "lot".

I could move the new docs into a separate PR if you prefer such "minimization", but IMO documentation should be added in the same PR which adds new APIs.

P.S.: We are not getting anywhere, so this comment probably will be my last here until others express their opinion.

Yes, I made the executive decision to merge the Generator PR in its current state. I haven't decided what to release yet, though I am leaning towards keeping the Generator trait.

I have merged #36.

Next, I would like to consider the subset of #24 not concerning block code. If you'd like to make a PR for that I would be delighted. You may move code to new modules if you wish, though please clarify what is moved vs new code.

Feel free to adapt whatever you want from my PRs based on your sole vision. They represent what I would've liked to see (well apart from the le -> utils renaming) and I do not plan to split them or work on them any further.

I will not block any future PRs in this repository to hopefully speed up the v0.10 release, but I probably also will not be submitting my approvals either, so feel free to just merge your PRs.

@newpavlov

Of the issues noted in the opening comment above (largely inferred from #24):

the results buffer is internal: the original impl of Replace block module with helper functions #24 did not use an internal buffer, but all subsequent approaches have moved away from this on the grounds that the original design had an obvious footgun (constructing the buffer with Default::default() would result in bad output), thus no current proposals solve this issue.
BlockRng64 shouldn't need to be separate from BlockRng: solved by both my code (merged) and Replace block module with helper functions #24
BlockRngCore::Results supports variable-sized buffer types like Vec<u32> which it really shouldn't: solved by both my code (merged) and Replace block module with helper functions #24
BlockRngCore::Results requires a Default bound: solved by both my code (merged) and Replace block module with helper functions #24
this is quite a lot of code for what is supposed to just be an RNG-interface crate: 8007948 (last before Replace block module with helper functions #24) had 762 lines of Rust + 444 lines of comments. Updated block_buffer branch has 545 + 535 lines. The current master has 587 + 445 lines.
(Not listed above but mentioned multiple times) Optimise buffer size by placing index in buffer[0]: done by both my code (merged) and master

Alternatives

Issue (1) implies another possible design we haven't tried: allowing usage of a naked [W; N] buffer without embedding the index in the buffer. This would of course not meet goal (6). It would also complicate helper functions since (1) the buffer, (2) the buffer index and (3) the generator closure would all need to be passed in as parameters (probably not a perf. issue due to inlining but certainly not a neat API).

`Generator` trait

I think the main question is whether we want a trait for block generation or not. Its primary use is ReseedingRng and I am not sure it's a sufficient motivation.

I believe that we don't need BlockRng and (Crypto)Generator traits and the simple helper BlockBuffer type is sufficient.

@newpavlov hit the nail on the head two weeks ago: this is the largest point of contention. I apologise for annoyingly want to look at all the other questions first, but I do like to be able to break multi-faceted issues down into goals and sub-components.

The biggest cost/benefit from removing block::Generator is that we'd need/want to remove ReseedingRng too. rust-random/rand#1686 shows that it's not strictly necessary for rand::rngs::ThreadRng and ReseedingRng doesn't have much other usage — a GitHub search shows some cases but most look unmaintained. It would also imply that ChaCha*Core should perhaps not be public (visible) exports.

I'm still unsure on this, though leaning towards keeping block::Generator.

It would potentially pair better with the above alternative (closer to the original #24 but without the index in the buffer) since the block_buffer branch (i.e. #24) would require an unsafe call to zeroize_flat_type in chacha20.

@vks @josephlr please feel free to chime in.

the block_buffer branch (i.e. #24) would require an unsafe call to zeroize_flat_type in chacha20.

It's not a big concern for chacha20 if rand_core is to promise that BlockBuffer is a wrapper around [u32/u64; N].
We are working on zeroize::optimization_barrier, which would allow us to implement buffer erasure as:

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

@tarcieri

To quote @tarcieri from the RustCrypto/utils#1252 PR:

I'm a bit worried about the notion that there's a user pulling in one crate which is expected to do some otherwise insecure zeroing, then separately pulling in zeroize to observe the writes the other crate is supposedly doing. Ensuring the entire operation is secure relies on implicit coupling between those two crates which the user is orchestrating, and the user has little way to tell if e.g. the zeroing operation in one crate has been removed and the observe is useless.

It's worse than just this: the result relies on all future editors of that code understanding the link between two operations without an obvious code link and without even unsafe to remind people to pay close attention. In my opinion it is a misguided approach to avoid usage of unsafe in chacha20.

@tarcieri

And the current approach with the Generator::drop is better? IMO it's even more coupled and convoluted. As you can see, @tarcieri could not understand how its application works from the first glance.

The quote discusses the snippet with inherent zeroize method which breaks the buffer invariant. It was a bad example from me and it's not worth to discuss it here.

The quote does not apply to the snippet above. The worst what could happen with it is that writing the default value will not overwrite everything in the destination (e.g. if we are to change the impl to use MaybeUninit). But it can be simply amended by promising in the type docs that the default value initializes all bits of the buffer state.

I do not understand what kind of "link" you are talking about. We overwrite the buffer field with the default value and "observe" it to prevent the compiler from removing the write. That's it.

I do not understand what kind of "link" you are talking about. We overwrite the buffer field with the default value and "observe" it to prevent the compiler from removing the write. That's it.

These two operations are each ineffective on their own, yet together they have some effect. (See also the linked PR.)

And the current approach with the Generator::drop is better? IMO it's even more coupled and convoluted.

Coupled? Yes. (Perhaps semantically it should be separate from block::Generator but realistically we don't have any use for another trait.) Convoluted? It's more code than straight up calling zeroize_flat_type on the buffer but less than writing a custom wrapper around buffer just to implement a Drop handler, so it's fine in my opinion.

The above is rather besides the point IMO however, which is that we should choose between:

Keeping "block generators" like ChaCha20Core as documented public exports of CSPRNG crates along with block::Generator as a formal interface over these. Additionally, keeping ReseedingRng as a public adapter
Hiding block generators (but still exporting ChaCha12Core as a hidden public item since it is used by rand::rngs::ThreadRng) and removing ReseedingRng (inlining functionality into ThreadRng).

I released https://crates.io/crates/rand_core/0.10.0-rc-3 with the new block::Generator API.

We have some motivation for keeping block::Generator: ThreadRng evidences a use for ChaCha12Core and Generator provides a description of this. (I'm less convinced that ReseedingRng should remain public, but that's a rand issue.)

I will leave this issue open until the v0.10.0 release in case anyone else wishes to comment.

Have you checked generated assembly for fill_bytes? I suspect it will be far from most efficient. The code in #24 was carefully optimized using generated assembly as a guide, but I guess you just went and ignored it...

This is what I find strangest about your approach actually: doing a lot of work on a PR significantly altering the API before getting buy-in on the general direction.

As for optimization, I just used benchmarks and a rough comparison between the code (including #24 which uses quite a lot more code for this function). Measured speed was good; see #38.

What does "most efficient" mean? Lowest instruction count? Theoretically fastest on some simple CPU, maybe?

altering the API

The public API is the same, I am talking about internal implementation of the fill_bytes method.

What does "most efficient" mean?

Absence of panics, minimal number of branches, smaller instruction count, efficient "hot" loop which enables optimization of writing generated blocks directly into destination buffer.

@josephlr

(Regarding the above on optimisations / assembly: see #40.)

@josephlr do you have an opinion on this or #41?

Many (but not all) of the noted issues have been solved and v0.10 is released; with that this issue is closed (but see also #71).

Navigation Menu

Repository navigation

Block generation & buffering API design #31

Alternatives

`Generator` trait

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Block generation & buffering API design #31

Description

Activity

dhardy commented on Nov 30, 2025

dhardy commented on Nov 30, 2025

dhardy commented on Nov 30, 2025

dhardy commented on Nov 30, 2025

newpavlov commented on Dec 2, 2025

dhardy commented on Dec 2, 2025

dhardy commented on Dec 2, 2025

newpavlov commented on Dec 10, 2025

dhardy commented on Dec 10, 2025

dhardy commented on Dec 10, 2025

newpavlov commented on Dec 10, 2025

dhardy commented on Dec 10, 2025

newpavlov commented on Dec 10, 2025

dhardy commented on Dec 10, 2025

newpavlov commented on Dec 15, 2025

dhardy commented on Dec 17, 2025

Alternatives

Generator trait

newpavlov commented on Dec 17, 2025

dhardy commented on Dec 17, 2025

newpavlov commented on Dec 17, 2025

dhardy commented on Dec 18, 2025

dhardy commented on Dec 22, 2025

newpavlov commented on Dec 22, 2025

dhardy commented on Dec 22, 2025

newpavlov commented on Dec 22, 2025

dhardy commented on Dec 31, 2025

dhardy commented on Feb 1, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`Generator` trait