feat: translation evaluations: refactor + soundness fix #12051
Conversation
| @@ -157,50 +157,8 @@ void ECCVMProver::execute_pcs_rounds() | |||
| sumcheck_output.round_univariates, | |||
| sumcheck_output.round_univariate_evaluations); | |||
|
|
|||
| // Get the challenge at which we evaluate all transcript polynomials as univariates | |||
There was a problem hiding this comment.
isolated into a separate method
| @@ -0,0 +1,90 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
It's a new class in a way similar to ZKSumcheckData. It is supposed to accept the eccvm transcript wires, extract, and concatenate the masking terms to prepare them for the SmallSubgroupIPA.
| commitments.transcript_z1, | ||
| commitments.transcript_z2 }; | ||
|
|
||
| const OpeningClaim translation_opening_claim = reduce_verify_translation_evaluations(transcript_commitments); |
There was a problem hiding this comment.
again, isolated the translation evaluations logic into a separarte method to improve readability and make it uniform with the prover class
| @@ -209,9 +167,6 @@ void ECCVMProver::execute_pcs_rounds() | |||
|
|
|||
| // Compute the opening proof for the batched opening claim with the univariate PCS | |||
| PCS::compute_opening_proof(key->commitment_key, batch_opening_claim, ipa_transcript); | |||
|
|
|||
| // Produce another challenge passed as input to the translator verifier | |||
| translation_batching_challenge_v = transcript->template get_challenge<FF>("Translation:batching_challenge"); | |||
There was a problem hiding this comment.
This was a very strange step, the challenge that has to be propagated is the batching_challenge_v produced in reduce_translation_evaluations
| // Load the proof produced by the translator prover | ||
| transcript->load_proof(proof); | ||
|
|
||
| Flavor::VerifierCommitments commitments{ key }; | ||
| Flavor::CommitmentLabels commitment_labels; | ||
|
|
||
| const auto circuit_size = transcript->template receive_from_prover<uint32_t>("circuit_size"); | ||
| evaluation_input_x = transcript->template receive_from_prover<BF>("evaluation_input_x"); |
There was a problem hiding this comment.
this was not sound, the prover could've sent anything here, switched to the propagation of these challenges from the ECCVMVerifier to TranslatorVerifier.
iakovenkos
changed the title
…ocol/aztec-packages into si/fix-translation-evaluations
iakovenkos
changed the title
| @@ -103,6 +103,36 @@ TEST_F(GoblinRecursiveVerifierTests, Basic) | |||
| } | |||
| } | |||
|
|
|||
| // Check that the GoblinRecursiveVerifier circuit does not depend on the inputs. | |||
There was a problem hiding this comment.
Added this test to catch any discrepancies caused by interactions between different components of goblin
| @@ -24,4 +24,7 @@ static constexpr uint32_t MASKING_OFFSET = 4; | |||
| // For ZK Flavors: the number of the commitments required by Libra and SmallSubgroupIPA. | |||
| static constexpr uint32_t NUM_LIBRA_COMMITMENTS = 3; | |||
| static constexpr uint32_t NUM_LIBRA_EVALUATIONS = 4; | |||
| // There are 5 distinguished wires in ECCVM that have to be opened as univariates to establish the connection between | |||
| // ECCVM and Translator | |||
| static constexpr uint32_t NUM_TRANSLATION_EVALUATIONS = 5; | |||
There was a problem hiding this comment.
Seems like the eccvm flavor might be a better place for this constant
There was a problem hiding this comment.
It was also implicitly used in TranslationEvaluations struct that is propagated to Translator, I just modified it to use this constant explicitly. It is also used by the SmallSubgroupIPA in #12244, so I'd keep it here. But probably we'll need to revise these constants at some point.
barretenberg/cpp/src/barretenberg/goblin/translation_evaluations.hpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_verifier.hpp
Outdated
Show resolved
Hide resolved
In the ECCVM, we have a special sub-protocol to prove the univariate evaluations of
Op,Px,Py,z1, andz2. The corresponding logic used to be un-rolled inexecute_pcs_rounds()andverify_proof(). I isolated this logic into separate methods for the following reasons:The following Goblin soundness issue has been discovered:
evaluation_challenge_xsampled in ECCVM would be propagated from theECCVMProverto theTranslatorProverand sent to theTranslatorVerifiermeaning that a maliciousGoblinProverwas free to send any field element and chooseaccumulated_result.ECCVMProver/TranslatorVerifier) the translation evaluations proof seemed redundant and would have blocked the further changes. Namely, in the fix being implemented, we must enforce that the batching challenge used to compute the batched claim is the batching challenge used by the Translator.TranslatorVerifierretrievesevaluation_challenge_xandbatching_challenge_vas class members of ECCVMVerifier.