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.

Sign up for GitHub

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

Jump to bottom

Replace rand with rand_core+rand_os #222

Merged
merged 2 commits into from
Feb 14, 2019
Merged

Replace rand with rand_core+rand_os #222

merged 2 commits into from
Feb 14, 2019

Conversation

newpavlov
Copy link
Contributor

You don't have to depend on the whole rand, instead rand_core and rand_os can be used.

@hdevalence
Copy link
Contributor

What's the benefit of depending on rand_core+rand_os over rand?

Also, is it possible for us to make this change without breaking compatibility? Since the rand trait is part of our public API, I'm not sure we can do this without bumping the major version (which we're planning to do at the next time the rand crate is updated, but hopefully not beforehand)...

@newpavlov
Copy link
Contributor Author

newpavlov commented Jan 9, 2019 •
edited
Loading

rand_core and rand_os are much smaller crates, which were introduced in part specifically for crates like this one. In addition to being more lightweight, using those crates will make it easier to review the whole dependency tree of your crates. Also API surface is significantly smaller (especially comparing rand_core vs rand), so they will have breaking changes much less often.

I think this PR is backwards compatible, as Rng is an extension trait of RngCore. And IIUC OsRng is an inner implementation detail, so it does not matter from where it comes. Essentially this PR just removes wrapper layer, as rand currently uses those crates and just re-exports/extends them.

@tarcieri
Copy link
Contributor

tarcieri commented Jan 10, 2019 •
edited
Loading

What's the benefit of depending on rand_core+rand_os over rand

See rust-random/rand#648. The rand crate presently pulls in a small zoo of subcrates including non-cryptographically-secure RNGs as well as “90s crypto” RNGs

@newpavlov newpavlov mentioned this pull request Jan 25, 2019
Closed
@hdevalence hdevalence changed the base branch from master to develop February 14, 2019 20:19
@hdevalence hdevalence merged commit 4e47303 into dalek-cryptography:develop Feb 14, 2019
@hdevalence
Copy link
Contributor

Thanks! This should be included in 1.1.0-pre.0, published as a prerelease to test that generalizing the trait bound doesn't cause problems.

@hdevalence hdevalence mentioned this pull request Feb 15, 2019
Closed
hdevalence added a commit that referenced this pull request Feb 15, 2019
@hdevalence
Revert #219.
9a62386 
See discussion at #232 , copied below:

`1.1` changed the trait bounds for `RistrettoPoint::random` and `Scalar::random`, see #222 and #219.

These changes have two benefits:
* they unlink us from the `rand` crate and make us depend only on `rand_core`;
* they allow passing both owned and borrowed RNGs.

The change was not supposed to be a breaking change, since the new bounds are strictly more general than the old ones (as every `RngCore` is an `Rng` and every `&mut RngCore` is an `RngCore`), so the new bound is satisfied in every situation where the old bound applied.

The `1.1.0-pre.0` version didn't cause problems on the crates I tested it on, but there was an unexpected problem: https://github.com/interstellar/slingshot/blob/ce71c93a9a29ac3b4f69ce71feb987bd64d6c4ec/spacesuit/src/value.rs#L160-L161 broke, since it took a borrow as input and used it twice. So there was slight breakage.

One option is to revert the changes (probably just the ones from #219) and release 1.1.3; another would be to fix up `slingshot` and leave the new bound.
pinkforest pushed a commit to pinkforest/curve25519-dalek that referenced this pull request Jun 27, 2023
@tarcieri
ed25519 v2.0.0-pre.0 (dalek-cryptography#222)
d4cffc7 
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newpavlov @hdevalence @tarcieri