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

provide RandomMod::try_random_mod and Random::try_random methods #770

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

baloo
Copy link
Member

@baloo baloo commented Feb 19, 2025

@baloo
Copy link
Member Author

baloo commented Feb 19, 2025

I think this is fine as random_mod is rejection sampling and it's not constant time in the first place, but please confirm that.

@baloo baloo force-pushed the baloo/try_random_mod branch from 8e6435b to 3a48484 Compare February 19, 2025 20:44
@baloo baloo force-pushed the baloo/try_random_mod branch from 3a48484 to 2480249 Compare February 19, 2025 21:24
@newpavlov
Copy link
Member

We need to decide whether we need explicit ?Sized bounds or not. If they are not needed, then we need to modify rsa accordingly. Otherwise, you can revert my two commits.

@baloo
Copy link
Member Author

baloo commented Feb 19, 2025

I think crypto-primes relies fairly heavily on the ?Sized

@fjarri
Copy link
Contributor

fjarri commented Feb 19, 2025

So I started this whole ?Sized thing because it was mentioned in that RSA PR that switched to crypto-bigint. And also it seemed like we're not losing anything by relaxing this bound.

I have a use for it in another project where I need to pass an RNG through a dyn-safe trait API. Before it was available I just had a wrapper that boxed a dyn CryptoRngCore and could be used as a Sized argument, which was kind of inconvenient and added a minor performance overhead. That project also uses crypto-primes, so I relaxed the bounds there as well.

@newpavlov newpavlov force-pushed the baloo/try_random_mod branch from f98d5a7 to 2480249 Compare February 19, 2025 22:17
@newpavlov
Copy link
Member

As discussed above, the ?Sized bounds are indeed necessary, so I reverted my two commits.

@baloo baloo force-pushed the baloo/try_random_mod branch from 2480249 to c0e21cb Compare February 19, 2025 22:31
@baloo baloo changed the title provide a try_random_mod method provide RandomMod::try_random_mod and Random::try_random methods Feb 19, 2025
@baloo baloo force-pushed the baloo/try_random_mod branch from c0e21cb to 23329d2 Compare February 19, 2025 22:34
@baloo baloo force-pushed the baloo/try_random_mod branch from 23329d2 to 8b09ce8 Compare February 20, 2025 05:45
@Snowiiii
Copy link

When this is ready ?

@baloo
Copy link
Member Author

baloo commented Feb 20, 2025

I think it's mostly ready, but we might want to wait for a release of rand_core.

@baloo baloo force-pushed the baloo/try_random_mod branch 2 times, most recently from 894b780 to 481464e Compare February 20, 2025 17:42
@newpavlov newpavlov mentioned this pull request Feb 21, 2025
@baloo baloo force-pushed the baloo/try_random_mod branch from 481464e to 8c1c6c7 Compare February 21, 2025 17:26
@newpavlov
Copy link
Member

newpavlov commented Feb 21, 2025

@baloo
BTW we generally use "squash and merge" for all PRs, so having a clear commit history inside PRs is not very important. Force push also breaks links on the "notifications" page, resulting in a "couldn’t find those commits" page, and makes it a bit harder to track changes during PR development.

(I know that some consider it a sloppy approach, but in my opinion it's more convinient and less time consuming for a forge-centric development)

@baloo
Copy link
Member Author

baloo commented Feb 21, 2025

This last push was fixing a merge conflict with #772
I've switched to using Jujutsu a month or so ago, and it's less natural to keep piling commits with it than it would be with a straight git. Sorry for the notification noise.

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.

5 participants