MSM/Serialization: start lookup #1944
Conversation
dannywillems
force-pushed
the
dw/implement-lookup-rangecheck-msm-serialization
branch
3 times, most recently
from
ef67192
to
bc46279
Compare
| Self::RangeCheck15 => (0..(1 << 15)) | ||
| .map(|i| Lookup { | ||
| table_id: LookupTable::RangeCheck15, | ||
| numerator: -F::one(), |
There was a problem hiding this comment.
Why are these -1 by default? I'd assume that by default we are not using any of the elements, so they should have 0?
There was a problem hiding this comment.
See equation (15): https://eprint.iacr.org/2022/1530.pdf.
There was a problem hiding this comment.
Ah, ok, I think I misunderstood what this function does. Please tell me if I'm right: what you're doing is given a fixed table you're building a set of queries for that table covering every element of this table?
| @@ -102,11 +145,83 @@ mod tests { | |||
| constraints.push(cst.clone()) | |||
| } | |||
| } | |||
|
|
|||
| for (j, lookup) in witness_env.rangecheck4_lookups.iter().enumerate() { | |||
There was a problem hiding this comment.
Probably next step, but this code that collects something from witness and passes to MVLookup can be arguably moved to witness.rs so that witness_env.get_inputs would give you the inputs directly
| pub lookup_t_multiplicities_rangecheck4: Box<[Fp; 1 << 4]>, | ||
|
|
||
| /// Keep track of the RangeCheck15 lookup multiplicities | ||
| /// No t multiplicities as we do suppose we have a domain of |
There was a problem hiding this comment.
How does this work? You still need to construct the multiplicities for t, I guess you wanted to say that it's easier in the case of 2^15 domain size, but why? 🤔
There was a problem hiding this comment.
See equation (14): https://eprint.iacr.org/2022/1530.pdf
There was a problem hiding this comment.
I thought (14) does not apply to our case because our t(x) is fully injective, since every element in the range table is unique? Why do we need normalised multiplicities?
| &self, | ||
| _domain: EvaluationDomains<Fp>, | ||
| ) -> Vec<Fp> { | ||
| self.lookup_multiplicities_rangecheck15.to_vec() |
There was a problem hiding this comment.
Ah OK, answered my own question about why 15bit case does not have t. The multiplicities are /exactly/ equal to the "called" vector that we build in the witness, so they are much easier to compute.
| .into_iter() | ||
| .zip(self.lookup_t_multiplicities_rangecheck4.iter()) | ||
| .enumerate() | ||
| .for_each(|(i, (m_f, m_t))| m[i] = m_f / m_t); |
There was a problem hiding this comment.
Why do we need to do this (field? or is it integer?) division here? Is it related to the dummy values?..
There was a problem hiding this comment.
See equation 14: https://eprint.iacr.org/2022/1530.pdf
| rangecheck15[j].push(lookup.clone()) | ||
| } | ||
|
|
||
| witness_env.add_rangecheck4_table_value(i); |
There was a problem hiding this comment.
I feel like this needs to be called somewhere from range_check4() function on the witness impl InterpreterEnv side, no?
There was a problem hiding this comment.
I would like to move it somewhere else, yes. The table should be built outside the interpreter as it is a fixed table. We should have a way to handle multiple domain sizes.
| if i < (1 << 4) { | ||
| self.lookup_t_multiplicities_rangecheck4[i] += Fp::one(); | ||
| } else { | ||
| self.lookup_t_multiplicities_rangecheck4[0] += Fp::one(); |
There was a problem hiding this comment.
Is this dummy value handling? Wondering if it's not bringing any soundness issues, since in the first if case you also add to the [0] element, right?
There was a problem hiding this comment.
See equation 14: https://eprint.iacr.org/2022/1530.pdf
dannywillems
force-pushed
the
dw/implement-lookup-rangecheck-msm-serialization
branch
from
bc46279
to
c9690f2
Compare
dannywillems
deleted the
dw/implement-lookup-rangecheck-msm-serialization
branch
Constraints coming in a follow-up PR.
We can verify that the commitments are handled properly by commenting in the verifier the absorption of the multiplicities, for instance.