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.

Sign up for GitHub

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

Jump to bottom

fix: Simplify ECCVM prover constructor and add a TODO #5681

Merged
merged 4 commits into from
Apr 11, 2024
Merged

fix: Simplify ECCVM prover constructor and add a TODO #5681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e961af9
simplify prover constructor
ledwards2225 Apr 10, 2024
c41d898
Merge branch 'master' into lde/eccvm_cleanup
ledwards2225 Apr 10, 2024
6326320
woops, share all
ledwards2225 Apr 10, 2024
ce83d5e
comment
ledwards2225 Apr 10, 2024
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
47 changes: 14 additions & 33 deletions barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,44 +12,26 @@

namespace bb {

/**
* Create ECCVMProver from proving key, witness and manifest.
*
* @param input_key Proving key.
* @param input_manifest Input manifest
*
* @tparam settings Settings class.
* */
ECCVMProver::ECCVMProver(const std::shared_ptr<ProvingKey>& input_key,
const std::shared_ptr<CommitmentKey>& commitment_key,
const std::shared_ptr<Transcript>& transcript)
: transcript(transcript)
, key(input_key)
, commitment_key(commitment_key)
{ // this will be initialized properly later
key->z_perm = Polynomial(key->circuit_size);
for (auto [prover_poly, key_poly] : zip_view(prover_polynomials.get_unshifted(), key->get_all())) {
ASSERT(flavor_get_label(prover_polynomials, prover_poly) == flavor_get_label(*key, key_poly));
prover_poly = key_poly.share();
}
for (auto [prover_poly, key_poly] : zip_view(prover_polynomials.get_shifted(), key->get_to_be_shifted())) {
ASSERT(flavor_get_label(prover_polynomials, prover_poly) == (flavor_get_label(*key, key_poly) + "_shift"));
prover_poly = key_poly.shifted();
}
}

ECCVMProver::ECCVMProver(CircuitBuilder& builder, const std::shared_ptr<Transcript>& transcript)
: transcript(transcript)
, prover_polynomials(builder)
Copy link
Contributor

Choose a reason for hiding this comment

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

In ulltra/goblin_ultra, we go the other way: construct a proving_key and then construct a prover_polynomials from that. I supposed its not a big deal if we do it like this although things are inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I tried to capture this in the TODO. It used to be the case that the pkey "owned" the polys and prover_polys was just an immutable view into them. That's changed so now one is really just a copy of the other (without the expense due to the shared memory), plus the shifted polys but those don't use additional memory. I think its possible we should do away with one or the other. Maybe pkey just has a prover_polys

{
BB_OP_COUNT_TIME_NAME("ECCVMProver(CircuitBuilder&)");
// compute wire polynomials and copy into proving key
auto local_key = std::make_shared<ProvingKey>(builder);
ProverPolynomials polynomials(builder); // WORKTODO: inefficient
for (auto [prover_poly, key_poly] : zip_view(polynomials.get_wires(), local_key->get_wires())) {

// TODO(https://github.com/AztecProtocol/barretenberg/issues/939): Remove redundancy between
// ProvingKey/ProverPolynomials and update the model to reflect what's done in all other proving systems.

// Construct the proving key; populates all polynomials except for witness polys
key = std::make_shared<ProvingKey>(builder);

// Share all unshifted polys from the prover polynomials to the proving key. Note: this means that updating a
// polynomial in one container automatically updates it in the other via the shared memory.
for (auto [prover_poly, key_poly] : zip_view(prover_polynomials.get_unshifted(), key->get_all())) {
ASSERT(flavor_get_label(prover_polynomials, prover_poly) == flavor_get_label(*key, key_poly));
std::copy(prover_poly.begin(), prover_poly.end(), key_poly.begin());
key_poly = prover_poly.share();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the impact on client ivc bench for removing these copys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to the description. 22.8

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like none or very little impact?

}

*this = ECCVMProver(std::move(local_key), std::make_shared<CommitmentKey>(local_key->circuit_size), transcript);
commitment_key = std::make_shared<CommitmentKey>(key->circuit_size);
}

/**
Expand Down Expand Up @@ -98,7 +80,6 @@ void ECCVMProver::execute_log_derivative_commitments_round()
compute_logderivative_inverse<Flavor, typename Flavor::LookupRelation>(
prover_polynomials, relation_parameters, key->circuit_size);
transcript->send_to_verifier(commitment_labels.lookup_inverses, commitment_key->commit(key->lookup_inverses));
prover_polynomials.lookup_inverses = key->lookup_inverses.share();
Copy link
Contributor

Choose a reason for hiding this comment

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

So since these have already been shared before, this line doesn't do anything correct, as the backing_memory, size, and coeff ptr are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

}

/**
Expand Down
4 changes: 0 additions & 4 deletions barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ class ECCVMProver {
using CircuitBuilder = typename Flavor::CircuitBuilder;

public:
explicit ECCVMProver(const std::shared_ptr<ProvingKey>& input_key,
const std::shared_ptr<CommitmentKey>& commitment_key,
const std::shared_ptr<Transcript>& transcript = std::make_shared<Transcript>());

explicit ECCVMProver(CircuitBuilder& builder,
const std::shared_ptr<Transcript>& transcript = std::make_shared<Transcript>());

Expand Down
18 changes: 18 additions & 0 deletions barretenberg/cpp/src/barretenberg/polynomials/polynomial.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,24 @@ template <typename Fr> class Polynomial {
size_ = 0;
}

/**
* @brief Check whether or not a polynomial is identically zero
*
*/
bool is_zero()
{
if (is_empty()) {
ASSERT(false);
info("Checking is_zero on an empty Polynomial!");
}
for (size_t i = 0; i < size(); i++) {
if (coefficients_[i] != 0) {
return false;
}
}
return true;
}

bool operator==(Polynomial const& rhs) const;

// Const and non const versions of coefficient accessors
Expand Down
Loading