[Discussion] rand::RngCore for blocking RNG trait

[fmckeogh]

The RngCore trait seems like a good candidate for a blocking RNG trait. It is minimal, already in fairly wide use and no-std.

[eldruin]

Good question.
Here the alternatives:

1. We could reexport (some of) the traits from rand_core:
   • 👍 Widely used traits, seems mostly stable.
   • 👍 Maintained by Rust team itself.
   • 👎 External dependency.
   • 👎 rand_core is not stable yet so it would hold back embedded-hal 1.0.
   • ❓ Too much complexity?
   • ❓ Reexport everything or only a subset?
   • ❓ Remove embedded_hal::rng?
2. We encourage direct dependency on rand_core for HAL/HWRNG impls like we do with e.g. embedded-dma:
   • 👍 Flexibility for HAL impls to choose rand_core version.
   • 👎 Fragmentation in rand_core versions across ecosystem.
   • ❓ Too much complexity for HAL/HWRNG impls?
   • ❓ Remove embedded_hal::rng?
3. Keep embedded_hal::rng as-is:
   • 👍 No dependencies.
   • 👎 Separate RNG trait to what the Rust community is already using detriments interoperability.
   • 👍 Much simpler than rand_core traits.
   • ❓ Too simple?

I am unaware about how many implementations of embedded_hal::rng::Read there are. As of 0.2.x it is marked as unproven.
Does somebody know if it is being used?

I think I like option 2 the most for now, including removing embedded_hal::rng if there are no users. Then in a future release maybe do option 1.

Opinions from hardware RNG implementers would be helpful here.

[therealprof]

Funny enough I had the exact same trait definition and implementation in my nrf51-hal which seems to have gotten dropped by the consolidation of all nRF HALs. This one is implementing rand_core now: https://github.com/nrf-rs/nrf-hal/blob/master/nrf-hal-common/src/rng.rs

[eldruin]

stm32l4xx-hal does implement embedded_hal::rng::Read. See here
cc @nickray

[ryankurte]

hmm, all interesting options! it's definitely neat to have consistent rand everywhere. i am also perhaps a little cautious about the weird breakages we might see trying to match libraries with different rand_core versions (like, e-h versions is already a constraint, e-h and rand versions with the hal is going to make things even trickier, and it looks like they don't semver trick the rand-core crate...

in related things there's also getrandom which is great for, making rand just work but not so good for binding in peripherals, and i ran into a bunch of problems actually using the rng that ended in rand-facade, it'd be neat to discuss / document how to effectively use rngs as part of any improvements.

[nickray]

No strong opinions; I did also implement this unproven trait in our LPC55 HAL, just to not reinvent the wheel. Practically, all I need is a method to fill byte buffers with entropy, with the ability to ignore errors.

Might it be possible to nudge rand_core towards a v1? I do fear a bit of a mess and tangle for what's essentially filling a byte buffer. Personally, despite "doing cryptography" I've avoided the rand ecosystem so far.

In my opinion though RngCore makes the mistake of not making up its mind whether fill_bytes/try_fill_bytes can fail or not (the documentation's example of "reading a file of entropy" seems both contrived and misguided). By definition, a CSPRNG is supposed to transform a good entropic seed (the goodness of which is untestable) into random bits "forever", whereas a hardware TRNG typically can perform a self-test on startup (so the fallibility would be in the constructor - which if anything would be a separate trait NewRng) and then doesn't really have a reason to suddenly start failing. I understand that the RngCore trait wants to "offer both", but what's a driver supposed to use or rely on?

I'd tend to prefer an infallible method for these reasons, but certainly someone is going to come up with some valid corner case on embedded Linux or something 😅.

In any case, if the consensus is for a fallible method, then #229 seems applicable, I don't think NonZeroU32 without additional interpretation is a particularly useful error type.

cc @tarcieri as it would be great if the eventual trait integrated nicely with RustCrypto.

[tarcieri]

Outside the embedded space, rand_core::OsRng provides an infallible wrapper for the fallible getrandom crate. That approach seems to work well enough.

We've adopted rand_core pretty ubiquitously in the https://github.com/RustCrypto crates. I also wish it were 1.0 and it's been something of a complication in other 1.0 crates (e.g. curve25519-dalek), but we've managed to live with that so far.

[therealprof]

In yesterdays meeting we mostly hovered around going option 2 with the possible path to go option 1 in the future if things get too wild.

Personally I think if there's a perfectly fine universal solution like rand_core it doesn't really make sense to re-export it unless we manage to create a good chunk of value-add on top of it. It certainly makes sense to nudge the maintainers into stabilizing it for good to get rid of the moving target.

[therealprof]

This is basically waiting for someone to PR the removal of embedded_hal::rng traits and adding some documentation/pointers on how to use rand_core instead.

[tarcieri]

I think you'll be a lot better off not re-exporting it, as it's one of the most common triggers of this bug:

rust-lang/cargo#7916

The std feature of rand_core is activated by all sorts of commonly used dev-dependencies (e.g. criterion, proptest), and until features=dev_dep (rust-lang/cargo#7916) is stable it's a giant headache.