Implemented Mova folding scheme #161
Conversation
Co-Authored-By: Togzhan Barakbayeva <[email protected]> Co-Authored-By: Ilia Vlasov <[email protected]> Co-Authored-By: matthew-a-klein <[email protected]>
|
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! |
|
@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. |
|
Agree!! |
|
Feel free to let me know if you need any help from me. |
There was a problem hiding this comment.
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 RefactorArithtrait #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.
===
UPDATE: the abstraction has been made at a07e17e , and Mova ported to the abstraction at 54e6b60 .
There was a problem hiding this comment.
A couple of comments.
Also, interested on seeing your thoughts about @arnaucube's proposal.
| #[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>, | ||
| } |
There was a problem hiding this comment.
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..
| 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 was a problem hiding this comment.
There's a trait for this which would likely make more sense to implement.
| impl<C: CurveGroup> CommittedInstance<C> { | ||
| pub fn dummy(io_len: usize) -> Self { |
| // 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); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sorry for missing your comment! This is in fact introduced by me in https://github.com/privacy-scaling-explorations/sonobe/blame/cb1b8e37aa07dda255d3f3651385a45613beed4e/folding-schemes/src/folding/nova/mod.rs#L112-L121. Let me check if this can be resolved by changing the trait bound. Will create an issue or PR in these days!
| // 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) | ||
| } |
There was a problem hiding this comment.
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;
| mleE2_prime: &C::ScalarField, | ||
| mleT: &C::ScalarField, | ||
| ) -> Result<CommittedInstance<C>, Error> { | ||
| let a2 = a * a; |
There was a problem hiding this comment.
| let a2 = a * a; | |
| let a_squared = a * a; |
To avoid confusion with 1 and 2 indices in other variables names.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
in this case I think there is no space for confusion, since it's clear that a2=a*a, and the usage is only right at the following line.
| ) = 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); |
There was a problem hiding this comment.
Is this some kind of domain separator?
There was a problem hiding this comment.
It is actually not needed for Mova, so I removed it.
| /// [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 |
There was a problem hiding this comment.
| /// [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. |
| } | ||
| /// Proof from step 1 protocol 6 |
There was a problem hiding this comment.
| } | |
| /// 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
* refactor NIFSTrait interface to fit Nova variants (Nova,Mova,Ova) Refactor NIFSTrait interface to fit Nova variants (Nova,Mova,Ova). The relevant change is instead of passing the challenge as input, now it passes the transcript and computes the challenges internally (Nova & Ova still compute a single challenge, but Mova computes multiple while absorbing at different steps). * port Mova impl to the NIFSTrait * remove unnecessary wrappers in the nova/zk.rs * remove Nova NIFS methods that are no longer needed after the refactor * put together the different NIFS implementations (Nova, Mova, Ova) so that they can interchanged at usage. The idea is that Nova and its variants (Ova & Mova) share most of the logic for the circuits & IVC & Deciders, so with the abstracted NIFS interface we will be able to reuse most of the already existing Nova code for having the Mova & Ova circuits, IVC, and Decider.
Refactor NIFSTrait & port Mova impl to it
adapt Nova's DeciderEth prepare_calldata & update examples to it
small update to fix solidity tests
6d8f297
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