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.

Sign up for GitHub

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

Jump to bottom

Refactor identity methods into Identity trait #42

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

Refactor identity methods into Identity trait #42

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42f58a0
Refactor identity methods into `Identity` trait
daxpedda Dec 20, 2022
9b570d4
Use an associated constant instead of `identity()`
daxpedda Dec 20, 2022
73d7bdf
Address review
daxpedda Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/cofactor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use core::ops::{Mul, Neg};
use ff::PrimeField;
use subtle::{Choice, CtOption};

use crate::Identity;
use crate::{prime::PrimeGroup, Curve, Group, GroupEncoding, GroupOps, GroupOpsOwned};

/// This trait represents an element of a cryptographic group with a large prime-order
Expand Down Expand Up @@ -66,6 +67,7 @@ pub trait CofactorCurve:
/// in the correct prime order subgroup.
pub trait CofactorCurveAffine:
GroupEncoding
+ Identity
+ Copy
+ Clone
+ Sized
Expand All @@ -85,16 +87,9 @@ pub trait CofactorCurveAffine:
type Scalar: PrimeField;
type Curve: CofactorCurve<Affine = Self, Scalar = Self::Scalar>;

/// Returns the additive identity.
fn identity() -> Self;

/// Returns a fixed generator of unknown exponent.
fn generator() -> Self;

/// Determines if this point represents the point at infinity; the
/// additive identity.
fn is_identity(&self) -> Choice;

/// Converts this element to its curve representation.
fn to_curve(&self) -> Self::Curve;
}
19 changes: 12 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ impl<T, Rhs, Output> ScalarMulOwned<Rhs, Output> for T where T: for<'r> ScalarMu

/// This trait represents an element of a cryptographic group.
pub trait Group:
Clone
Identity
+ Clone
+ Copy
+ fmt::Debug
+ Eq
Expand All @@ -78,15 +79,9 @@ pub trait Group:
/// This function is non-deterministic, and samples from the user-provided RNG.
fn random(rng: impl RngCore) -> Self;

/// Returns the additive identity, also known as the "neutral element".
fn identity() -> Self;

/// Returns a fixed generator of the prime-order subgroup.
fn generator() -> Self;

/// Determines if this point is the identity.
fn is_identity(&self) -> Choice;

/// Doubles this element.
#[must_use]
fn double(&self) -> Self;
Expand Down Expand Up @@ -172,3 +167,13 @@ pub trait UncompressedEncoding: Sized {
/// the point at infinity.
fn to_uncompressed(&self) -> Self::Uncompressed;
}

/// Obtain or determine the identity point.
Copy link
Member

Choose a reason for hiding this comment

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

Same issue ("point" instead of "element").

I don't know how to describe this trait, which is concerning. It's not an extension trait because it's being added as a required bound. But the constant and method it defines do not make sense in a vacuum; they need to be in the context of a group.

This makes me question the premise of the PR: what is the motivating use case for generalising over both the affine and efficient forms of a curve point, such that a shared trait is needed?

Copy link
Author

Choose a reason for hiding this comment

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

Apologies for the delay.

The original motivation was RustCrypto/traits#1170. We currently use Default to obtain the identity element, which isn't ideal and doesn't support const.

Unfortunately this was a long time ago and I don't have all the details in my head anymore, but I assume these sort of requirements will keep popping up in elliptic-curve because of it's nature: it introduces types that are generic over many different things, so it requires traits that are equally generic to support this idea.

Maybe @tarcieri has more to say on this topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a use case for such a generalization

pub trait Identity: Sized {
/// Returns the additive identity.
fn identity() -> Self;
daxpedda marked this conversation as resolved.
Show resolved Hide resolved

/// Determines if this point represents the point at infinity; the
/// additive identity.
daxpedda marked this conversation as resolved.
Show resolved Hide resolved
fn is_identity(&self) -> Choice;
}
11 changes: 2 additions & 9 deletions src/prime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use core::fmt;
use core::ops::{Mul, Neg};
use ff::PrimeField;
use subtle::Choice;

use crate::{Curve, Group, GroupEncoding};
use crate::{Curve, Group, GroupEncoding, Identity};

/// This trait represents an element of a prime-order cryptographic group.
pub trait PrimeGroup: Group + GroupEncoding {}
Expand All @@ -19,6 +18,7 @@ pub trait PrimeCurve: Curve<AffineRepr = <Self as PrimeCurve>::Affine> + PrimeGr
/// Affine representation of an elliptic curve point guaranteed to be
/// in the correct prime order subgroup.
pub trait PrimeCurveAffine: GroupEncoding
+ Identity
+ Copy
+ Clone
+ Sized
Expand All @@ -35,16 +35,9 @@ pub trait PrimeCurveAffine: GroupEncoding
type Scalar: PrimeField;
type Curve: PrimeCurve<Affine = Self, Scalar = Self::Scalar>;

/// Returns the additive identity.
fn identity() -> Self;

/// Returns a fixed generator of unknown exponent.
fn generator() -> Self;

/// Determines if this point represents the point at infinity; the
/// additive identity.
fn is_identity(&self) -> Choice;

/// Converts this element to its curve representation.
fn to_curve(&self) -> Self::Curve;
}
2 changes: 1 addition & 1 deletion src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use rand_xorshift::XorShiftRng;
use crate::{
prime::{PrimeCurve, PrimeCurveAffine},
wnaf::WnafGroup,
GroupEncoding, UncompressedEncoding,
GroupEncoding, Identity, UncompressedEncoding,
};

pub fn curve_tests<G: PrimeCurve>() {
Expand Down