Implemented Mova folding scheme #161

NiDimi · 2024-09-19T16:23:57Z

Added code for the Mova folding scheme (as specified in the https://eprint.iacr.org/2024/1220) into the list of the folding-schemes supported.

This PR addresses issue: #133

Co-Authored-By: Togzhan Barakbayeva <[email protected]> Co-Authored-By: Ilia Vlasov <[email protected]> Co-Authored-By: matthew-a-klein <[email protected]>

CPerezz · 2024-09-23T14:03:06Z

Aiming for a review next week. Would you be interested @arnaucube on reading the paper + reviewing this as a pair task? @dmpierre you're welcome too ofc!

arnaucube · 2024-09-24T16:12:08Z

@CPerezz sure, happy to do a call over it. I must say that this past weekend I went trough the paper to get ready to review this PR, was planning to do a first pass of review this week. But let's coordinate to comment it in a call too.

There is another thing, which may affect this PR, which is the refactor of the Arith traits that @winderica is doing in #162 . Since #162 might be faster to review and unlocks other PRs that @winderica has in the pipeline, might get merged before this PR (#161), and might bring some git conflicts to it. But I think that the changes should not bring much trouble, since would be adapting the Mova code to do a similar thing than what the Nova update is doing in the #162.

CPerezz · 2024-09-25T08:38:55Z

Agree!!
Let's unblock @winderica and refactor this accordingly then.
That will also give me time to actually read the paper and be able to pair-review!

NiDimi · 2024-09-25T18:03:32Z

Feel free to let me know if you need any help from me.

arnaucube

LGTM! Thanks for this addition! ^^

Just left two small comments in the code.

Two topics (first one is a comment for the future, second one is a question):

Since Mova and Ova are variations that build from Nova I think that it will make sense to have a shared codebase (current Nova's one) with Mova & Ova being some kind of flag options there, which basically affect on which strategy is applied on the error/slack terms (E) and its commitment (or the respective representation, ie MLE), but with most of the rest of the logic remaining the same, avoiding most of duplicated code (not only in the current NIFS implementation but also on future circuits & IVC & Decider implementation). And this will be easier now that @winderica improved the Arith related traits in Refactor Arith trait #162.

But I think that for a first iteration we're good with having a separated Mova code as it is in this PR, and we can do the abstractions in a next iteration.

One question, this PR implements the Mova's NIFS logic, and the following steps would be implementing the circuit that does the NIFS.Verify logic (in-circuit) in order to then build an IVC out of it so that we can do IVC with Mova.
I'm checking the Nethermint Mova impl and I'm only finding the NIFS implementation there too.
Is implementing the circuit (with the NIFS.V) and the IVC logic something that you plan to do? Since the Mova implementation was done to accompany the paper, I was assuming that it will stay as it is, but I'm asking just to avoid understanding it wrong and duplicating efforts.

I'm asking, not to put pressure on you (don't feel any pressure, really), but just in terms of high level planning, to see if it would make sense for us at some point to go into that. I think that then it would make sense to merge this PR, then the @CPerezz 's PR adding the Ova NIFS, and then doing the abstraction mentioned at the beginning of this message (point 1), so that then building the IVC with Mova (and Ova too) is more straightforward over the already existing Nova codebase, since most of the logic needed will be shared except for the approach related to the E and its commitment or MLE, and same for the Decider circuits and Decider logic.

folding-schemes/src/folding/mova/mod.rs

arnaucube · 2024-09-30T11:24:49Z

folding-schemes/src/folding/mova/mod.rs

+    // MLE of E
+    pub mleE: C::ScalarField,


This is not the MLE of E but the evaluation of the MLE of E at r_E, right?

CPerezz

A couple of comments.

Also, interested on seeing your thoughts about @arnaucube's proposal.

CPerezz · 2024-10-01T10:36:00Z

folding-schemes/src/folding/mova/mod.rs

+#[derive(Debug, Clone, Eq, PartialEq, CanonicalSerialize, CanonicalDeserialize)]
+pub struct Witness<C: CurveGroup> {
+    pub E: Vec<C::ScalarField>,
+    pub W: Vec<C::ScalarField>,
+    pub rW: C::ScalarField,
+}
+
+#[derive(Debug, Clone, Eq, PartialEq, CanonicalSerialize, CanonicalDeserialize)]
+pub struct InstanceWitness<C: CurveGroup> {
+    pub ci: CommittedInstance<C>,
+    pub w: Witness<C>,
+}


Maybe we could add some comments for these structs. Specially since both have w inside. So would be nice to refer to the paper to understand the diffs etc..

CPerezz · 2024-10-01T10:38:03Z

folding-schemes/src/folding/mova/mod.rs

+    pub fn dummy(w_len: usize, e_len: usize) -> Self {
+        let rW = C::ScalarField::zero();
+        let w = vec![C::ScalarField::zero(); w_len];
+
+        Self {
+            E: vec![C::ScalarField::zero(); e_len],
+            W: w,
+            rW,
+        }
+    }


There's a trait for this which would likely make more sense to implement.

See: https://github.com/privacy-scaling-explorations/sonobe/blob/main/folding-schemes/src/folding/nova/mod.rs#L82

CPerezz · 2024-10-01T10:39:58Z

folding-schemes/src/folding/mova/mod.rs

+impl<C: CurveGroup> CommittedInstance<C> {
+    pub fn dummy(io_len: usize) -> Self {


CPerezz · 2024-10-01T10:48:02Z

folding-schemes/src/folding/mova/mod.rs

+        // We cannot call `to_native_sponge_field_elements(dest)` directly, as
+        // `to_native_sponge_field_elements` needs `F` to be `C::ScalarField`,
+        // but here `F` is a generic `PrimeField`.
+        self.cmW
+            .to_native_sponge_field_elements_as_vec()
+            .to_sponge_field_elements(dest);
+    }


We should change the trait bounds here @winderica @arnaucube right? It should not affect the rest of impls. We will just need to enforce the bounds sometimes.

Maybe @NiDimi can you file an issue for this?

CPerezz · 2024-10-01T10:52:28Z

folding-schemes/src/folding/mova/nifs.rs

+    // Just a wrapper for Nova compute_T (compute cross-terms T) since the process is the same
+    pub fn compute_T(
+        r1cs: &R1CS<C::ScalarField>,
+        u1: C::ScalarField,
+        u2: C::ScalarField,
+        z1: &[C::ScalarField],
+        z2: &[C::ScalarField],
+    ) -> Result<Vec<C::ScalarField>, Error> {
+        Nova::<C, CS, H>::compute_T(r1cs, u1, u2, z1, z2)
+    }


Can't you simply call the Nova NIFS then? It should not be an issue as it doesn't require &self as parameter.
You can import it with an alias just for the computation of T like: use folding::nova::..::NIFS as NovaNIFS;

CPerezz · 2024-10-01T10:57:08Z

folding-schemes/src/folding/mova/nifs.rs

+        mleE2_prime: &C::ScalarField,
+        mleT: &C::ScalarField,
+    ) -> Result<CommittedInstance<C>, Error> {
+        let a2 = a * a;


Suggested change

let a2 = a * a;

let a_squared = a * a;

To avoid confusion with 1 and 2 indices in other variables names.

This change makes sense to me but I think it would make more sense if we switched all the usage of 2 after the letter to symbolise square (e.g. Mova's fold_witness, Nova's fold_witness and fold_committed_instance etc.)

CPerezz · 2024-10-01T11:00:22Z

folding-schemes/src/folding/mova/nifs.rs

+        ) = PointVsLine::<C, T>::prove(transcript, ci1, ci2, w1, w2)?;
+
+        // Protocol 7
+


Suggested change

CPerezz · 2024-10-01T11:01:23Z

folding-schemes/src/folding/mova/nifs.rs

+        let alpha_scalar = C::ScalarField::from_le_bytes_mod_order(b"alpha");
+        transcript.absorb(&alpha_scalar);


Is this some kind of domain separator?

CPerezz · 2024-10-01T11:02:05Z

folding-schemes/src/folding/mova/nifs.rs

+    /// [Mova](https://eprint.iacr.org/2024/1220.pdf)'s section 4. It verifies the results from the proof
+    /// Both the folding and the pt-vs-line proof
+    /// returns the folded committed instance


Suggested change

/// [Mova](https://eprint.iacr.org/2024/1220.pdf)'s section 4. It verifies the results from the proof

/// Both the folding and the pt-vs-line proof

/// returns the folded committed instance

/// [Mova](https://eprint.iacr.org/2024/1220.pdf)'s section 4.

/// It verifies the results from both the folding and the pt-vs-line proofs.

/// Returns the folded committed instance.

CPerezz · 2024-10-01T11:03:51Z

folding-schemes/src/folding/mova/pointvsline.rs

+}
+/// Proof from step 1 protocol 6


Suggested change

}

/// Proof from step 1 protocol 6

}

/// Proof from step 1 protocol 6

…defining a common interface between the three Nova variants The recent Ova NIFS PR #163 (#163) and Mova NIFS PR #161 (#161) PRs add Nova NIFS variants implementations which differ from Nova in the logic done for the `E` error terms of the instances. The current Ova implementation (#163) is based on the existing Nova NIFS code base and adds the modifications to the `E` logic on top of it, and thus duplicating the code. Similarly for the Mova NIFS impl. The rest of the Mova & Ova schemes logic that is not yet implemented is pretty similar to Nova one (ie. the IVC logic, the circuits and the Decider), so ideally that can be done reusing most of the already existing Nova code without duplicating it. This PR is a first step in that direction for the existing Ova NIFS code. This commit adds the NIFS trait abstraction with the idea of allowing to reduce the amount of duplicated code for the Ova's NIFS impl on top of the Nova's code.

* add NIFS trait abstraction (based on the needs for Nova, Mova, Ova), defining a common interface between the three Nova variants The recent Ova NIFS PR #163 (#163) and Mova NIFS PR #161 (#161) PRs add Nova NIFS variants implementations which differ from Nova in the logic done for the `E` error terms of the instances. The current Ova implementation (#163) is based on the existing Nova NIFS code base and adds the modifications to the `E` logic on top of it, and thus duplicating the code. Similarly for the Mova NIFS impl. The rest of the Mova & Ova schemes logic that is not yet implemented is pretty similar to Nova one (ie. the IVC logic, the circuits and the Decider), so ideally that can be done reusing most of the already existing Nova code without duplicating it. This PR is a first step in that direction for the existing Ova NIFS code. This commit adds the NIFS trait abstraction with the idea of allowing to reduce the amount of duplicated code for the Ova's NIFS impl on top of the Nova's code. * add Ova variant on top of the new NIFS trait abstraction This is done from the existing Ova implementation at `folding/ova/{mod.rs,nofs.rs}`, but removing when possible code that is not needed or duplicated from the Nova logic. * rm old Ova duplicated code This commit combined with the other ones (add nifs abstraction & port Ova to the nifs abstraction) allows to effectively get rid of ~400 lines of code that were duplicated in the Ova NIFS impl from the Nova impl. * small polishing & rebase to latest `main` branch updates

PR comment Co-authored-by: arnaucube <[email protected]>

NiDimi and others added 3 commits September 19, 2024 17:18

Adding Mova

637dc3c

Co-Authored-By: Togzhan Barakbayeva <[email protected]> Co-Authored-By: Ilia Vlasov <[email protected]> Co-Authored-By: matthew-a-klein <[email protected]>

Merge branch 'privacy-scaling-explorations:main' into introducing_mova

3305e18

Fix CLI

4940b48

arnaucube added the Nova variations label Sep 19, 2024

Updated from main

a38eba0

arnaucube approved these changes Sep 30, 2024

View reviewed changes

arnaucube requested a review from CPerezz September 30, 2024 11:38

CPerezz reviewed Oct 1, 2024

View reviewed changes

This was referenced Oct 2, 2024

Nova vairants (Ova) NIFS abstraction #165

Merged

Implement OVA NIFS #163

Merged

Solution to stop the CLI from complaining about deadcode

9e60b9d

PR comment Co-authored-by: arnaucube <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Mova folding scheme #161

Implemented Mova folding scheme #161

NiDimi commented Sep 19, 2024

CPerezz commented Sep 23, 2024

arnaucube commented Sep 24, 2024

CPerezz commented Sep 25, 2024

NiDimi commented Sep 25, 2024

arnaucube left a comment •

edited

Loading

arnaucube Sep 30, 2024

CPerezz left a comment

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

NiDimi Oct 6, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

CPerezz Oct 1, 2024

		impl<C: CurveGroup> CommittedInstance<C> {
		pub fn dummy(io_len: usize) -> Self {

		) = PointVsLine::<C, T>::prove(transcript, ci1, ci2, w1, w2)?;

		// Protocol 7

		let alpha_scalar = C::ScalarField::from_le_bytes_mod_order(b"alpha");
		transcript.absorb(&alpha_scalar);

Implemented Mova folding scheme #161

Are you sure you want to change the base?

Implemented Mova folding scheme #161

Conversation

NiDimi commented Sep 19, 2024

CPerezz commented Sep 23, 2024

arnaucube commented Sep 24, 2024

CPerezz commented Sep 25, 2024

NiDimi commented Sep 25, 2024

arnaucube left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz 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

arnaucube left a comment •

edited

Loading