-
Notifications
You must be signed in to change notification settings - Fork 576
chore: civc tidy 3 #16671
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
chore: civc tidy 3 #16671
Conversation
| hide_op_queue_accumulation_result(circuit); | ||
|
|
||
| // Propagate the public inputs of the tail kernel by converting them to public inputs of the hiding circuit. | ||
| auto num_public_inputs = static_cast<size_t>(honk_vk->num_public_inputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tail now uses the databus - it has no public inputs to propagate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah great! I think I removed it and had some tests failing in my previous clean up.
| ClientIVC::VerificationKey ClientIVC::get_vk() const | ||
| { | ||
| return { honk_vk, std::make_shared<ECCVMVerificationKey>(), std::make_shared<TranslatorVerificationKey>() }; | ||
| BB_ASSERT_EQ(verification_queue.size(), 1UL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now just extract the hiding kernel VK from the queue
| public: | ||
| size_t num_circuits_accumulated = 0; // number of circuits accumulated so far | ||
|
|
||
| ProverFoldOutput fold_output; // prover accumulator and fold proof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only track the accumulator since the proof is stored internal to the verification queue
|
|
||
| for (size_t j = 0; j < num_circuits; ++j) { | ||
| // Use default test settings for the mock hiding kernel since it's size must always be consistent | ||
| if (j == num_circuits - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now handled internal to the mock circuit produced
| { | ||
| // Note: a structured trace is not used for the hiding kernel | ||
| auto hiding_decider_pk = std::make_shared<DeciderZKProvingKey>(circuit, TraceSettings(), bn254_commitment_key); | ||
| honk_vk = std::make_shared<MegaZKVerificationKey>(hiding_decider_pk->get_precomputed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prior to this PR we were recomputing the hiding VK on the fly rather than using the precomputed one. This is why we weren't getting errors even though the precomputed VK was incorrectly using a structured trace
| * @brief Verify an IVC proof | ||
| * | ||
| */ | ||
| bool verify_ivc(ClientIVC::Proof& proof, ClientIVC& ivc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
| ivc->goblin.merge_verification_queue.emplace_back(acir_format::create_mock_merge_proof()); | ||
| // If the type is PG_FINAL, we also need to populate the ivc instance with a mock decider proof | ||
| if (type == ClientIVC::QUEUE_TYPE::PG_FINAL) { | ||
| // we have to create a mock honk vk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anymore we dont
|
|
||
| # Circuits matching these patterns we have client-ivc keys computed, rather than ultra-honk. | ||
| readarray -t ivc_patterns < <(jq -r '.[]' "../client_ivc_circuits.json") | ||
| readarray -t ivc_tail_patterns < <(jq -r '.[]' "../client_ivc_tail_circuits.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just correcting some naming here from "tail" to "hiding"
kashbrti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG! thanks for the clean up
| hide_op_queue_accumulation_result(circuit); | ||
|
|
||
| // Propagate the public inputs of the tail kernel by converting them to public inputs of the hiding circuit. | ||
| auto num_public_inputs = static_cast<size_t>(honk_vk->num_public_inputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah great! I think I removed it and had some tests failing in my previous clean up.
| } | ||
| auto verifier_instance = std::make_shared<DeciderVerificationKey_<Flavor>>(honk_vk); | ||
| FoldingProver folding_prover({ fold_output.accumulator, proving_key }, | ||
| FoldingProver folding_prover({ prover_accumulator, proving_key }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite funny. I think we were only using the accumulator of the fold_output but keeping the whole object. did we historically use another member of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other member was the proof. Can't remember if we ever used it but wouldnt make much sense since its stored in the queue anyway
|
|
||
| // Hiding circuit is proven by a MegaZKProver | ||
| MegaZKProver prover(hiding_decider_pk, hiding_circuit_vk, transcript); | ||
| MegaZKProver prover(hiding_decider_pk, verification_key, transcript); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make sure I understand correctly. We do not keep the honk_vk member anymore (that use to keep the vk of the last proof that needs to be recursively verified in the hiding circuit) and we're just passing it as an input here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ever since the updates from you/leila that final VK (hiding VK) is a precomputed input just like for all of the other circuits - until now we just weren't using it and instead recomputed it on the fly
| std::pair<ClientCircuit, std::shared_ptr<VerificationKey>> create_next_circuit_and_vk(ClientIVC& ivc, | ||
| TestSettings settings = {}) | ||
| { | ||
| // If this is a mock hiding kernel, remove the settings and use a default (non-structured) trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops seems like I forgot about this when refactoring the tests. thanks!
BEGIN_COMMIT_OVERRIDE fix: Origin Tags edgecase (#16921) chore: cycle group cleanup #2 (#16876) chore: civc tidy 3 (#16671) refactor(bb): optimize batch_mul_with_endomorphism (#16905) feat: check op queue wires are zero past minicircuit in Translator (#16858) feat: Add CPU scaling benchmark script for remote execution (#16918) fix: Add free witness tag to field constructor (#16827) fix(bb): darwin build (#16957) END_COMMIT_OVERRIDE
More ClientIvc cleanup: - use precomputed VK for hiding kernel rather than recomputing it at runtime - don't use structured trace for hiding kernel VK construction in mock circuit producer - remove use of non-static `verify()` and `prove_and_verify()` in favor of static `ClientIvc::verify()` - remove no longer needed propagation of tail public inputs in hiding kernel (it no longer has any!) - simplify `ClientIvc` member variables: remove `honk_vk` and replace `ProverFoldOutput` instance with a simple `prover_accumulator`
More ClientIvc cleanup: - use precomputed VK for hiding kernel rather than recomputing it at runtime - don't use structured trace for hiding kernel VK construction in mock circuit producer - remove use of non-static `verify()` and `prove_and_verify()` in favor of static `ClientIvc::verify()` - remove no longer needed propagation of tail public inputs in hiding kernel (it no longer has any!) - simplify `ClientIvc` member variables: remove `honk_vk` and replace `ProverFoldOutput` instance with a simple `prover_accumulator`
More ClientIvc cleanup: - use precomputed VK for hiding kernel rather than recomputing it at runtime - don't use structured trace for hiding kernel VK construction in mock circuit producer - remove use of non-static `verify()` and `prove_and_verify()` in favor of static `ClientIvc::verify()` - remove no longer needed propagation of tail public inputs in hiding kernel (it no longer has any!) - simplify `ClientIvc` member variables: remove `honk_vk` and replace `ProverFoldOutput` instance with a simple `prover_accumulator`
More ClientIvc cleanup:
verify()andprove_and_verify()in favor of staticClientIvc::verify()ClientIvcmember variables: removehonk_vkand replaceProverFoldOutputinstance with a simpleprover_accumulator