o1-labs / proof-systems

The proof systems used by Mina
https://o1-labs.github.io/proof-systems/
Apache License 2.0
410 stars 91 forks source link

Enforce length of evaluations #751

Closed mimoo closed 1 year ago

mimoo commented 2 years ago

All evaluations in proof::EvaluationProof and proof:LookupEvaluationProof should be of length 1. We should enforce that at the very beginning when receive a proof in verifier.rs.

duguorong009 commented 2 years ago

@mimoo I would like to take care of this issue. 😄

One question. I think the fields proof::EvaluationProof and proof:LookupEvaluationProof are from ProverProof.

/// The proof that the prover creates from a [ProverIndex](super::prover_index::ProverIndex) and a `witness`.
#[serde_as]
#[derive(Clone, Serialize, Deserialize)]
#[serde(bound = "G: ark_serialize::CanonicalDeserialize + ark_serialize::CanonicalSerialize")]
pub struct ProverProof<G: AffineCurve> {
    /// All the polynomial commitments required in the proof
    pub commitments: ProverCommitments<G>,

    /// batched commitment opening proof
    pub proof: OpeningProof<G>,

    /// Two evaluations over a number of committed polynomials
    // TODO(mimoo): that really should be a type Evals { z: PE, zw: PE }
    pub evals: [ProofEvaluations<Vec<G::ScalarField>>; 2],

    /// Required evaluation for [Maller's optimization](https://o1-labs.github.io/mina-book/crypto/plonk/maller_15.html#the-evaluation-of-l)
    #[serde_as(as = "o1_utils::serialization::SerdeAs")]
    pub ft_eval1: G::ScalarField,

    /// The public input
    #[serde_as(as = "Vec<o1_utils::serialization::SerdeAs>")]
    pub public: Vec<G::ScalarField>,

    /// The challenges underlying the optional polynomials folded into the proof
    pub prev_challenges: Vec<RecursionChallenge<G>>,
}

I would like to ask why it is needed to enforce the length of Evaluations, even though the evals field is already typed as [ProofEvaluations; 2].

mimoo commented 2 years ago

ah so, there are two evaluations points (zeta and zeta * omega) which is the 2 you're seeing here. But then for each ProofEvaluations we have vectors of field elements which represent evaluated chunks

note: evaluated chunks are basically taking a polynomial f(x) and seeing it as several polynomials of the same degree f(x) = f_0(x) + 2^n f_1(x) and then producing the evaluations [f_0(z), f_1(z)] for some evaluation point z.

In our current situation we should only have vectors of size 1 (i.e. there's only f(x) = f_0(x))

Btw I think it'd be easier to tackle the other issue that transforms these vectors into an actual type ChunkedEvaluations or something :o before taking a stab at this one.

duguorong009 commented 2 years ago

Thanks for the detail! @mimoo I will keep trying to understand more.

github-actions[bot] commented 1 year ago

Stale issue message

github-actions[bot] commented 1 year ago

Stale issue message

duguorong009 commented 1 year ago

@mimoo Please review the PR #989

mimoo commented 1 year ago

So an update on the issue, which I think still misses a number of things:

for the latter one, see this example in https://github.com/o1-labs/proof-systems/pull/989/files#diff-fc95db4d289fd33bc7fbefe8a487926ce8b47cbe71f047a1dd6933d7444b5d5aR521:

if let Some(LookupEvaluations {

which will not check if there's no lookup evaluations. But if the index says that lookup should be here then we should error here instead. I talked about it here as well: https://github.com/o1-labs/proof-systems/issues/540

duguorong009 commented 1 year ago

@mimoo I think the issue is going beyond the original purpose. As the name suggests, the purpose of this issue is to add the logic of enforcing the length of evaluations. I suggest we solve the issues you mentioned step by step, in their own issues. And we can close this issue & merge PR. What do you think?

mimoo commented 1 year ago

sounds good! merged yours and moving to this one https://github.com/o1-labs/proof-systems/issues/700