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

Remove ristretto255 trait implementations #83

Open
str4d opened this issue May 3, 2020 · 3 comments
Open

Remove ristretto255 trait implementations #83

str4d opened this issue May 3, 2020 · 3 comments

Comments

@str4d
Copy link

str4d commented May 3, 2020

I noticed today that the curv traits are implemented for ristretto255.

ristretto255 is an abstract group, not an elliptic curve. See the ristretto255 RFC draft for more details. The curv traits provide APIs for elliptic curves, and thus it is a type error to implement them for ristretto255.

I briefly looked at the trait implementation, and found an example of where the type error manifests as an implementation error:

https://github.com/KZen-networks/curv/blob/b326b2708560eaa0e8fd86f2c2d7d20d6eafbdce/src/elliptic/curves/curve_ristretto.rs#L278-L289

Besides the fact that ristretto255 elements do not have coordinates, this code is parsing an encoded ristretto255 element (which includes the representation of a field element modulo p = 2255 - 19) as a ristretto255 scalar (which are field elements modulo ℓ = 2252 + 27742317777372353535851937790883648493), which is a type error, and returns a scalar that has been reduced from a field element, losing information.

@omershlo
Copy link
Contributor

omershlo commented May 4, 2020

Hey @str4d, thanks for looking into the code!
For Ristretto we are using curve25519-dalek which I belive is an instantiation of Ristretto for curve25519.

curve25519-dalek is a library providing group operations on the Edwards and Montgomery forms of Curve25519, and on the prime-order Ristretto group.

What am I missing?

About the x/y coordinates - I agree, there's an error there. In practice we never use the coordinates so I think we will make it unimplemented as well. However, I don't see how it relates to your first point?

@str4d
Copy link
Author

str4d commented May 4, 2020

For Ristretto we are using curve25519-dalek which I belive is an instantiation of Ristretto for curve25519.

curve25519-dalek is implementing ristretto255 as specified in the RFC draft. It happens to use the Edwards form of Curve25519 as its internal representation, but that is an internal implementation detail, not exposed as part of the Ristretto API.

curve25519-dalek is a library providing group operations on the Edwards and Montgomery forms of Curve25519, and on the prime-order Ristretto group.

What am I missing?

Exactly as that documentation says, it provides group operations on the Curve25519 elliptic curve, and on the prime-order Ristretto group.

I strongly recommend reading the ristretto255 RFC draft, but quoting two relevant sections:

Ristretto implements an abstract prime-order group interface that exposes only the behavior that is useful to higher-level protocols, without leaking curve-related details and pitfalls.

ristretto255 is an abstraction which implements a prime-order group, and ristretto255 elements are represented by curve points, but they are not curve points.

In short, ristretto255 not an elliptic curve, and therefore the ECPoint trait should not be implemented for it, as that trait is intended for elliptic curves (as documented in this crate). A more generic group trait (such as the one I am currently working on for the group crate) would make sense for both ristretto255 and the various elliptic curves, but users of the ECPoint trait can reasonably assume that they will only be given elliptic curves, not generic groups.

@omershlo
Copy link
Contributor

omershlo commented May 4, 2020

I see your point. You are concerned because we named the trait in this library ECPoint, is that right?
This is probably a bad naming issue because this trait aims to offer an interface for generic group element (I admit that it might not be the most accurate set of operation required but this is what we needed for our generic ECC [put aside the issue with coordinates for a second]).

When you use this outside this is the names you see:

pub type GE = Secp256k1Point;
pub type FE = Secp256k1Scalar;

If we were to rename ECPoint to something more generic over groups, would it solve the issue?

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

No branches or pull requests

2 participants