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

Update arkworks to 0.4.2 (compatible) #15940

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Aug 19, 2024

@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch 9 times, most recently from a17d527 to dac2318 Compare August 27, 2024 11:27
@volhovm
Copy link
Member Author

volhovm commented Aug 29, 2024

So with a small fix to lagrange_with_correctness (8427f58) I get this table below, which is almost no regression (2%, on my computer the baseline is 10.9sec). Still WIP trying to figure out /why/ this is happening.

 |--|--|--|--|--|--|--|
| 1| 0| 1| 1| 0.002689| 31.234906| SSS|
| 2| 1| 0| 2| 0.002189| 28.844748| SPS|
| 3| 1| 1| 0| 0.591425| 20.599770| SSP|
| 4| 2| 0| 1| 0.002820| 33.306827| SPP|
| 5| 0| 2| 0| 0.003296| 20.982033| SSSS|
| 6| 1| 1| 1| 0.003581| 31.269684| SPSS|
| 7| 2| 0| 2| 0.003182| 44.806364| SPPS|
| 8| 2| 1| 0| 0.003203| 33.706688| SSPP|
| 9| 3| 0| 1| 0.005633| 50.111081| SPPP|
| 10| 0| 2| 1| 0.003502| 29.794971| SSSSS|
| 11| 1| 1| 2| 0.003308| 43.466537| SPSSS|
| 12| 1| 2| 0| 0.003143| 31.687099| SSPSS|
| 13| 2| 0| 3| 0.003207| 57.165510| SPSPS|
| 14| 2| 1| 1| 0.002846| 45.451092| SPPSS|
| 15| 3| 0| 2| 0.003033| 60.535922| SPPPS|
| 16| 3| 1| 0| 0.003211| 51.672375| SSPPP|
| 17| 4| 0| 1| 0.005148| 62.339954| SPPPP|
[1] Total time was: 11.299m

@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch 3 times, most recently from 1a23041 to d19a8d5 Compare August 30, 2024 13:21
@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch from d19a8d5 to 74e1863 Compare September 10, 2024 17:53
@volhovm
Copy link
Member Author

volhovm commented Sep 10, 2024

After several tweaks (and the last bugfix addressing Projective::new_unchecked) the result is as follows. The total duration is the same as on compatible, but individual tests look a bit different from what the first post in this PR shows. Maybe it's just variance. I'll have to check.

 |--|--|--|--|--|--|--|
| 1| 0| 1| 1| 0.002259| 33.694302| SSS|
| 2| 1| 0| 2| 0.003644| 27.806821| SPS|
| 3| 1| 1| 0| 0.643404| 21.184189| SSP|
| 4| 2| 0| 1| 0.003087| 29.237815| SPP|
| 5| 0| 2| 0| 0.002623| 18.782681| SSSS|
| 6| 1| 1| 1| 0.003933| 29.008238| SPSS|
| 7| 2| 0| 2| 0.003149| 41.927683| SPPS|
| 8| 2| 1| 0| 0.003459| 36.408857| SSPP|
| 9| 3| 0| 1| 0.004387| 47.412234| SPPP|
| 10| 0| 2| 1| 0.002930| 29.062774| SSSSS|
| 11| 1| 1| 2| 0.003123| 40.635612| SPSSS|
| 12| 1| 2| 0| 0.003435| 35.129932| SSPSS|
| 13| 2| 0| 3| 0.003533| 50.753416| SPSPS|
| 14| 2| 1| 1| 0.003512| 44.899023| SPPSS|
| 15| 3| 0| 2| 0.003684| 53.183280| SPPPS|
| 16| 3| 1| 0| 0.003730| 46.482733| SSPPP|
| 17| 4| 0| 1| 0.002793| 59.126710| SPPPP|
[1] Total time was: 10.763m

And here's a second run, one minute faster than another. A lot of variance, but there seems to be no regression anymore:

 |--|--|--|--|--|--|--|
| 1| 0| 1| 1| 0.002019| 21.839172| SSS|
| 2| 1| 0| 2| 0.003117| 25.139927| SPS|
| 3| 1| 1| 0| 0.572486| 18.041869| SSP|
| 4| 2| 0| 1| 0.003350| 25.659761| SPP|
| 5| 0| 2| 0| 0.003087| 15.331588| SSSS|
| 6| 1| 1| 1| 0.002972| 25.199792| SPSS|
| 7| 2| 0| 2| 0.002867| 38.947949| SPPS|
| 8| 2| 1| 0| 0.002999| 27.164677| SSPP|
| 9| 3| 0| 1| 0.004256| 38.713006| SPPP|
| 10| 0| 2| 1| 0.003181| 27.512733| SSSSS|
| 11| 1| 1| 2| 0.003324| 39.920118| SPSSS|
| 12| 1| 2| 0| 0.002918| 30.049566| SSPSS|
| 13| 2| 0| 3| 0.004414| 50.216203| SPSPS|
| 14| 2| 1| 1| 0.003126| 40.666172| SPPSS|
| 15| 3| 0| 2| 0.003500| 51.471188| SPPPS|
| 16| 3| 1| 0| 0.003585| 42.233987| SSPPP|
| 17| 4| 0| 1| 0.003083| 55.831861| SPPPP|
[1] Total time was: 9.581m

@volhovm
Copy link
Member Author

volhovm commented Sep 11, 2024

Here's a summary of more runs on my machine. Seems that there is no regression anymore, the difference is most likely within error range potentially caused by uneven system load.


|         | Compatible | Arkworks 0.4.2 |
|---------+------------+----------------|
| SSS     |      22.98 |          23.06 |
| SPS     |      23.85 |          24.02 |
| SSP     |      16.25 |          16.28 |
| SPP     |      25.76 |          25.65 |
| SSSS    |      15.56 |          15.60 |
| SPSS    |      25.32 |          25.33 |
| SPPS    |      36.03 |          35.08 |
| SSPP    |      27.14 |          27.05 |
| SPPP    |      37.11 |          36.97 |
| SSSSS   |      25.18 |          25.23 |
| SPSSS   |      35.07 |          35.01 |
| SSPSS   |      26.81 |          27.16 |
| SPSPS   |      45.50 |          45.02 |
| SPPSS   |      36.83 |          36.72 |
| SPPPS   |      46.10 |          46.63 |
| SSPPP   |      38.16 |          38.57 |
| SPPPP   |      47.80 |          48.23 |
|---------+------------+----------------|
| Tot (m) |       8.87 |           8.87 |

Data from runs:

Compatible runs (80a2b55583a06381d50711799c7a6f801fef3dd1):

|         |       |       |       |       |       |       |       |       | Average |
|---------+-------+-------+-------+-------+-------+-------+-------+-------+---------|
| SSS     | 26.51 | 22.47 | 22.45 | 22.56 | 22.25 | 22.81 | 21.82 | 21.82 |   22.98 |
| SPS     | 23.98 | 23.74 | 23.87 | 23.99 | 23.73 | 24.43 | 23.23 | 23.71 |   23.85 |
| SSP     | 15.93 | 16.51 | 16.73 | 16.40 | 16.33 | 16.11 | 15.77 | 15.69 |   16.25 |
| SPP     | 25.37 | 25.57 | 25.81 | 27.18 | 25.82 | 25.49 | 25.09 | 24.64 |   25.76 |
| SSSS    | 15.45 | 15.49 | 15.66 | 16.46 | 15.45 | 15.35 | 15.09 | 15.08 |   15.56 |
| SPSS    | 24.96 | 25.36 | 25.48 | 27.10 | 24.86 | 25.13 | 24.40 | 25.14 |   25.32 |
| SPPS    | 35.30 | 40.58 | 35.67 | 37.06 | 34.43 | 34.75 | 34.45 | 33.92 |   36.03 |
| SSPP    | 26.79 | 27.50 | 26.81 | 28.58 | 26.94 | 26.79 | 26.58 | 26.07 |   27.14 |
| SPPP    | 36.23 | 37.01 | 38.52 | 38.68 | 36.52 | 37.04 | 35.83 | 36.08 |   37.11 |
| SSSSS   | 25.18 | 24.96 | 25.35 | 26.90 | 24.54 | 24.85 | 24.54 | 24.18 |   25.18 |
| SPSSS   | 34.56 | 34.90 | 36.24 | 37.44 | 34.06 | 34.49 | 33.83 | 35.07 |   35.07 |
| SSPSS   | 26.26 | 26.75 | 27.22 | 28.26 | 26.61 | 26.66 | 25.92 | 27.16 |   26.81 |
| SPSPS   | 44.08 | 44.90 | 47.90 | 49.20 | 44.54 | 44.86 | 43.04 | 44.74 |   45.50 |
| SPPSS   | 35.78 | 36.16 | 39.73 | 38.51 | 36.07 | 36.57 | 35.00 | 36.32 |   36.83 |
| SPPPS   | 45.90 | 46.87 | 46.88 | 46.86 | 45.32 | 46.41 | 44.52 | 45.96 |   46.10 |
| SSPPP   | 37.59 | 38.32 | 40.21 | 38.96 | 37.45 | 37.85 | 36.75 | 37.94 |   38.16 |
| SPPPP   | 47.34 | 48.09 | 48.26 | 48.69 | 47.66 | 48.06 | 46.51 | 48.36 |   47.80 |
|---------+-------+-------+-------+-------+-------+-------+-------+-------+---------|
| Tot (m) |  8.80 |  8.94 |  9.06 |  9.23 |  8.73 |  8.81 |  8.56 |  8.71 |    8.87 |
#+TBLFM: $10=vmean($2..$8)

Arkworks runs:

|         |       |       |       |       |       |       |       | Average |
|---------+-------+-------+-------+-------+-------+-------+-------+---------|
| SSS     | 26.97 | 22.11 | 22.06 | 22.12 | 22.66 | 22.75 | 22.79 |   23.06 |
| SPS     | 24.75 | 23.70 | 23.98 | 23.44 | 23.92 | 24.23 | 24.18 |   24.02 |
| SSP     | 16.43 | 16.00 | 16.24 | 16.14 | 16.30 | 16.44 | 16.44 |   16.28 |
| SPP     | 25.90 | 25.25 | 25.21 | 25.31 | 25.61 | 26.29 | 25.98 |   25.65 |
| SSSS    | 15.87 | 15.30 | 15.33 | 15.86 | 15.48 | 15.51 | 15.85 |   15.60 |
| SPSS    | 25.94 | 24.85 | 25.17 | 24.90 | 25.35 | 25.52 | 25.64 |   25.33 |
| SPPS    | 35.84 | 34.47 | 34.44 | 35.08 | 34.70 | 35.28 | 35.75 |   35.08 |
| SSPP    | 27.84 | 26.51 | 26.47 | 27.07 | 26.84 | 27.35 | 27.29 |   27.05 |
| SPPP    | 37.97 | 36.38 | 35.97 | 36.38 | 37.34 | 37.32 | 37.44 |   36.97 |
| SSSSS   | 25.84 | 24.58 | 25.90 | 24.83 | 24.83 | 25.54 | 25.10 |   25.23 |
| SPSSS   | 36.06 | 34.56 | 34.55 | 34.55 | 34.24 | 35.12 | 36.04 |   35.01 |
| SSPSS   | 27.44 | 27.75 | 26.63 | 26.85 | 26.92 | 27.62 | 26.95 |   27.16 |
| SPSPS   | 45.17 | 44.06 | 43.79 | 44.76 | 44.12 | 45.11 | 48.14 |   45.02 |
| SPPSS   | 37.21 | 35.75 | 35.78 | 36.36 | 36.36 | 36.76 | 38.82 |   36.72 |
| SPPPS   | 47.17 | 45.28 | 45.19 | 46.62 | 46.02 | 46.45 | 49.69 |   46.63 |
| SSPPP   | 39.32 | 37.55 | 37.25 | 38.31 | 37.63 | 39.06 | 40.93 |   38.57 |
| SPPPP   | 47.51 | 47.44 | 46.83 | 47.83 | 48.16 | 48.19 | 51.68 |   48.23 |
|---------+-------+-------+-------+-------+-------+-------+-------+---------|
| Tot (m) |  9.07 |  8.71 |  8.69 |  8.78 |  8.79 |  8.92 |  9.16 |    8.87 |
#+TBLFM: $9=vmean($2..$8)

@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch from 74e1863 to d5b3e13 Compare September 11, 2024 12:04
@volhovm volhovm marked this pull request as ready for review September 11, 2024 12:08
@volhovm volhovm requested a review from a team as a code owner September 11, 2024 12:08
@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch from d5b3e13 to f8e2e1a Compare September 11, 2024 12:13
@volhovm volhovm force-pushed the volhovm/arkworks042-compatible branch from f8e2e1a to c698f33 Compare September 11, 2024 12:15
@volhovm
Copy link
Member Author

volhovm commented Sep 11, 2024

!ci-build-me

@dannywillems
Copy link
Member

Testing running the same command than here.

@dannywillems
Copy link
Member

dannywillems commented Sep 12, 2024

This branch:

| No.| Proof updates| Non-proof pairs| Non-proof singles| Mempool verification time (sec)| Transaction proving time (sec)|Permutation|
 |--|--|--|--|--|--|--|
| 1| 0| 1| 1| 0.002135| 21.863263| SSS|
| 2| 1| 0| 2| 0.124713| 23.553421| SPS|
| 3| 1| 1| 0| 0.127687| 15.844198| SSP|
| 4| 2| 0| 1| 0.150672| 24.803744| SPP|
| 5| 0| 2| 0| 0.002753| 14.848090| SSSS|
| 6| 1| 1| 1| 0.121320| 24.348720| SPSS|
| 7| 2| 0| 2| 0.148621| 33.771574| SPPS|
| 8| 2| 1| 0| 0.144767| 25.758881| SSPP|
| 9| 3| 0| 1| 0.160497| 36.384535| SPPP|
| 10| 0| 2| 1| 0.003604| 27.288622| SSSSS|
| 11| 1| 1| 2| 0.132252| 36.008024| SPSSS|
| 12| 1| 2| 0| 0.132049| 27.229494| SSPSS|
| 13| 2| 0| 3| 0.153119| 46.314862| SPSPS|
| 14| 2| 1| 1| 0.159532| 38.238365| SPPSS|
| 15| 3| 0| 2| 0.173702| 50.008762| SPPPS|
| 16| 3| 1| 0| 0.188140| 40.835809| SSPPP|
| 17| 4| 0| 1| 0.215326| 51.996506| SPPPP|
[1] Total time was: 9.026m

On compatible (34af10d), I'm getting a segfault atm:

_build/default/src/app/cli/src/mina.exe transaction-snark-profiler --zkapps --k 1 --max-num-updates 4 --min-num-updates 2


Generated zkapp transactions with 3 updates and 0 proof updates in 0.007822 secs
Generated updates permutation 0: SSS
Updating authorizations...
[1]    1720068 segmentation fault (core dumped)  _build/default/src/app/cli/src/mina.exe transaction-snark-profiler --zkapps  

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

See above. LGTM reg benchmark otherwise.

src/lib/pickles/wrap_verifier.ml Show resolved Hide resolved
@volhovm
Copy link
Member Author

volhovm commented Sep 12, 2024

Re segfault: I also had this during the first time I was trying to reproduce the build, but then it disappeared. make clean can help. I'm compiling the project as follows:

nix develop mina#impure
opam switch import opam.export; eval $(opam env)
dune build src/app/cli/src/mina.exe
_build/default/src/app/cli/src/mina.exe transaction-snark-profiler --zkapps --k 1 --max-num-updates 4 --min-num-updates 2

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Sorry, I did not want to approve, yet.

@dannywillems
Copy link
Member

Re segfault: I also had this during the first time I was trying to reproduce the build, but then it disappeared. make clean can help. I'm compiling the project as follows:

nix develop mina#impure
opam switch import opam.export; eval $(opam env)
dune build src/app/cli/src/mina.exe
_build/default/src/app/cli/src/mina.exe transaction-snark-profiler --zkapps --k 1 --max-num-updates 4 --min-num-updates 2

Smart, smart... Forgot to recompile mina.exe... 🫤

@volhovm
Copy link
Member Author

volhovm commented Sep 16, 2024

Thanks @dannywillems! Merging now cc @georgeee @mrmr1993. In summary:

  • There's no regression anymore
  • I also ran the o1js tests and they all pass, following this instructions:
    clone o1js
    checkout the corresponding mina commit in src/mina and the proof-system commit in src/mina/.../proof-system
    npm i install all dependencies
    run npm run build:bindings (first check)
    run all tests (second check)

    npm run test
    npm run test:integration
    npm run test unit

    check regression tests (third test)

    ./run tests/vk-regression/vk-regression.ts --bundle 

That is I think it's a reasonable evidence we're still backward compatible for now.

@volhovm volhovm merged commit 4dff45d into compatible Sep 16, 2024
46 checks passed
@volhovm volhovm deleted the volhovm/arkworks042-compatible branch September 16, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants