Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could rand changes slow down and simplify the API before making new versions? #704

Closed
hdevalence opened this issue Jan 24, 2019 · 16 comments

Comments

@hdevalence
Copy link

Hi there,

apologies if this has already been discussed in other places, but there's a lot of issues and discussion and it's hard to keep track as an outsider.

Just from the point of view of (this) user of the rand crate, the changes are kind of hard to keep track of, and it feels like there's a lot of churn without really making fundamental changes.

Here are some comments, from my perspective, for consideration:

Ideally, my preference would be for the rand crate to expose only two RNGs:

  • an OsRng, which calls a secure platform API to get random bytes, or fails closed.
  • a ThreadRng, which uses a secure, well-studied construction internally, such as AESNI when available, or ChaCha20 otherwise.

Any cases where people really care about performance (e.g., someone needs to generate a lot of floats) are probably cases where AESNI is available, and since AESNI costs <1 cpb, there should not be a performance concern.

The RngCore trait could also be simplified, removing the next_u32 and next_u64 methods. In this way, the core RNG functionality can be byte-oriented, and all other functionality can be layered on top by means of extension traits.

Also, the semver trick is really difficult to get right, and subtle interactions cause a lot of unexpected breakage. My preference would be to get the API right, and then make a clean break.

@hdevalence
Copy link
Author

Just to be clear, this is feedback only from my perspective as a crate user, I'm not claiming that this is the be-all, end-all perspective, just writing it down for consideration.

I'd be happy to help make progress towards these changes if that's the direction the rand crate wants to go in, but as I mentioned, right now the overarching plan isn't really legible to me.

@huonw
Copy link
Contributor

huonw commented Jan 24, 2019

There's a tension between cryptographic uses of rand and non-cryptographic uses (e.g. simulation and testing). It's true that most cryptographic uses are fine with generating byte arrays, but next_u32 and next_u64 are very useful for performance, for non-cryptographic ones: it is likely a lot of overhead for an PRNG like xorshift or PCG (that generate u32 or u64 "natively") to generate bytes into a slice of runtime length, only for those bytes to be immediately turned back into a u32 or f32, etc.

@dhardy
Copy link
Member

dhardy commented Jan 24, 2019

A lot of thought went into the design of RngCore already, but if it looks like it's trying to do two things at once, that's because it is. Unfortunately the compiler doesn't appear to be able to optimise away the overhead of using a byte-oriented API for word-sized data, and last time we checked it did make a big performance difference.

We could possibly use two levels of API: crypto-oriented byte-only, then general-purpose RNG — but this would be more complexity, not less.

OsRng is currently being revised; see the new getrandom crate.

As to a crate exposing only ThreadRng, I've suggested it before but there was never much interest. It's a possibility.

Why bring up the algorithms here? There are other threads on that, and it has nothing to do with the APIs.

We deliberately do not keep the versions in sync between crates to minimise the effort needed to update. The rand_core crate is fairly stable, and unlikely to see big changes (except possibly to the error types). The rand crate is less stable and still needs a fair amount of work.

@hdevalence
Copy link
Author

last time we checked it did make a big performance difference.

Okay, if that's the case, it's the case; a pure byte-oriented API isn't essential (although an Rng implementation could just fill a buffer with bytes and consume it, rather than calling fill_bytes every time).

OsRng is currently being revised; see the new getrandom crate.

That's great, is the plan for the getrandom crate to be used by the rand-provided OsRng, or are users supposed to know about it and choose it on their own?

As to a crate exposing only ThreadRng, I've suggested it before but there was never much interest. It's a possibility.

I didn't suggest that there should be a crate exposing only ThreadRng; I suggested that the rand crate should provide only two RNGs:

  • one secure RNG implementation that pulls from a platform source;
  • one secure RNG implementation that uses a fast, secure userspace implementation.

This is a suggestion about the rand crate itself, suggesting that it provide secure defaults only.

Why bring up the algorithms here?

I brought up algorithms because they do matter, and because there exist secure algorithms that provide exceptionally high speed (< 1 cycle per byte), obviating the need for the rand crate to provide other, insecure implementations. There's no need for rand to provide non-cryptographically secure RNGs if the secure ones are fast enough, and I would prefer if the focus was on making that the case.

We deliberately do not keep the versions in sync between crates to minimise the effort needed to update.

With respect, I and others see a lot of breakage, so, as a user, this decision doesn't seem to actually translate into minimized effort.

@dhardy
Copy link
Member

dhardy commented Jan 25, 2019

That's great, is the plan for the getrandom crate to be used by the rand-provided OsRng, or are users supposed to know about it and choose it on their own?

We only want to implement the OS stuff once so we will re-use the implementation. Once it is ready we will of course advertise the new function.

@dhardy
Copy link
Member

dhardy commented Jan 25, 2019

This is a suggestion about the rand crate itself, suggesting that it provide secure defaults only.

Opinion noted. Now go do a search and see why some people are using SmallRng or even XorShiftRng. Not everyone needs a crypto RNG and some users want a small simple RNG. This is a "rand" crate, not a "crypto" crate, though I do sometimes wonder if trying to be both is a mistake.

High speed on aggregate usage isn't all we look for in algorithms; low memory usage and highly predictable performance also have their perks. Most crypto algorithms are optimised to produce large blocks of data and not for other uses.

With respect, I and others see a lot of breakage

What kind of breakage?

@newpavlov
Copy link
Member

Note that you can already use rand_os which exposes OsRng and depends only on rand_core and it will be a backwards compatible change, and I've already submitted PRs with those changes: dalek-cryptography/curve25519-dalek#222 and dalek-cryptography/ed25519-dalek#68, I am not sure if going all way down to getrandom will be sensible in your case, as your code is generic over RngCore in several places.

I am not sure why next_u32 and next_u64 bother you, if you are not using them they shouldn't be a problem, no? For example RDRAND produces 64-bit integers, so having default implementations saves some boilerplate and also helps non-cryptographic RNGs as well.

I think we should expose ThreadRng as a separate crate in future, but it will be blocked on re-working EntropyRng or dropping it altogether.

@dhardy
Maybe we could start an experimental rand_thread crate which will use OsRng instead of EntropyRng?

@dhardy
Copy link
Member

dhardy commented Jan 25, 2019

@newpavlov I'd rather get this getentropy working correctly first with integrated RDRAND, then stop using JitterRng and deprecate EntropyRng. You're still working on getentropy I take it?

@newpavlov
Copy link
Member

Yes, I will try to create a PR on this weekend.

@oleganza
Copy link

oleganza commented Jan 25, 2019

image

E.g. #705
image

@dhardy
Copy link
Member

dhardy commented Jan 28, 2019

Fair point, we did deprecate the old ThreadRng path in 0.6.0 (only released in November). And we are considering changes to EntropyRng and JitterRng, though I assume the OP of this thread doesn't care about those.

I think the answer to your question in the title can only be no. (I'm not actually sure how to simplify without making new versions.) In any case, we still try to support Rand back to 0.4 (even with a new 0.3 release recently), so there shouldn't be too much pressure to upgrade, however all versions since 0.5 support the same core RngCore trait (via upgrade shims on rand_core).

There are already two threads on potential changes to the ThreadRng algorithm: #463, #299. If you wish to contribute, please see those discussions; I don't see any need for further discussion on that here (and can't tell from your attitude whether you consider this an urgent topic or not).

The only remaining point I see here is regarding wanting only ThreadRng without the rest of Rand's functionality. I just argued against the need for a separate crate for this in #713 and wish to see some reduction in the features included in rand (#290), though I am still open to considering making a dedicated rand_thread crate: #716

Is there anything else to take away from this issue before closing it?

BTW: tracker for 0.7 features: #715

@BurntSushi
Copy link

Here is an example of breakage that someone else ran into: BurntSushi/quickcheck#228

(I don't know how they got into that state, and I'm not 100% sure on my diagnosis, but it certainly seems related to the semver trick.)

@oleganza
Copy link

oleganza commented Feb 15, 2019

Recently our main branch broke unexpectedly even tho this code was not touched in weeks:

image

Edit: i'm investigating this, may be not directly related to rand.

@newpavlov
Copy link
Member

@oleganza
Does Scalar comes from curve25519-dalek? Recently it has added support for both owned and borrowed RNGs (original PR is dalek-cryptography/curve25519-dalek#219). Looks like this change was not as backwards compatible as was initially believed.

@hdevalence
Copy link
Author

hdevalence commented Feb 15, 2019

Yes, that problem was caused by my changes, it's not related to this issue. The new trait bound uses RngCore to be uncoupled from the rand crate; I believed that the change would be backwards-compatible because the new trait bound was strictly more general than the old one, and tested it on some crates, but there's an additional interaction that wasn't caught in any of the crates I tested with.

@dhardy
Copy link
Member

dhardy commented Mar 23, 2019

For those who want to use it, the getrandom crate is now available. To those who want RNGs but none of the algorithms, we already have much of this available outside the rand crate, e.g. traits in rand_core, a system interface in rand_os and PRNGs in rand_chacha etc., though not everything: please comment in #716 (separate rand_thread crate) and #760 (system interface and fallback) if you are interested in these topics.

You may also be interested to know that the rand_core error type may have breaking changes in the name of simplification (rust-random/getrandom#3 and #715). Hopefully this will be more helpful than a nuisance; unfortunately we cannot improve without some breakage.

@dhardy dhardy closed this as completed Mar 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants