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

SRS caching #192

Merged
merged 25 commits into from
Oct 31, 2023
Merged

SRS caching #192

merged 25 commits into from
Oct 31, 2023

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented Oct 25, 2023

Cache SRS and Lagrange basis

This

  • relies on Parallel SRS creation proof-systems#1300 to create SRS in parallel
  • moves bindings implementation of srs and lagrange commitments creation to TS
  • in the new implementation, supports user-definable cache to read/write srs and lagrange basis for different domain sizes

@mitschabaude mitschabaude changed the base branch from main to feature/pickles-js-caching October 25, 2023 12:31
@mitschabaude mitschabaude marked this pull request as ready for review October 31, 2023 08:32
@mitschabaude mitschabaude requested a review from a team as a code owner October 31, 2023 08:32
@@ -0,0 +1,31 @@
#!/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

Could be nice to run https://www.shellcheck.net/, in CI and locally.

scripts/update-wasm-and-types.sh Show resolved Hide resolved
scripts/update-wasm-and-types.sh Show resolved Hide resolved
scripts/update-wasm-and-types.sh Show resolved Hide resolved

impl_srs!(caml_fp_srs, WasmPastaFp, WasmG, Fp, G, WasmPolyComm, Fp);

#[wasm_bindgen]
Copy link
Member

Choose a reason for hiding this comment

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

It seems it is duplicated code that can be parametrized by the curve (I might be wrong by reading only GitHub panel). Can it be moved into the macro impl_srs!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right, could be in the macro. I think it's easier to maintain outside the macro, see my slack thread here: https://o1-labs.slack.com/archives/C01UNQH8XEZ/p1698141695847899

Copy link
Member

Choose a reason for hiding this comment

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

Mmh, I see. I understand your point, and I do agree (I hate macros...). However, IMO, it is good to stay with one convention, and maybe initiate a refactorization after.
Not a blocking comment for this PR, only an opinion.

Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

lgtm, cant really say too much about the Rust part tho!

unsafe { &mut *(std::sync::Arc::as_ptr(&srs) as *mut _) };
let domain = EvaluationDomain::<$F>::new(1 << (log2_size as usize)).expect("invalid domain size");
ptr.add_lagrange_basis(domain);
crate::rayon::run_in_pool(|| {
Copy link
Member

Choose a reason for hiding this comment

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

looks like a simple but powerful change lol

crypto/bindings/srs.ts Outdated Show resolved Hide resolved
crypto/bindings/srs.ts Outdated Show resolved Hide resolved
crypto/bindings/srs.ts Outdated Show resolved Hide resolved

impl_srs!(caml_fp_srs, WasmPastaFp, WasmG, Fp, G, WasmPolyComm, Fp);

#[wasm_bindgen]
Copy link
Member

Choose a reason for hiding this comment

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

Mmh, I see. I understand your point, and I do agree (I hate macros...). However, IMO, it is good to stay with one convention, and maybe initiate a refactorization after.
Not a blocking comment for this PR, only an opinion.

scripts/update-wasm-and-types.sh Show resolved Hide resolved
Base automatically changed from feature/pickles-js-caching to main October 31, 2023 13:34
@mitschabaude mitschabaude merged commit be0e3c5 into main Oct 31, 2023
@mitschabaude mitschabaude deleted the feature/parallel-srs-create branch October 31, 2023 14:08
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