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

Chunking RFC #2

Merged
merged 10 commits into from
Oct 26, 2023
Merged

Chunking RFC #2

merged 10 commits into from
Oct 26, 2023

Conversation

mrmr1993
Copy link
Member

@mrmr1993 mrmr1993 commented Jun 6, 2023

Copy link
Contributor

@barriebyron barriebyron left a comment

Choose a reason for hiding this comment

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

Congratulations and kudos @mrmr1993 for submitted our first O(1) Labs official PRD.

Your PRD is a model of completeness, context, benefit, and problems solved. Well done!

0002-chunking.md Show resolved Hide resolved
0002-chunking.md Outdated Show resolved Hide resolved

### Technical approach in kimchi

Support for chunked commitments already exists [in kimchi's supporting polynomial commitment library](https://github.com/o1-labs/proof-systems/blob/ca0b9e53d15e3ab5db6fb8c20617979cbf74215e/poly-commitment/src/srs.rs#L89). This RFC proposes to extend that support through to the kimchi prover, where an attempt to use the feature currently [results in an error](https://github.com/o1-labs/proof-systems/blob/ca0b9e53d15e3ab5db6fb8c20617979cbf74215e/kimchi/src/prover.rs#L170). This RFC also proposes to add support for chunked commitments and evaluations to pickles, where there is currently no support.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we "buried the lede" here, this detail would be very helpful in RFC summary section

0002-chunking.md Outdated Show resolved Hide resolved
0002-chunking.md Outdated Show resolved Hide resolved
0002-chunking.md Outdated Show resolved Hide resolved
0002-chunking.md Outdated

Due to the requirement for proof layout homogeneity, the `Pickles.compile` interface should take parameters for the `step_num_chunks` and `wrap_num_chunks` at the toplevel. These arguments may be optional labelled arguments, since they will not be regularly used in the protocol circuits. These arguments will have to be passed explicitly by the user / caller when needed, since it will be prohibitively expensive to try all of the various options programatically on every compile.

The `Pickles.compile` function should be modified to check for the number of chunks in each inductive rule's previous rule tags. When these are not consistent between inductive rules, the code must throw an error in order to achieve layout homogeneity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `Pickles.compile` function should be modified to check for the number of chunks in each inductive rule's previous rule tags. When these are not consistent between inductive rules, the code must throw an error in order to achieve layout homogeneity.
The `Pickles.compile` function must be modified to check for the number of chunks in each inductive rule's previous rule tags. When these numbers are not consistent between inductive rules, the code must throw an error in order to achieve layout homogeneity.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrmr1993 I love this first PRD, congratulations! What do we mean by "these" (these tags? or these numbers?)

Copy link
Member Author

Choose a reason for hiding this comment

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

These numbers is correct. The pickles recursion layer needs everything to have the 'same shape', but the 'number of chunks' result in different layouts for the proofs that it consumes. Is there a way to word this that would make that detail clearer?

mrmr1993 and others added 5 commits June 6, 2023 23:28
Co-authored-by: Barrie Byron <[email protected]>
Co-authored-by: Barrie Byron <[email protected]>
Co-authored-by: Barrie Byron <[email protected]>
Co-authored-by: Barrie Byron <[email protected]>
Co-authored-by: Barrie Byron <[email protected]>
* [Add zero-knowledge for chunked proofs](https://github.com/o1-labs/proof-systems/pull/1045)
* [Update bindings to enable chunking](https://github.com/MinaProtocol/mina/pull/13286)

## Test Plan and Functional Requirements
Copy link
Member

Choose a reason for hiding this comment

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

This section needs a little more detail I think -- unit testing what specifically at each layer? Which parts of the system? And then we should sketch out the SnarkyJS test also in a bit more detail, but maybe that entails figuring out the SnarkyJS details first: I suppose something with ZkProgram though?

## Detailed design
[detailed-design]: #detailed-design

### Theory overview
Copy link
Member

Choose a reason for hiding this comment

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

This is amazing content, but maybe it's better served as a chapter of the Mina book and linked to from this RFC, so it's more accessible? What do you think?

This is of course not clear from the current template, so we can adjust after we settle this.

0002-chunking.md Show resolved Hide resolved
domain_size = circuit_size + zk_rows
```

### Technical approach in kimchi
Copy link
Member

Choose a reason for hiding this comment

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

This part I think is definitely perfect to remain in the RFC


This proposal optimizes for compatibility with the kimchi and pickles [versions proposed for deployment in the Berkeley hard-fork](https://github.com/MinaProtocol/MIPs/blob/main/MIPS/mip-kimchi.md), as well as the [proposed zkApps feature](https://github.com/MinaProtocol/MIPs/blob/main/MIPS/mip-zkapps.md), at the expense of some potential loss of performance when the feature is in use. Timing measurements can be found in the PR description for the [prototype implementation](https://github.com/o1-labs/proof-systems/pull/1033).

## Detailed design
Copy link
Member

Choose a reason for hiding this comment

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

Does this impact anything other than Pickles/Kimchi and SnarkyJS (and the bindings)? Probably not, but just double checking. Maybe worth calling out explicitly? I tried to do that in the RFC I just drafted.

[unresolved-questions]: #unresolved-questions

* What parts of the design do you expect to resolve through the RFC process before this gets merged?
- SnarkyJS design
Copy link
Member

Choose a reason for hiding this comment

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

What exactly do we need to expose to the user? The number of chunks? Can't we compute that at circuit compile time? This is important for the SnarkyJS API design

@mitschabaude
Copy link
Contributor

mitschabaude commented Jun 15, 2023

Amazing RFC, @mrmr1993! I was intrigued by this because I love Lagrange polynomials:

Value columns in the Mina proof are represented in evaluation form, as a list of values. In order to support efficient evaluations over these values, the LagrangeBasisEvaluations structure will need to be generalized to support evaluations of the 'chunked lagrange basis'. The approach to achieve this is equivalent to the one used in the SRS's add_lagrange_basis function, but using the explicit powers of z (z^0, z^1, ..., z^n) in place of the SRS curve points. This uses an iFFT, so will result in a performance degradation from linear time (O(n)) to O(c n log(n)) when chunking is in use, where c is the number of chunks.

I hope I didn't misunderstand the problem statement, but I think I found a way to do polynomial evaluations in O(n).

So the problem, as I understood it, is to evaluate $f_0(z), ..., f_3(z)$ at an arbitrary point $z$ (sticking with your example of 4 chunks), given the evaluations of

$$f = f_0 + x^n f_1 + x^{2n} f_2 + x^{3n} f_3$$

over the domain $1,\omega,...,\omega^{4n-1}$. Here $\omega$ is a $4n$-th root of 1.

The rough idea to solve it is to use this formula for the Lagrange polynomials:

$$L_i(x) = \frac{1}{4n} \sum_{j=0}^{4n-1} (\omega^{-i} x)^j$$

And now split up each Lagrange polynomial in a way that matches the splitting up of $f$ into $f_0,...,f_3$. In fact you can split it into

$$L_i(x) = L^0_i + (\omega^{-i}x)^{n}L^0_i + (\omega^{-i}x)^{2n}L^0_i + (\omega^{-i}x)^{3n}L^0_i$$

where

$$L^0_i(x) = \frac{1}{4n} \sum_{j=0}^{n-1} (\omega^{-i} x)^j.$$

Plugging that into the evaluation formula for $f$,

$$f(x) = \sum_{i=0}^{4n-1} f(\omega^i) L_i(x),$$

you arrive at corresponding formulas for the chunks $f_k$, $k = 0,1,2,3$,

$$f_k(x) = \sum_{i=0}^{4n-1} f(\omega^i) \omega^{-ikn} L^0_i(x).$$

All the $L^0_i(x)$ can be evaluated in ${O}(n)$, and so, all the $f_k$ can as well. In fact, if you split up $i$ by its residue modulo 4, like $i \rightarrow 4i + k$, where $i = 0,...,n-1$ and $k=0,1,2,3$, I can show that

$$4 L^0_{4i + k}(x) = \frac{\omega^{4i}}{n} \prod_{j\ne i}^{n-1} (\omega^{-k}x - \omega^{4j}) = L^n_i (\omega^{-k}x)$$

where the right hand side is just the $i$-th Lagrange polynomial with respect to the sub-domain $1,\omega^{4},...,\omega^{4(n-1)}$, generated by the $n$-th root of 1, $\omega^4$.

(As an aside, I think the LagrangeBasisEvaluations can be improved to need fewer multiplications and no division.)

Copy link

@jspada jspada left a comment

Choose a reason for hiding this comment

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

Really nice write-up. Love the detail and the level it was written at. I've asked some questions that occur to me, which may be useful to the reader if answered with some detail.

```
by the curve point
```text
g_0^{a_0} + g_1^{a_1} + ... + g_n^{a_n}
Copy link

Choose a reason for hiding this comment

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

I really like the way you describe this as what it is.

and
```
(x - omega^(n-zk_rows+1)) *
(x - omega^(n-zk_rows+2))
Copy link

Choose a reason for hiding this comment

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

nit: missing a * here

```
(x - omega^(n-zk_rows+1)) *
(x - omega^(n-zk_rows+2))
(x - omega^(n-1))
Copy link

Choose a reason for hiding this comment

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

and here

```
zk_polynomial(x) *
( aggregation(x)
* (w_0(x) + gamma + x * beta * shift_0)
Copy link

Choose a reason for hiding this comment

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

It would be helpful (at least to me) to explain what shift is


This prevents us from adding the `zk_rows` of randomness to the `aggregation` polynomial, since we cannot create a large enough `zk_polynomial` to mask out the relevant values.

However, we *can* choose the values at `n-zk_rows+1`, and `n-zk_rows+2` at random. The verifier may still be able to guess the *ratio* `aggregation(x * omega)/aggregation(x)` at every non-randomised point in the domain, but they will have negligible (`~1/2^254`) probability of choosing the *actual value* of `aggregation(x)` for all values between `n-zk_rows+1` and `n-1`.
Copy link

Choose a reason for hiding this comment

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

You lost me here. Could you explain what the targeted rows n-zk_rows+1 and n-zk_rows+2 are and why it helps to make them random? How could the verifier guess the ratio? What's the math that works out the negligible (~1/2^254) probability? Why is it important not to guess only those values between n-zk_rows+1 and n-1 and not all the other rows?


The permutation argument interacts with the `c` chunks in parallel, so it is
possible to cross-correlate between them to compromise zero knowledge. We know
that there is some `c >= 1` such that `zk_rows = 2*c + k` from the above. Thus,
Copy link

Choose a reason for hiding this comment

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

I thought we could not have zk_rows = 2c + k because it's too big for the d8 domain size?

@barriebyron
Copy link
Contributor

@mrmr1993 is this PR ready to merge?
I am on deck to write a blog using this content

@joseandro joseandro merged commit 6a06671 into add-template Oct 26, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 26, 2023
* Adds template for RFCs

* Chunking RFC (#2)

Added the chunking RFC

---------

Co-authored-by: Brandon Kase <[email protected]>
Co-authored-by: Matthew Ryan <[email protected]>
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.

6 participants