Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial experimental HyperNova multifolding support #175

Draft
wants to merge 102 commits into
base: main
Choose a base branch
from

Conversation

oskarth
Copy link
Contributor

@oskarth oskarth commented Jun 2, 2023

Update: PR now includes multifolding work we've done

First cut of experimental HyperNova multifolding support.

Main issue: privacy-scaling-explorations#13

cargo test --features hypernova ccs

Has the following tests:

test ccs::cccs::tests::test_compute_q ... ok
test ccs::cccs::tests::test_compute_Q ... ok
test ccs::lcccs::tests::test_lcccs_v_j ... ok
test ccs::util::tests::test_fix_variables ... ok
test ccs::util::tests::test_compute_all_sum_Mz_evals ... ok
test ccs::util::virtual_poly::test::test_eq_xr ... ok
test ccs::lcccs::tests::test_bad_v_j ... ok
test ccs::lcccs::tests::satisfied_ccs_is_satisfied_lcccs ... ok
test ccs::multifolding::tests::test_compute_g ... ok
test ccs::util::tests::test_compute_sum_Mz_over_boolean_hypercube ... ok
test ccs::cccs::tests::test_Q_against_q ... ok
test ccs::multifolding::tests::test_compute_sigmas_and_thetas ... ok
test ccs::util::virtual_poly::test::test_virtual_polynomial_mul_by_mle ... ok
test ccs::multifolding::tests::test_lcccs_fold ... ok
test ccs::util::virtual_poly::test::test_virtual_polynomial_additions ... ok
test ccs::test::test_tiny_ccs ... ok

Code is written to be merged as-is if there's interest. All code is feature flagged under a hypernova flag that is off by default, so it has no impact on other parts of the code or users of the Nova library.

Logic roughly follows the structure set out in r1cs.rs and only changing the parts that are different for CCS.

Work will continue in the PSE Nova fork (hypernova branch: https://github.com/privacy-scaling-explorations/nova/tree/hypernova). We're also doing some experimental work in arkworks in a separate repo (https://github.com/privacy-scaling-explorations/multifolding-poc), with the goal of eventually integrating it into the Nova code base.

We are open to ideas for direction w.r.t. getting it integrated into the Nova code base. Comments welcome!

@oskarth
Copy link
Contributor Author

oskarth commented Jun 2, 2023

@microsoft-github-policy-service agree

@srinathsetty
Copy link
Collaborator

Hi, @oskarth Thanks for the PR! We certainly welcome contributions in this direction. This looks like an exciting PR! I'll review the PR soon and provide any comments that might come up.

@srinathsetty srinathsetty self-requested a review June 2, 2023 18:09
src/ccs.rs Outdated Show resolved Hide resolved
src/ccs.rs Outdated Show resolved Hide resolved
#[cfg(test)]
mod tests {
use super::*;
use pasta_curves::Fq;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were recent PRs (e.g., #171, #165) to make tests generic over the curves. I wonder if it makes sense to continue that approach to allow library users to switch curves and still run these tests more easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, agree! Will adjust tests accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for this module. We did it w.r.t. PrimeField for these types as that's all they need.

(Will update other tests similarly once consensus on approach, see privacy-scaling-explorations#34)

oskarth and others added 24 commits July 11, 2023 13:17
Still quite rough and lots of bugs
Simplified in recent commit microsoft@b28aaf7
1. Add documentation and test for spartan/polynomial.rs.
- EqPolynomial: $\tilde{eq}$
- MultilinearPolynomial: multilinear polynomial
- SparsePolynomial

2. Remove duplicate get_bits in SparsePolynomial::evaluate.
@@ -0,0 +1,390 @@
use super::cccs::{CCCSInstance, CCCSShape};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just call this file nifs.rs to make it consistent with the existing code?

// instances. Then the rest of CCS, CCCS, LCCCS hold references to it.
// Is our single source of data.
#[derive(Debug)]
pub struct Multifolding<G: Group> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify the purpose of this type? In particular, it is not clear how this type will be used.

#[derive(Debug)]
pub struct Multifolding<G: Group> {
ccs: CCSShape<G>,
ccs_mle: Vec<MultilinearPolynomial<G::Scalar>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If multilinear polynomials are represented over the Boolean hypercube, then there is no difference between the original representation and the MLE-encoded representation. Is it necessary to have two separate ways to storing the same data?

I wonder if we can define certain MLE-related traits and just implement them on CCSShape so there is no need for separately tracking MLE-encoded data.

}

/// Compute g(x) polynomial for the given inputs.
pub fn compute_g(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted above, I think there is no need to compute this polynomial g explicitly. In general, it is more efficient to run the sum-check protocol on an implicit representation of g (i.e., by using the multilinear polynomials that directly constitute g than explicitly materializing g). We can find an example of this in the src/spartan/ where the sum-check is run on degree-3 multivariate polynomials by just working with multilinear polynomials.

sigmas: &[G::Scalar],
thetas: &[G::Scalar],
r_x_prime: Vec<G::Scalar>,
rho: G::Scalar,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't \rho be generated from the RO (i.e., with the sponge)?

.collect();

// XXX: There's no handling of r_w atm. So we will ingore until all folding is implemented,
// let r_w = w1.r_w + rho * w2.r_w;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is r_w?

}

/// Evaluate eq polynomial.
pub fn eq_eval<F: PrimeField>(x: &[F], y: &[F]) -> F {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a type called EqPolynomial in src/spartan/polynomial.rs. Should we use that?

@@ -0,0 +1,498 @@
use crate::hypercube::BooleanHypercube;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a polynomial type in src/spartan/polynomial.rs, which was sufficient to implement the sum-check protocol for R1CS. I wonder if we can reuse that functionality rather than create an alternate type with overlapping functionality.

@@ -0,0 +1,66 @@
//! This module defines basic types related to Boolean hypercubes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Nova does not require this functionality and is needed only for the multi-folding scheme, does it make sense to move this to the ccs module?

use itertools::Itertools;

#[derive(Debug)]
pub(crate) struct BooleanHypercube<F: PrimeField> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type might be unnecessary since multilinear polynomials are going to be stored in evaluation form over the Boolean hypercube. In particular, given a vector z that represents evaluations of multilinear polynomial \widetilde{z}, if we need the evaluation of \widetilde{z}(x) where x is an appropriate bit string represented with usize, we could simply return z[x]. In essence, we are treating bitstrings as indexes into the vector (we can see examples of this in src/spartan/polynomial.rs). This would avoid two separate representations for polynomials.

#[cfg(feature = "hypernova")]
mod hypercube;
#[cfg(feature = "hypernova")]
mod utils;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this to the ccs module as well as this is used only by the hypernova code?

@@ -69,10 +100,13 @@ impl<Scalar: PrimeField> MultilinearPolynomial<Scalar> {
self.Z.len()
}

// NOTE: this is equivalent to Espresso/HyperPlonk's 'fix_last_variables' method
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this comment as it could cause confusion: Espresso/HyperPlonk came way after Spartan (the code below is inherited from Spartan) rather than the other way around :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wins as best comment of the review.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this was an internal note for internal clarity while fixing stuff, not to be send in a PR.

pub fn bound_poly_var_top(&mut self, r: &Scalar) {
let n = self.len() / 2;

let (left, right) = self.Z.split_at_mut(n);

// XXX: What does this do? Seems like nothing?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I think in the past, we weren't resizing the Z vector, so this was necessary, but we do resize Z (in the code below), so this code is likely not needed (I'll double-check).

@oskarth
Copy link
Contributor Author

oskarth commented Jul 20, 2023

@srinathsetty Thanks for the review of this still fairly rough code! Useful to get some feedback on direction to make sure we are on the right track and can prioritize work.

We went over comments and most things made sense to us; will comment as they are addressed. Some are quick fixes, others are TODO/WIP, and some will require a bit more work.

Also as a FYI we have a draft PR for changing API to make it more NIMFS-centric: privacy-scaling-explorations#41 - this should address several of the points raised.

Will mark this PR as draft again for now so you don't have to review it again until we've addressed most or all of your points.

What do you think about splitting this PR up into smaller chunks? It might make it easier to review and merge partial progress. Only where it makes sense and is a self-contained PR of course. Whatever you prefer!

@oskarth oskarth marked this pull request as draft July 20, 2023 06:06
arnaucube and others added 3 commits July 20, 2023 09:35
- Move bit_decompose into hypercube.rs which is closer conceptually and
  only used there (except for a VirtualPolynomial test)
- Remove unused bit_decompose imports from other files
)

Fixes additional clippy errors I see when I upgraded my Rust toolchain
(had some issues with clippy and Homebrew)

```
error: use of `default` to create a unit struct
   --> src/ccs/util/virtual_poly.rs:120:29
    |
120 |         phantom: PhantomData::default(),
    |                             ^^^^^^^^^^^ help: remove this call to `default`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#default_constructed_unit_structs
note: the lint level is defined here
```
@srinathsetty
Copy link
Collaborator

@srinathsetty Thanks for the review of this still fairly rough code! Useful to get some feedback on direction to make sure we are on the right track and can prioritize work.

We went over comments and most things made sense to us; will comment as they are addressed. Some are quick fixes, others are TODO/WIP, and some will require a bit more work.

Also as a FYI we have a draft PR for changing API to make it more NIMFS-centric: privacy-scaling-explorations#41 - this should address several of the points raised.

Thank you, @oskarth!

Will mark this PR as draft again for now so you don't have to review it again until we've addressed most or all of your points.

Sounds good!

What do you think about splitting this PR up into smaller chunks? It might make it easier to review and merge partial progress. Only where it makes sense and is a self-contained PR of course. Whatever you prefer!

Yes, that certainly sounds good!

CPerezz and others added 4 commits July 31, 2023 18:44
This is a proposal for the new API of the Hypernova/multifolding current
impl.

The idea is that the API is
[NIMFS](https://github.com/privacy-scaling-explorations/Nova/blob/change%2FNIMFS_centric_API/src/ccs/multifolding.rs#L39-L44)-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: 
```rust
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.
- Remove custom eq_poly and use EqPolynomial instead
- Update polynomial docs
- Ensures order of evaluation is consistent between for VirtualPoly and
EqPolynomial
- Remove old build_eq_x_r_vec and build_eq_x_r_helper
- Tests around boolean hypercube and other introduced functions to make
endianness explicit etc

Addresses #19
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.
Since the Verifier in the protocol does not have access to the
witnesses, we should not have `z`s inside of the LCCCS and CCCS
instances and instead, work with the witness sepparatedly.

This PR implements this sort of behaviour.
Resolves: #46
Sync upstream

---------

Co-authored-by: François Garillot <[email protected]>
Co-authored-by: Srinath Setty <[email protected]>
@oskarth
Copy link
Contributor Author

oskarth commented Aug 21, 2023

Update: Since the last post some of the comments above have been addressed. However, there's still a lot more work to be done get this to a mergeable state.

Work on HyperNova was a timeboxed bet as part of a "Team Novi" effort. As that concluded a few weeks ago, we decided to pause our effort to integrate HyperNova upstream. Future work on Nova and related things will continue but likely in other forms for the time being.

Outside of comments above, here are some issues that remain: https://github.com/privacy-scaling-explorations/Nova/issues (e.g. privacy-scaling-explorations#50) and privacy-scaling-explorations#28

It is very likely that this PR and other work can be cut up further for easier integration (e.g. like #215), perhaps under an experimental hypernova feature flag.

If someone wants to take this work over and run with it that'd be great! Hopefully it is useful to someone and not a complete waste :) Feel free to take commits above and go for it!

huitseeker pushed a commit to huitseeker/Nova that referenced this pull request Dec 18, 2023
microsoft#175)

* refactor circuit_index dependence

* add check on circuit arities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants