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.

Sign up for GitHub

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

Jump to bottom

Automatic generating of proof / public parameters JSONs #53

Merged
merged 5 commits into from
Feb 20, 2024
Merged

Automatic generating of proof / public parameters JSONs #53

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b68041
added CI scrypt for generating pk and compression-snark from Nova rep…
asambler Feb 8, 2024
862e5ee
Changed CI e2e configuration to work with NOVA_URL and NOVA_COMMIT pa…
artem-bakuta Feb 15, 2024
0bb97f8
Changed Script name. Return back from generating test in runtime
artem-bakuta Feb 19, 2024
1cef728
Changed Script name. Return back from generating test in runtime
artem-bakuta Feb 19, 2024
20fe182
Changed Script and CI config.
artem-bakuta Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/end2end.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)


- 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


- name: Check proof verification status
run: |
Expand Down
24 changes: 24 additions & 0 deletions ci_pasta_keys_verifier_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import subprocess
import sys
import os

# python3 ci_pasta_keys_verifier_script.py https://github.com/lurk-lab/Nova.git d5f5fb5e557b99cb111f2d5693aa34fe30722750
if __name__ == "__main__":
# Configurations
nova_repo_url = sys.argv[1]
nova_commit_hash = sys.argv[2]
target_directory = "verify_cache"
nova_directory = "nova"

# Main logic
if os.path.exists(target_directory):
subprocess.run(['rm', '-rf', target_directory], check=True)
os.mkdir(target_directory)
os.mkdir(target_directory + "/" + nova_directory)
os.chdir(target_directory)
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 •
edited
Loading

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 •
edited
Loading

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?

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

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

Loading