Implement `ff` and `group` traits #473

str4d · 2022-12-12T01:56:22Z

These are placed behind the new group feature flag.

str4d · 2022-12-12T22:38:59Z

I'll rebase this.

These are placed behind the new `ff` feature flag.

str4d · 2022-12-12T22:40:54Z

Rebased on release/4.0.

daxpedda · 2022-12-16T13:17:46Z

Cc #373.

str4d · 2022-12-16T19:23:59Z

I haven't added group trait impls in this PR because there are upcoming breaking changes to some of those traits that I'm hoping to make soon, and we can avoid needing to bring them in as part of a 5.0 if we wait until after that.

tarcieri · 2022-12-16T19:47:58Z

@str4d you could add ff and group to these SemVer exceptions, which allow for breaking upgrades around minor versioned releases of curve25519-dalek rather than major versions: https://github.com/dalek-cryptography/curve25519-dalek#public-api-semver-exemptions

str4d · 2022-12-16T20:59:17Z

If that's a strategy that the users of this crate (specifically the users of the ff trait impls) are happy with, then that's fine with me. It's definitely easier to set this up from the start, and doesn't affect anyone not enabling the ff feature. It does mean that cargo update doesn't work without specific care, so I'd like the README or somewhere else to give a bit of guidance on how crate users should handle these minor version bumps if they can't follow the update yet.

tarcieri · 2022-12-16T21:06:05Z

@str4d if you had the full traits wired up, you could just have a group feature which pulls ff in as group::ff.

I expect most users who care about the ff impls also want group (here's one anecdote) and it would make for one fewer knob.

Re: making cargo update work, it's fine if you use this sort of SemVer predicate:

curve25519-dalek = ">= 5, < 5.1"

...which will still allow for patch-level updates, though users would need to keep an eye out for minor version updates.

These ranges can also be expanded once it's been verified that a minor version bump doesn't affect functionality a given downstream dependent crate happens to be using, e.g.

curve25519-dalek = ">= 5.1, < 5.4"

str4d · 2022-12-18T10:52:17Z

@str4d if you had the full traits wired up, you could just have a group feature which pulls ff in as group::ff.

I expect most users who care about the ff impls also want group (here's #373 (comment)) and it would make for one fewer knob.

Given that ff would be optional, we get an implicit knob for it anyway (until MSRV is at least 1.60, at which point the implicit feature for an optional crate can be disabled). So we may as well have it behave correctly (i.e. have it enable rand_core).

...which will still allow for patch-level updates, though users would need to keep an eye out for minor version updates.

These ranges can also be expanded once it's been verified that a minor version bump doesn't affect functionality a given downstream dependent crate happens to be using

If I understand you correctly, the recommended workflow for using these "breaking changes in minor version bumps" feature flags is:

When you enable the feature flag, bound your crate's dependency to < X.{Y+1} where X.Y is the latest release.
When X.{Y+1}.0 is released, check that it works for you, and then cut a backwards-compatible point release of your crate with the bound increased to < X.{Y+2}.

It does require proactivity, but it seems fine as long as it is documented. So assuming my understanding is correct, I'm fine with adding group trait impls to this PR as well, or I can add them in a separate PR (as per above, we'll be keeping the ff feature flag in the near term).

tarcieri · 2022-12-18T16:47:50Z

Given that ff would be optional, we get an implicit knob for it anyway (until MSRV is at least 1.60, at which point the implicit feature for an optional crate can be disabled). So we may as well have it behave correctly (i.e. have it enable rand_core).

What you're suggesting would work just fine with namespaced features in 1.60 if you did something like this:

[dependencies]
ff = { version = "0.13", optional = true }
group = { version = "0.13", optional = true }
rand_core = { version = "0.6", optional = true }

[features]
ff = ["dep:ff", "rand_core"]
group = ["dep:group", "ff"]

However prior to 1.60 you can't rely on feature unification to activate other features in the same crate by way of transitive dependencies, i.e. if you just have this:

[dependencies]
ff = { version = "0.13", optional = true }
group = { version = "0.13", optional = true }
rand_core = { version = "0.6", optional = true }

Activating the ff feature will not automatically activate the rand_core feature, nor will activating the group feature automatically activate ff and rand_core features, even though ff has rand_core as a transitive dependency and group has ff as a transitive dependency.

str4d · 2022-12-18T18:48:13Z

I know, which is why the current PR uses the crate renaming trick to change the name of the implicit feature, so we can instead add an explicit feature that does have the correct setup. I was just arguing that we may as well preserve this because we're going to have an ff feature in the simple case. An alternative would be to use the renaming trick to "hide" the implicit feature and not add an explicit one, but that feels like busywork (as the renamed implicit feature is still an implicit feature).

str4d · 2022-12-19T11:04:11Z

Oh wait, I'm not tired now and see what you meant. Accessing ff as group::ff means we don't need an explicit dependency on ff, which means we don't have the implicit feature flag. Sure, that works fine for me, and we can always add the ff feature flag in a backwards-compatible way if users say they want it.

str4d · 2022-12-19T12:30:14Z

This PR now includes implementations of the group traits. A required crate::edwards::SubgroupPoint struct is introduced, to enable impl CofactorGroup for EdwardsPoint. I have not implemented group::Curve etc. because curve25519-dalek does not (currently) expose an affine representation of an Edwards point.

str4d · 2022-12-19T19:44:56Z

Force-pushed to fix test imports.

src/edwards.rs

src/ristretto.rs

str4d · 2022-12-20T22:15:32Z

Force-pushed to address most of @tarcieri's comments. I'll add a follow-up commit with the de-duplication of decompression.

rozbb

Looks good, thank you! Left a few comments. Structurally, I think it also might be cleaner to put all the trait impls into a separate file. edwards.rs has almost 1/4 of its non-test code spent on impls.

rozbb · 2022-12-27T10:35:06Z

src/ristretto.rs

@@ -260,6 +267,27 @@ impl CompressedRistretto {
    ///
    /// - `None` if `self` was not the canonical encoding of a point.
    pub fn decompress(&self) -> Option<RistrettoPoint> {
+        let (s_encoding_is_canonical, s_is_negative, s) = decompress::step_1(self);
+
+        if s_encoding_is_canonical.unwrap_u8() == 0u8 || s_is_negative.unwrap_u8() == 1u8 {


Same thing: unwrap_u8 -> bool::from

rozbb · 2022-12-27T10:35:46Z

src/edwards.rs

    pub fn decompress(&self) -> Option<EdwardsPoint> {
-        let Y = FieldElement::from_bytes(self.as_bytes());
+        let (is_valid_y_coord, X, Y, Z) = decompress::step_1(self);
+        if is_valid_y_coord.unwrap_u8() != 1u8 {


I'd use the bool::from impl here rather than unwrap_u8()

rozbb · 2022-12-27T10:41:44Z

src/scalar.rs

+        group::ff::helpers::sqrt_tonelli_shanks(
+            self,
+            [
+                0xcb02_4c63_4b9e_ba7d,


What is this constant? Put a comment here

rozbb · 2022-12-27T10:46:27Z

src/scalar.rs

+            0, 0, 0,
+        ],
+    };
+    const S: u32 = 2;


FWIW these constants are documented in the trait definition itself:

https://docs.rs/ff/latest/ff/trait.PrimeField.html#associatedconstant.S

rozbb · 2022-12-27T10:46:35Z

src/scalar.rs

+            0xef, 0x8c, 0xb5, 0x06,
+        ],
+    };
+    const DELTA: Self = Self {


What is Delta?

MULTIPLICATIVE_GENERATOR ^ (2^s) mod p, i.e. s squarings of MULTIPLICATIVE_GENERATOR

rozbb · 2022-12-27T10:46:42Z

src/scalar.rs

+        ],
+    };
+    const S: u32 = 2;
+    const ROOT_OF_UNITY: Self = Self {


Which root of unity is this?

It's MULTIPLICATIVE_GENERATOR ^ t mod p where t = (modulus - 1) >> s

rozbb · 2022-12-27T10:47:47Z

src/scalar.rs

+    }
+
+    const MODULUS: &'static str =
+        "0x1000000000000000000000000000000014def9dea2f79cd65812631a5cf5d3ed";


Should cite this and/or add a test for it in the tests. This constant occurs in the l() function on page 52 of RFC 8032.

rozbb · 2022-12-27T10:48:03Z

src/scalar.rs

+        // DELTA^{t} mod m == 1
+        assert_eq!(
+            Scalar::DELTA.pow(&[
+                0x9604_98c6_973d_74fb,


What is t?

t = (modulus - 1) >> s

tarcieri · 2022-12-27T14:07:00Z

Cargo.toml

@@ -65,6 +66,7 @@ packed_simd = { version = "0.3.4", package = "packed_simd_2", features = ["into_
 [features]
 default = ["alloc"]
 alloc = ["zeroize/alloc"]
+group = ["group_crate", "rand_core"]


With #485 landed this can now be:

Suggested change

group = ["group_crate", "rand_core"]

group = ["dep:group", "rand_core"]

ycscaly · 2023-03-28T15:55:06Z

What is preventing the merge?

rozbb · 2023-03-29T06:00:01Z

This feature is taking a back seat to the other things we need before 4.0. This can likely come in 4.1.

ycscaly · 2023-03-29T09:16:07Z

This feature is taking a back seat to the other things we need before 4.0. This can likely come in 4.1.

Thanks, and is there any rough estimation on when 4.1 will come?

Also, is the code here already working or needs much more work?

tarcieri · 2023-06-25T16:20:18Z

@str4d if you have some time, would you mind rebasing this?

I think @rozbb intends to cut a 4.0 release soon, and it'd be good to at least preflight this PR prior to a release to make sure it can be merged afterward without breaking changes.

dconnolly · 2023-07-22T21:59:44Z

@str4d if you have some time, would you mind rebasing this?

I think @rozbb intends to cut a 4.0 release soon, and it'd be good to at least preflight this PR prior to a release to make sure it can be merged afterward without breaking changes.

4.0 is out 🎉

tarcieri · 2023-08-13T12:31:59Z

Would love to get this merged soon if possible. People are still asking me about it.

pinkforest · 2023-08-15T16:55:55Z

Might just move this along:

Implement ff and group traits - Rebased #562

@str4d

Originally authored by @str4d as #473

rozbb · 2023-08-28T06:02:18Z

Superseded by #562

tarcieri mentioned this pull request Dec 12, 2022

[WIP] Impl ff::Field for Scalar #471

Closed

tarcieri force-pushed the have-scalar-from-canonical-bytes-return-ctoption branch from c51ba24 to e665a4c Compare December 12, 2022 14:28

str4d mentioned this pull request Dec 12, 2022

Change Scalar::from_canonical_bytes to return CtOption #472

Merged

str4d changed the base branch from have-scalar-from-canonical-bytes-return-ctoption to release/4.0 December 12, 2022 22:39

Implement ff traits for Scalar

f95de3a

These are placed behind the new `ff` feature flag.

str4d force-pushed the ff branch from deac0cf to f95de3a Compare December 12, 2022 22:40

str4d force-pushed the ff branch from 7fd3e4e to a8cc9ec Compare December 19, 2022 12:29

str4d changed the title ~~Implement ff traits for Scalar~~ Implement ff and group traits Dec 19, 2022

str4d force-pushed the ff branch from a8cc9ec to 0f25c70 Compare December 19, 2022 19:44

tarcieri reviewed Dec 20, 2022

View reviewed changes

src/edwards.rs Outdated Show resolved Hide resolved

tarcieri reviewed Dec 20, 2022

View reviewed changes

src/edwards.rs Outdated Show resolved Hide resolved

tarcieri reviewed Dec 20, 2022

View reviewed changes

src/edwards.rs Outdated Show resolved Hide resolved

tarcieri reviewed Dec 20, 2022

View reviewed changes

src/ristretto.rs Outdated Show resolved Hide resolved

str4d added 3 commits December 20, 2022 21:46

Move ff traits behind group feature flag

1e25bfc

Implement group traits for RistrettoPoint

dccdb92

Implement group traits for EdwardsPoint

715e438

str4d force-pushed the ff branch from 0f25c70 to 715e438 Compare December 20, 2022 22:14

Deduplicate constant-time and non-constant-time decompression code

926b33e

tarcieri approved these changes Dec 20, 2022

View reviewed changes

tarcieri requested a review from rozbb December 20, 2022 22:34

daxpedda mentioned this pull request Dec 24, 2022

Use Group instead of Curve when appropriate RustCrypto/traits#1177

Open

tarcieri mentioned this pull request Dec 27, 2022

Weakly activate zeroize?/alloc; MSRV 1.60 #485

Merged

rozbb reviewed Dec 27, 2022

View reviewed changes

tarcieri reviewed Dec 27, 2022

View reviewed changes

tarcieri mentioned this pull request Jan 1, 2023

curve25519: ed25519, x25519 and ristretto255 RustCrypto/elliptic-curves#497

Closed

kayabaNerve mentioned this pull request Jan 4, 2023

Future of dalek-ff-group serai-dex/serai#201

Open

tarcieri mentioned this pull request Jan 5, 2023

Compatibility with RustCypto/traits/elliptic-curve trait Curve on API side. #492

Open

str4d mentioned this pull request Mar 15, 2023

Scalar arithmetic is inconsistent with point multiplication most of the time #514

Closed

tarcieri mentioned this pull request Jul 27, 2023

getters for edwards point #553

Closed

pinkforest mentioned this pull request Aug 15, 2023

Implement ff and group traits - Rebased #562

Merged

tarcieri pushed a commit that referenced this pull request Aug 27, 2023

curve: implement ff and group traits (#562)

4373695

Originally authored by @str4d as #473

rozbb closed this Aug 28, 2023

	group = ["group_crate", "rand_core"]
	group = ["dep:group", "rand_core"]

Implement ff and group traits #473

Implement ff and group traits #473

Conversation

str4d commented Dec 12, 2022 • edited Loading

str4d commented Dec 12, 2022

str4d commented Dec 12, 2022

daxpedda commented Dec 16, 2022

str4d commented Dec 16, 2022

tarcieri commented Dec 16, 2022 • edited Loading

str4d commented Dec 16, 2022

tarcieri commented Dec 16, 2022

str4d commented Dec 18, 2022 • edited Loading

tarcieri commented Dec 18, 2022 • edited Loading

str4d commented Dec 18, 2022

str4d commented Dec 19, 2022

str4d commented Dec 19, 2022

str4d commented Dec 19, 2022

str4d commented Dec 20, 2022

rozbb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycscaly commented Mar 28, 2023

rozbb commented Mar 29, 2023

ycscaly commented Mar 29, 2023

tarcieri commented Jun 25, 2023

dconnolly commented Jul 22, 2023

tarcieri commented Aug 13, 2023

pinkforest commented Aug 15, 2023

rozbb commented Aug 28, 2023 • edited Loading

Implement `ff` and `group` traits #473

Implement `ff` and `group` traits #473

str4d commented Dec 12, 2022 •

edited

Loading

tarcieri commented Dec 16, 2022 •

edited

Loading

str4d commented Dec 18, 2022 •

edited

Loading

tarcieri commented Dec 18, 2022 •

edited

Loading

rozbb commented Aug 28, 2023 •

edited

Loading