Skip to content

fix: CIVC components share a transcript #14144

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

Merged
merged 19 commits into from
May 20, 2025

Conversation

iakovenkos
Copy link
Contributor

@iakovenkos iakovenkos commented May 7, 2025

@iakovenkos iakovenkos self-assigned this May 7, 2025
* circuits
*
*/
TEST_F(GoblinTests, MultipleCircuits)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a duplicate of GoblinRecursiveVerifierTests::NativeVerification

@@ -14,24 +14,24 @@ namespace bb::stdlib::recursion::honk {
*/
GoblinRecursiveVerifierOutput GoblinRecursiveVerifier::verify(const GoblinProof& proof)
{
// Verify the final merge step
MergeVerifier merge_verifier{ builder, transcript };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ECCVM challenges used in compute_translation_opening_claims() sub-protocol must depend on the commitments to merge wires

@@ -227,7 +236,24 @@ TEST_F(GoblinRecursiveVerifierTests, TranslatorMergeConsistencyFailure)
auto [proof, verifier_input] = create_goblin_prover_output();
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1298):
// Better recursion testing - create more flexible proof tampering tests.
proof.translator_proof[0] += 1; // tamper with part of the limb of op commitment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually modifying accumulated_result, so the CircuitChecker would output false, but for a wrong reason

}

/**
* @brief Construct the hiding circuit then produce a proof of the circuit's correctness with MegaHonk.
*
* @return HonkProof - a Mega proof
*/
std::pair<HonkProof, ClientIVC::MergeProof> ClientIVC::construct_and_prove_hiding_circuit()
HonkProof ClientIVC::construct_and_prove_hiding_circuit()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. cleaner functionality
  2. merge proof has to happen after proving the hiding circuit, as the latter contains the last subtable

@@ -21,11 +21,6 @@ DeciderVerifier_<Flavor>::DeciderVerifier_(const std::shared_ptr<DeciderVerifica
, transcript(transcript)
{}

template <typename Flavor>
DeciderVerifier_<Flavor>::DeciderVerifier_(const std::shared_ptr<DeciderVerificationKey>& accumulator)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant constructor


public:
using Proof = ClientIVC::Proof;
using FoldVerifierInput = FoldingVerifier::VerifierInput;
using GoblinVerificationKey = Goblin::VerificationKey;
using Output = GoblinRecursiveVerifierOutput;

struct VerifierInput {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused struct

} else {
inner_prover.transcript->deserialize_full_transcript();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this tool can't be used with ECCVM, Mega, and MegaZK anymore (cause they don't have serde methods in Flavor::Transcript), so I simplified it a little and wrote a tool below that works for those Flavors

if constexpr (!std::is_same_v<InnerFlavor, ECCVMFlavor>) {
inner_proof = inner_prover.export_proof();

inner_proof = inner_prover.export_proof();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhat ugly because IPA transcript in the flavors with IPAAcc is not a part of the main transcript. decided to keep it less invasive in UltraProver class and introduce such assignments in tests instead

@@ -185,6 +185,15 @@ void TranslatorRecursiveVerifier_<Flavor>::verify_consistency_with_final_merge(
// These are witness commitments sent as part of the proof, so their coordinates are already in reduced form.
// This approach is preferred over implementing assert_equal for biggroup, as it avoids the need to handle
// constants within biggroup logic.
bool consistency_check_failed = (merge_commitment.y.get_value() != translator_commitment.y.get_value()) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we keep modifying merge, it's probably useful to have this print statement here

*
* @return Goblin::MergeProof
*/
Goblin::MergeProof Goblin::prove_final_merge()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may try to keep a single prove_merge() method if this seems unreasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont love this but I also dont think it would be any better to have a single method with a default argument or something. It might eventually be the case that the\ merge always needs to share its transcript with something else (e.g. the PG prover in the main protocol) so maybe that will take care of this. I think its ok for now.

@iakovenkos iakovenkos marked this pull request as ready for review May 16, 2025 16:20
@iakovenkos iakovenkos requested a review from ledwards2225 May 16, 2025 16:59
@iakovenkos iakovenkos changed the title fix: CIVC rec verifier components share a transcript fix: CIVC components share a transcript May 16, 2025
@fcarreiro fcarreiro removed their request for review May 16, 2025 17:52
Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

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

Overall looks great. Left a few comments and minor suggestions. If you can get rid of the defaulted transcript input in any of the constructors I think that would be valuable. Its not ideal to have test-only functionality leak into the production constructors

@@ -920,538 +920,6 @@ class ECCVMFlavor {

using VerifierCommitments = VerifierCommitments_<Commitment, VerificationKey>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully we're not giving up to much by getting rid of this. There's a world where we could write failure tests that check every single one of these individual components but its certainly a bit brittle as it is now.

// Constructor accepting existing transcript
explicit ECCVMVerifier(const std::shared_ptr<Transcript>& transcript)
: transcript(transcript){};

explicit ECCVMVerifier(const std::shared_ptr<VerificationKey>& verifier_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these two constructors just go away now that the VK is fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! also removed the default constructor

*
* @return Goblin::MergeProof
*/
Goblin::MergeProof Goblin::prove_final_merge()
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont love this but I also dont think it would be any better to have a single method with a default argument or something. It might eventually be the case that the\ merge always needs to share its transcript with something else (e.g. the PG prover in the main protocol) so maybe that will take care of this. I think its ok for now.

@@ -35,7 +35,7 @@ template <typename Flavor> class OinkRecursiveVerifier_ {
*/
explicit OinkRecursiveVerifier_(Builder* builder,
const std::shared_ptr<RecursiveDeciderVK>& verification_key,
std::shared_ptr<Transcript> transcript,
const std::shared_ptr<Transcript>& transcript,
Copy link
Contributor

Choose a reason for hiding this comment

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

yay, no default!

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.

ClientIVC soundness: HidingCircuit, ECCVM, and Translator must share the transcript
2 participants