Skip to content

elliptic-curve: make hash2curve traits depend on Group rather than CurveArithmetic #1925

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

Closed
wants to merge 2 commits into from

Conversation

carloskiki
Copy link
Contributor

This was previously requested as part of #1177, and some related discussion took place in #1911.

This seems relevant as the main trait name is GroupDigest.

I will also open a PR to all implementing crates.

@tarcieri
Copy link
Member

tarcieri commented Jul 7, 2025

cc @daxpedda

@daxpedda
Copy link
Contributor

daxpedda commented Jul 7, 2025

In #1177 the point was to make it possible to use the types provided in elliptic-curve (PublicKey, SecretKey and so on) with curve25519-dalek as well. I don't know the details, but apparently ed448-goldilocks is able to implement Curve and CurveArithmetic for both Ed448 and Decaf448 (stalled on RustCrypto/elliptic-curves#1229), so I assume the same can eventually be done in curve25519-dalek.

Regarding the change: I do think that implementing MapToCurve directly on ProjectivePoint is indeed more appropriate because it only deals with ProjectivePoint types. However the same can't be said for GroupDigest because it also acts on Scalar. So I do think GroupDigest should remain on the curve label itself. I don't know a trait that can facilitate this apart from CurveArithmetic.

In the end however this is just a preference and while I don't want to approve the change I'm not against it either. Either way works in practice.

@carloskiki
Copy link
Contributor Author

Indeed, we can implement the Curve traits for curve25519-dalek, I have a draft implementation of it already, but I don't know if we should.

However the same can't be said for GroupDigest because it also acts on Scalar.

The Group trait has an associated Scalar type, which fulfills everything we need for GroupDigest. If you look at CurveArithmetic:

pub trait CurveArithmetic: Curve {
    // type AffinePoint: ...;

    type ProjectivePoint: group::Group<Scalar = Self::Scalar> /* + ... */;

    // type Scalar: ...;

We require that CurveArithmetic::Scalar is Group::Scalar of the projective point. So the implementation in this PR is correct in using Group::Scalar in place of CurveArithmetic::Scalar.

@daxpedda
Copy link
Contributor

daxpedda commented Jul 8, 2025

However the same can't be said for GroupDigest because it also acts on Scalar.

The Group trait has an associated Scalar type, which fulfills everything we need for GroupDigest.

I understand that its possible. What I was trying to say is that its quite strange to go through ProjectivePoint to call hash_to_scalar().

@daxpedda
Copy link
Contributor

daxpedda commented Jul 8, 2025

How about we just split GroupDigest into two traits: one to implement on ProjectivePoint and the other on Scalar? I honestly think this is the most appropriate given how the end-user actually ends up using the API.

I guess this PR can be closed and moved to elliptic-curves now that hash2curve is extracted in its own crate.

@tarcieri
Copy link
Member

tarcieri commented Jul 9, 2025

Closing because hash2curve is now in https://github.com/RustCrypto/elliptic-curves but we can still discuss this as a possible change if you want to reopen a new PR

@tarcieri tarcieri closed this Jul 9, 2025
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.

3 participants