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

Clarify guarantees in traits around point validity #27

Closed
ValarDragon opened this issue Nov 9, 2020 · 4 comments · Fixed by #447
Closed

Clarify guarantees in traits around point validity #27

ValarDragon opened this issue Nov 9, 2020 · 4 comments · Fixed by #447
Labels
D-medium Difficulty: medium P-medium Priority: medium T-bug Type: bug T-refactor Type: cleanup/refactor

Comments

@ValarDragon
Copy link
Member

The elliptic curve point API's (AffineCurve, ProjectiveCurve) currently claim that the interface should be providing quite strong guarantees, all points should be on the curve, and moreover in the prime order subgroup.

However this guarantee isn't achieved at the moment.

I've only looked at short weierstrass jacobian curves so far (which encompasses the BLS12 curves), and they don't guarantee that points are on the curve, or that they are in the prime order subgroup for AffineCurve.

The new() method for AffineGroup (which implements AffineCurve) does not ensure that points are on the curve.

Also the sw curve's Affine Curve implementation of from_random_bytes() uses get_point_from_x(), which as stated in its comments provides no guarantees around being in the prime order subgroup.

I believe that the trait comments should drop the description that they are in the prime order subgroup, and we should have new traits that enforce that this is the case. (Or alternatively, make the two above methods enforce the claimed properties)

@ValarDragon ValarDragon added the T-bug Type: bug label Nov 9, 2020
@Pratyush
Copy link
Member

Pratyush commented Nov 9, 2020

@kobigurk @paberr do you have any thoughts around this? We could have a separate trait that handles the boundary conversions, and leave AffineCurve to be only prime order.

@Pratyush
Copy link
Member

Pratyush commented Nov 9, 2020

One idea would be to put these methods in SWModelParameters and in TEModelParameters (I don't see how to put them in ModelParameters, because the trait doesn't understand the notion of AffineCurve.

@kobigurk
Copy link
Contributor

Good point! In my opinion, the current methods are useful and needed so I'd leave them as is.
New traits/methods sound like a good idea to me. Ideally they'd be wrappers that check validity and maybe one that multiplies by the cofactor, but that should be marked clearly. That bit us on the gadget side when we migrated our pre-refactor gadgets to the new constrain system.

@Pratyush Pratyush mentioned this issue Jun 5, 2021
6 tasks
@Pratyush Pratyush added D-medium Difficulty: medium P-medium Priority: medium T-refactor Type: cleanup/refactor labels Sep 9, 2021
@Pratyush
Copy link
Member

We now have a new_unchecked which doesn't check for subgroup membership, and have changed new to instead do the on-curve and in-subgroup checks.

(These methods were anyway methods on the structs, and not part of the corresponding traits.)

@Pratyush Pratyush linked a pull request Sep 2, 2022 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-medium Difficulty: medium P-medium Priority: medium T-bug Type: bug T-refactor Type: cleanup/refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants