dyn RNG

[ounsworth]

This change set makes the RNG trait dyn-compatible and also leverages it within ML-DSA and ML-KEM so that keygen() and encaps() have versions that can accept a &dyn RNG and source their randomness from that.

This change brings bc-rust a bit closer to bc-java.

[romen]

Is passing &mut dyn RNG really necessary?

Rather than

fn keygen_from_rng<R: RNG>(rng: R) -> Result<(PK, SK), KEMError> { ... }

Committing to dynamic dispatch is not necessarily always evil, but does prevent the compiler to optimize as much, and does have some runtime performance penalty due to vtable indirection to resolve the dispatching.

On the other hand, if you intentionally want to support RNG implementations that are known only at runtime and not at compile time, a dyn RNG ref is the way to go.

[romen]

I am not sure this whitespace change is intentional!

Also s/comment/common/

[romen]

See https://doc.rust-lang.org/std/keyword.dyn.html for a terse review of the tradeoffs

[Quant-TheodoreFelix]

The choice of &mut dyn RNG seems appropriate for this code path. While @romen point about the cost of dynamic dispatch is generally correct, it is almost negligible here, and there are actually several reasons why dyn is advantageous in this case. These can be summarized as follows:

1. The performance impact is negligible. keygen/encaps call the RNG once at a coarse granularity, after which they perform thousands of cycles of lattice operations such as NTT and sampling. The vtable indirect call cost is only a few nanoseconds, while the subsequent work takes microseconds to milliseconds. The problematic case for dynamic dispatch is when the RNG is called in a tight inner hot loop on a per-byte basis — this calling pattern is not that.

2. dyn appears to be better for binary size. keygen/encaps are already monomorphized per set of const generic parameters. Adding a <R: RNG> generic on top would multiply the code size by the number of concrete RNG types used. For a no_std/embedded-oriented library, using dyn is the better choice to prevent code bloat. This is a clear advantage of dyn.

3. It aligns with runtime RNG selection and bc-java compatibility. The very goal of this PR — bc-java parity — presupposes “RNG determined at runtime.” &mut dyn RNG / Box<dyn RNG> is the natural expression for this, whereas generics force the type to be fixed at compile time.

4. There is no loss in usability at call sites. Callers with a concrete RNG type can simply pass &mut concrete_rng, which is automatically coerced (unsized) to &mut dyn RNG. Even those who want static dispatch lose nothing at the call site.

That said, if we really want to leave room for static dispatch, a common idiomatic compromise is to keep the public methods generic over <R: RNG + ?Sized> and delegate to a single dyn implementation. However, due to point 4 above, the practical benefit is small. Additionally, since this PR chose the opposite approach (+ ?Sized generics) in Sp80090ADrbg, the same RNG crate now contains two different conventions. It would be good to either unify them or document the intent clearly, e.g., “RNG is a public abstraction that requires object safety, while Sp80090ADrbg is for internal use and keeps monomorphization.”

Conclusion: There is no need to revert dyn RNG. However, I recommend following up with cleanup items such as unifying the RNG injection surface on the core trait, handling Default guarantees, ensuring from_rng consistency with ?Sized, and separating the symmetric-key traits.

[ounsworth]

Thanks @romen . I am still very much wrapping my head around how dyn works. This discussion is helpful.

I hadn't really considered that these two versions are alternate ways to achieve the same behaviour, but with some different tradeoffs

    fn keygen_from_rng(rng: &mut dyn RNG) -> Result<(PK, SK), KEMError> {
        let mut seed = KeyMaterial::<64>::new();
        rng.fill_keymaterial_out(&mut seed)?;
        Self::keygen_from_seed(&seed)
    }

vs

    fn keygen_from_rng<R: RNG>(rng: R) -> Result<(PK, SK), KEMError> {
        let mut seed = KeyMaterial::<64>::new();
        rng.fill_keymaterial_out(&mut seed)?;
        Self::keygen_from_seed(&seed)
    }

My feeling is that Dyn is correct here; especially if we expect this to be highly dynamic at runtime -- for example if the calling application wakes up, looks around, decides what kinds of OS or HW RNGs are present on its system, and then chooses an RNG impl to provide to the alg. I also think that Dyn will play nicer with RNGFactory, which again is an attempt to abstract away the concrete type
https://github.com/bcgit/bc-rust/blob/main/crypto/factory/src/rng_factory.rs#L61

But this is great. Please continue educating me on the tradeoffs!

[ounsworth]

@Quant-TheodoreFelix I have to ask, are all your comments AI-generated?

I find this comment very odd:

Conclusion: There is no need to revert dyn RNG. However, I recommend following up with cleanup items such as ... separating the symmetric-key traits.

That is referring to a TODO comment that I added in this PR, that is completely unrelated to this discussion, so why would you "recommend" that?