Add MLKZG support (Nova forward port) #172

huitseeker · 2023-12-11T21:17:00Z

This forward-ports the following Nova PRs:

Support for multilinear KZG commitments microsoft/Nova#269 (MLKZG support)
make Minroot example generic over the supported curve cycles microsoft/Nova#272 (minroot on MLKZG)

Warning

This is not just a typical forward port from Nova. The second commit includes a significant, opinionated refactor of the MLKZG computation traits to reduce redundancy and boilerplate code.

Note

Items left to address in a future PR include:

the TranscriptReprTrait bounds that have been left on DlogGroup since Refactor traits that allows implementing different engines for the same curve cycle microsoft/Nova#263 are surprisingly hard to use and should probably be relocated,
Refactoring the UniversalKZGParam struct and abstracting it behind a trait, given the varying public parameter requirements in different schemes (e.g., MLKZG requires fewer G2 points).
~~Renaming UVKZGProverKey and UVKZGVerifierKey as they serve dual roles in multiple multilinear schemes.~~
Fixing the discrepancy between TranscriptReprTrait<G> for $name::Affine and TranscriptReprTrait<E::GE> for Commitment<E> and TranscriptReprTrait<E::G1> for UVKZGCommitment<E> (only the last 2 agree with each other)

Minroot tests

We get a slight performance improvement through the refactoring on the second commit, as seen on the Minroot example:
. The machine is an M1 Mac (CPU only), and the "dataset size" is the number of Minroot iterations in the folded circuit (multiply by roughly 10^3 to get the number of constraints, go see the full log to get exact numbers).

Minroot example highlights

[email protected]➜~/tmp/nova(port_mlkzg)» curl https://gist.githubusercontent.com/huitseeker/ecd67d7fa6cd1640ed244dc6131fd73f/raw/6bd7d80a8222c9174563599b570b5b316b850fc6/before.log |grep -Ee 'CompressedSNARK.*?(prove|verify)'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8981  100  8981    0     0   137k      0 --:--:-- --:--:-- --:--:--  143k
CompressedSNARK::prove: true, took 15.17823149s
CompressedSNARK::verify: true, took 853.667266ms
CompressedSNARK::prove: true, took 15.770005823s
CompressedSNARK::verify: true, took 872.555664ms
CompressedSNARK::prove: true, took 16.66667875s
CompressedSNARK::verify: true, took 866.624533ms
CompressedSNARK::prove: true, took 19.877718086s
CompressedSNARK::verify: true, took 866.385118ms
CompressedSNARK::prove: true, took 21.594911779s
CompressedSNARK::verify: true, took 888.627582ms
CompressedSNARK::prove: true, took 27.125401955s
CompressedSNARK::verify: true, took 905.389405ms
CompressedSNARK::prove: true, took 36.04063618s
CompressedSNARK::verify: true, took 978.210747ms
[email protected]➜~/tmp/nova(port_mlkzg)» curl https://gist.githubusercontent.com/huitseeker/ecd67d7fa6cd1640ed244dc6131fd73f/raw/6bd7d80a8222c9174563599b570b5b316b850fc6/after.log | grep -Ee 'CompressedSNARK.*?(prove|verify)'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9004  100  9004    0     0   131k      0 --:--:-- --:--:-- --:--:--  137k
CompressedSNARK::prove: true, took 14.448755297s
CompressedSNARK::verify: true, took 823.961437ms
CompressedSNARK::prove: true, took 15.217142053s
CompressedSNARK::verify: true, took 824.207437ms
CompressedSNARK::prove: true, took 16.729647665s
CompressedSNARK::verify: true, took 828.918186ms
CompressedSNARK::prove: true, took 19.04217293s
CompressedSNARK::verify: true, took 852.980864ms
CompressedSNARK::prove: true, took 21.752410675s
CompressedSNARK::verify: true, took 857.06144ms
CompressedSNARK::prove: true, took 27.025667753s
CompressedSNARK::verify: true, took 878.894013ms
CompressedSNARK::prove: true, took 34.267848667s
CompressedSNARK::verify: true, took 892.194007ms

The switch to the FBMSM also improves the generation time of the public parameters (despite needing to generate 2x the points as it's using the same universal setup data structure as Zeromorph):

Full logs: https://gist.github.com/huitseeker/ecd67d7fa6cd1640ed244dc6131fd73f

adr1anh

All of the house-keeping relating to the unification with ZM seem good with me!

There are a lot of missed optimizations in MLKZG, both in terms of parallelization and at the protocol-level. For the latter, many elements are added to the transcript multiple times.

It also seems reasonable to factor out the batch KZG opening into its own sub-protocol.

src/provider/mlkzg.rs

adr1anh · 2023-12-15T09:18:34Z

src/provider/mlkzg.rs

+    let kzg_verify_batch = |vk: &KZGVerifierKey<E>,
+                            C: &Vec<E::G1Affine>,
+                            W: &Vec<E::G1Affine>,
+                            u: &Vec<E::Fr>,
+                            v: &Vec<Vec<E::Fr>>,
+                            transcript: &mut <NE as NovaEngine>::TE|
+     -> bool {


Both ZM and MLKZG use this the functionality for batching the evaluations of multiple polynomials at different points. A common code path would make sense.

adr1anh · 2023-12-15T11:22:56Z

src/provider/mlkzg.rs

+
+      // Compute the commitment to the batched polynomial B(X)
+      let c_0: E::G1 = C[0].into();
+      let C_B = (c_0 + NE::GE::vartime_multiscalar_mul(&q_powers[1..k], &C[1..k])).preprocessed();


This is not required since it's only used as an intermediary computation for L later on

Fixed in #231

adr1anh · 2023-12-15T11:24:54Z

src/provider/mlkzg.rs

+      //
+      // We group terms to reduce the number of scalar mults (to seven):
+      // In Rust, we could use MSMs for these, and speed up verification.
+      let L = E::G1::from(C_B) * (E::Fr::ONE + d[0] + d[1])


Instead of computing C_B * (1 + d[0] + d[1]), we should directly compute the MSM of C with q_powers[..] * (1 + d[0] + d[1]).

Fixed in #231

adr1anh · 2023-12-15T11:27:48Z

src/provider/mlkzg.rs

+    polys.push(hat_P.to_vec());
+    for i in 0..ell {
+      let Pi_len = polys[i].len() / 2;
+      let mut Pi = vec![E::Fr::ZERO; Pi_len];
+
+      #[allow(clippy::needless_range_loop)]
+      for j in 0..Pi_len {
+        Pi[j] = x[ell-i-1] * polys[i][2*j + 1]            // Odd part of P^(i-1)
+                      + (E::Fr::ONE - x[ell-i-1]) * polys[i][2*j]; // Even part of P^(i-1)
+      }


The Pis need to be computed sequentially, but their derivation can be parallelized.

Fixed in #233

src/provider/mlkzg.rs

storojs72

Great work!

Usually, when some performance numbers are presented, it is useful to also give information about system/computer where experiments were executed.

storojs72 · 2023-12-15T14:18:00Z

src/provider/mlkzg.rs

+  ) -> E::Fr {
+    transcript.absorb(b"C", C);
+    transcript.absorb(b"y", y);
+    transcript.absorb(b"c", &com.to_vec().as_slice());


What is the expected size of com slice (upper bound)?

It should be log(N) where N is max of the number of entries in ABC, and the number of constraints, and 2 x number of variables.

storojs72 · 2023-12-15T14:19:32Z

src/provider/mlkzg.rs

+    v: &[Vec<E::Fr>],
+    transcript: &mut impl TranscriptEngineTrait<NE>,
+  ) -> E::Fr {
+    transcript.absorb(b"C", &C.to_vec().as_slice());


ditto. I'm interesting in upper bounds of expected sizes for C, u, v slices

U is 3, v is 3*log(N)

* multilinear KZG PCS as a provider; builds * fix two tests * fix third test; cut duplicate code * Tidy up source code comments Signed-off-by: Greg Zaverucha <[email protected]> * impl PairingGroup for bn256 * remove unneeded imports * simplify CommitmentKey * fix build; migrate G1Affine * fmt * checkpoint * migrate G2Affine and pairing * fix clippy; use unimplemented! * switch to affine form for compressed commitments * add a test with mlkzg * cargo fmt * cleanup * go back to compressed group * address clippy * rename * cleanup * add an alias * deduplicate * Revert "add an alias" This reverts commit 97cade6c8751deacbc8b5b0e0df1579e3baa1477. * Use an alias for PreprocessedGroupElements Signed-off-by: Greg Zaverucha <[email protected]> * cargo fmt * update README.md --------- Signed-off-by: Greg Zaverucha <[email protected]> Co-authored-by: Greg Zaverucha <[email protected]>

Summary: - THe MLKZG implementation re-implements some group traits, so as to give it maximum generality and depende maximally on the Nova traits. - However, the way in which it imports a pairing (using pairing::Engine) already implicitly constrains perfrectly usable group implementations to be available on the same types. This commit therefore removes the boilerplate and uses those external traits. - Finally, so as to mutualize part of the pairing implementation, this commit also leverages the MultiMillerLoop trait, a subtrait of `pairing::Engine`. - In sum, this commit only moves types - no actual data was harmed in its making. In detail: - Removed the `PairingGroup` trait and its related implementations from the `traits.rs` and `bn256_grumpkin.rs` files. - Simplified the imports from `halo2curves::bn256` in `bn256_grumpkin.rs` and removed unused types such as `pairing`, `G2Affine`, `G2Compressed`, `Gt`, and `G2`. - Deleted substantial amount of code associated with `G2` from `bn256_grumpkin.rs`.

* make Minroot example generic over the supported curve cycles * upgrade version

…ipt_bytes` - Enhanced the functionality of `to_transcript_bytes` method in `TranscriptReprTrait` for `Affine` in both `pasta.rs` and `traits.rs`. - Combined the x and y coordinates with the `is_infinity_byte` into a single byte stream for ease of handling. - Integrated additional checks for 'infinity' conditions to ensure accurate extractions of coordinate values.

- Restructure the `provider` module by moving `msm` to the `util` subdirectory.

- chore: move comment - fix: standardize power sequences computation - fix: parallelize several poly computations refactor: Refactor `EvaluationArgument` struct in mlkzg.rs - Renamed several fields in `EvaluationArgument` struct within `src/provider/mlkzg.rs` for increased clarity. - Adjusted the `prove` and `verify` methods in `src/provider/mlkzg.rs` to reflect these name changes. - Modified test code to align with the updates in the `EvaluationArgument` structure.

huitseeker · 2023-12-19T13:16:43Z

Usually, when some performance numbers are presented, it is useful to also give information about system/computer where experiments were executed.

Right, added to PR description.

huitseeker requested review from adr1anh, porcuquine and mpenciak December 11, 2023 21:18

huitseeker force-pushed the port_mlkzg branch 4 times, most recently from 9f29b52 to 473b5d1 Compare December 12, 2023 17:30

adr1anh reviewed Dec 15, 2023

View reviewed changes

storojs72 reviewed Dec 15, 2023

View reviewed changes

huitseeker mentioned this pull request Dec 18, 2023

Implement a fancier msm for halo2curves-related crates #193

Closed

huitseeker force-pushed the port_mlkzg branch 4 times, most recently from 45e0f9b to 594bb10 Compare December 18, 2023 21:56

srinathsetty and others added 6 commits December 18, 2023 17:29

make Minroot example generic over the supported curve cycles (#272)

2e3de78

* make Minroot example generic over the supported curve cycles * upgrade version

refactor: Relocate multi-scalar multiplication module

5e726de

- Restructure the `provider` module by moving `msm` to the `util` subdirectory.

chore: Rename UV(KZG{ProverKey, VerifierKey}|UniversalKZGParam) -> \1

624cbb9

huitseeker force-pushed the port_mlkzg branch 2 times, most recently from 3612039 to becaa7f Compare December 18, 2023 22:41

huitseeker force-pushed the port_mlkzg branch from becaa7f to 0ccb6c8 Compare December 18, 2023 22:41

adr1anh approved these changes Dec 19, 2023

View reviewed changes

storojs72 self-requested a review December 19, 2023 14:42

storojs72 approved these changes Dec 19, 2023

View reviewed changes

huitseeker added this pull request to the merge queue Dec 19, 2023

Merged via the queue into dev with commit a1e6feb Dec 19, 2023
4 checks passed

huitseeker deleted the port_mlkzg branch December 19, 2023 14:54

This was referenced Dec 20, 2023

chore: Remove redundant absorptions to transcript in MLKZG implementation (PR#172) #202

Closed

chore: Remove redundant absorptions in MLKZG implementation #206

Merged

This was referenced Jan 3, 2024

Avoid computing polynomial which is constant #228

Closed

Refactoring the kzg_verify_batch in MLKZG implementation #229

Closed

Parallelise Pi computing #232

Closed

huitseeker mentioned this pull request Jan 17, 2024

Load KZG setup parameters from file #270

Open

huitseeker mentioned this pull request Jan 26, 2024

Zeromorph and HyperKZG improvement (Arecibo backports) microsoft/Nova#301

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MLKZG support (Nova forward port) #172

Add MLKZG support (Nova forward port) #172

huitseeker commented Dec 11, 2023 •

edited

Loading

adr1anh left a comment •

edited

Loading

adr1anh Dec 15, 2023

adr1anh Dec 15, 2023

storojs72 Jan 4, 2024

adr1anh Dec 15, 2023

storojs72 Jan 4, 2024

adr1anh Dec 15, 2023

storojs72 Jan 4, 2024

storojs72 left a comment

storojs72 Dec 15, 2023

adr1anh Dec 16, 2023

storojs72 Dec 15, 2023

adr1anh Dec 16, 2023

huitseeker commented Dec 19, 2023

Add MLKZG support (Nova forward port) #172

Add MLKZG support (Nova forward port) #172

Conversation

huitseeker commented Dec 11, 2023 • edited Loading

Minroot tests

adr1anh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker commented Dec 19, 2023

huitseeker commented Dec 11, 2023 •

edited

Loading

adr1anh left a comment •

edited

Loading