Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf(stark-curve): no subgroup check on prime-order curve #349

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

yelhousni
Copy link
Collaborator

No description provided.

@yelhousni yelhousni added the perf label Mar 3, 2023
@yelhousni yelhousni requested a review from ivokub March 3, 2023 09:18
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks good.

I'm trying to understand what was the previous behaviour - it seems that the method didn't depend on p at all and always returned true?

For long-term, should we add tests which try to pass non-valid points and check that the result is false?

@yelhousni
Copy link
Collaborator Author

yelhousni commented Mar 3, 2023

I'm trying to understand what was the previous behaviour - it seems that the method didn't depend on p at all and always returned true?

Oh right! the res point was always the zero point which is always on curve and has z=0 in Jacobian coordinates. So the tests pass no matter what p is.. it should have been res.ScalarMultiplication(p, fr.Modulus()). But since the curve has a prime order we just check if p is on the curve (as for bn254/g1 and secp256k1).

For long-term, should we add tests which try to pass non-valid points and check that the result is false?

Some invalid test vectors here would definitely be useful !

@yelhousni yelhousni merged commit 84be66f into develop Mar 3, 2023
@yelhousni yelhousni deleted the perf/subgroup-check-stark branch March 3, 2023 09:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants