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

multiexp: avoid direct coordinate access to check for zero points #414

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Jun 25, 2023

This PR introduces a new IsZero() bool method for G1/G2 JacExtended points to check if they're the zero element.

In the multiexp algorithm, this is required when aggregating Pippenger buckets while doing the running sum. Apart from multiexp, it can be helpful for clients in general. I can imagine some tension around IsZero() vs IsInfinity() in the naming, but I leaned towards IsZero() since feels more general (since points at infinity aren't always the zero points in all curves).

Considering the new method is very simple, the compiler inlines it, so it shouldn't have any performance hit.


The context for this PR is about something other than implementation purity. I was trying to adapt the multiexp algorithm to a tw. Edwards curve (i.e: Bandersnatch), where I have to "adapt" these Jacobian points to, e.g., Projective points. So I created point adapters that would override method calls to the other underlying point. But, these direct coordinate accesses made this impossible.

To be clear, adapting multiexp to an underlying tw. Edwards curve is probably not optimal for performance since there're some affine batch addition tricks applied here that only make sense for short Wiestrass curves. But, since our MSMs were somewhat short, I just wanted to experiment with "how bad" (or good!) using an "untouched" MSM implementation could work for us.

It would be very nice to have some MSM algorithm optimized for tw. Edwards! And in particular, for short MSMs (in our case, we're doing MSM on a basis length of 256). That's another argument to expect the current multiexp not to be ideal since it's more optimized for big MSMs (e.g., SNARKs). I don't know if that's on the roadmap, but it would be great :)

None of what I mentioned in this section is an argument for merging or justifying this PR, but to explain where this came from.

@gbotrel gbotrel changed the base branch from master to develop June 26, 2023 02:24
@gbotrel gbotrel requested a review from yelhousni August 18, 2023 15:00
Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, it seems I missed this PR. No problem on my side to merge this if it makes your life easier guys :)

For MSM on twisted Edwards curves I would suggest to look at this paper and accompanying code: https://github.com/gbotrel/zprize-mobile-harness. In the MSM code of gnark, we use batch affine only for window sizes 10 and bigger which correspond to large MSM instances. For size 256 the code would use extended Jacobian coordinates (cost 10m per addition in the bucket). In the paper and the code linked, we use twisted Edwards form with "modified" extended coordinates (7m per addition) for all sizes. So I expect that this code would be faster for your use-case.

@jsign
Copy link
Contributor Author

jsign commented Aug 18, 2023

Sorry for the late reply, it seems I missed this PR. No problem on my side to merge this if it makes your life easier guys :)

For MSM on twisted Edwards curves I would suggest to look at this paper and accompanying code: https://github.com/gbotrel/zprize-mobile-harness. In the MSM code of gnark, we use batch affine only for window sizes 10 and bigger which correspond to large MSM instances. For size 256 the code would use extended Jacobian coordinates (cost 10m per addition in the bucket). In the paper and the code linked, we use twisted Edwards form with "modified" extended coordinates (7m per addition) for all sizes. So I expect that this code would be faster for your use-case.

Thanks for the suggestion and pointing to the paper; I'll look!

Feel free to merge. :)

@gbotrel gbotrel changed the base branch from develop to master August 22, 2023 21:08
@gbotrel gbotrel merged commit 208eeba into Consensys:master Aug 22, 2023
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