search

divviup / libprio-rs

Implementation of Prio in Rust.
Mozilla Public License 2.0
98 stars 30 forks source link

Do we still need `getrandom`? #955

Open cjpatton opened 4 months ago

cjpatton commented 4 months ago

I recall I initially pushed for using getrandom in lieu of rand in order to have compatibility with WASM. First, I'm not sure this was ever technically necessary. Second, we no longer need WASM for Daphne: https://github.com/cloudflare/daphne/blob/b59507a0092e1267e7ec7c3f99c33b59bd1deb34/daphne_worker_test/Cargo.toml#L31-L34

What do you think about removing the dependency? cc/ @mendess

divergentdave commented 4 months ago

rand and rand_core rely on the getrandom crate with default features enabled, and that backs the OsRng implementation, so I think it's six of one or half a dozen of the other.

cjpatton commented 4 months ago

Right, but can we avoid using it directly here? I.e., could we replace getrandom() with thread_rng().fill()?

tgeoghegan commented 4 months ago

Dropping a direct dependency would be nice, but it sounds like this would have no other functional effect. On the other hand, we have a couple error enums with variants:

 /// Failure when calling getrandom().
 #[error("getrandom: {0}")]
 GetRandom(#[from] getrandom::Error)

That means that dropping getrandom is a public API change. Not one I'm opposed to per se, but we'd have to wait for prio 0.17.

cjpatton commented 4 months ago

I think it's worth doing, and yeah this could wait until 0.17.

divergentdave commented 1 month ago

Note that dropping our direct dependency on getrandom would now require removing our wasm-compat feature added in #1059. The README for the rand crate says that, for wasm32-unknown-unknown in a JavaScript environment, you have to turn on the getrandom/js feature separately from your rand dependency.

cjpatton commented 1 month ago

Looks like we're good to drop the "wasm-compat" feature in daphne: https://github.com/cloudflare/daphne/pull/613