Skip to content
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: add goblin ops in add_gates_to_ensure_all_polys_are_non_zero #5468

Merged
merged 9 commits into from
Mar 27, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class ClientIVCTests : public ::testing::Test {
{
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments on this function may be a bit confusing/misleading - maybe we can change to say something like "default gates added to avoid nonzero polynomials will bump size to next power of 2"

Builder circuit{ ivc.goblin.op_queue };
MockCircuits::construct_arithmetic_circuit(circuit, log2_num_gates);
MockCircuits::construct_goblin_ecc_op_circuit(circuit);
return circuit;
}

Expand Down Expand Up @@ -114,11 +113,11 @@ TEST_F(ClientIVCTests, Full)
Builder function_circuit = create_mock_circuit(ivc);
ivc.initialize(function_circuit);

auto function_vk = std::make_shared<VerificationKey>(ivc.prover_fold_output.accumulator->proving_key);
auto foo_verifier_instance = std::make_shared<VerifierInstance>(function_vk);
// Accumulate kernel circuit (first kernel mocked as simple circuit since no folding proofs yet)
Builder kernel_circuit = create_mock_circuit(ivc);
FoldProof kernel_fold_proof = ivc.accumulate(kernel_circuit);
auto function_vk = std::make_shared<VerificationKey>(ivc.prover_fold_output.accumulator->proving_key);
auto foo_verifier_instance = std::make_shared<VerifierInstance>(function_vk);
// This will have a different verification key because we added the recursive merge verification to the circuit
auto function_vk_with_merge = std::make_shared<VerificationKey>(ivc.prover_instance->proving_key);
auto kernel_vk = function_vk_with_merge;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,6 @@ GoblinUltraCircuitBuilder create_circuit(const AcirFormat& constraint_system,
bool has_valid_witness_assignments = !witness.empty();
acir_format::build_constraints(builder, constraint_system, has_valid_witness_assignments);

// TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the
// associated polynomials are non-zero and to give ECCVM and Translator some ECC ops to process.
MockCircuits::construct_goblin_ecc_op_circuit(builder);

return builder;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ void GoblinAcirComposer::create_circuit(acir_format::AcirFormat& constraint_syst

// Populate constraints in the builder via the data in constraint_system
acir_format::build_constraints(builder_, constraint_system, true);

Copy link
Contributor

Choose a reason for hiding this comment

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

Its debatable whether we should remove the ops here. In the case of UGH, removing them is fine since eventually we'd like a world where we dont NEED goblin ops to make a valid proof. This one, however, is a Goblin specific test so it would defeat the purpose of the test to not have any ops. So when/if we get rid of the add_gates_to_ensure.., this test would be quietly made useless. I think we should continue to explicitly add ops here in one way or another, with a comment explaining why, so someone who thinks they're clever doesn't go and remove them later. What do you think?

// TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the
// associated polynomials are non-zero and to give ECCVM and Translator some ECC ops to process.
MockCircuits::construct_goblin_ecc_op_circuit(builder_);
}

std::vector<bb::fr> GoblinAcirComposer::accumulate_and_prove()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ TEST_F(GoblinRecursionTests, Vanilla)
// Construct and accumulate a mock function circuit
GoblinUltraCircuitBuilder function_circuit{ goblin.op_queue };
MockCircuits::construct_arithmetic_circuit(function_circuit, /*target_log2_dyadic_size=*/8);
MockCircuits::construct_goblin_ecc_op_circuit(function_circuit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

goblin.merge(function_circuit);
auto function_accum = construct_accumulator(function_circuit);

Expand Down
12 changes: 5 additions & 7 deletions barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ class GoblinMockCircuits {
stdlib::generate_sha256_test_circuit(builder, NUM_ITERATIONS); // min gates: ~39k
stdlib::generate_ecdsa_verification_test_circuit(builder, NUM_ITERATIONS); // min gates: ~41k
stdlib::generate_merkle_membership_test_circuit(builder, NUM_ITERATIONS); // min gates: ~29k

// Note: its not clear whether goblin ops will be supported for function circuits initially but currently
// UGH can only be used if some op gates are included so for now we'll assume each function circuit has
// some.
MockCircuits::construct_goblin_ecc_op_circuit(builder);
}

/**
Expand All @@ -92,8 +87,11 @@ class GoblinMockCircuits {
{
bb::GoblinUltraCircuitBuilder builder{ op_queue };

// Add some goblinized ecc ops
MockCircuits::construct_goblin_ecc_op_circuit(builder);
// Add some goblinized ecc ops: a mul accum op and an equality op
auto point = Point::one() * FF::random_element();
auto scalar = FF::random_element();
builder.queue_ecc_mul_accum(point, scalar);
builder.queue_ecc_eq();

op_queue->set_size_data();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_gates_to_ensure_
// dummy gate to be read into by previous poseidon internal gate via shifts
this->create_dummy_gate(
this->blocks.poseidon_internal, this->zero_idx, this->zero_idx, this->zero_idx, this->zero_idx);

// add dummy mul accum op and an equality op
this->queue_ecc_mul_accum(bb::g1::affine_element::one() * FF::random_element(), FF::random_element());
maramihali marked this conversation as resolved.
Show resolved Hide resolved
this->queue_ecc_eq();
}

/**
Expand Down
Loading