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

Zeromorph EvaluationEngine trait #71

Merged
merged 17 commits into from
Oct 26, 2023
Merged

Zeromorph EvaluationEngine trait #71

merged 17 commits into from
Oct 26, 2023

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Oct 13, 2023

  • makes tests generic in the EvaluationEngine by adopting refactor: Parametrization of lib.rs tests for various EvaluationEngines #70,
  • documents the gap between ZM / IPA: "endianness" w/ which coefficients must be supplied to the scheme,
  • implements a CommitmentKey with a token universal setup, which allows implementing a CommitmentEngineTrait for KZG commitments,
  • implements the prove & verify methods for the EvaluationEngineTrait implementation for ZMPCS. The scheme is slow (due to poor univariate KZG batching)

Closes #19

TODO:

  • modify gen_srs_for_testing so the tests run in reasonable time.
  • cleaning up "point endianness" a bit

Next steps:

@huitseeker huitseeker requested a review from mpenciak October 13, 2023 20:44
@huitseeker huitseeker force-pushed the zeromorph_wip branch 2 times, most recently from 5f826c7 to d8b6c2f Compare October 14, 2023 12:31
@huitseeker huitseeker changed the title Zeromorph improvements Zeromorph EvaluationEngine trait Oct 14, 2023
@huitseeker huitseeker force-pushed the zeromorph_wip branch 4 times, most recently from a5bcb1c to bf4cf70 Compare October 24, 2023 17:55
- Added an additional test `test_dense_evaliations()` to provide more comprehensive testing for the `evaluate()` and `evaluate_opt()` functions in `MultilinearPolynomial`.
- Added serialization, deserialization, and Abomonation traits to UVUniversalKZGParam struct in `non_hiding_kzg.rs` file along with implementing comparison and length evaluation traits.
- Created a new file `kzg_commitment.rs` which implements KZG Commitment Engine and setup, commit functions.
- Integrated `kzg_commitment` in module `provider` and set up conversions between Commitment and UVKZGCommitment.
- Enhanced assertion in `minroot_serde.rs` file from clone comparison to dereferenced comparison in MinRoot delay case.
- Updated `prove` and `verify` methods in `ZMPCS<E>` struct within `non_hiding_zeromorph.rs` with actual logic for commitment, evaluation, and verification.
- Adjusted the construction of `ZMCommitment` and `ZMEvaluation` within `prove` and `verify` methods.
- Commented on portions of the code in `non_hiding_zeromorph.rs` that need further modification and refinement.
- Modified test value for `test_pp_digest_with` in the `test_supernova_pp_digest` test case to match the new expected output.
@huitseeker huitseeker marked this pull request as ready for review October 24, 2023 18:55
Copy link
Contributor

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

Shouldn't we use pp-spartan by default in Arecibo, since we consider it more efficient and Ethereum-friendly than "regular" Spartan? At least I would propose mandatory having e2e proof/verify test instances in lib.rs for Bn256/Grumpkin curve cycle as we consider it as default in our solidity-verifier.

Trying to run test_ivc_nontrivial_with_compression with zeromorph stuff using pp-spartan, e.g.:

test_ivc_nontrivial_with_compression_with::<
      bn256::Point,
      grumpkin::Point,
      SPrime<bn256::Point, ZM<halo2curves::bn256::Bn256>>,
      SPrime<grumpkin::Point, EE<_>>,
    >();

gives me following assertion error (at setup phase):

thread 'tests::test_ivc_nontrivial_with_compression' panicked at 'assertion failed: ck.length() >= Self::commitment_key_floor()(S)', src/spartan/ppsnark.rs:912:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::test_ivc_nontrivial_with_compression ... FAILED

src/lib.rs Show resolved Hide resolved
@huitseeker
Copy link
Contributor Author

@storojs72 your point about the default doesn't really apply to Arecibo, since there isn't any "default" per se, Arecibo/Nova just provides the two APIs (compression, spark_compression) side-by-side. But it does apply to Lurk!

Trying to run test_ivc_nontrivial_with_compression with zeromorph stuff using pp-spartan, e.g.:

Yes, that's normal and due to microsoft/Nova#203, or #5 here. test_ivc_nontrivial_with_compression is meant to run with spartan but not computational commitment. The test you're looking for is test_ivc_nontrivial_with_spark_compression a few lines downstream. The reason it doesn't exhibit the same error is in the initialization of the public parameters.

@@ -1330,7 +1326,7 @@ mod tests {
test_ivc_nontrivial_with_compression_with::<
bn256::Point,
grumpkin::Point,
S<bn256::Point, EE<_>>, // SZM<bn256::Point, halo2curves::bn256::Bn256>,
S<bn256::Point, ZM<halo2curves::bn256::Bn256>>,
Copy link
Contributor Author

@huitseeker huitseeker Oct 25, 2023

Choose a reason for hiding this comment

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

This translates the former test_ivc_nontrivial_with_zm_compression

@huitseeker huitseeker force-pushed the zeromorph_wip branch 2 times, most recently from ceaf9ae to d89fd77 Compare October 26, 2023 13:14
@huitseeker huitseeker force-pushed the zeromorph_wip branch 4 times, most recently from 1711f39 to 729ba78 Compare October 26, 2023 17:29
Copy link
Contributor

@mpenciak mpenciak 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!

@huitseeker huitseeker added this pull request to the merge queue Oct 26, 2023
Merged via the queue into zeromorph with commit 6f677be Oct 26, 2023
2 checks passed
@huitseeker huitseeker deleted the zeromorph_wip branch October 26, 2023 20:24
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