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(bb): remove poly downsizing, other fast-follow from structured polys #8475

Merged
merged 1 commit into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/bb/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ void prove(const std::string& bytecodePath, const std::string& witnessPath, cons

acir_proofs::AcirComposer acir_composer{ 0, verbose_logging };
acir_composer.create_circuit(constraint_system, witness);
init_bn254_crs(acir_composer.get_dyadic_circuit_size() * 2);
init_bn254_crs(acir_composer.get_dyadic_circuit_size());
acir_composer.init_proving_key();
auto proof = acir_composer.create_proof();

Expand Down
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ class ECCVMFlavor {
{
// Storage is only needed after the first partial evaluation, hence polynomials of size (n / 2)
for (auto& poly : this->get_all()) {
poly = Polynomial(circuit_size / 2, circuit_size / 2);
poly = Polynomial(circuit_size / 2);
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ void construct_lookup_read_counts(typename Flavor::Polynomial& read_counts,
{
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1033): construct tables and counts at top of trace
size_t offset = dyadic_circuit_size - circuit.get_tables_size();
read_counts = typename Flavor::Polynomial(circuit.get_tables_size(), dyadic_circuit_size, /*start index*/ offset);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is NOT safe to do - Its not guaranteed that all values of read_counts/tags are set below. My suggestion was to remove the TODO, not the initialization

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe these polys are being initialized elsewhere? in that case that would be fine. In general the code below will only set a small subset of the poly coeffs so it would not be safe to not init the entire block to zero

Copy link
Contributor

Choose a reason for hiding this comment

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

ok yeah I misunderstood - looks like these are probably initialized with all the other polys in the constructor so these lines were always redundant, I guess that was your point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I wasn't really thinking redundant - but it's weird to be inconsistent about how we down-size these polys, so I was thinking it could all be done in one place perhaps

// TODO(AD) we dont need to initialize read_tags
read_tags = typename Flavor::Polynomial(circuit.get_tables_size(), dyadic_circuit_size, /*start index*/ offset);

size_t table_offset = offset; // offset of the present table in the table polynomials
// loop over all tables used in the circuit; each table contains data about the lookups made on it
Expand All @@ -71,7 +68,7 @@ void construct_lookup_read_counts(typename Flavor::Polynomial& read_counts,

// increment the read count at the corresponding index in the full polynomial
size_t index_in_poly = table_offset + index_in_table;
read_counts.at(index_in_poly) = read_counts[index_in_poly] + 1;
read_counts.at(index_in_poly)++;
read_tags.at(index_in_poly) = 1; // tag is 1 if entry has been read 1 or more times
}
table_offset += table.size(); // set the offset of the next table within the polynomials
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void compute_grand_product(typename Flavor::ProverPolynomials& full_polynomials,
// calling get_row which creates full copies. avoid?
for (size_t i = start; i < end; ++i) {
for (auto [eval, full_poly] : zip_view(evaluations.get_all(), full_polynomials.get_all())) {
eval = full_poly.size() > i ? full_poly.get(i) : 0;
eval = full_poly.size() > i ? full_poly[i] : 0;
}
numerator.at(i) = GrandProdRelation::template compute_grand_product_numerator<Accumulator>(
evaluations, relation_parameters);
Expand Down
21 changes: 10 additions & 11 deletions barretenberg/cpp/src/barretenberg/polynomials/polynomial.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,16 @@ template <typename Fr> class Polynomial {
Fr compute_kate_opening_coefficients(const Fr& z)
requires polynomial_arithmetic::SupportsFFT<Fr>;

// /**
// * @brief Divides p(X) by (X-r₁)⋯(X−rₘ) in-place.
// * Assumes that p(rⱼ)=0 for all j
// *
// * @details we specialize the method when only a single root is given.
// * if one of the roots is 0, then we first factor all other roots.
// * dividing by X requires only a left shift of all coefficient.
// *
// * @param roots list of roots (r₁,…,rₘ)
// */
// void factor_roots(std::span<const Fr> roots) { polynomial_arithmetic::factor_roots(std::span{ *this }, roots); };
/**
* @brief Divides p(X) by (X-r) in-place.
* Assumes that p(rⱼ)=0 for all j
*
* @details we specialize the method when only a single root is given.
* if one of the roots is 0, then we first factor all other roots.
* dividing by X requires only a left shift of all coefficient.
*
* @param root a single root r
*/
void factor_roots(const Fr& root) { polynomial_arithmetic::factor_roots(coeffs(), root); };

Fr evaluate(const Fr& z, size_t target_size) const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ class UltraFlavor {
// Storage is only needed after the first partial evaluation, hence polynomials of
// size (n / 2)
for (auto& poly : this->get_all()) {
poly = Polynomial(circuit_size / 2, circuit_size / 2);
poly = Polynomial(circuit_size / 2);
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ class UltraKeccakFlavor {
// Storage is only needed after the first partial evaluation, hence polynomials of
// size (n / 2)
for (auto& poly : this->get_all()) {
poly = Polynomial(circuit_size / 2, circuit_size / 2);
poly = Polynomial(circuit_size / 2);
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ class TranslatorFlavor {
{
// Storage is only needed after the first partial evaluation, hence polynomials of size (n / 2)
for (auto& poly : this->get_all()) {
poly = Polynomial(circuit_size / 2, circuit_size / 2);
poly = Polynomial(circuit_size / 2);
}
}
};
Expand Down
Loading