NIMFS-centric API refactor for multifolding impl #41

CPerezz · 2023-07-19T10:19:25Z

This is a proposal for the new API of the Hypernova/multifolding current impl.

The idea is that the API is NIMFS-centric. That means, the end user only needs to deal for now with CCS(this can be prevented) and the NIMFS object.

The rest of interactions with CCCS & LCCCS have been "masked" or at the very least, is not necessary to import these structs to have full functionallity.

The current workflow that this API brings can be clearly seen here:

let ccs = CCS::<G>::from_r1cs(r1cs_shape);
  let mles = ccs.matrix_mles();
  let mut nimfs = NIMFS::init(
    &mut rng,
    ccs,
    mles,
    // Note we constructed z on the fly with the previously-used witness.
    vec![
      G::Scalar::ONE,
      G::Scalar::from(3u64),
      G::Scalar::from(35u64),
    ],
  );

  // Now, the NIMFS should satisfy correctly as we have inputed valid starting inpuits for the first LCCCS contained instance:
  assert!(nimfs.is_sat().is_ok());

  // Now let's create a valid CCCS instance and fold it:
  let valid_cccs = nimfs.gen_cccs(vec![
    G::Scalar::ONE,
    G::Scalar::from(2u64),
    G::Scalar::from(15u64),
  ]);
  nimfs.fold(&mut rng, valid_cccs);

  // Since the instance was correct, the NIMFS should still be satisfied.
  assert!(nimfs.is_sat().is_ok());

  // Now let's create am invalid CCCS instance and fold it:
  let invalid_cccs = nimfs.gen_cccs(vec![
    G::Scalar::ONE,
    G::Scalar::from(5u64),
    G::Scalar::from(55u64),
  ]);
  nimfs.fold(&mut rng, invalid_cccs);

  // Since the instance was wrong, the NIMFS should not be satisfied correctly.
  assert!(nimfs.is_sat().is_err());

This is part of a test, located in /~https://github.com/privacy-scaling-explorations/Nova/blob/change%2FNIMFS_centric_API/tests/nimfs.rs

The idea is that the user needs to type as less as possible in order to get a fold done.

I would appreciate some feedback on this @arnaucube @oskarth @asn-d6

oskarth · 2023-07-20T05:48:39Z

I'll review once there's a description of proposed changes and rationale, short is fine!

oskarth

Overall direction makes sense to me! Left some questions and comments

oskarth · 2023-07-21T03:10:09Z

src/ccs/cccs.rs

-
-  pub(crate) ccs: CCSShape<G>,
+#[derive(Clone, Debug, PartialEq, Eq /*Serialize, Deserialize*/)]
+//#[serde(bound(deserialize = "'de: 'a"))]


What is this?

Ohh.. Leftover!
Fixed in 52f6a62

oskarth · 2023-07-21T03:11:17Z

src/ccs/cccs.rs

-}
+impl<G: Group> CCCS<G> {
+  /// Generates a new CCCS given a reference to it's original CCS repr & the multilinear extension of it's matrixes.
+  /// Then, given the input vector `z`.


This sentence seems incomplete

Fixed in 52f6a62

oskarth · 2023-07-21T03:13:00Z

src/ccs/cccs.rs

-  pub(crate) x: Vec<G::Scalar>,
-}
+impl<G: Group> CCCS<G> {
+  /// Generates a new CCCS given a reference to it's original CCS repr & the multilinear extension of it's matrixes.


Suggested change

/// Generates a new CCCS given a reference to it's original CCS repr & the multilinear extension of it's matrixes.

/// Generates a new CCCS given a reference to its original CCS representation and the multilinear extension of its matrices.

Fixed in 52f6a62

oskarth · 2023-07-21T03:15:00Z

src/ccs/cccs.rs

-  pub(crate) ccs: CCSShape<G>,
+#[derive(Clone, Debug, PartialEq, Eq /*Serialize, Deserialize*/)]
+//#[serde(bound(deserialize = "'de: 'a"))]
+pub struct CCCS<G: Group> {


Calling this just CCCS seems a bit weird to me since it stands for "Committed Customizable Constraint Systems". Should this be CCCS Shape or Instance instead?

Not sure it is a big deal though. In any case, I can definitely change it. What do the rest think?

I actually think is better to have shorter names. Also, we have always called it CCCS indeed in our calls. So that's why I considered a better alternative.

I guess point would be to stay consistent with code base, but I don't feel super strongly about it, can see both sides

src/ccs/cccs.rs

oskarth · 2023-07-21T03:26:50Z

src/ccs/multifolding.rs

-    let ccs_mle = ccs.M.iter().map(|matrix| matrix.to_mle()).collect();
-    Self { ccs, ccs_mle }
+impl<G: Group> NIMFS<G> {
+  /// Generates a new NIMFS instance based on the given CCS.


It seems like it takes more than just ccs

Fixed in 52f6a62

oskarth · 2023-07-21T03:27:39Z

src/ccs/multifolding.rs

  /// Compute sigma_i and theta_i from step 4
  pub fn compute_sigmas_and_thetas(
    &self,
-    z1: &Vec<G::Scalar>,
-    z2: &Vec<G::Scalar>,
+    z2: &[G::Scalar],


Why is this signature so different?

Because now z1 is hold inside the LCCCS instance inside the NIMFS instance. Hence, there's no need to pass it!

Maybe can add this as a comment?

src/ccs/multifolding.rs

asn-d6 · 2023-07-21T10:30:04Z

Some nitpicky thoughts on the APi:

This let mles = ccs.matrix_mles(); seems a bit needless. Why would the API user need to know how to get the MLEs? Why can't the object the initial NIMFS object have the MLEs inside of it, instead of having to manually compute them? Alternatively, why don't you call matrix_mles() inside init()?
This gen_cccs() function is test-only right? Or is it supposed to be used in the real world as well? If so, maybe a better name would be welcome, since "gen" reminds me of "generate random thing".

CPerezz · 2023-07-24T10:11:44Z

This let mles = ccs.matrix_mles(); seems a bit needless. Why would the API user need to know how to get the MLEs? Why can't the object the initial NIMFS object have the MLEs inside of it, instead of having to manually compute them? Alternatively, why don't you call matrix_mles() inside init()?

This is nice! Thanks! I did not see it though. Will only pass the CCS then and compute the MLEs on the fly as they just need to be computed once.

This gen_cccs() function is test-only right? Or is it supposed to be used in the real world as well? If so, maybe a better name would be welcome, since "gen" reminds me of "generate random thing".

I don't have any strong feelings on this. So changing it.
Addressed in 6d642dd. @asn-d6

This PR moves a tep forward to the NIMFS-centric API. First, removes all the `_Witness` & `_Instance` structs for CCS & CCCS as they added a bunch of overhead. Sedonc, it also re-assembles internal fm APIs so that they ask for slices instead of refs to `Vec`. Finally, reassembles all the tests to indeed modify CCS & CCCS interaction as described avobe.

Removes CCS & CCS_MLEs from the struct as well as renaming some variables. Aside from that, the functions are being modified so that they always get the removed fields as refs.

This gets rid of conversions that made no sense from a theoretical standpoint and were overtaken by a better API design.

Since we need to generate LCCCS instances from no-where to actually be able to test things or startup the NIMFS object.

LCCCS already contains `z` inside of it. So it's non-sensical to require twice the same thing that was already served to the struct in the pass as a creation attribute.

This allows to either generate a `Multifolding` instance from all of it's params or, instead, from the first `z` and confectioning the LCCCS.

CPerezz · 2023-07-25T05:55:06Z

This is ready to be merged with the rebase done and all comments addressed.
cc: @arnaucube @asn-d6 @oskarth

arnaucube

Partial review done, nice work with the PR ^^
I have pending to review the src/ccs/multifolding.rs and tests/nimfs.rs files, I hope to have some time this night to do it.

arnaucube · 2023-07-25T12:39:58Z

src/ccs/cccs.rs

+  /// Generates a new CCCS given a reference to it's original CCS repr and it's public and private inputs.
+  pub(crate) fn new(
+    ccs: &CCS<G>,
+    ccs_matrix_mle: &[MultilinearPolynomial<G::Scalar>],


How do you feel about moving let mles = ccs.matrix_mles(); inside this method and avoiding the need to pass the MultilinearPolynomials as input in this method? Or the css_matrix_mle is carried from other upper level logic and here is just reusing that already computed data?

Or the css_matrix_mle is carried from other upper level logic and here is just reusing that already computed data?

That's exactly what happens. Ideally, I'll have the mles (as well as the CCS) serialized or given by the wire. So I don't want to spend time computing MLES. Specially since the matrixes are sparse. Which has the idea to minimize memory usage.

src/ccs/cccs.rs

src/ccs/lcccs.rs

oskarth

LGTM!

oskarth · 2023-07-25T14:00:49Z

src/ccs/cccs.rs

-  pub(crate) ccs: CCSShape<G>,
+#[derive(Clone, Debug, PartialEq, Eq /*Serialize, Deserialize*/)]
+//#[serde(bound(deserialize = "'de: 'a"))]
+pub struct CCCS<G: Group> {


I guess point would be to stay consistent with code base, but I don't feel super strongly about it, can see both sides

oskarth · 2023-07-25T14:06:46Z

src/ccs/multifolding.rs

-// Is our single source of data.
+/// The NIMFS structure is the center of operations of the folding scheme.
+/// Once generated, it allows us to fold any upcomming CCCS instances within it without needing to do much.
+// XXX: Pending to add doc examples.


Ok let's do in follow up PR so we don't forget it

oskarth · 2023-07-25T14:07:29Z

src/ccs/multifolding.rs

  /// Compute sigma_i and theta_i from step 4
  pub fn compute_sigmas_and_thetas(
    &self,
-    z1: &Vec<G::Scalar>,
-    z2: &Vec<G::Scalar>,
+    z2: &[G::Scalar],


Maybe can add this as a comment?

arnaucube

regarding src/ccs/multifolding.rs.
I find interesting the idea of having the running LCCCS data as a NIMFS structure parameter, since it feels more close to the idea of having a running instance which is folded with new instances on each iteration.

One thing to note though, is that there can be multiple LCCCS instances and multiple CCCS instances being folded in a single iteration.

src/ccs/multifolding.rs

arnaucube · 2023-07-26T10:51:37Z

src/ccs/multifolding.rs

+    // Here we perform steps 7 & 8 of the section 5 of the paper. Were we actually fold LCCCS & CCCS instances.
+    self.lcccs.w_comm += cccs.w_comm.mul(rho);
+    self.lcccs.v = folded_v;
+    self.lcccs.r_x = r_x_prime;
+    self.fold_z(cccs, rho);


Maybe the instance & witness parts should be separated? in a similar way that in [multifolding-poc]((/~https://github.com/privacy-scaling-explorations/multifolding-poc/tree/d118ea56418dadde005b5854b5cbbf38a686d24d/src/multifolding.rs#L202), since later the prover has both, but the verifier does not have the witness part of the folding.

hmmmm I can definitely do that. I just went that way as we were folding both things at the end and is simpler to write and forces us to hold less attributes.

In any case, I can change it if you think is better.

I think that this will be needed, since the verifier does not have the witness part of z. But we can do it in a future iteration, filled #46 to keep track of it.

tests/nimfs.rs

CPerezz · 2023-07-26T13:54:15Z

One thing to note though, is that there can be multiple LCCCS instances and multiple CCCS instances being folded in a single iteration.

I know.
That's why #37 was raised. The plan was to stabilize firs the API and then upgrade to N->1

arnaucube

LGTM 😸

Regarding

That's why #37 was raised. The plan was to stabilize firs the API and then upgrade to N->1

Make sense, when doing the n-to-1 fold we will need to check how to adapt the running instance to multiple of them (eg. check if it makes sense to have an array of LCCCS in the NIMFS data structure or to just pass them as parameter together with the CCCS instances). I've left a comment to #37 to have it in mind then.

arnaucube · 2023-07-27T07:59:26Z

src/ccs/multifolding.rs

+    // Note `z2` represents the input of the incoming CCCS instance.
+    // As the current IVC accumulated input is holded inside of the NIMFS(`self`).
+    z: &[G::Scalar],


maybe z2 in the comment should match z in the code now that has been updated (or incoming_z if finally named that way as mentioned in #41 (comment) )

CPerezz · 2023-07-31T16:18:51Z

This leaves pending #46 as I haven't found a good enough API abstraction and I don't want to block the merge anymore.

oskarth · 2023-07-31T16:29:34Z

Yes let's merge this and other one

The PR introduces usage from the associated trait/type of `Group` called `TranscriptEngineTrait` to perform the Fiat-Shamir inside the multifolding for the Sigmas and Thetas computations as well as gammas. This should be rebased on the top of `main` once #41 is merged and then reviewed. As there could be places where FS should be applied but it isn't.

…plorations#41) * Enforce program counter in SuperNova StepCircuit. * Improve documentation. * Rename pseudo_pc to rom_index. * Cleanup TestROM example. --------- Co-authored-by: porcuquine <porcuquine@users.noreply.github.com>

oskarth mentioned this pull request Jul 20, 2023

Add initial experimental HyperNova multifolding support microsoft/Nova#175

Draft

CPerezz force-pushed the change/NIMFS_centric_API branch from 31c3929 to a947fc7 Compare July 20, 2023 07:50

CPerezz marked this pull request as ready for review July 20, 2023 13:35

CPerezz requested review from oskarth, asn-d6 and arnaucube July 20, 2023 13:35

oskarth reviewed Jul 21, 2023

View reviewed changes

CPerezz requested a review from oskarth July 24, 2023 15:57

CPerezz added 19 commits July 24, 2023 18:07

change: Rework CCCS to only hold Comms & z

600ccc3

change: First LCCCS update

b91b68f

Removes CCS & CCS_MLEs from the struct as well as renaming some variables. Aside from that, the functions are being modified so that they always get the removed fields as refs.

remove: to_cccs & to_lcccs fns for CCS

f2cc4af

This gets rid of conversions that made no sense from a theoretical standpoint and were overtaken by a better API design.

add: LCCCS::new fn to allow for easier testing

8c8c291

Since we need to generate LCCCS instances from no-where to actually be able to test things or startup the NIMFS object.

change: Not to require z for LCCCS assoc-fns

3051023

LCCCS already contains `z` inside of it. So it's non-sensical to require twice the same thing that was already served to the struct in the pass as a creation attribute.

add: new & init functions for Multifolding

fa32f62

This allows to either generate a `Multifolding` instance from all of it's params or, instead, from the first `z` and confectioning the LCCCS.

add: is_sat fn for multifolding struct

97eac64

fix: Ammend util tests

4e2c301

change: Minor final details to CCCS API

d3c1711

fix: Minor details adapting to CCCS API

90de394

fix: Change w by z on compute_v_j

45630e7

fix: passing CCCS should be passing LCCCS

75db868

fix: Update r_x in folding fn for r_x_prime

c19c9e1

fix: Remove u field from LCCCS & hold z

292d0ac

change: Rename CCCSInstance to CCCS

e116e46

change: Rename Multifolding struct as NIMFS

372ceed

fix: Remove NIFS type alias and fix derived errs

1357460

add: Comments and fn visibilities

356d563

CPerezz added 5 commits July 25, 2023 07:20

fix: Address review comments

c0f979f

update: Remove mles CCS getter + rename gen_cccs

fea8411

remove: Unused leftover comments

0ebe292

change: Remove/comment negative folding tests

dec8796

fix: Rebase errors

6e6bed0

CPerezz force-pushed the change/NIMFS_centric_API branch from d2239ee to 6e6bed0 Compare July 25, 2023 05:54

CPerezz mentioned this pull request Jul 25, 2023

Transcript usage inclusion into the codebase #43

Merged

arnaucube reviewed Jul 25, 2023

View reviewed changes

oskarth approved these changes Jul 25, 2023

View reviewed changes

This was referenced Jul 25, 2023

Fix the usage of r_w across the crate. #44

Open

Add documentation for NIMFS usage as doc comments #45

Open

remove: Unused CommitmentKeys passed to some fn

4231bed

CPerezz force-pushed the change/NIMFS_centric_API branch from 7966341 to 4231bed Compare July 25, 2023 16:46

chore: Add clarification comments for Zs

fc47467

CPerezz requested a review from arnaucube July 25, 2023 16:50

arnaucube reviewed Jul 26, 2023

View reviewed changes

CPerezz added 2 commits July 26, 2023 15:23

chore: Unify ccs instances naming

e52e174

chore: Rename z2 to z

70b8176

arnaucube approved these changes Jul 26, 2023

View reviewed changes

arnaucube reviewed Jul 27, 2023

View reviewed changes

chore: update z2 to z

e775f46

CPerezz merged commit 1119702 into main Jul 31, 2023

CPerezz deleted the change/NIMFS_centric_API branch July 31, 2023 16:44

CPerezz mentioned this pull request Aug 2, 2023

[HyperNova] Rework the API to be NIMFS-centric #22

Closed

	/// Generates a new CCCS given a reference to it's original CCS repr & the multilinear extension of it's matrixes.
	/// Generates a new CCCS given a reference to its original CCS representation and the multilinear extension of its matrices.

NIMFS-centric API refactor for multifolding impl #41

NIMFS-centric API refactor for multifolding impl #41

Conversation

CPerezz commented Jul 19, 2023 • edited Loading

oskarth commented Jul 20, 2023

oskarth 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asn-d6 commented Jul 21, 2023

CPerezz commented Jul 24, 2023

CPerezz commented Jul 25, 2023

arnaucube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth 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

arnaucube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Jul 26, 2023

arnaucube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Jul 31, 2023

oskarth commented Jul 31, 2023

CPerezz commented Jul 19, 2023 •

edited

Loading

CPerezz Jul 26, 2023 •

edited

Loading