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

perf: non-native modular multiplication #749

Merged
merged 9 commits into from
Dec 4, 2023
Merged

perf: non-native modular multiplication #749

merged 9 commits into from
Dec 4, 2023

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Jul 3, 2023

Description

This PR implements improved non-native modular multiplication as described in https://hackmd.io/3cpvkONzQl-TF5Oj8VXEHQ.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this been tested?

The existing test suite passes.

How has this been benchmarked?

One of the biggest circuits we have - BW6-761 PLONK verifier in BN254 PLONK reduced from 180M constraints to 80M.

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

@ivokub ivokub marked this pull request as ready for review December 1, 2023 01:28
@ivokub ivokub self-assigned this Dec 1, 2023
@ivokub ivokub added perf zk-evm P1: High Issue priority: high labels Dec 1, 2023
@ivokub ivokub requested a review from gbotrel December 1, 2023 01:37
Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

couldn't find pointer issues, but my instinct tells me it's a bit risky ^^
also, did write some fuzz tests and ran for sometime, nothing found, but since we have a hint that does all the work, it just says the hint seems to be correct, not that everything is sound and one can't find a bad :)

lgtm 👍

@ivokub
Copy link
Collaborator Author

ivokub commented Dec 4, 2023

couldn't find pointer issues, but my instinct tells me it's a bit risky ^^ also, did write some fuzz tests and ran for sometime, nothing found, but since we have a hint that does all the work, it just says the hint seems to be correct, not that everything is sound and one can't find a bad :)

lgtm 👍

Yes, it is not perfect, but imo works well enough. I have some ideas on how to make it more robust. Namely we can keep some DB of "element pointer -> hash of limbs" and if we see during some non-native operation that there is a conflict then we can panic.

The problem with using values was that then I cannot re-use the element-as-polynomial evaluation value when same element is used in multiple muls. And this really impacted some non-native code. I could possibly try having some additional DB "hash of limbs -> stored evaluation", but dunno if this would be any better than the current approach + integrity check in prev paragraph.

@ivokub ivokub merged commit 64c88cb into master Dec 4, 2023
7 checks passed
@ivokub ivokub deleted the feat/cleanmodexp branch December 4, 2023 22:30
@ivokub ivokub mentioned this pull request Feb 23, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1: High Issue priority: high perf zk-evm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants