Implement OVA NIFS #163
Implement OVA NIFS #163
Conversation
The implementation follows the spec outlined by Bunz in: https://hackmd.io/V4838nnlRKal9ZiTHiGYzw?view. With this, the NIFS works and passes all tests.
There was a problem hiding this comment.
While reviewing this PR, it felt like lot of code could be saved (same with the Mova NIFS PR), since they (Mova & Ova) are variants of the Nova NIFS, which would not require duplicating all the current code.
I've openend the #165 PR adding some abstractions and merging the Ova NIFS into the Nova codebase to avoid part of the duplication.
So overall, LGTM.
(I've left a couple of comments, but I think we're fine merging this PR, since they're solved already at the #165)
| crate::folding::nova::nifs::NIFS::<C, CS, H>::compute_T( | ||
| r1cs, | ||
| C::ScalarField::one(), | ||
| mu, | ||
| &[vec![C::ScalarField::one()], x_i.to_vec(), w_i.w.to_vec()].concat(), | ||
| &[vec![mu], X_i.to_vec(), W_i.w.to_vec()].concat(), | ||
| ) |
There was a problem hiding this comment.
I think that this comment: https://github.com/privacy-scaling-explorations/sonobe/pull/161/files#r1782555470 also applies here. Would be just a matter of rearranging the inputs. But would not mind if it stays as it is for this PR.
There was a problem hiding this comment.
This would be at the expense of creating all these z vectors in the middle of the code. I think this function is done to "hide" all that boilerplate.
Also, notice that I don't need to pass mu2. So I can save up operations.
There was a problem hiding this comment.
but you construct it anyways here in this method in order to call the Nova's compute_T. Anyways, as mentioned in the other comments I'm fine with the current state of the code in the PR and I think we can merge
| /// Not only that, but notice that the incoming-instance `mu` parameter is always | ||
| /// equal to 1. Therefore, we can save the some computations. | ||
| pub fn compute_E( | ||
| r1cs: &R1CS<C::ScalarField>, | ||
| W_i: &Witness<C>, | ||
| X_i: &[C::ScalarField], | ||
| mu: C::ScalarField, | ||
| ) -> Result<Vec<C::ScalarField>, Error> { | ||
| let (A, B, C) = (r1cs.A.clone(), r1cs.B.clone(), r1cs.C.clone()); | ||
|
|
||
| let z_prime = [&[mu], X_i, &W_i.w].concat(); | ||
| // this is parallelizable (for the future) | ||
| let Az_prime = mat_vec_mul(&A, &z_prime)?; | ||
| let Bz_prime = mat_vec_mul(&B, &z_prime)?; | ||
| let Cz_prime = mat_vec_mul(&C, &z_prime)?; | ||
|
|
||
| let Az_prime_Bz_prime = hadamard(&Az_prime, &Bz_prime)?; | ||
| let muCz_prime = vec_scalar_mul(&Cz_prime, &mu); |
There was a problem hiding this comment.
here it says that "mu is always 1 therefore we can save some computations", but in fact mu might not be 1, and also the code of the method is using mu.
But as with #163 (comment), don't mind if it stays as it is, since it is fixed already in #165
…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
The implementation follows the spec outlined by Bunz in: https://hackmd.io/V4838nnlRKal9ZiTHiGYzw?view.
With this, the NIFS works and passes all tests.
I think with not much work, we should be able to get the IVC circuit + deciders.
Tasks:
OvaandMovaaren't a lot of duplicated code.