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

Optimize verify_kzg_proof_multi_impl #3584

Closed
wants to merge 16 commits into from

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Jan 17, 2024

Note: this only pertains to peerDAS (EIP-7594). When adding a new reverse_bits_limited helper function I noticed that verify_kzg_proof_multi_impl was pretty inefficient. This new code is originally based on @dankrad's implementation of check_proof_multi in the eth-research repo. We use this new implementation in c-kzg-4844's das branch.

On my system, it took 613ms to verify a single cell proof with the old implementation. With this PR, it takes 22ms. It could be even faster if the roots of unity were pre-computed, but that only takes about 5ms so I don't think it's worth changing.

Fixes #3575.

@hwwhww hwwhww added the EIP-7594 PeerDAS label Jan 19, 2024
@hwwhww hwwhww requested a review from dankrad January 20, 2024 06:31
@jtraglia
Copy link
Member Author

While this PR does provide a pretty big speed up in cell verification times, we've decided that we would rather have a slower but more readable spec. This implementation is more complex than we'd like. Also, cell verification is not the bottleneck in the tests; proof generation is by far the slowest (most expensive) part. I'm going to close this PR. If in the future we decide this PR would be useful, we can re-open it then.

@jtraglia jtraglia closed this Apr 18, 2024
@mratsim
Copy link
Contributor

mratsim commented Apr 22, 2024

If implementations were to use this as an optional optimization, would they still be compliant with the current spec, is there a change in security-level?

@jtraglia
Copy link
Member Author

@mratsim Yes, it would still be compliant. We expect implementations to use optimizations. With that in mind, the reference tests (which are a work in progress) should help identify issues. I don't believe this changes any security assumptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reverse_bits_limited helper for polynomial-commitments-sampling.md
5 participants