halo2
halo2 copied to clipboard
privacy-scaling-explorations
MSM optimization
Depends on https://github.com/appliedzkp/pairing/pull/7, which should be merged first so this doesn't need to depend on any external repo's.
Credit for the core ideas behind these optimizations to Zac Williamson/Barretenberg. The implementation in this PR is based on barretenberg (and I think most other implementations doing similar things are as well) which contains comments that largely explain things pretty well, other references mainly used to complement those comments:
- https://github.com/AztecProtocol/barretenberg/blob/master/barretenberg/src/aztec/ecc/curves/bn254/scalar_multiplication/scalar_multiplication.cpp
- https://github.com/AztecProtocol/barretenberg/blob/master/barretenberg/src/aztec/ecc/pippenger.md
- https://github.com/celo-org/zexe/pull/4
- https://github.com/arkworks-rs/algebra/issues/60
- https://www.youtube.com/watch?v=Bl5mQA7UL2I
- https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h
More info inside arithmetic_msm.rs, but the main speedups are from:
- Using affine point addition for the bucket additions
- Use of endomorphism to reduce half number of rounds
- Use of WNAF represenation of the coefficients to half the number of buckets
On my computer it's around twice as fast as best_multiexp but haven't done a lot of testing yet on other computers with more cores.
The new implementation is used inside the prover, although for the best performance a shared cache object needs to be shared:
- Ideally can be shared between different proof generations.
- Needs to be mutable because the data is used as a scratch buffer so allocations aren't necessary.
Because the bases were already stored in Params I just made that object mutable everywhere where needed but I'm not sure if that's the best idea. Perhaps a separate object may make more sense.
The code coverage currently fails. Because everything else seems to run fine I think it's because the code coverage runs with --all-features and so also enables the x86 specific assembly (I don't think anything else enables those features). I enabled those features (disabled by default) here because they weren't yet for testing (and I added a new prefetch feature that also depends on some x86 assembly).
Does anyone know if the ci machine supports x86 instructions?
Does anyone know if the ci machine supports x86 instructions?
@AronisAt79
Could you also chekc why the bench i snot running here :)
@Brechtpd, @barryWhiteHat
during halo2 automated benches implementation, we had agreed that the tests will be triggered from kzg-rebase branch. Therefore, the pr that introduced the workflow code was only merged with kzg-rebase. Now this branch is obsolete? I thought that halo2 optimizations would be submitted via branches sourcing from kzg-rebase. Was that not the case? Regardless of that, there is still a needed fix on the workflow. Where should it be directed?
thought that halo2 optimizations would be submitted via branches sourcing from kzg-rebase. Was that not the case?
Nah we switched to schplonk you can get that by just removing the flag as its default.
Regardless of that, there is still a needed fix on the workflow. Where should it be directed?
Not sure probably just good to remove the kzg-rebase branch from benchmarks and just use master.
thought that halo2 optimizations would be submitted via branches sourcing from kzg-rebase. Was that not the case?
Nah we switched to schplonk you can get that by just removing the flag as its default.
Regardless of that, there is still a needed fix on the workflow. Where should it be directed?
Not sure probably just good to remove the kzg-rebase branch from benchmarks and just use master. @barryWhiteHat Thanks! It is clear. I will merge the proper workflow to master
@Brechtpd @barryWhiteHat necessary adaptations of the workflow yml file have been merged to main branch via https://github.com/appliedzkp/halo2/pull/48.
What's the status of this PR? It doesn't seem like there's any blockers, and a faster MSM would certainly be very nice. @Brechtpd
Ready for review but yeah it's been a while. :)
However, I believe the fastest approach will very likely exploit the fact that in plonk many MSMs are done over the same bases (https://github.com/zcash/halo2/issues/641), instead of just optimizing a single MSM as much as possible as in this PR. I think it's probably best to analyze the combined MSM approach and check its performance before continuing with this PR, and see if the approach in this PR still makes sense/is compatible with the combined approach.
Ah thanks for pointing out that other issue. That is very interesting. I can't guess one way or the other which would end up being faster for the halo2 particular use case. Something to investigate in the future...
I believe some people (kilic if I remember correctly) already started looking into this but it is a bit complex and pretty low priority so I guess the review never got finished.
In any case, at this point would definitely first look into the combined MSM approach and then potentially integrate that into this PR as that will require a different set of changes which are potentially simpler.
@kilic any input on that???
@CPerezz @jonathanpwang
This PR is a bit old and using pse/pairing as backend the one we used before we move to pse/halo2curves. Now trying to rebase it or possibly move it to pse/halo2curves with recent endomorhism features that covers both pasta and bn and is about to be added with privacy-scaling-explorations/halo2curves#24.
@Brechtpd Can you confirm msm tests are passing at your side? I cannot hit expected results. I'm trying to debugging but maybe you can spot the issue much faster
I'm having some trouble getting things building again because of old packages for some reason. I still had an old checkout and for some reason that does have test_commit_lagrange and test_roundtrip failing, but I don't remember there being any issues (but yeah, it's been a while so don't know for sure). The tests were working here on github as can still be seen, though the details have been purged because too old presumably. Not sure what's going on unfortunately.
trouble getting things building again because of old packages for some reason. I still had an old checkout and for some reason that does have
test_commit_lagrangeandtest_roundtripfailing, but I don't remember there being any issues (but yeah, it's been a while so don't know for sure). The tests were working here on github as can still be seen, though the details have been purged because too old presumably. Not sure what's going on unfortunately.
I see. No worries. I think I'm getting closer
Closing in favour of https://github.com/privacy-scaling-explorations/halo2curves/pull/29