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

[Architecture explanation] MV Lookup PR #16

Draft
wants to merge 28 commits into
base: scroll-before-mvlookup
Choose a base branch
from

Conversation

mratsim
Copy link

@mratsim mratsim commented Jan 29, 2024

cc @AlekseiVambol , you can ask your questions here in-line with the code

akinovak and others added 28 commits July 4, 2023 09:49
Co-authored-by: ying tong <therealyingtong@users.noreply.github.com>
Co-authored-by: ying tong <therealyingtong@users.noreply.github.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Co-authored-by: therealyingtong <yingtong.lai@gmail.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Co-authored-by: Andrija <akinovak@gmail.com>
Author: therealyingtong <yingtong.lai@gmail.com>
.or_insert(format!("x{}", i));
}
eprintln!(" Lookup inputs:");
for input_expressions in lookup.inputs_expressions.iter() {

Choose a reason for hiding this comment

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

What is happening at the lines 549-625? Why are the "a.append(&mut b);" lines provided instead of arithmetic operations?

Copy link

Choose a reason for hiding this comment

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

Oh it just collects all the data to print out the information as given in the example here:

/// Lookup inputs:
/// L0 = x1 * x0 + (1 - x1) * 0x2
/// ^
/// | Cell layout in region 'Faulty synthesis':
/// | | Offset | A0 | F1 |
/// | +--------+----+----+
/// | | 1 | x0 | x1 | <--{ Lookup inputs queried here
/// |
/// | Assigned cell values:
/// | x0 = 0x5
/// | x1 = 1
.

So it parses the input expression to collect all the cell values that are used. So it doesn't actually try to calculate the input expression (so no arithmetic), it just prints all the used cell values for debugging purposes.

Copy link

@AlekseiVambol AlekseiVambol Feb 2, 2024

Choose a reason for hiding this comment

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

Thanks! Just noticed that the data type of the value being appended is not a field element, but a key-value pair:
image
Thus, this makes sense for me now.

@@ -185,7 +187,7 @@ pub struct Evaluator<C: CurveAffine> {
/// Custom gates evalution
pub custom_gates: GraphEvaluator<C>,
/// Lookups evalution
pub lookups: Vec<GraphEvaluator<C>>,
pub lookups: Vec<(Vec<GraphEvaluator<C>>, GraphEvaluator<C>)>,

Choose a reason for hiding this comment

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

Could you explain me the purpose of the "GraphEvaluator" data type (the line 200, if you already know the necessary details)? Perhaps, I will have to deal with it, though still not sure.

Copy link

Choose a reason for hiding this comment

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

It's a helper struct to do all the necessary evaluations in a more optimal way. So our circuits have lots of expressions, and most of them often overlap. So this tries to find sub expressions that are shared between all these expressions so it can optimize the calculation of all necessary expressions. Simple example: we have two expressions:

  • a + b + c
  • a + b + d

In a naive way, this requires 4 additions (two additions for each expression). However a + b is shared between the two expressions. So what the GraphEvaluator tries to do is calculate t := a + b once, and then calculate t + c and t + d to get the result of both expressions, but now only using 3 additions.

*MSM_COUNTER.lock().unwrap() = BTreeMap::new();
*FFT_COUNTER.lock().unwrap() = BTreeMap::new();
}

Choose a reason for hiding this comment

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

Do I correctly understand that at the line 81 ("let instance: Vec<InstanceSingleScheme::Curve> = instances") the array describing the sets of the instance collumns for each circuit intsance is converted into the array of corresponding polynomials, each of which is given in two forms: coefficients of the powers of x and coefficients of the Lagrange basis polynomials? And what is the boolean variable "P::QUERY_INSTANCE"?

Copy link

Choose a reason for hiding this comment

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

I think so yes.

P::QUERY_INSTANCE seems related to the commitment scheme. It is true for IPA and false for KZG. Why or what it does however, I do not know because this also seems something new (or at least I can't remember).

@jonathanpwang
Copy link

Just FYI, I discovered that the direct cherry-pick of Scroll's PR will cause an error because there is an implicit assumption about the behavior of parallelize which changed after @mratsim 's optimization. See axiom-crypto@45e940c

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.

7 participants