Automatic generating of proof / public parameters JSONs #53

artem-bakuta · 2024-02-08T20:53:05Z

Added script for autogenerating vk.json and compressed-snark.json for pasta.

Link to the script will also attach: script

It's also added in PR.

Partially fixes #24

References argumentcomputer/Nova#38

…o commit

storojs72 · 2024-02-08T22:00:10Z

!test

github-actions · 2024-02-08T22:01:31Z

End-to-end !test action succeeded! 🚀

https://github.com/lurk-lab/solidity-verifier/actions/runs/7836487677

storojs72

Thanks a lot!

I would like to have @samuelburnham input on this as well.

Firstly, let's rename the script to something more expressive, for instance: generate_contract_input.py

Secondly, it looks like the invocation of Python script for generating JSONs should be something like following:

python generate_contract_input.py <URL to Rust reference> <Commit Hash> <Actual cargo command or at least test name>

e.g.

python generate_contract_input.py 'https://github.com/lurk-lab/Nova.git' 'd5f5fb5e557b99cb111f2d5693aa34fe30722750' 'cargo +nightly test solidity_compatibility_e2e_pasta --release -- --ignored'

that would give us flexibility and allow changing parameters of JSON generating (we may have different URLs Nova / Arecibo, different commits and also different ways to run the Rust test application in order to generate required JSONs).

P.S. We can evaluate changes only with subsequent PRs, so I would suggest removing hardcoded JSONs as follow-up PR and see if it will work

storojs72 · 2024-02-09T13:42:21Z

.github/workflows/end2end.yml

@@ -55,9 +55,13 @@ jobs:
          echo "CONTRACT_ADDRESS=$(forge script script/Deployment.s.sol:NovaVerifierDeployer --fork-url $ANVIL_URL --private-key $ANVIL_PRIVATE_KEY --broadcast --non-interactive | sed -n 's/.*Contract Address: //p' | tail -1)" >> $GITHUB_OUTPUT
        id: deployment

+      - name: Generate proof and public parameters from commit hash
+        run: |
+          python ci_pasta_keys_verifier_script.py https://github.com/lurk-lab/Nova.git d5f5fb5e557b99cb111f2d5693aa34fe30722750


I wouldn't keep this information as a part of CI configuration. Instead, I would prefer explicitly having additional file in the repository with URL and commit hash (let's say rust-reference-info.txt or any other format convenient to utilise by CI). Then, during this step, CI should read that info and pass it as input to Python script.

Agreed, but I think even better would be to keep them as repository variables, e.g. $NOVA_URL and $NOVA_COMMIT

Agreed, but I think even better would be to keep them as repository variables, e.g. $NOVA_URL and $NOVA_COMMIT

That simplifies fetching the information about reference in CI config, but we are losing: 1) some explicitness (as definition of those variables are hidden in repository settings) and 2) some flexibility if we want to target multiple branches (which is the case, as soon as we have pasta, grumpkin and I believe eventually we will need e2e testing in gas_optimising and main)

storojs72 · 2024-02-09T13:45:46Z

ci_pasta_keys_verifier_script.py

+    os.system(f"git clone {nova_repo_url} {nova_directory}")
+    os.chdir(nova_directory)
+    os.system(f"git checkout {nova_commit_hash}")
+    os.system(f"cargo +nightly test solidity_compatibility_e2e_pasta --release -- --ignored")


I would make the test name (solidity_compatibility_e2e_pasta ) also configurable using additional flag. That would allow us changing the application that generates JSONs for us if necessary (for Grumpkin, for example we will use another test).

how do you look to just to rename test solidity_compatibility_e2e ? All other steps seems to be the same for generating keys for Grumpkin.

For grumpkin, I believe you will need separate test, something like:

// cargo +nightly test test_ivc_nontrivial_with_compression_grumpkin --release -- --nocapture --ignored #[test] #[ignore] fn solidity_compatibility_e2e_grumpkin() { test_ivc_nontrivial_with_compression_with::<bn256::Point, grumpkin::Point>(true); }

at Nova. So I don't know how to reuse single test for both pasta / grumpkin JSONs generating.

And also I don't have 100% sure that I used test_ivc_nontrivial_with_compression for Grumpkin JSONs initially. We will need to detect the correct commit and test name similarly as we did for pasta (by comparing md5 hashes)

how do you resolve this in branch were both pasta / grumpkin are located ?

I think, we can add one more commit with mentioned test for grumpkin on top of argumentcomputer/Nova@ea4f75c. Then, using new commit hash, your script will be able to run two distinct tests and generate appropriate JSONs for each necessary branch in solidity-verifier. Does that make sense?

storojs72 · 2024-02-09T13:54:53Z

ci_pasta_keys_verifier_script.py

+    os.system(f"git checkout {nova_commit_hash}")
+    os.system(f"cargo +nightly test solidity_compatibility_e2e_pasta --release -- --ignored")
+    print("Copy generated keys form Nova...")
+    os.system(f"cp vk.json compressed-snark.json ../../.")


Before copying, I would also add explicit check that two files exist and exit(1) if they are absent. That way we would definitely know that possible issues with the whole test is exactly at running this script

good appointment

better to delete already saved keys and throw an exception if new generated keys will fail.

Yeah, the intention is actually getting some panic which allows us detecting problematic CI job's step

samuelburnham

Looks great! I generally agree with the points made by @storojs72, but I wouldn't over-optimize the CI UX in this PR as we can do more customization in future work.

For example, if the integration test parameters change frequently between runs, we could take them as input in the !test comment itself.

samuelburnham · 2024-02-09T18:13:49Z

.github/workflows/end2end.yml

@@ -55,9 +55,13 @@ jobs:
          echo "CONTRACT_ADDRESS=$(forge script script/Deployment.s.sol:NovaVerifierDeployer --fork-url $ANVIL_URL --private-key $ANVIL_PRIVATE_KEY --broadcast --non-interactive | sed -n 's/.*Contract Address: //p' | tail -1)" >> $GITHUB_OUTPUT
        id: deployment

+      - name: Generate proof and public parameters from commit hash
+        run: |
+          python ci_pasta_keys_verifier_script.py https://github.com/lurk-lab/Nova.git d5f5fb5e557b99cb111f2d5693aa34fe30722750


Agreed, but I think even better would be to keep them as repository variables, e.g. $NOVA_URL and $NOVA_COMMIT

samuelburnham · 2024-02-09T18:14:32Z

ci_pasta_keys_verifier_script.py

+    os.system(f"git clone {nova_repo_url} {nova_directory}")
+    os.chdir(nova_directory)
+    os.system(f"git checkout {nova_commit_hash}")
+    os.system(f"cargo +nightly test solidity_compatibility_e2e_pasta --release -- --ignored")


Why is +nightly needed here? It seems to run fine without this flag in Nova

Right. It is not needed

artem-bakuta · 2024-02-10T18:20:27Z

It is good to know that the work was not done in vain. I will change the script to take params from the local file.

storojs72 · 2024-02-12T14:53:00Z

It is good to know that the work was not done in vain

It is 100% not a vain

storojs72 · 2024-02-13T12:59:53Z

For example, if the integration test parameters change frequently between runs, we could take them as input in the !test comment itself.

@samuelburnham, that is a good idea. I think we can combine both approaches. With !test comment we can run integration tests with default parameters explicitly specified in the file committed to to the repository. At the same time, if there are some additional input in !test comment, CI can try running the flow using this input instead of default one from file. WDYT?

github-actions · 2024-02-13T13:00:58Z

End-to-end !test action succeeded! 🚀

https://github.com/lurk-lab/solidity-verifier/actions/runs/7886885034

samuelburnham · 2024-02-13T16:18:57Z

With !test comment we can run integration tests with default parameters explicitly specified in the file committed to to the repository. At the same time, if there are some additional input in !test comment, CI can try running the flow using this input instead of default one from file. WDYT?

@storojs72 sounds good, I think to keep this PR simple we should use the input file in all cases and then we can configure the ! test comment (~~edited to not trigger e2e tests~~ it still triggered 🤦 I'll just say test from now on) in another PR. Would you mind opening an issue with more context on the inputs required, e.g. Nova URL and commit hash?

github-actions · 2024-02-13T16:20:34Z

End-to-end !test action succeeded! 🚀

https://github.com/lurk-lab/solidity-verifier/actions/runs/7889516043

storojs72 · 2024-02-13T17:24:00Z

Would you mind opening an issue with more context on the inputs required, e.g. Nova URL and commit hash

Sure! Let's get the modified script from @artem-bakuta and then iterate over required CI changes

…rams. Python script changed to work with any version of nova commit. PS: Previous test on nova/src/lib.rs can be removed.

artem-bakuta · 2024-02-15T12:42:40Z

!test NOVA_URL=https://github.com/lurk-lab/Nova.git NOVA_COMMIT=d5f5fb5e557b99cb111f2d5693aa34fe30722750

storojs72

The approach when we are actually patching Nova reference implementation using Rust code blocks from the script is interesting, but it forces us to encapsulate Rust code (which is difficult to maintain) in Python and there are no guarantees that it will work in future, when with explicit solidity_compatibility_e2e test at Nova side - we actually delegate the problem of maintainability to Nova developers.

Plus, in future, by having compatibility tests at Nova side, we can enforce Nova developers to avoid committing changes to Nova that breaks compatibility with solidity-verifier without strong arguments of doing that

storojs72 · 2024-02-15T14:31:19Z

.github/workflows/end2end.yml

+      - name: Parse PR comment
+        id: parse-comment
+        run:
+            # Extract NOVA_URL and NOVA_COMMIT from the comment body


@samuelburnham , could you please check how this step is implemented?

I think these changes are out of scope for this PR, I'd prefer to just use the hardcoded values for $NOVA_URL and $NOVA_COMMIT in CI for now.

Agree. @artem-bakuta, let's leave it to @samuelburnham as he knows how to test this properly having wider permissions in LurkLab Github org. So please leave just:

echo "::set-env name=NOVA_URL::https://github.com/lurk-lab/Nova" echo "::set-env name=NOVA_COMMIT::ea4f75c225cb29f523780858ec84f1ff51c229bc"

.github/workflows/end2end.yml

storojs72 · 2024-02-15T14:34:46Z

.github/workflows/end2end.yml

      - name: Load proof and public parameters
        run: |
-          python loader.py pp-verifier-key.json pp-compressed-snark.json ${{steps.deployment.outputs.CONTRACT_ADDRESS}} $ANVIL_URL $ANVIL_PRIVATE_KEY
+          python loader.py vk.json compressed-snark.json ${{steps.deployment.outputs.CONTRACT_ADDRESS}} $ANVIL_URL $ANVIL_PRIVATE_KEY


I would leave this change for the next PR, when we remove hardcoded pp-verifier-key.json and pp-compressed-snark.json from the repository and actually try autogenerating

ci_pasta_keys_verifier_script.py

rust-reference-info.txt

ci_pasta_keys_verifier_script.py

storojs72 · 2024-02-18T16:44:11Z

!test NOVA_URL=https://github.com/lurk-lab/Nova.git NOVA_COMMIT=d5f5fb5e557b99cb111f2d5693aa34fe30722750

github-actions · 2024-02-18T16:45:24Z

End-to-end !test action succeeded! 🚀

https://github.com/lurk-lab/solidity-verifier/actions/runs/7950371692

storojs72

In general, I support merging this, with two following discussions finally resolved: #53 (comment), #53 (comment)

storojs72 · 2024-02-19T17:29:59Z

generate_contract_input.py

+    return variables
+
+
+def add_function_before_last_brace(file_path, function_definition):


Do we use this function in new (last) version of script? If not, I would either remove it or comment it if it is needed for some future developments

storojs72 · 2024-02-19T17:35:30Z

.github/workflows/end2end.yml

+      - name: Parse PR comment
+        id: parse-comment
+        run:
+            # Extract NOVA_URL and NOVA_COMMIT from the comment body


Agree. @artem-bakuta, let's leave it to @samuelburnham as he knows how to test this properly having wider permissions in LurkLab Github org. So please leave just:

echo "::set-env name=NOVA_URL::https://github.com/lurk-lab/Nova" echo "::set-env name=NOVA_COMMIT::ea4f75c225cb29f523780858ec84f1ff51c229bc"

storojs72

Thanks, a lot, @artem-bakuta!

I'm ok to merge this!

storojs72 · 2024-02-20T13:48:29Z

.github/workflows/end2end.yml

+      - name: Set environment variables for generate_contract_input.py (Nova URL and commit)
+        id: parse-comment
+        run:
+          echo "::set-env name=NOVA_URL::https://github.com/lurk-lab/Nova"


@samuelburnham , as we discussed - it would be nice to make specifying NOVA_URL and NOVA_COMMIT via ! test comment. In case if they are specified in comment, it is necessary to take them instead of extracting from rust-reference-info.txt. As you are able to carefully test this behaviour it is better to have a separate PR from you

…uter#53) * added CI scrypt for generating pk and compression-snark from Nova repo commit * Changed CI e2e configuration to work with NOVA_URL and NOVA_COMMIT params. Python script changed to work with any version of nova commit. PS: Previous test on nova/src/lib.rs can be removed. * Changed Script name. Return back from generating test in runtime * Changed Script name. Return back from generating test in runtime * Changed Script and CI config. --------- Co-authored-by: Artem <asamblers@gmail.com>

* Automatic generating of proof / public parameters JSONs (#53) * added CI scrypt for generating pk and compression-snark from Nova repo commit * Changed CI e2e configuration to work with NOVA_URL and NOVA_COMMIT params. Python script changed to work with any version of nova commit. PS: Previous test on nova/src/lib.rs can be removed. * Changed Script name. Return back from generating test in runtime * Changed Script name. Return back from generating test in runtime * Changed Script and CI config. --------- Co-authored-by: Artem <asamblers@gmail.com> * Remove hardcoded JSONs (#54) * Remove hardcoded JSONs * Update CI configuration * Drop end2end.yml as it is not relevant for custom branches * ci: Configure `!test` comment * Address feedback --------- Co-authored-by: artem-bakuta <47213324+artem-bakuta@users.noreply.github.com> Co-authored-by: Artem <asamblers@gmail.com> Co-authored-by: Artem Storozhuk <storojs72@gmail.com>

added CI scrypt for generating pk and compression-snark from Nova rep…

9b68041

…o commit

storojs72 changed the title ~~Added CI script for generating pk and compression-snark from Nova repository commit~~ Automatic generating of proof / public parameters JSONs Feb 9, 2024

storojs72 requested changes Feb 9, 2024

View reviewed changes

storojs72 requested a review from samuelburnham February 9, 2024 13:59

samuelburnham reviewed Feb 9, 2024

View reviewed changes

Changed CI e2e configuration to work with NOVA_URL and NOVA_COMMIT pa…

862e5ee

…rams. Python script changed to work with any version of nova commit. PS: Previous test on nova/src/lib.rs can be removed.

artem-bakuta requested a review from storojs72 February 15, 2024 12:38

storojs72 requested changes Feb 15, 2024

View reviewed changes

artem-bakuta added 2 commits February 19, 2024 11:07

Changed Script name. Return back from generating test in runtime

0bb97f8

Changed Script name. Return back from generating test in runtime

1cef728

storojs72 self-requested a review February 19, 2024 17:32

storojs72 reviewed Feb 19, 2024

View reviewed changes

Changed Script and CI config.

20fe182

artem-bakuta requested a review from storojs72 February 20, 2024 13:16

storojs72 approved these changes Feb 20, 2024

View reviewed changes

storojs72 merged commit 017f91d into argumentcomputer:pasta Feb 20, 2024
1 check passed

storojs72 mentioned this pull request Feb 20, 2024

Remove hardcoded JSONs #54

Merged

artem-bakuta deleted the pasta branch February 20, 2024 16:54

samuelburnham mentioned this pull request Feb 20, 2024

ci: Automatic Nova parameter generation & testing #55

Merged

storojs72 mentioned this pull request Mar 6, 2024

Removing hardcoded JSONs (grumpkin) #62

Merged

		return variables


		def add_function_before_last_brace(file_path, function_definition):

Automatic generating of proof / public parameters JSONs #53

Automatic generating of proof / public parameters JSONs #53

Conversation

artem-bakuta commented Feb 8, 2024 • edited by storojs72 Loading

storojs72 commented Feb 8, 2024

github-actions bot commented Feb 8, 2024

storojs72 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-bakuta Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

storojs72 Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelburnham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-bakuta commented Feb 10, 2024

storojs72 commented Feb 12, 2024

storojs72 commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

samuelburnham commented Feb 13, 2024 • edited Loading

github-actions bot commented Feb 13, 2024

storojs72 commented Feb 13, 2024

artem-bakuta commented Feb 15, 2024 • edited Loading

storojs72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 commented Feb 18, 2024

github-actions bot commented Feb 18, 2024

storojs72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-bakuta commented Feb 8, 2024 •

edited by storojs72

Loading

storojs72 left a comment •

edited

Loading

artem-bakuta Feb 10, 2024 •

edited

Loading

storojs72 Feb 12, 2024 •

edited

Loading

samuelburnham commented Feb 13, 2024 •

edited

Loading

artem-bakuta commented Feb 15, 2024 •

edited

Loading