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

Use OsRng for thread_rng. #133

Closed

Conversation

tomprince
Copy link
Contributor

Fixes #78.

@bluss
Copy link
Contributor

bluss commented Jan 9, 2017

I think we need to examine this for performance impact.

@alexcrichton
Copy link
Contributor

Yes unfortunately when we've done this in the past the performance has been unacceptably slow afterwards, unfortunately. @tomprince did you find evidence to the contrary, however?

@alexcrichton alexcrichton added the E-question Participation: opinions wanted label Jun 14, 2017
@dhardy
Copy link
Member

dhardy commented Aug 17, 2017

@alexcrichton what precisely has unacceptable performance? This change has been argued for quite a bit in the crate evaluation thread.

If we're talking about specific applications, we may still want to use OsRng by default and let those applications decide. If we're talking about usage by the standard library, e.g. to randomise hash functions, then how big a deal is this?

@alexcrichton
Copy link
Contributor

@dhardy historically the thread_rng performance has affected HashMap::new in the standard library, which IIRC was the main motivation for caching it. This dates way far back but the tl;dr is that if getting system randomness is slower than periodically reseeding a user-space RNG, so if you have a core piece of functionality that relies on getting random numbers a lot then using the OsRng will slow you down a lot.

Now that's mainly something to deal with the standard library, and it's probably the case no matter what that we'll never switch HashMap over to the OS RNG by default. That being said, the decision there in the standard library doesn't necessarily imply a correct decision for this crate! We'd just want to explicitly document that if you need a lot of random numbers really quickly you're in a "niche" case and provide a path to a different RNG.

@arthurprs
Copy link
Contributor

arthurprs commented Aug 18, 2017

Can we not do that? No matter the origins, this will lead to catastrophic performance in some programs as people use thread_rng liberally as an almost secure but still fast rng.

@burdges
Copy link
Contributor

burdges commented Aug 18, 2017

I'd propose this approach :

Abstract ThreadRng as some new type ReseedingRng<Out: Rng+SeedableRng,Src> that instantiates an Out and periodically reseeds it from Src where Src: Rng or preferably Src: CryptoRng. Initializing ReseedingRng might fail when Src: CryptoRng, so ReseedingRng::new(src: Src) -> Result<ReseedingRng,Src::Error> or whatever. After Initialization ReseedingRng should handle any errors produced by Src by running Out longer. I donno if Src blocking requires any cleverness.

Replace IsaacWordRng in StdRng with some cryptographic CSPRNG, maybe some fast Keccak or ChaCha variant, but permit different platforms to use different PRNGs. Now set ThreadRng to be a wrapper around ReseedingRng<StdRng,OsRng> that employs the thread_local! trick. Add a try_thread_rng() function because the ReseedingRng::new might fail, but make thread_rng() unwraps it.

Add an FastThreadRng that wraps roughly ReseedingRng<IsaacWordRng,ThreadRng> or whatever for people who only want extremely fast random numbers.

I think this provides a ThreadRng that runs reasonably fast, reasonably securely, does not panic, and rarely blocks for the common use case of seeding data structures like HashMap, but keeps it simple to optimize for specific use cases.

@dhardy
Copy link
Member

dhardy commented Aug 19, 2017

@burdges given that the current thread_rng already reseeds from OsRng and panics on error, I'm not sure your design achieves much else, other than using a different PRNG, and try_thread_rng (extremely unlikely ever to error).

@dhardy
Copy link
Member

dhardy commented Dec 8, 2017

Closing (see reason in #78).

@dhardy dhardy closed this Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants