Arrabiata: prepare constraints for cross-terms computation #2702

dannywillems · 2024-10-14T07:45:45Z

This pull request prepares the constraints to compute the cross-terms after the different columns and arguments have been computed. Challenges must be treated as a variable, and will be in a future PR.

Previous attempt: #2695

codecov · 2024-10-14T09:59:07Z

Codecov Report

Attention: Patch coverage is 53.84615% with 30 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@5b4ac14). Learn more about missing BASE report.
Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
mvpoly/src/prime.rs	0.00%	11 Missing ⚠️
mvpoly/src/monomials.rs	83.33%	7 Missing ⚠️
arrabbiata/src/columns.rs	0.00%	6 Missing ⚠️
arrabbiata/src/main.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #2702   +/-   ##
=========================================
  Coverage          ?   72.10%           
=========================================
  Files             ?      257           
  Lines             ?    61154           
  Branches          ?        0           
=========================================
  Hits              ?    44094           
  Misses            ?    17060           
  Partials          ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marcbeunardeau88 · 2024-10-29T09:11:43Z

mvpoly/src/lib.rs

+    polys: Vec<T>,
+    combiner1: F,
+    combiner2: F,
+    eval1: [F; N],
+    eval2: [F; N],
+    u1: F,
+    u2: F,


I m not sure I understand what is computed here ?
the text mentioned combining the polys with one combiner, and we have two of them here.
I understand that the two combiners are the alpha from each instances we fold ?
But then I don't get the assymetrical role ?

See https://hackmd.io/@dannywillems/Syo5MBq90#Computation-of-cross-terms-for-shifted-polynomials

marcbeunardeau88 · 2024-10-29T09:18:01Z

arrabiata/tests/constraints.rs

+
+// This test is mostly an example to show to compute the cross-terms when we do
+// have the expressions, and some evaluations.
+// It doesn't test anything in particular. It is mostly an "integration" test.


A way to make this test test thing would be to checked the cross term property
poly(a +r*b) = poly(a) + r^n poly(b) + \sum_i r^i cross_term_i(a,b)
Also a nit: I wouldn't call that an integration test

"Also a nit: I wouldn't call that an integration test"

That's the reason "integration" is double quoted.

marcbeunardeau88 · 2024-10-29T09:20:18Z

mvpoly/src/lib.rs

            // Computing α'^i * value, and adding it to the next power of r
-            let alpha_i = combiner2.pow([i as u64]);
+            // Handling 0^0 = 1


isn't combiner a random term that cannot be zero ?

Yes, but it was monstly for the tests (see test directory). I didn't handle it initially. Even if the case is negligible, I think it is ok to have it. Maybe we should fail instead?

I'd ignore it to simplify the code, and remove the testing with zero, but do as you feel

marcbeunardeau88 · 2024-10-29T09:25:59Z

mvpoly/src/utils.rs

+/// It is useful when the number of nested loop is large and the size of the
+/// loops is large as well. `filter_map` is used to avoid computing the indices
+/// that are not needed and avoid a `capacity_overflow` error.
+pub fn compute_indices_nested_loop_upper_bound(


I would not expose this function, keep this implementation, and expose compute_indices_nested_loop implemented with that implementation, while computing the upper bound on the fly with a max.
Then we have one function instead of two, which is the efficient variant, and only need one argument

marcbeunardeau88 · 2024-10-29T09:29:17Z

I could not convinced my self of the correctness of the algorithm, and there is no test that reassure me.
Otherwise only small remarks.

marcbeunardeau88

I'd like a test on the defining property of cross terms to convince me of the correctness of the algorithm, which I wasn't able to check manually

dannywillems · 2024-10-29T10:30:08Z

mvpoly/src/lib.rs

+/// linearly combined using the power of a combiner, often called `α`.
+/// Powers of the combiner are considered as an additional variable of the
+/// multi-variate polynomials, therefore increasing the degree of the sum of all
+/// polynomials by one. In other words, if we combine (M + 1) polynomials


See the comment just above:

/// Powers of the combiner are considered as an additional variable of the /// multi-variate polynomials, therefore increasing the degree of the sum of all /// polynomials by one.

Instead of having a power, we do consider them as a different variable.

dannywillems · 2024-10-29T10:30:28Z

mvpoly/src/lib.rs

+/// cross-terms of the polynomial:
+///
+/// ```text
+/// P(X_1, ..., X_N, α_1, ..., α_M) =     P_1(X_1, ..., X_N)


See https://github.com/o1-labs/proof-systems/pull/2702/files#r1820537584

dannywillems · 2024-10-29T10:35:57Z

I could not convinced my self of the correctness of the algorithm, and there is no test that reassure me.

See https://hackmd.io/@dannywillems/Syo5MBq90#Computation-of-cross-terms-for-shifted-polynomials. And we also discussed it in the past in person. Agreed to have more comments and more PBT.

We do suppose evaluations of the public inputs will be given after the witness.

marcbeunardeau88 · 2024-12-19T09:26:02Z

arrabbiata/src/columns.rs

 impl From<Column> for usize {
    fn from(val: Column) -> usize {
        match val {
            Column::X(i) => i,
-            _ => unimplemented!("Only the private inputs are supported for now"),
+            Column::PublicInput(i) => NUMBER_OF_COLUMNS + i,


Doesn't having (at least) a full columns for PI can create issue on the perfs of the verifier.
More importantely, this translate in the folder verifier which will handle a big PI.
We can use the PLONK way to handle PI to avoid a PI being linear in the number of rows

Since this is not the topic of the PR, we can see that later

Yes, I want to investigate other solutions to handle PI.

marcbeunardeau88 · 2024-12-19T11:08:24Z

mvpoly/src/lib.rs

+    /// the polynomial `P`, i.e. `D`.
+    ///
+    /// The output is a map of `D` values that represents the cross-terms
+    /// for each power of `r`.


I'd add in the description of the funciton something like the following to summarize
let res = compute_cross_terms_scaled(P,eval1,eval2,u1,u2,scalar1,scalar2)
let P'[X_1, ..., X_n,U,Alpha] be P homogenised by U and scaled by Alpha
Then for all r,
P'(eval_1,u1,scalar1) + r^{D+1} P'(eval_2,u2,scalar2) + \sum_{i=1}^{D} r^i res[i] =
P'(eval_1 +reval_2, u1 + ru_2,scalar1+r*scalar2)

Sure, I'll do it in a follow-up.

Updated in the HackMD.

marcbeunardeau88

I did not check the maths but the test is convincing enough

dannywillems self-assigned this Oct 14, 2024

dannywillems marked this pull request as draft October 14, 2024 07:45

Base automatically changed from dw/remove-old-methods-in-interpreter to master October 14, 2024 15:57

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 5 times, most recently from 4f07a8a to f713363 Compare October 15, 2024 06:56

dannywillems changed the base branch from master to dw/mvpoly-from-expr-generic-over-challenge October 15, 2024 08:26

dannywillems mentioned this pull request Oct 15, 2024

Arrabiata: WIP #2695

Closed

dannywillems force-pushed the dw/mvpoly-from-expr-generic-over-challenge branch from a35fa79 to 0ef15d6 Compare October 15, 2024 08:29

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 4 times, most recently from 70addbd to 0129e25 Compare October 15, 2024 14:29

dannywillems mentioned this pull request Oct 15, 2024

MVPoly: improve the library + fix sub in monomials #2713

Merged

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 0129e25 to 32a4f93 Compare October 15, 2024 14:39

dannywillems changed the base branch from dw/mvpoly-from-expr-generic-over-challenge to dw/mvpoly-misc-changes October 15, 2024 14:39

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 32a4f93 to 97fbed5 Compare October 15, 2024 14:56

Base automatically changed from dw/mvpoly-misc-changes to master October 15, 2024 16:27

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 97fbed5 to 9eff145 Compare October 15, 2024 16:56

dannywillems changed the base branch from master to dw/improve-from-variable-implementation October 15, 2024 16:58

dannywillems force-pushed the dw/improve-from-variable-implementation branch 3 times, most recently from 0b6dd27 to 594f689 Compare October 16, 2024 12:52

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 2 times, most recently from 6b94b8b to bb75668 Compare October 16, 2024 13:17

dannywillems requested a review from marcbeunardeau88 October 16, 2024 13:19

dannywillems marked this pull request as ready for review October 16, 2024 13:20

marcbeunardeau88 reviewed Oct 29, 2024

View reviewed changes

marcbeunardeau88 requested changes Oct 29, 2024

View reviewed changes

dannywillems commented Oct 29, 2024

View reviewed changes

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 2 times, most recently from 44f7e95 to 2f3661d Compare December 11, 2024 13:32

This was referenced Dec 11, 2024

MVPoly/monomials: improve generation of random generation #2870

Merged

mvpoly: introduce optional param upper bound to compute nested loops #2872

Merged

dannywillems added 3 commits December 16, 2024 19:42

Arrabiata/main: add more future tasks to remind

29ca8f1

Arrabiata/Column: implement column to usize

5655290

Arrabiata/Column: support conversion of public inputs into usize

0083c30

We do suppose evaluations of the public inputs will be given after the witness.

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 5 times, most recently from 1d283cd to b6a4e54 Compare December 18, 2024 19:50

dannywillems added 2 commits December 18, 2024 20:53

mvpoly: compute cross terms of a scaled polynomial

2d2265c

mvpoly/monomials: allow to artificially increase the degree

00a6119

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from b6a4e54 to 00a6119 Compare December 18, 2024 19:55

marcbeunardeau88 reviewed Dec 19, 2024

View reviewed changes

dannywillems requested a review from marcbeunardeau88 December 19, 2024 09:43

marcbeunardeau88 reviewed Dec 19, 2024

View reviewed changes

marcbeunardeau88 approved these changes Dec 19, 2024

View reviewed changes

dannywillems merged commit a87933c into master Dec 19, 2024
8 checks passed

dannywillems deleted the dw/mvpoly-arrabiata-convert-into branch December 19, 2024 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arrabiata: prepare constraints for cross-terms computation #2702

Arrabiata: prepare constraints for cross-terms computation #2702

dannywillems commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024 •

edited

Loading

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

marcbeunardeau88 commented Oct 29, 2024

marcbeunardeau88 left a comment

dannywillems Oct 29, 2024

dannywillems Oct 29, 2024

dannywillems commented Oct 29, 2024 •

edited

Loading

marcbeunardeau88 Dec 19, 2024

dannywillems Dec 19, 2024

marcbeunardeau88 Dec 19, 2024

dannywillems Dec 19, 2024

dannywillems Dec 19, 2024

marcbeunardeau88 left a comment

Arrabiata: prepare constraints for cross-terms computation #2702

Arrabiata: prepare constraints for cross-terms computation #2702

Conversation

dannywillems commented Oct 14, 2024 • edited Loading

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannywillems Oct 29, 2024 • edited Loading

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

marcbeunardeau88 commented Oct 29, 2024

marcbeunardeau88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannywillems commented Oct 29, 2024 • edited Loading

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

marcbeunardeau88 left a comment

Choose a reason for hiding this comment

dannywillems commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

dannywillems Oct 29, 2024 •

edited

Loading

dannywillems commented Oct 29, 2024 •

edited

Loading