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

fix: remove finalize from acir create circuit #6585

Merged
merged 9 commits into from
May 22, 2024
Merged
10 changes: 6 additions & 4 deletions barretenberg/cpp/src/barretenberg/bb/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ bool proveAndVerifyHonkAcirFormat(acir_format::AcirFormat constraint_system, aci
// Construct a bberg circuit from the acir representation
auto builder = acir_format::create_circuit<Builder>(constraint_system, 0, witness);

size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size());
auto num_extra_gates = builder.get_num_gates_added_to_ensure_nonzero_polynomials();
size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size() + num_extra_gates);
Copy link
Contributor

Choose a reason for hiding this comment

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

does this not double count the ensure_nonzero gates? Since get_num_gates_added_to_ensure_nonzero_polynomials will add these gates in, and then get_total_circuit_size would already account for these extra gates.

Copy link
Contributor Author

@ledwards2225 ledwards2225 May 22, 2024

Choose a reason for hiding this comment

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

get_total_circuit_size doesn't account for the added gates for non-zero polys, only the gates that have been added explicitly plus those that will be added via the finalize method.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is that calling get_num_gates_added_to_ensure_nonzero_polynomials will add the non-zero gates and then get_total_circuit_size calls get_num_gates which calls get_num_gates_split_into_components which sets the count to be this->num_gates which should include the added non-zero gates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the added non-zero gates aren't added until we construct an instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, maybe the confusion is this: get_num_gates_added_to_ensure_nonzero_polynomials doesn't actually add the gates. It creates a new builder, adds them to that, then counts them. The original builder is unaffected

Copy link
Contributor

Choose a reason for hiding this comment

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

ah got it. I didn't look hard enough at that function oops

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 added some comments to make that a bit more explicit

init_bn254_crs(srs_size);

// Construct Honk proof
Expand Down Expand Up @@ -591,8 +592,8 @@ void prove_honk(const std::string& bytecodePath, const std::string& witnessPath,

auto builder = acir_format::create_circuit<Builder>(constraint_system, 0, witness);

size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size());

auto num_extra_gates = builder.get_num_gates_added_to_ensure_nonzero_polynomials();
size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size() + num_extra_gates);
init_bn254_crs(srs_size);

// Construct Honk proof
Expand Down Expand Up @@ -662,7 +663,8 @@ template <IsUltraFlavor Flavor> void write_vk_honk(const std::string& bytecodePa
auto constraint_system = get_constraint_system(bytecodePath);
auto builder = acir_format::create_circuit<Builder>(constraint_system, 0, {});

size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size());
auto num_extra_gates = builder.get_num_gates_added_to_ensure_nonzero_polynomials();
size_t srs_size = builder.get_circuit_subgroup_size(builder.get_total_circuit_size() + num_extra_gates);
init_bn254_crs(srs_size);

ProverInstance prover_inst(builder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,6 @@ UltraCircuitBuilder create_circuit(const AcirFormat& constraint_system,
bool has_valid_witness_assignments = !witness.empty();
build_constraints(builder, constraint_system, has_valid_witness_assignments, honk_recursion);

builder.finalize_circuit();

return builder;
};

Expand Down Expand Up @@ -378,8 +376,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, honk_recursion);

builder.finalize_circuit();

return builder;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ TEST_F(join_split_tests, test_0_input_notes_and_detect_circuit_change)
// The below part detects any changes in the join-split circuit
constexpr size_t DYADIC_CIRCUIT_SIZE = 1 << 16;

constexpr uint256_t CIRCUIT_HASH("0xc9d7e9c29d8555514b4ee1ed5cccc6da7e5a81585c6404f7ce379404f07414ec");
constexpr uint256_t CIRCUIT_HASH("0x470358e4d91c4c5296ef788b1165b2c439cd498f49c3f99386b002753ca3d0ee");

const uint256_t circuit_hash = circuit.hash_circuit();
// circuit is finalized now
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ namespace bb {

template <typename FF> void GoblinUltraCircuitBuilder_<FF>::finalize_circuit()
{
if (!this->circuit_finalized) {
add_goblin_gates_to_ensure_all_polys_are_non_zero();
UltraCircuitBuilder_<UltraHonkArith<FF>>::finalize_circuit();
}
// All of the gates involved in finalization are part of the Ultra arithmetization
UltraCircuitBuilder_<UltraHonkArith<FF>>::finalize_circuit();
}

/**
Expand All @@ -28,8 +26,11 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::finalize_circuit()
// TODO(#423): This function adds valid (but arbitrary) gates to ensure that the circuit which includes
// them will not result in any zero-polynomials. It also ensures that the first coefficient of the wire
// polynomials is zero, which is required for them to be shiftable.
template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_goblin_gates_to_ensure_all_polys_are_non_zero()
template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_gates_to_ensure_all_polys_are_non_zero()
{
// Most polynomials are handled via the conventional Ultra method
UltraCircuitBuilder_<UltraHonkArith<FF>>::add_gates_to_ensure_all_polys_are_non_zero();

// All that remains is to handle databus related and poseidon2 related polynomials. In what follows we populate the
// calldata with some mock data then constuct a single calldata read gate

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
}

void finalize_circuit();
void add_goblin_gates_to_ensure_all_polys_are_non_zero();
void add_gates_to_ensure_all_polys_are_non_zero();

size_t get_num_constant_gates() const override { return 0; }

Expand All @@ -130,6 +130,22 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
return num_ultra_gates + num_goblin_ecc_op_gates;
}

/**
* @brief Dynamically compute the number of gates added by the "add_gates_to_ensure_all_polys_are_non_zero" method
* @note This does NOT add the gates to the present builder
*
*/
size_t get_num_gates_added_to_ensure_nonzero_polynomials()
{
GoblinUltraCircuitBuilder_<FF> builder; // instantiate new builder

size_t num_gates_prior = builder.get_num_gates();
builder.add_gates_to_ensure_all_polys_are_non_zero();
size_t num_gates_post = builder.get_num_gates(); // accounts for finalization gates

return num_gates_post - num_gates_prior;
}

/**x
* @brief Print the number and composition of gates in the circuit
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,14 @@ template <typename Arithmetization> void UltraCircuitBuilder_<Arithmetization>::
* our circuit is finalized, and we must not to execute these functions again.
*/
if (!circuit_finalized) {
add_gates_to_ensure_all_polys_are_non_zero();
process_non_native_field_multiplications();
process_ROM_arrays();
process_RAM_arrays();
process_range_lists();
circuit_finalized = true;
} else {
// Gates added after first call to finalize will not be processed since finalization is only performed once
info("WARNING: Redudant call to finalize_circuit(). Is this intentional?");
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,22 @@ class UltraCircuitBuilder_ : public CircuitBuilderBase<typename Arithmetization_
return count + romcount + ramcount + rangecount + nnfcount;
}

/**
* @brief Dynamically compute the number of gates added by the "add_gates_to_ensure_all_polys_are_non_zero" method
* @note This does NOT add the gates to the present builder
*
*/
size_t get_num_gates_added_to_ensure_nonzero_polynomials()
{
UltraCircuitBuilder_<Arithmetization> builder; // instantiate new builder

size_t num_gates_prior = builder.get_num_gates();
builder.add_gates_to_ensure_all_polys_are_non_zero();
size_t num_gates_post = builder.get_num_gates(); // accounts for finalization gates

return num_gates_post - num_gates_prior;
}

/**
* @brief Get combined size of all tables used in circuit
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ template <class Flavor> class ProverInstance_ {
ProverInstance_(Circuit& circuit, bool is_structured = false)
{
BB_OP_COUNT_TIME_NAME("ProverInstance(Circuit&)");
circuit.add_gates_to_ensure_all_polys_are_non_zero();
circuit.finalize_circuit();

// If using a structured trace, ensure that no block exceeds the fixed size
Expand Down
Loading