adds a Field::try_from_rng method #127

baloo · 2025-02-26T18:46:49Z

This is stacked on #126

Depends: - zkcrypto/ff#126 - zkcrypto/ff#127 This is to provide an `ecdsa::SigningKey::try_from_rng` API (RustCrypto/signatures#915)

str4d · 2025-03-09T03:44:28Z

src/lib.rs

    fn random<R: RngCore + ?Sized>(rng: &mut R) -> Self;

+    /// Returns an element chosen uniformly at random using a user-provided RNG.
+    fn try_from_rng<R: TryRngCore + ?Sized>(rng: &mut R) -> Result<Self, R::Error>;


Concept ACK (this will address #109), but I want to understand the ?Sized change first (#126 (comment)).

Also, this change to the trait should be documented in the changelog.

str4d

Reviewed 94d4f18.

My main question is what approach should the API take:

Should we require trait implementers to implement both of these manually (as the PR currently does)? Are there platforms where the infallible RNG APIs are not simply wrappers around the fallible APIs?
Should this become the main API that gets implemented, and Field::random gains a default method that calls Self::try_from_rng(rng).expect("must not fail")?
- If this is what all downstream RNG APIs do, then we can do the same.
Should Field::try_from_rng have a default impl of Ok(Self::random(rng)) (making this a pure addition)?
- I like this option the least, as it hides a panic inside an API that claims it reports errors.

str4d · 2025-03-09T04:18:36Z

src/lib.rs

+    fn random<R: RngCore + ?Sized>(rng: &mut R) -> Self;
+
+    /// Returns an element chosen uniformly at random using a user-provided RNG.
+    fn try_from_rng<R: TryRngCore + ?Sized>(rng: &mut R) -> Result<Self, R::Error>;


Same as #126 (comment), ff-derive needs to be updated to generate the new API.

str4d · 2025-03-09T04:20:15Z

CHANGELOG.md

 ## [Unreleased]

+### Added
+- `Field::try_from_rng` method.


This is a change to the crate, not an addition, because it adds a new method to the Field trait without a default implementation.

baloo · 2025-03-09T05:29:27Z

Should this become the main API that gets implemented, and Field::random gains a default method that calls Self::try_from_rng(rng).expect("must not fail")?

If this is what all downstream RNG APIs do, then we can do the same.

yeah I went with that, the blanket TryRngCore we get has an Infallible error and made that explicit to protect the panic against a refactor.

str4d

utACK 339e82c once ff_derive comment is addressed.

As with #126 (review), this change will need to go through a zkcrypto RFC before we cut the final 0.14.0 release.

str4d · 2025-03-09T05:46:32Z

ff_derive/src/lib.rs


            /// Computes a uniformly random element using rejection sampling.
-            fn random<R: ::ff::derive::rand_core::RngCore + ?Sized>(rng: &mut R) -> Self {
+            fn try_from_rng<R: ::ff::derive::rand_core::TryRngCore + ?Sized>(rng: &mut R) -> core::result::Result<Self, R::Error> {


We always use ::crate_name notation to defend against the import decisions within the module in which the derive is used (see the rest of ff_derive for examples).

Suggested change

fn try_from_rng<R: ::ff::derive::rand_core::TryRngCore + ?Sized>(rng: &mut R) -> core::result::Result<Self, R::Error> {

fn try_from_rng<R: ::ff::derive::rand_core::TryRngCore + ?Sized>(rng: &mut R) -> ::core::result::Result<Self, R::Error> {

str4d · 2025-03-09T05:54:27Z

CHANGELOG.md

 - `ff::Field::random(rng: impl RngCore) -> Self` has been changed back to
  `Field::random<R: RngCore + ?Sized>(rng: &mut R) -> Self`, to enable passing a
  trait object as the RNG.
+- `Field::try_from_rng` added. It accepts a `rand_core::TryRngCore` mutable reference.


Suggested change

- `Field::try_from_rng` added. It accepts a `rand_core::TryRngCore` mutable reference.

- `ff::Field::try_from_rng` is a new trait method that must be implemented by

downstreams. `Field::random` now has a default implementation that calls it.

str4d · 2025-03-09T05:56:19Z

src/lib.rs

-    fn random<R: RngCore + ?Sized>(rng: &mut R) -> Self;
+    fn random<R: RngCore + ?Sized>(rng: &mut R) -> Self {
+        Self::try_from_rng(rng)
+            .map_err(|e: Infallible| e)


I confirmed that due to a blanket impl<R: RngCore> TryRngCore<Error = Infallible> for R, this does not impose any additional constraints or require any changes to the trait method documentation.

str4d

utACK ae84736

baloo force-pushed the baloo/try_from_rng branch from 72a85c2 to 535bfe6 Compare February 26, 2025 18:48

baloo marked this pull request as draft February 26, 2025 19:41

baloo force-pushed the baloo/try_from_rng branch from 535bfe6 to 4ccfef5 Compare February 26, 2025 20:02

This was referenced Feb 26, 2025

elliptic-curve: adds Scalar::try_from_rng method RustCrypto/traits#1774

Merged

ecdsa: provide a SigningKey::try_from_rng RustCrypto/signatures#915

Merged

tarcieri pushed a commit to RustCrypto/traits that referenced this pull request Mar 4, 2025

elliptic-curve: adds Scalar::try_from_rng method (#1774)

27835aa

Depends: - zkcrypto/ff#126 - zkcrypto/ff#127 This is to provide an `ecdsa::SigningKey::try_from_rng` API (RustCrypto/signatures#915)

baloo marked this pull request as ready for review March 5, 2025 01:15

str4d reviewed Mar 9, 2025

View reviewed changes

baloo force-pushed the baloo/try_from_rng branch from 4ccfef5 to fed79ef Compare March 9, 2025 03:57

baloo changed the base branch from main to release-0.14.0 March 9, 2025 03:58

baloo force-pushed the baloo/try_from_rng branch from fed79ef to 94d4f18 Compare March 9, 2025 03:59

str4d requested changes Mar 9, 2025

View reviewed changes

baloo force-pushed the baloo/try_from_rng branch 2 times, most recently from 5dbbdee to f015932 Compare March 9, 2025 05:28

baloo force-pushed the baloo/try_from_rng branch 3 times, most recently from 0d75807 to 339e82c Compare March 9, 2025 05:45

str4d requested changes Mar 9, 2025

View reviewed changes

adds a Field::try_from_rng method

ae84736

baloo force-pushed the baloo/try_from_rng branch from 339e82c to ae84736 Compare March 9, 2025 05:59

baloo mentioned this pull request Mar 9, 2025

Release 0.14.0 #130

Draft

str4d approved these changes Mar 9, 2025

View reviewed changes

str4d merged commit 1bb6345 into zkcrypto:release-0.14.0 Mar 9, 2025
11 checks passed

baloo deleted the baloo/try_from_rng branch March 9, 2025 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

adds a Field::try_from_rng method #127

adds a Field::try_from_rng method #127

baloo commented Feb 26, 2025

Uh oh!

str4d Mar 9, 2025

Uh oh!

str4d left a comment

Uh oh!

str4d Mar 9, 2025

Uh oh!

str4d Mar 9, 2025

Uh oh!

baloo commented Mar 9, 2025

Uh oh!

str4d left a comment •

edited

Loading

Uh oh!

str4d Mar 9, 2025

Uh oh!

str4d Mar 9, 2025

Uh oh!

str4d Mar 9, 2025

Uh oh!

str4d left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	fn try_from_rng<R: ::ff::derive::rand_core::TryRngCore + ?Sized>(rng: &mut R) -> core::result::Result<Self, R::Error> {
	fn try_from_rng<R: ::ff::derive::rand_core::TryRngCore + ?Sized>(rng: &mut R) -> ::core::result::Result<Self, R::Error> {

	- `Field::try_from_rng` added. It accepts a `rand_core::TryRngCore` mutable reference.
	- `ff::Field::try_from_rng` is a new trait method that must be implemented by
	downstreams. `Field::random` now has a default implementation that calls it.

adds a Field::try_from_rng method #127

adds a Field::try_from_rng method #127

Conversation

baloo commented Feb 26, 2025

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

baloo commented Mar 9, 2025

Uh oh!

str4d left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

str4d Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

str4d left a comment •

edited

Loading