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

Automatic generating of proof / public parameters JSONs #53

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

artem-bakuta
Copy link
Contributor

@artem-bakuta artem-bakuta commented Feb 8, 2024

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

@storojs72
Copy link
Member

!test

Copy link

github-actions bot commented Feb 8, 2024

End-to-end !test action succeeded! 🚀

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

@storojs72 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
Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

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")
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@artem-bakuta artem-bakuta Feb 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@storojs72 storojs72 Feb 12, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

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 ../../.")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good appointment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@samuelburnham samuelburnham left a comment

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Right. It is not needed

@artem-bakuta
Copy link
Contributor Author

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
Copy link
Member

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

It is 100% not a vain

@storojs72
Copy link
Member

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?

Copy link

End-to-end !test action succeeded! 🚀

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

@samuelburnham
Copy link
Member

samuelburnham commented Feb 13, 2024

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?

Copy link

End-to-end !test action succeeded! 🚀

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

@storojs72
Copy link
Member

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
Copy link
Contributor Author

artem-bakuta commented Feb 15, 2024

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

Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
- 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
Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
rust-reference-info.txt Outdated Show resolved Hide resolved
ci_pasta_keys_verifier_script.py Outdated Show resolved Hide resolved
@storojs72
Copy link
Member

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

Copy link

End-to-end !test action succeeded! 🚀

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

@storojs72 storojs72 self-requested a review February 19, 2024 17:32
Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

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

return variables


def add_function_before_last_brace(file_path, function_definition):
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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"

Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

Thanks, a lot, @artem-bakuta!

I'm ok to merge this!

- 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"
Copy link
Member

Choose a reason for hiding this comment

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

@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

@storojs72 storojs72 merged commit 017f91d into argumentcomputer:pasta Feb 20, 2024
1 check passed
@storojs72 storojs72 mentioned this pull request Feb 20, 2024
@artem-bakuta artem-bakuta deleted the pasta branch February 20, 2024 16:54
samuelburnham pushed a commit to samuelburnham/solidity-verifier that referenced this pull request Feb 20, 2024
…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>
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2024
* 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>
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.

4 participants