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

Update rand and rand_core #794

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Update rand and rand_core #794

wants to merge 25 commits into from

Conversation

wysiwys
Copy link
Contributor

@wysiwys wysiwys commented Feb 6, 2025

This pull request updates the dependencies rand from 0.8 -> 0.9 and rand_core from 0.6 -> 0.9. It is based on dependabot's PRs (see #787 and #786).

Tests and benchmarks are also updated.

Occasionally, a feature (classic-mceliece in libcrux-psq) or a benchmark requires an older version of rand or rand_core. In this case, the older version of the affected crate is added to Cargo.toml with the name rand_old or rand_core_old, respectively.

  • Use new trait TryRngCore where necessary
  • Where applicable, use OsRng::try_fill_bytes() instead of OsRng::fill_bytes() (since OsRng no longer implements the CryptoRng trait, which provides this function).
  • To handle certain cases where try_fill_bytes() returns an error, add an Error::InsufficientOSRandomness variant

@wysiwys wysiwys self-assigned this Feb 6, 2025
@wysiwys wysiwys requested a review from a team as a code owner February 6, 2025 10:22
@wysiwys wysiwys marked this pull request as draft February 6, 2025 10:26
@jschneider-bensch
Copy link
Collaborator

jschneider-bensch commented Feb 10, 2025

It looks like many of the examples and tests expect OsRng to implement RngCore instead of TryRngCore. I think in many we can just .unwrap() on try_fill_bytes().
In some, we run into the issue that OsRng no longer implements CryptoRng, but TryCryptoRng. E.g. the top level ECDH API requires CryptoRng for key generation, but internally key generation already uses try_fill_bytes, so I think TryCryptoRng would be sufficient (at least in this case that I checked). But maybe that can be a separate PR, since it changes the public API. We can use unwrap_err in these cases for now to get a CryptoRng from a TryCryptoRng.

@franziskuskiefer
Copy link
Member

@wysiwys what's the state here? Do you need input or want someone else to take over?

@wysiwys
Copy link
Contributor Author

wysiwys commented Feb 18, 2025

@franziskuskiefer One thing I ran into that is holding back development on this branch is connected to a third-party crate, which is built on top of rand version 0.8. Specifically, there is a trait definition issue that arises when calling classic_mceliece_rust::keypair_boxed() in psq.rs.

To go into more detail, the inputs to that function must implement the rand::CryptoRng and rand_core::RngCore traits, but the versions of those traits from older versions of those libraries. However, in rand version 0.9 and the new, corresponding rand_core version, the definitions of these traits have been changed, which leads to a build error in this crate, since the input previously passed into it is no longer valid.

Once classic-mceliece-rust has released a new version, and depends on the latest version of rand, the development on the dependency update in this branch should go smoothly. In the meantime, is there a different solution or workaround that could be applied for this issue?

@franziskuskiefer
Copy link
Member

We can't keep everything back because there's a bad dependency in a small part.
Either you update everything but psq to 0.9, or everything including psq but add a second API entrypoint for mceliece that gets the old rng.

@wysiwys
Copy link
Contributor Author

wysiwys commented Feb 18, 2025

We can't keep everything back because there's a bad dependency in a small part. Either you update everything but psq to 0.9, or everything including psq but add a second API entrypoint for mceliece that gets the old rng.

Thanks @franziskuskiefer . I went with the option of adding a second entrypoint for mceliece, since psq depends on libcrux_kem (and then the same rand dependency issue would apply to it as well).

@wysiwys
Copy link
Contributor Author

wysiwys commented Feb 18, 2025

The only other changes to the crates' public APIs are replacing inputs that are &mut (impl RngCore + CryptoRng) with &mut impl CryptoRng, since CryptoRng is now a supertrait of RngCore.

@wysiwys wysiwys marked this pull request as ready for review February 19, 2025 09:11
@wysiwys wysiwys marked this pull request as draft February 19, 2025 09:14
@wysiwys wysiwys marked this pull request as ready for review February 19, 2025 12:51
@wysiwys
Copy link
Contributor Author

wysiwys commented Feb 19, 2025

Also thanks @jschneider-bensch for providing this workaround for the rand=0.8 dependency requirement for classic-mceliece-rust. I incorporated the commit from your branch into this pull request.

@keks keks self-requested a review February 19, 2025 14:22
Copy link
Member

@keks keks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good in general! I am a bit unsure around the generated code:

  1. The toolchain hashes are gone in the headers and codegen.txt, I don't think this is supposed to happen.
  2. This adds a header.txt, which seems like it maybe shouldn't be there?
  3. The rlimit in the F* code is different, is that correct?

@wysiwys wysiwys force-pushed the wysiwys/update-rand branch from 151b2a3 to a1975ae Compare February 19, 2025 14:58
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

Successfully merging this pull request may close these issues.

4 participants