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

Add a command to download any SRS files of any size #37

Merged
merged 13 commits into from
Feb 23, 2024

Conversation

ppoliani
Copy link
Contributor

@ppoliani ppoliani commented Feb 12, 2024

Delegate the downloading of SRS files to the end-user.

Based on the discussion here #36 and here #7

@ppoliani ppoliani changed the title Refactor/remove srs Add a command to download any SRS files of any size Feb 12, 2024
Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR! I also apologize because I know I'm on the hook to review the other ones :)

In addition to the comment I pointed out, I'm wondering if we want to follow this direction. So the question is: do we want to let the application manage the SRS, or do we want the user to do so? In the latter case, we would just ask the user to pass a path to the SRS.

I like the latter path for two reasons:

  1. some applications might want to use their own SRS
  2. the request::get is probably much less optimized than a wget for large files (and some of the useful SRS can get really large!)

wdyt?

let srs_path = download_srs().await;

// always check integrity of file
let hash = sha256::try_digest(&srs_path).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should keep the checks on the digest!

@ppoliani
Copy link
Contributor Author

@mimoo I don't have a particular preference to be honest. What you say make sense to me. In fact the first version of this PR worked the way you described 😆

Let me revert some of the commits.

@ppoliani
Copy link
Contributor Author

@mimoo I've removed the new command. Users would have to provide the srs-path instead.

@@ -26,6 +26,14 @@ To install `snarkjs`, just run:
npm install -g snarkjs@latest
```

### Download SRS File

To create a ZKP you would need to download the correct SRS file (based on your circuit size). For example, if your circuit has around 65K constraints then you would need to download the following file https://storage.googleapis.com/zkevm/ptau/powersOfTau28_hez_final_16.ptau.
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the slow review, we should add a link to https://github.com/iden3/snarkjs#7-prepare-phase-2 which is more user friendly

Copy link
Contributor

Choose a reason for hiding this comment

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

also it'd be good to add a wget premade command for the lazy asses

Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

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

nice! we should probably download that 16 srs in CI for the tests to run properly

@mimoo mimoo merged commit 3155c95 into sigma0-dev:main Feb 23, 2024
1 check passed
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.

2 participants