Skip to content

perf(ecmul): use GLV with safe handling of edge cases in EVM ecmul #976

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.

Sign up for GitHub

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

Jump to bottom
Merged
yelhousni merged 9 commits into from
Jan 16, 2024
Merged

perf(ecmul): use GLV with safe handling of edge cases in EVM ecmul #976

yelhousni merged 9 commits into from
Jan 16, 2024

Conversation

@yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Dec 20, 2023

Description

Another tiny PR to use GLV with safe handling of edge cases in EVM ecmul precompile.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

Tests in bn_test.go pass.

How has this been benchmarked?

ECMUL precompile cost goes from 781,437 to 673,659 SCS.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@yelhousni yelhousni added type: perf dep: linea Issues affecting Linea downstream priority: P2-medium Issue priority: medium labels Dec 20, 2023
@yelhousni yelhousni added this to the v0.9.0 milestone Dec 20, 2023
@yelhousni yelhousni requested a review from ivokub December 20, 2023 13:42
@yelhousni
Copy link
Contributor Author

yelhousni commented Jan 9, 2024

@ivokub just re-pinging you here

ivokub
ivokub approved these changes Jan 9, 2024
View reviewed changes
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.

PR looks good. I'll accept it, but maybe I can try implementing the safe scalarmul with option instead to see how it looks. I have actually also thought about adding an option which would allow to define for MSM which inputs may be unsafe (i.e. 0 scalar or 0 point) and then these options would complement each other quite nicely.

@ivokub
Copy link
Collaborator

ivokub commented Jan 10, 2024

@yelhousni - I implemented using an option and merged the implementations. See how it looks and feels. If seems excessive then I'm good with reverting. On one hand I do not want to overuse using options, but I still like having unified implementation.

Another thing - currently the default is to use unsafe and we use option to use safe version. But I guess it would make more sense to have the defaults reversed -- i.e. without options we use safe formulas and then can optionally toggle to unsafe formulas for performance. I think we may have incoming bug reports due to the defaults being the other way.
But I didn't want to change now as it would have required a lot of refactoring all over, so dunno.

@yelhousni yelhousni merged commit e93fa76 into master Jan 16, 2024
@yelhousni yelhousni deleted the perf/ecmul-precompile branch January 16, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivokub ivokub ivokub approved these changes

Assignees

No one assigned

Labels

dep: linea Issues affecting Linea downstream priority: P2-medium Issue priority: medium type: perf

Projects

None yet

Milestone

v0.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@yelhousni @ivokub