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

refactor: Protogalaxy verifier matches prover 1 #8414

Merged
merged 12 commits into from
Sep 11, 2024
2 changes: 2 additions & 0 deletions barretenberg/cpp/.clangd
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ Diagnostics:
- modernize-use-nodiscard
# Misleading; linker error fixed by adding const in declaration
- readability-avoid-const-params-in-decls
# const size_t circuit_size = static_cast<size_t>(...) is not better as const auto; auto is for clarity or cheap local polymorphism
- modernize-use-auto

--- # this divider is necessary
# Disable some checks for Google Test/Bench
Expand Down
1 change: 0 additions & 1 deletion barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,6 @@ class ECCVMFlavor {
// IPA verification key requires one more point.
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1025): make it so that PCSs inform the crs of
// how many points they need
info("eccvmvk: ", proving_key->circuit_size + 1);
this->pcs_verification_key = std::make_shared<VerifierCommitmentKey>(proving_key->circuit_size + 1);
this->circuit_size = proving_key->circuit_size;
this->log_circuit_size = numeric::get_msb(this->circuit_size);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ template <class DeciderProvingKeys_> class ProtogalaxyProver_ {
// TODO(https://github.com/AztecProtocol/barretenberg/issues/878)
, commitment_key(keys_to_fold[1]->proving_key.commitment_key){};

// Returns the accumulator, which is the first element in DeciderProvingKeys. The accumulator is assumed to have the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function is no longer in use.

// FoldingParameters set and be the result of a previous round of folding.
std::shared_ptr<DeciderPK> get_accumulator() { return keys_to_fold[0]; }

/**
* @brief For each key produced by a circuit, prior to folding, we need to complete the computation of its
* prover polynomials; commit to witnesses and generate the relation parameters; and send the public data ϕ of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,23 @@
namespace bb {

template <class DeciderVerificationKeys>
void ProtogalaxyVerifier_<DeciderVerificationKeys>::receive_and_finalise_key(const std::shared_ptr<DeciderVK>& keys,
const std::string& domain_separator)
void ProtogalaxyVerifier_<DeciderVerificationKeys>::run_oink_verifier_on_one_incomplete_key(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming function to match Prover.

const std::shared_ptr<DeciderVK>& keys, const std::string& domain_separator)
{
OinkVerifier<Flavor> oink_verifier{ keys, transcript, domain_separator + '_' };
oink_verifier.verify();
}

template <class DeciderVerificationKeys>
void ProtogalaxyVerifier_<DeciderVerificationKeys>::prepare_for_folding(const std::vector<FF>& fold_data)
void ProtogalaxyVerifier_<DeciderVerificationKeys>::run_oink_verifier_on_each_incomplete_key(
const std::vector<FF>& proof)
{
transcript = std::make_shared<Transcript>(fold_data);
transcript = std::make_shared<Transcript>(proof);
size_t index = 0;
auto key = keys_to_fold[0];
auto domain_separator = std::to_string(index);
if (!key->is_accumulator) {
receive_and_finalise_key(key, domain_separator);
run_oink_verifier_on_one_incomplete_key(key, domain_separator);
key->target_sum = 0;
key->gate_challenges = std::vector<FF>(static_cast<size_t>(key->verification_key->log_circuit_size), 0);
}
Expand All @@ -30,33 +31,34 @@ void ProtogalaxyVerifier_<DeciderVerificationKeys>::prepare_for_folding(const st
for (auto it = keys_to_fold.begin() + 1; it != keys_to_fold.end(); it++, index++) {
auto key = *it;
auto domain_separator = std::to_string(index);
receive_and_finalise_key(key, domain_separator);
run_oink_verifier_on_one_incomplete_key(key, domain_separator);
}
}

template <class DeciderVerificationKeys>
std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier_<
DeciderVerificationKeys>::verify_folding_proof(const std::vector<FF>& fold_data)
DeciderVerificationKeys>::verify_folding_proof(const std::vector<FF>& proof)
{
prepare_for_folding(fold_data);
run_oink_verifier_on_each_incomplete_key(proof);

auto delta = transcript->template get_challenge<FF>("delta");
auto accumulator = get_accumulator();
auto deltas =
compute_round_challenge_pows(static_cast<size_t>(accumulator->verification_key->log_circuit_size), delta);
const FF delta = transcript->template get_challenge<FF>("delta");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used explicit types and made things const.

const std::shared_ptr<const DeciderVK>& accumulator = keys_to_fold[0]; // WORKTODO: move
Copy link
Contributor

Choose a reason for hiding this comment

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

WORKTODO?


std::vector<FF> perturbator_coeffs(static_cast<size_t>(accumulator->verification_key->log_circuit_size) + 1, 0);
const size_t log_circuit_size = static_cast<size_t>(accumulator->verification_key->log_circuit_size);
const std::vector<FF> deltas = compute_round_challenge_pows(log_circuit_size, delta);

std::vector<FF> perturbator_coeffs(log_circuit_size + 1, 0);
if (accumulator->is_accumulator) {
for (size_t idx = 1; idx <= static_cast<size_t>(accumulator->verification_key->log_circuit_size); idx++) {
for (size_t idx = 1; idx <= log_circuit_size; idx++) {
perturbator_coeffs[idx] =
transcript->template receive_from_prover<FF>("perturbator_" + std::to_string(idx));
}
}

perturbator_coeffs[0] = accumulator->target_sum;
Polynomial<FF> perturbator(perturbator_coeffs);
FF perturbator_challenge = transcript->template get_challenge<FF>("perturbator_challenge");
auto perturbator_at_challenge = perturbator.evaluate(perturbator_challenge);
const Polynomial<FF> perturbator(perturbator_coeffs);
const FF perturbator_challenge = transcript->template get_challenge<FF>("perturbator_challenge");
const FF perturbator_evaluation = perturbator.evaluate(perturbator_challenge);

// The degree of K(X) is dk - k - 1 = k(d - 1) - 1. Hence we need k(d - 1) evaluations to represent it.
std::array<FF, DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH - DeciderVerificationKeys::NUM>
Expand All @@ -65,10 +67,10 @@ std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier
combiner_quotient_evals[idx] = transcript->template receive_from_prover<FF>(
"combiner_quotient_" + std::to_string(idx + DeciderVerificationKeys::NUM));
}
Univariate<FF, DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH, DeciderVerificationKeys::NUM> combiner_quotient(
combiner_quotient_evals);
FF combiner_challenge = transcript->template get_challenge<FF>("combiner_quotient_challenge");
auto combiner_quotient_at_challenge = combiner_quotient.evaluate(combiner_challenge);
const Univariate<FF, DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH, DeciderVerificationKeys::NUM>
combiner_quotient(combiner_quotient_evals);
const FF combiner_challenge = transcript->template get_challenge<FF>("combiner_quotient_challenge");
const FF combiner_quotient_evaluation = combiner_quotient.evaluate(combiner_challenge);

constexpr FF inverse_two = FF(2).invert();
FF vanishing_polynomial_at_challenge;
Expand All @@ -95,20 +97,9 @@ std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier
static_assert(DeciderVerificationKeys::NUM < 5);

// TODO(https://github.com/AztecProtocol/barretenberg/issues/881): bad pattern
auto next_accumulator = std::make_shared<DeciderVK>(accumulator->verification_key);
next_accumulator->verification_key = std::make_shared<VerificationKey>(
accumulator->verification_key->circuit_size, accumulator->verification_key->num_public_inputs);
next_accumulator->verification_key->pcs_verification_key = accumulator->verification_key->pcs_verification_key;
next_accumulator->verification_key->pub_inputs_offset = accumulator->verification_key->pub_inputs_offset;
next_accumulator->verification_key->contains_recursive_proof =
accumulator->verification_key->contains_recursive_proof;
next_accumulator->verification_key->recursive_proof_public_input_indices =
accumulator->verification_key->recursive_proof_public_input_indices;

if constexpr (IsGoblinFlavor<Flavor>) { // Databus commitment propagation data
next_accumulator->verification_key->databus_propagation_data =
accumulator->verification_key->databus_propagation_data;
}
auto next_accumulator = std::make_shared<DeciderVK>();
Copy link
Contributor Author

@codygunton codygunton Sep 11, 2024

Choose a reason for hiding this comment

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

Before: construct decider with a honk vk, which just default initializes and and std::moves the honk vk to be owned by the decider vk, then immediately overwrite that vk with something that looks like it should just be a copy constructor minus the honk vk being set.

After: just default initialize and set the pointer to a honk vk.

Note two things that do not work (basic Pg test fails; unsure why):

  1. Just doing auto next_accumulator = std::make_shared<DeciderVK>(accumulator->verification_key); and no additional vk overwriting.
  2. Just doing auto next_accumulator = std::make_shared<DeciderVK>(); then next_accumulator->verification_key = accumulator->verification_key;

WIll work to clarify in follow-on but this is an improvement as it is.

next_accumulator->verification_key = std::make_shared<VerificationKey>(*accumulator->verification_key);
Copy link
Contributor

@ledwards2225 ledwards2225 Sep 11, 2024

Choose a reason for hiding this comment

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

Why not just next_accumulator->verification_key = accumulator->verification_key;? Oh I guess this exactly what you're saying doesnt work

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the version you have is making a copy of VerificationKey, so the two things are definitely different. not totally clean why one would work and one wouldn't tho

next_accumulator->is_accumulator = true;

size_t commitment_idx = 0;
for (auto& expected_vk : next_accumulator->verification_key->get_all()) {
Expand All @@ -120,15 +111,10 @@ std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier
}
commitment_idx++;
}
next_accumulator->verification_key->num_public_inputs = accumulator->verification_key->num_public_inputs;
next_accumulator->public_inputs =
std::vector<FF>(static_cast<size_t>(next_accumulator->verification_key->num_public_inputs), 0);

next_accumulator->is_accumulator = true;

// Compute next folding parameters
next_accumulator->target_sum =
perturbator_at_challenge * lagranges[0] + vanishing_polynomial_at_challenge * combiner_quotient_at_challenge;
perturbator_evaluation * lagranges[0] + vanishing_polynomial_at_challenge * combiner_quotient_evaluation;
next_accumulator->gate_challenges =
update_gate_challenges(perturbator_challenge, accumulator->gate_challenges, deltas);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,21 @@ template <class DeciderVerificationKeys> class ProtogalaxyVerifier_ {

std::shared_ptr<Transcript> transcript = std::make_shared<Transcript>();

CommitmentLabels commitment_labels;

ProtogalaxyVerifier_(const std::vector<std::shared_ptr<DeciderVK>>& keys)
: keys_to_fold(DeciderVerificationKeys(keys)){};
~ProtogalaxyVerifier_() = default;

std::shared_ptr<DeciderVK> get_accumulator() { return keys_to_fold[0]; }
/**
* @brief Process the public data ϕ for the decider verification keys to be folded.
*/
void run_oink_verifier_on_one_incomplete_key(const std::shared_ptr<DeciderVK>&, const std::string&);

/**
* @brief Instatiate the vks and the transcript.
*
* @param fold_data The data transmitted via the transcript by the prover.
*/
void prepare_for_folding(const std::vector<FF>&);

/**
* @brief Process the public data ϕ for the decider verification keys to be folded.
*/
void receive_and_finalise_key(const std::shared_ptr<DeciderVK>&, const std::string&);
void run_oink_verifier_on_each_incomplete_key(const std::vector<FF>&);
Copy link
Contributor

Choose a reason for hiding this comment

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

thank god. every time I stepped through these two methods I felt like I was going insane


/**
* @brief Run the folding protocol on the verifier side to establish whether the public data ϕ of the new
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ struct DatabusPropagationData {

// Is this a kernel circuit (used to determine when databus consistency checks can be appended to a circuit in IVC)
bool is_kernel = false;

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 wanted to print these out, and I thought why not leave this function in place (it lets you write info on a DatabusPropagationData).

friend std::ostream& operator<<(std::ostream& os, DatabusPropagationData const& data)
{
os << data.contains_app_return_data_commitment << ",\n"
<< data.contains_kernel_return_data_commitment << ",\n"
<< data.app_return_data_public_input_idx << ",\n"
<< data.kernel_return_data_public_input_idx << ",\n"
<< data.is_kernel << "\n";
return os;
};
};

} // namespace bb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ template <class Flavor, size_t NUM_ = 2> class DeciderVerificationKey_ {

DeciderVerificationKey_() = default;
DeciderVerificationKey_(std::shared_ptr<VerificationKey> vk)
: verification_key(std::move(vk))
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 just is sub-microsecond optimization that we don't need and which hurts clarity here IMO.

: verification_key(vk)
{}

MSGPACK_FIELDS(verification_key,
Expand Down
Loading