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

HyperKZG+Shplonk primary PCS #348

Merged
merged 5 commits into from
Feb 29, 2024
Merged

HyperKZG+Shplonk primary PCS #348

merged 5 commits into from
Feb 29, 2024

Conversation

storojs72
Copy link
Contributor

@storojs72 storojs72 commented Feb 26, 2024

Fixes #302

I tried to merge Shplonk changes carefully, but patch appeared to be a little bit weird, sorry...

Specifically for this PR, I run benchmarks for comparing HyperKZG, Shplonk, HyperKZG + Shplonk as a separate PCSs.

PCS-Proving 10/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                           
                        time:   [6.9820 ms 7.0066 ms 7.0454 ms]
                        change: [+0.1964% +1.2246% +2.5014%] (p = 0.05 < 0.05)
                        Change within noise threshold.
PCS-Proving 10/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                           
                        time:   [5.9565 ms 6.0459 ms 6.2970 ms]
                        change: [+0.4083% +8.8162% +20.275%] (p = 0.09 > 0.05)
                        No change in performance detected.
PCS-Proving 10/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                           
                        time:   [6.2230 ms 6.3189 ms 6.5589 ms]
                        change: [+0.3784% +9.2852% +22.930%] (p = 0.13 > 0.05)
                        No change in performance detected.

PCS-Verifying 10/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.0999 ms 2.1640 ms 2.2544 ms]
                        change: [-40.584% -23.931% -4.5307%] (p = 0.06 > 0.05)
                        No change in performance detected.
PCS-Verifying 10/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.2250 ms 2.2365 ms 2.2519 ms]
                        change: [-46.005% -38.080% -28.056%] (p = 0.00 < 0.05)
                        Performance has improved.
PCS-Verifying 10/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.0987 ms 2.1889 ms 2.3168 ms]
                        change: [-56.676% -45.701% -29.023%] (p = 0.00 < 0.05)
                        Performance has improved.

PCS-Proving 12/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                            
                        time:   [17.588 ms 18.252 ms 19.199 ms]
                        change: [-5.1670% -0.9550% +4.2390%] (p = 0.74 > 0.05)
                        No change in performance detected.
PCS-Proving 12/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                            
                        time:   [15.916 ms 16.591 ms 17.580 ms]
                        change: [-1.8914% +8.4608% +25.526%] (p = 0.34 > 0.05)
                        No change in performance detected.
PCS-Proving 12/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                            
                        time:   [16.011 ms 16.201 ms 16.449 ms]
                        change: [+0.5723% +3.3010% +6.2646%] (p = 0.05 < 0.05)
                        Change within noise threshold.

PCS-Verifying 12/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.2496 ms 2.2656 ms 2.2885 ms]
                        change: [+0.6543% +2.8355% +4.9343%] (p = 0.03 < 0.05)
                        Change within noise threshold.
PCS-Verifying 12/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.2884 ms 2.2920 ms 2.2949 ms]
                        change: [-47.661% -32.363% -10.737%] (p = 0.02 < 0.05)
                        Performance has improved.
PCS-Verifying 12/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.1665 ms 2.1698 ms 2.1735 ms]
                        change: [+1.8756% +2.3530% +2.7648%] (p = 0.00 < 0.05)
                        Performance has regressed.

PCS-Proving 14/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                            
                        time:   [52.661 ms 52.929 ms 53.480 ms]
                        change: [+8.1111% +9.1010% +10.196%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Proving 14/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                            
                        time:   [46.717 ms 46.900 ms 47.153 ms]
                        change: [+7.3156% +10.450% +12.783%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Proving 14/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                            
                        time:   [45.732 ms 45.883 ms 46.074 ms]
                        change: [+6.1817% +8.7129% +10.866%] (p = 0.00 < 0.05)
                        Performance has regressed.

PCS-Verifying 14/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.3381 ms 2.3415 ms 2.3442 ms]
                        change: [-38.895% -19.277% +2.2437%] (p = 0.26 > 0.05)
                        No change in performance detected.
PCS-Verifying 14/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.3034 ms 2.3110 ms 2.3168 ms]
                        change: [+2.0384% +2.3770% +2.7497%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Verifying 14/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.1818 ms 2.1902 ms 2.2014 ms]
                        change: [-21.442% -8.3108% +1.4716%] (p = 0.38 > 0.05)
                        No change in performance detected.

PCS-Proving 16/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                           
                        time:   [176.33 ms 176.63 ms 177.03 ms]
                        change: [+6.5308% +7.8212% +9.0214%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Proving 16/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                           
                        time:   [160.14 ms 165.05 ms 168.22 ms]
                        change: [+4.6270% +7.2158% +10.242%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Proving 16/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                           
                        time:   [155.98 ms 156.67 ms 158.32 ms]
                        change: [+3.4618% +6.6481% +9.8972%] (p = 0.00 < 0.05)
                        Performance has regressed.

PCS-Verifying 16/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.2236 ms 2.2256 ms 2.2294 ms]
                        change: [-1.5444% +0.9676% +2.5628%] (p = 0.50 > 0.05)
                        No change in performance detected.
PCS-Verifying 16/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.3190 ms 2.3443 ms 2.3702 ms]
                        change: [+2.2457% +3.0450% +3.7298%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Verifying 16/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.1793 ms 2.1976 ms 2.2262 ms]
                        change: [+1.6639% +2.4983% +3.4770%] (p = 0.00 < 0.05)
                        Performance has regressed.

PCS-Proving 18/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                           
                        time:   [659.95 ms 661.69 ms 663.26 ms]
                        change: [-15.198% -4.2353% +4.7939%] (p = 0.61 > 0.05)
                        No change in performance detected.
PCS-Proving 18/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                           
                        time:   [539.59 ms 541.02 ms 542.01 ms]
                        change: [-2.1303% +2.1013% +6.0244%] (p = 0.36 > 0.05)
                        No change in performance detected.
PCS-Proving 18/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                           
                        time:   [546.50 ms 551.58 ms 561.21 ms]
                        change: [-0.4316% +3.0722% +6.7702%] (p = 0.12 > 0.05)
                        No change in performance detected.

PCS-Verifying 18/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.2535 ms 2.2741 ms 2.2956 ms]
                        change: [+0.8558% +1.9863% +3.0903%] (p = 0.00 < 0.05)
                        Change within noise threshold.
PCS-Verifying 18/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.4567 ms 2.4737 ms 2.4905 ms]
                        change: [+2.3518% +2.9013% +3.4723%] (p = 0.00 < 0.05)
                        Performance has regressed.
PCS-Verifying 18/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.3324 ms 2.3545 ms 2.3702 ms]
                        change: [+2.5511% +3.3620% +4.2107%] (p = 0.00 < 0.05)
                        Performance has regressed.

Benchmarking PCS-Proving 20/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 100.0s. You may wish to increase target time to 148.2s or enable flat sampling.
PCS-Proving 20/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arec...                                                                          
                        time:   [2.6565 s 2.7433 s 2.8107 s]
                        change: [+15.940% +18.602% +21.363%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking PCS-Proving 20/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 100.0s. You may wish to increase target time to 138.5s or enable flat sampling.
PCS-Proving 20/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, areci...                                                                          
                        time:   [2.3490 s 2.3936 s 2.4406 s]
                        change: [+11.512% +13.714% +16.054%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking PCS-Proving 20/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 100.0s. You may wish to increase target time to 122.0s or enable flat sampling.
PCS-Proving 20/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn2...                                                                          
                        time:   [2.1519 s 2.1604 s 2.1715 s]
                        change: [+8.2301% +9.2779% +10.349%] (p = 0.00 < 0.05)
                        Performance has regressed.

PCS-Verifying 20/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, ar...                                                                           
                        time:   [2.2633 ms 2.2655 ms 2.2700 ms]
                        change: [+0.4023% +0.7010% +0.9512%] (p = 0.00 < 0.05)
                        Change within noise threshold.
PCS-Verifying 20/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, are...                                                                           
                        time:   [2.4542 ms 2.4572 ms 2.4592 ms]
                        change: [+0.1057% +0.3054% +0.4634%] (p = 0.01 < 0.05)
                        Change within noise threshold.
PCS-Verifying 20/arecibo::provider::hyperkzg_shplonk::EvaluationEngine<halo2curves::bn256::engine::B...                                                                           
                        time:   [2.3273 ms 2.3293 ms 2.3316 ms]
                        change: [+0.4167% +0.6466% +0.9050%] (p = 0.00 < 0.05)
                        Change within noise threshold.

On my dev machine, prover HyperKZG + Shplonk has the best prover performance in 4 cases (12, 14, 16, 20), in 1 case (10) Shplonk is better.

As for the verifier, HyperKZG + Shplonk is better than Shplonk in all cases (10, 12, 14, 18, 20) and better than HyperKZG in the 3 cases (12, 14, 16).

Those results give me some confidence that HyperKZG + Shplonk provides better prover and at more or less comparable verifier, so I'm removing leaving it as a default (dropping rest from the codebase).

It would be nice to see HyperKZG + Shplonk benches results executed on dedicated machine and compared with previously stored ones for pure HyperKZG

Folks suggest using 'measurement_time' equals to 100s in order to
make measurements more resilient to transitory peak loads caused by
external programs.

bheisler/criterion.rs#322
@storojs72
Copy link
Contributor Author

!benchmark --bench pcs

Copy link
Contributor

Benchmark for 9c4b1ec

Click to view benchmark
Test Base PR %
PCS-Proving 10/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10 7.6±0.05ms 7.6±0.02ms 0.00%
PCS-Proving 10/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/10 52.5±0.39ms 52.6±0.23ms +0.19%
PCS-Proving 10/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/10 8.8±0.07ms 8.8±0.04ms 0.00%
PCS-Proving 10/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10 7.6±0.09ms N/A N/A
PCS-Proving 12/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12 15.4±0.08ms 14.5±0.03ms -5.84%
PCS-Proving 12/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/12 137.8±1.07ms 137.3±0.32ms -0.36%
PCS-Proving 12/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/12 15.1±0.20ms 15.1±0.03ms 0.00%
PCS-Proving 12/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12 15.2±0.29ms N/A N/A
PCS-Proving 14/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14 44.7±0.16ms 42.0±0.07ms -6.04%
PCS-Proving 14/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/14 460.8±1.31ms 461.1±0.65ms +0.07%
PCS-Proving 14/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/14 35.8±0.59ms 38.0±0.14ms +6.15%
PCS-Proving 14/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14 42.5±0.39ms N/A N/A
PCS-Proving 16/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16 139.6±1.35ms 142.3±0.35ms +1.93%
PCS-Proving 16/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/16 1749.2±5.00ms 1741.8±2.18ms -0.42%
PCS-Proving 16/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/16 119.2±1.11ms 119.3±0.40ms +0.08%
PCS-Proving 16/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16 145.0±0.81ms N/A N/A
PCS-Proving 18/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18 504.7±11.41ms 544.2±2.14ms +7.83%
PCS-Proving 18/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/18 6.9±0.01s 6.9±0.00s 0.00%
PCS-Proving 18/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/18 427.1±7.92ms 425.6±1.95ms -0.35%
PCS-Proving 18/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18 558.0±8.53ms N/A N/A
PCS-Proving 20/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20 2.1±0.03s 2.2±0.02s +4.76%
PCS-Proving 20/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/20 27.7±0.01s 27.6±0.02s -0.36%
PCS-Proving 20/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/20 1834.1±44.51ms 1811.2±21.14ms -1.25%
PCS-Proving 20/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20 2.4±0.04s N/A N/A
PCS-Verifying 10/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10 3.9±0.03ms 3.7±0.01ms -5.13%
PCS-Verifying 10/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/10 5.7±0.05ms 5.7±0.05ms 0.00%
PCS-Verifying 10/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/10 3.5±0.00ms 3.5±0.00ms 0.00%
PCS-Verifying 10/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10 3.7±0.02ms N/A N/A
PCS-Verifying 12/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12 4.1±0.04ms 3.9±0.00ms -4.88%
PCS-Verifying 12/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/12 9.2±0.11ms 9.0±0.06ms -2.17%
PCS-Verifying 12/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/12 3.6±0.02ms 3.6±0.00ms 0.00%
PCS-Verifying 12/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12 3.9±0.02ms N/A N/A
PCS-Verifying 14/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14 4.3±0.01ms 4.0±0.02ms -6.98%
PCS-Verifying 14/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/14 18.0±0.14ms 19.2±0.10ms +6.67%
PCS-Verifying 14/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/14 3.7±0.00ms 3.7±0.00ms 0.00%
PCS-Verifying 14/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14 4.0±0.01ms N/A N/A
PCS-Verifying 16/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16 4.4±0.02ms 4.0±0.01ms -9.09%
PCS-Verifying 16/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/16 49.6±0.85ms 49.5±0.16ms -0.20%
PCS-Verifying 16/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/16 3.8±0.01ms 3.8±0.00ms 0.00%
PCS-Verifying 16/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16 4.0±0.02ms N/A N/A
PCS-Verifying 18/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18 4.6±0.02ms 4.1±0.03ms -10.87%
PCS-Verifying 18/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/18 187.9±0.97ms 187.3±0.22ms -0.32%
PCS-Verifying 18/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/18 3.9±0.01ms 3.9±0.00ms 0.00%
PCS-Verifying 18/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18 4.1±0.01ms N/A N/A
PCS-Verifying 20/arecibo::provider::hyperkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20 4.7±0.06ms 4.1±0.03ms -12.77%
PCS-Verifying 20/arecibo::provider::ipa_pc::EvaluationEnginearecibo::provider::Bn256EngineIPA/20 729.8±10.08ms 731.5±2.81ms +0.23%
PCS-Verifying 20/arecibo::provider::non_hiding_zeromorph::ZMPCS<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineZM>/20 4.0±0.01ms 4.0±0.00ms 0.00%
PCS-Verifying 20/arecibo::provider::shplonk::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20 4.1±0.03ms N/A N/A

Copy link
Contributor

!benchmark action succeeded! 🚀

https://github.com/lurk-lab/arecibo/actions/runs/8053133064

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This is starting to look pretty good to me!

let d = f_x.coeffs.len();

// Compute h(x) = f(x)/(x - u)
let mut h = vec![E::Fr::ZERO; d];
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember you had doubts about the divide_with_q_and_r function which is indeed used in non_hiding_zeromorph::UVKZGPCS::open, which you could use here instead of reimplementing. FYI, I added tests of correctness in #344


// Phase 3 -- create response
let (w, evals) = kzg_open_batch(polys, &u, transcript);
// TODO: since this is a usual KZG10 we should use it as utility instead
Copy link
Contributor

Choose a reason for hiding this comment

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

And in a way that utility is there. I'm happy with code that mode or edits divide_with_q_and_r if you find it slower than it should be.

.collect::<Vec<E::Fr>>(),
);
let mut K_x = batched_Pi.clone();
K_x += &tmp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not opposed to adding impl<Fr> SubAssign<&Self> for UniPoly<Fr>, which would perhaps allow us to avoid the somewhat pedestrian iteration above by doing K_x -= &tmp here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added SubAssign in 2017748, using AddAssign's code actually with just changing plus on minus))

.collect::<Vec<_>>();

let pairing_result = E::multi_miller_loop(pairing_input_refs.as_slice()).final_exponentiation();
if pairing_result.is_identity().unwrap_u8() == 0x00 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is not the best way to unwrap a subtle::Choice: just use impl From<Choice> for bool, by calling .into()` here.

Copy link
Contributor Author

@storojs72 storojs72 Feb 28, 2024

Choose a reason for hiding this comment

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

We have Result<(), NovaError> as an output in verify function for EvaluationEngineTrait, so I can suggest:

let successful: bool = pairing_result.is_identity().into();
if !successful {
  return Err(NovaError::ProofVerifyError);
}
Ok(())

Comment on lines 65 to 71
NE: NovaEngine<GE = E::G1, Scalar = E::Fr, CE = KZGCommitmentEngine<E>>,
E::Fr: Serialize + DeserializeOwned,
E::G1Affine: Serialize + DeserializeOwned,
E::G2Affine: Serialize + DeserializeOwned,
E::G1: DlogGroup<ScalarExt = E::Fr, AffineExt = E::G1Affine>,
E::Fr: PrimeFieldBits,
<E::G1 as Group>::Base: TranscriptReprTrait<E::G1>,
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of points on this smorgasboard of constraints:

  • the bounds on E::Fr and E::G1Affine are implied by that on E::G1, and should be auto-satisfied. They should no longer be needed when associated_type_bounds are possible.
  • we probably want to put first the two sets of bounds that are actually defining what we need, then comment on what's going on with the rest:
  E: Engine,
  NE: NovaEngine<GE = E::G1, Scalar = E::Fr, CE = KZGCommitmentEngine<E>>,
  E::G1: DlogGroup<ScalarExt = E::Fr, AffineExt = E::G1Affine>,
  // the following bounds repeat existing, satisfied bounds on associated types of the above
  // but are required since the equality constraints we use in the above do not transitively carry bounds
  // we should be able to remove most of those constraints when rust supports associated_type_bounds
  E::Fr: Serialize + DeserializeOwned,
  E::G1Affine: Serialize + DeserializeOwned,
  E::G2Affine: Serialize + DeserializeOwned,
  E::Fr: PrimeFieldBits,
  <E::G1 as Group>::Base: TranscriptReprTrait<E::G1>,

@storojs72 storojs72 changed the title HyperKZG+Shplonk primary PCS verification HyperKZG+Shplonk primary PCS Feb 27, 2024
@storojs72
Copy link
Contributor Author

@huitseeker , I remember you initial request to keep PCS codebase as a separate module + utilising common code as much as possible, so I leave these tasks to myself for future patches in context of #304

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Excellent work!

Copy link
Contributor

@adr1anh adr1anh left a comment

Choose a reason for hiding this comment

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

This is looking great!

@storojs72 storojs72 added this pull request to the merge queue Feb 29, 2024
Merged via the queue into dev with commit ba34148 Feb 29, 2024
9 checks passed
@storojs72 storojs72 deleted the issue-302 branch February 29, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge Shplonk optimisation into HyperKZG
3 participants