-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: implement Zeromorph #145
Conversation
070ca9a
to
6b524d6
Compare
1f1e35a
to
f9fe962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work! The math all checks out, and was super easy to understand. Left some small nits but they are not blocking.
pub fn open( | ||
prover_param: impl Borrow<UVKZGProverKey<E>>, | ||
polynomial: &UVKZGPoly<E::Fr>, | ||
point: &E::Fr, | ||
) -> Result<(UVKZGProof<E>, UVKZGEvaluation<E>), NovaError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of C-CALLER-CONTROL
, the evaluation polynomial(point)
may already have been computed by the caller, and could be passed on directly. If not, then they can explicitly compute it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sort of the same problem, that is to properly check (at least in debug mode) that the passed evaluation is the correct one, we'd need PartialEq
on Engine
, and the derivation of PartialEq
on UVKZGEvaluation
will indeed introduce that E: PartialEq
bound.
While this didn't really matter on UVUniversalKZGParam
(those parameters are in some sense Engine
-agnostic), it arguably does here: you would not want an evaluation ear-marked for an E1: Engine
to be used with E2 != E1
.
This should in the end resolve by adding that bound to Engine
in Nova (see also the note in comments1 for another bound hat should be moved) and the code will be ripe to add that issue then. I'd like to open an issue to refactor this once that happens.
Footnotes
-
"due to the move of the bound
TranscriptReprTrait<G>
onG::Base
fromGroup
toEngine
" ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, it follows the paper description closely so I'm giving it an approval.
Remove toxic waste from setup parameters Refactorings ZeroMorph skeleton
Drafts a non-hiding variant of the KZG PCS on univariate Dense polynomial representations Details: - Changed access levels and ordering for various modules and functions like `sumcheck`, `cpu_best_multiexp`, and `mod kzg;`. - Upgraded `halo2curves` version and added new dependencies like `pairing`, `anyhow`, `rand`, and `group`. - Leveraged the UniPoly in sumcheck.rs - Created `non_hiding_kzg.rs` file, introducing new structures and functionalities like `UVUniversalKZGParam`, `UVKZGProverKey`, `UVKZGPoly`, and `UVKZGPCS` along with their implementation and tests. - Modified the import of `Engine` in `kzg.rs` from `halo2curves::pairing::Engine` to `pairing::Engine` (reflecting the new version of halo2curves). feat: add batch commit / open / prove
Minor adjustments
refactor: Remove kzg and zeromorph provider modules
340fca4
to
747fc7d
Compare
- Added new data structures and their related functions including the `ZMProverKey`, `ZMVerifierKey`, `ZMCommitment`, `ZMEvaluation`, `ZMProof` and `ZMPCS` in `non_hiding_zeromorph.rs` - Implemented new functionalities in `non_hiding_zeromorph.rs` and `spartan/polynomial.rs` to provide better handling of polynomials, including fetching commitments for a polynomial, computing the quotient polynomials of an existing polynomial. - Enhanced the `UniPoly` struct in `spartan/sumcheck.rs` with `Index` and `IndexMut` for better coefficient access, and `AddAssign` and `MulAssign` for scalar and self types. - Removed and replaced certain elements in `UVUniversalKZGParam` struct in `non_hiding_kzg.rs` for improved flexibility, and included import and implementation of `TranscriptReprTrait`. draft ZM verify set up test structure defer test_quo
66658a8
to
bb374f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huitseeker, thanks for that complicated work, which is done so diligently! I don't have remarks - just couple questions
let vk = vk.borrow(); | ||
|
||
// Receive commitments [q_k] | ||
proof.ck.iter().for_each(|c| transcript.absorb(b"quo", c)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the maximum size of proof.ck
field that we expect currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5 answers this, roughly :
- for anything but the pre-processing Spartan (ppsnark):
max(num_cons, num_var)
, - for the pre-processing Spartan:
max(max(num_cons, num_var), A.len()+B.len()+C.len())
where A, B, C are the R1CS matrices involved.
The variables above are relative to your step circuit, not the overall proof.
let len = label.len().min(32); | ||
bytes[..len].copy_from_slice(&label[..len]); | ||
let rng = &mut StdRng::from_seed(bytes); | ||
UVUniversalKZGParam::gen_srs_for_testing(rng, n.next_power_of_two()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curious - can we reuse the existing setup data grabbed from some production chain with current UVUniversalKZGParam
or it will require some adaptation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Universal KZG setups don't usually have many G2 points, and we need one with a suitable offset here:
https://github.com/lurk-lab/arecibo/pull/145/files#diff-da2ddb6b2715e51565732ee43533e01e2da3b5f3af045e2f9beb71e14c716f1eR61
So yes, it will require some adaptation. We plan to compare with the upstream implementation of MLKZG microsoft/Nova#269 and strategize from there (/cc @adr1anh )
- Temporarily disabled some assertions in both `non_hiding_zeromorph.rs` and `polynomial.rs` for debugging purposes. - Introduced `evaluate_opt`, `fix_variables`, and `fix_one_variable_helper` functions in `polynomial.rs` to support multilinear polynomial evaluation and partial evaluation. fix: add domain separators - Integrated additional functionality into the ZMPCS implementation in `non_hiding_zeromorph.rs` by adding a protocol name function. - Improved security in `non_hiding_zeromorph.rs` by integrating domain separators into transcript's `open` and `verify` functions. refactor: Refactor code and test performance for NHZM - Implement `AsRef` trait for `UniPoly` structure in `src/spartan/sumcheck.rs` to facilitate direct access to its coefficients. - Refactor code in `src/provider/non_hiding_zeromorph.rs` to directly use `into_iter` in map function when creating `quotients_polys`, avoiding a large clone - Enhance test performance in `src/provider/non_hiding_zeromorph.rs` by pre-generating a `universal_setup` for tests, introducing a `max_vars` variable and RNG in the `commit_open_verify_with` test function, and adjusting the range of num_vars used for testing. fix: adjust APIs & Serialize impls for the Nova traits
fix: ignore panicking test fix: add doctest for evaluate_opt fix: remove obsolete comments chore: move UniPoly methods where they should be test: make clear current zeromorph operates in monomial basis - Added an additional test `test_dense_evaliations()` to provide more comprehensive testing for the `evaluate()` and `evaluate_opt()` functions in `MultilinearPolynomial`. refactor TranscriptReprTrait impl for compat with Commitments
- 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.
feat: Improve `prove` and `verify` methods in `ZMPCS` struct - 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.
fix: remove superfluous eval functions fix: remove endianness shenanigans test: add evaluation unit test
… computation - Introduced a new module `util` within the `provider` module, implementing fixed-base MSM, - New trait constraint has been imposed for `E::Fr: PrimeFieldBits` within the `non_hiding_zeromorph.rs` file, and usages have been adjusted in the `test` module. - Adding the `PrimeFieldBits` import from the `ff` crate and importing `provider::util` from the local crate. refactor: Refactor utility functions for elliptic curve operations - Renamed and moved `util.rs` to `fb_msm.rs` under the `provider` directory. - documented the FB MSM
Co-authored-by: Adrian Hamelink <adrian.hamelink@gmail.com>
- Enhanced the clarity and accuracy of code comments and documentation in `non_hiding_zeromorph.rs`, specifically within the `ZMPCS` struct's methods. - Enriched the `quotients` function's documentation, detailing its mathematical underpinnings - Implemented a new rigorous test, `test_quotients`, to ensure the `quotients` function's output satisfies the polynomial identity.
* fix: remove a TODO using RefCast * fix: remove unsightly clone * doc: comment intermediate ZM steps * refactor: Refactor `open` function in zeromorph provider Extracted the computation of `q_hat` in the `open` function into a new function `batched_lifted_degree_quotient` for more modular code. * test: test batched_lifted_degree_quotient Addition of a new test `test_batched_lifted_degree_quotient` to validate batched degree quotient lifting for polynomials. * refactor: Refine computations in non_hiding_zeromorph.rs - Overhauled the 'eval_and_quotient_scalars' function with a revised return type with pair of vectors rather than scalar and a vector, - correspondingly split scalars `degree_check_q_scalars` and `zmpoly_q_scalars` within the `open` method of `non_hiding_zeromorph.rs`. - Adjusted the 'verify' method to accommodate the modified 'eval_and_quotient_scalars' function output changes. * test: test partially evaluated quotient \zeta_x - Introduced a new test case `test_partially_evaluated_quotient_zeta` to validate `zeta_x` construction. - correct an off-by-one error in the y_k terms * test: test partially evaluated quotient - Created a new function `phi` for evaluating using an inefficient formula in `non_hiding_zeromorph.rs` - Implemented a new test function `test_partially_evaluated_quotient_z` for validation of `Z_x` computation method in `non_hiding_zeromorph.rs` * fix: refine q_hat comment
- Updated and enhanced clarity and consistency of mathematical notation in comments across `non_hiding_kzg.rs` and `non_hiding_zeromorph.rs` files. - Implemented error handling in the `ZMPCS::verify` function within the `non_hiding_zeromorph.rs` file.
h/t all the prior implementing repos for inspiration, incl. Plonkish, Aztec, and Jellyfish (for the univariate KZG). The 3d party recognition files reflect all this, but some of them have been haphazardly backported to dev already (e.g. #32).
More references:
A few (marked) warts to clean up left, but this is in a reviewable state.