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

Tracker: Rand 0.7 #715

Closed
20 of 22 tasks
dhardy opened this issue Jan 28, 2019 · 22 comments
Closed
20 of 22 tasks

Tracker: Rand 0.7 #715

dhardy opened this issue Jan 28, 2019 · 22 comments
Labels
X-tracker Type: tracker
Milestone

Comments

@dhardy
Copy link
Member

dhardy commented Jan 28, 2019

Add getentropy crate:

  • design API, including error type
  • port OsRng code
  • port RDRAND code
  • make rand_os a wrapper around this
  • remove dependence on JitterRng by default

Target: resolve #648, #699, #681

Follow-up changes:

Clean up dependencies:

Distributions:

Sequences (may slip to later release):

Extras which may slip to later versions:

@vks
Copy link
Collaborator

vks commented Jan 28, 2019

If ChaCha becomes fast enough, we could consider deprecating SmallRng and instead suggest the appropriate rand_* crates.

@dhardy
Copy link
Member Author

dhardy commented Jan 28, 2019

Off-topic, and ChaCha is not very Small. We could consider just deprecating it though.

@vks
Copy link
Collaborator

vks commented Jan 28, 2019

It is 8.5 times larger than SmallRng.

@newpavlov
Copy link
Member

Since there are no separate issues yet I will voice my opinion here:

If the above makes EntropyRng redundant, should we remove it?

Considering that platform support and ability to overwrite system RNG will be moved to getrandom, and that we no longer need Jitter fallback I think EntropyRng is completely redundant and can be safely removed.

Should we remove the FromEntropy trait but add similar functionality into SeedableRng (i.e. make rand_core depend on getentropy?

Since getentropy will be foundational either way and implements dummy fallback for unsupported platforms, I think rand_core can depend on it without any problems, so FromEntropy becomes redundant as well and can be removed. It may be a mild annoyance for PRNG crates to transitively depend on crates like libc or winapi, but I guess not much harm in that.

Given that getentropy needs an error type, should we remove or revise the one in rand_core?

It's a bit of shame to remove rand_core::Error after amount of effort which was put into it, but I guess there is not much reason to keep a second error type, especially considering that OsRng will be a thin wrapper around getrandom.

@dhardy
Copy link
Member Author

dhardy commented Mar 7, 2019

On that topic, having a backup source in Rand may no longer be necessary but is still a nice thing to have. I would prefer using RDRAND over JitterRng now. So I wouldn't say EntropyRng is completely redundant, but it's also not especially useful.

On the other hand, we can't exactly make SeedableRng::from_entropy use a back-up source (unless perhaps we implement the RDRAND interface in rand_core), so EntropyRng is useless if we go this route.

There is also the question of where no_std compatibility fits into the picture: see rust-random/getrandom#4.

We could feature-gate the getentropy dependency (like std, default disabled in rand_core but enabled in rand). This avoids the extra dependency for only minor extra complexity.


Yes, a lot of effort was put into rand_core::Error, but it never quite met all requirements. One of the ideas was handling retry-on-interrupt in fill_bytes but not in try_fill_bytes which OsRng does, but getrandom's current API does not allow this to be implemented properly, and it's probably not very useful anyway. ReseedingRng does something similar. So if we simplify rand_core::Error, then RngCore::fill_bytes is redundant.

Currently JitterRng also uses this error type, but I guess this isn't very important.


To some up: there are a few steps here that would allow us to simplify Rand (the API at least). Should we take them?

@newpavlov
Copy link
Member

newpavlov commented Mar 11, 2019

may no longer be necessary but is still a nice thing to have.

I believe that without clear examples of non-pathological OS RNG failures we should not be considering including fallback by default at all. In other words today's OS RNGs are already "just work" for all common platforms. And even if such examples would have been present, fallback should be considered only on per-target basis. Also I would repeat again, but I don't think it's a good idea to give a preferential treatment to x86-64 targets by hardcoding RDRAND into the most fundamental layer.

We could feature-gate the getentropy dependency (like std, default disabled in rand_core but enabled in rand).

Yes, we already have std feature in rand_core, so it will be reasonable to add getrandom as an optional dependency and enable it for std (this way it will be also possible to enable from_entropy even for non-std targets like SGX).

handling retry-on-interrupt in fill_bytes but not in try_fill_bytes

We could add Error::is_retryable method which will match on error code and vary between platforms. By default it will match on a third error constant, so custom sources will be able to use this code for signalizing that getrandom call can be retried. But I think we better to continue this discussion in the relevant issue.

there are a few steps here that would allow us to simplify Rand (the API at least). Should we take them?

I think yes, we should do it.

BTW maybe later we should create an announcement on reddit and users forum with planned rand v0.7 changes to gather a wider feedback?

@dhardy
Copy link
Member Author

dhardy commented May 15, 2019

Progress report: we've been making good progress with the distribution work, and were also able to get a faster ChaChaRng (#789). Once #792 and #793 land (soon) a large part of the to-do list will be complete.

The remaining blocking issue is #771 on which @newpavlov and myself have yet to reach an agreement (since he hasn't replied in four weeks, at this point I assume he has lost interest).

@dhardy
Copy link
Member Author

dhardy commented Jun 5, 2019

We are now close to being ready for a 0.7 pre-release. We should still visit #572 first IMO. Anything else I've missed? (The tracker is out of date; seems my last edit was lost.)

After that some work on documentation is needed (especially in the book); this can happen after the pre-release.

@vks
Copy link
Collaborator

vks commented Jun 5, 2019

I don't think there is anything else. Is #572 just about documentation?

@dhardy
Copy link
Member Author

dhardy commented Jun 5, 2019

It is, though in my mind it is also an API change (not a breaking one but still a significant one), so I'd prefer that done first.

@dhardy
Copy link
Member Author

dhardy commented Jun 5, 2019

Found another blocker: we need to make a rand_distr release. Is this ready enough? I was intending to release the straight export from Rand as 0.1 and then make 0.2 with our changes, but we don't have to. @vks would you look into this?

I'm currently checking other crates (including rand_chacha 0.2 and rand_core 0.5).

@vks
Copy link
Collaborator

vks commented Jun 5, 2019

@dhardy Done in #816.

@dhardy
Copy link
Member Author

dhardy commented Jun 12, 2019

The pre-release is out. Reddit thread: https://www.reddit.com/r/rust/comments/bzp5ib/rand_070_prerelease/

@huangjj27
Copy link

huangjj27 commented Jun 15, 2019

thread_rng seems not supporting wasm32-unknown-wasi on rust stable, with wasmer runtime(on master branch). What should I use when I targeting that?

@dhardy
Copy link
Member Author

dhardy commented Jun 15, 2019

Please take a look at rust-random/getrandom#34, and if it doesn't help open an issue/PR on the getrandom repo. It may be that we should support both wasm32-unknown-wasi and wasm32-wasi?

@huangjj27
Copy link

It may be that we should support both wasm32-unknown-wasi and wasm32-wasi?

worked with wasm32-wasi but not wasm32-unknown-wasi

@vks
Copy link
Collaborator

vks commented Jun 25, 2019

I think only #828 is missing for the release? It is probably a good idea to have another pre-release, so we can check whether docs.rs works now.

@dhardy
Copy link
Member Author

dhardy commented Jun 25, 2019

There's still some doc work needed in the book too.

@dhardy
Copy link
Member Author

dhardy commented Jun 28, 2019

0.7 has been published.

@vks would you make the reddit announcement? I don't have much time.

@dhardy dhardy closed this as completed Jun 28, 2019
@newpavlov
Copy link
Member

@dhardy
Maybe also make getrandom v0.1.4 release?

@dhardy
Copy link
Member Author

dhardy commented Jun 28, 2019

@newpavlov I just added you to the maintainers team — you should be able to make the release yourself.

@vks
Copy link
Collaborator

vks commented Jun 28, 2019

@dhardy https://www.reddit.com/r/rust/comments/c6j6ge/rand_07_release/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-tracker Type: tracker
Projects
None yet
Development

No branches or pull requests

4 participants