-
Notifications
You must be signed in to change notification settings - Fork 191
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 2 #8477
Changes from all commits
22734be
b02be34
49bc83c
44680f1
71d59b5
5bc0082
45e6797
d75f2be
c08828f
49271d3
94551f5
70d85b2
5dd7c41
4130cca
bf4d429
6c553f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,40 +4,48 @@ | |
#include <vector> | ||
|
||
namespace bb { | ||
template <typename Curve> class CommitmentSchemesUtils { | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably this should be relocated somewhere but I need to move on. |
||
* @brief Utility for native batch multiplication of group elements | ||
* @note This is used only for native verification and is not optimized for efficiency | ||
*/ | ||
template <typename Commitment, typename FF> | ||
static Commitment batch_mul_native(const std::vector<Commitment>& _points, const std::vector<FF>& _scalars) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make utility function in a class be simply a function template (static class members useful for reducing verbosity when we have complicated types, e.g. around flavors). |
||
{ | ||
std::vector<Commitment> points; | ||
std::vector<FF> scalars; | ||
for (size_t i = 0; i < _points.size(); ++i) { | ||
const auto& point = _points[i]; | ||
const auto& scalar = _scalars[i]; | ||
|
||
public: | ||
using Commitment = typename Curve::AffineElement; | ||
using FF = typename Curve::ScalarField; | ||
|
||
/** | ||
* @brief Utility for native batch multiplication of group elements | ||
* @note This is used only for native verification and is not optimized for efficiency | ||
*/ | ||
static Commitment batch_mul_native(const std::vector<Commitment>& _points, const std::vector<FF>& _scalars) | ||
{ | ||
std::vector<Commitment> points; | ||
std::vector<FF> scalars; | ||
for (size_t i = 0; i < _points.size(); ++i) { | ||
const auto& point = _points[i]; | ||
const auto& scalar = _scalars[i]; | ||
|
||
// TODO: Special handling of point at infinity here due to incorrect serialization. | ||
if (!scalar.is_zero() && !point.is_point_at_infinity() && !point.y.is_zero()) { | ||
points.emplace_back(point); | ||
scalars.emplace_back(scalar); | ||
} | ||
// TODO: Special handling of point at infinity here due to incorrect serialization. | ||
if (!scalar.is_zero() && !point.is_point_at_infinity() && !point.y.is_zero()) { | ||
points.emplace_back(point); | ||
scalars.emplace_back(scalar); | ||
} | ||
} | ||
|
||
if (points.empty()) { | ||
return Commitment::infinity(); | ||
} | ||
if (points.empty()) { | ||
return Commitment::infinity(); | ||
} | ||
|
||
auto result = points[0] * scalars[0]; | ||
for (size_t idx = 1; idx < scalars.size(); ++idx) { | ||
result = result + points[idx] * scalars[idx]; | ||
} | ||
return result; | ||
auto result = points[0] * scalars[0]; | ||
for (size_t idx = 1; idx < scalars.size(); ++idx) { | ||
result = result + points[idx] * scalars[idx]; | ||
} | ||
return result; | ||
} | ||
|
||
/** | ||
* @brief Utility for native batch multiplication of group elements | ||
* @note This is used only for native verification and is not optimized for efficiency | ||
*/ | ||
template <typename FF> static FF linear_combination(const std::vector<FF>& as, const std::vector<FF>& bs) | ||
{ | ||
FF result = as[0] * bs[0]; | ||
for (size_t idx = 1; idx < as.size(); ++idx) { | ||
result += as[idx] * bs[idx]; | ||
} | ||
}; | ||
} // namespace bb | ||
return result; | ||
} | ||
|
||
} // namespace bb |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
#include "protogalaxy_verifier.hpp" | ||
#include "barretenberg/commitment_schemes/utils/batch_mul_native.hpp" | ||
#include "barretenberg/plonk_honk_shared/library/grand_product_delta.hpp" | ||
#include "barretenberg/protogalaxy/prover_verifier_shared.hpp" | ||
#include "barretenberg/ultra_honk/oink_verifier.hpp" | ||
|
@@ -35,123 +36,106 @@ void ProtogalaxyVerifier_<DeciderVerificationKeys>::run_oink_verifier_on_each_in | |
} | ||
} | ||
|
||
template <typename FF, size_t NUM> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just computed inline; I make it a function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, this computation could re-use some methods from polynomial_arithmetic/univariate/polynomial classes. Not necessary in terms of efficiency, but the code would look cleaner. Could do that in my refactoring work. |
||
std::tuple<FF, std::vector<FF>> compute_vanishing_polynomial_and_lagrange_evaluations(const FF& combiner_challenge) | ||
{ | ||
static_assert(NUM < 5); | ||
static constexpr FF inverse_two = FF(2).invert(); | ||
|
||
std::vector<FF> lagranges(NUM); | ||
FF vanishing_polynomial_at_challenge; | ||
if constexpr (NUM == 2) { | ||
vanishing_polynomial_at_challenge = combiner_challenge * (combiner_challenge - FF(1)); | ||
lagranges = { FF(1) - combiner_challenge, combiner_challenge }; | ||
} else if constexpr (NUM == 3) { | ||
vanishing_polynomial_at_challenge = | ||
combiner_challenge * (combiner_challenge - FF(1)) * (combiner_challenge - FF(2)); | ||
lagranges = { (FF(1) - combiner_challenge) * (FF(2) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (FF(2) - combiner_challenge), | ||
combiner_challenge * (combiner_challenge - FF(1)) * inverse_two }; | ||
} else if constexpr (NUM == 4) { | ||
static constexpr FF inverse_six = FF(6).invert(); | ||
vanishing_polynomial_at_challenge = combiner_challenge * (combiner_challenge - FF(1)) * | ||
(combiner_challenge - FF(2)) * (combiner_challenge - FF(3)); | ||
lagranges = { (FF(1) - combiner_challenge) * (FF(2) - combiner_challenge) * (FF(3) - combiner_challenge) * | ||
inverse_six, | ||
combiner_challenge * (FF(2) - combiner_challenge) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (combiner_challenge - FF(2)) * inverse_six }; | ||
} | ||
return std::make_tuple(vanishing_polynomial_at_challenge, lagranges); | ||
} | ||
|
||
template <class DeciderVerificationKeys> | ||
std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier_< | ||
DeciderVerificationKeys>::verify_folding_proof(const std::vector<FF>& proof) | ||
{ | ||
static constexpr size_t BATCHED_EXTENDED_LENGTH = DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH; | ||
static constexpr size_t NUM_KEYS = DeciderVerificationKeys::NUM; | ||
|
||
const std::shared_ptr<const DeciderVK>& accumulator = keys_to_fold[0]; | ||
const size_t log_circuit_size = static_cast<size_t>(accumulator->verification_key->log_circuit_size); | ||
|
||
run_oink_verifier_on_each_incomplete_key(proof); | ||
|
||
// Perturbator round | ||
const FF delta = transcript->template get_challenge<FF>("delta"); | ||
const std::shared_ptr<const DeciderVK>& accumulator = keys_to_fold[0]; // WORKTODO: move | ||
|
||
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 <= log_circuit_size; idx++) { | ||
perturbator_coeffs[idx] = | ||
transcript->template receive_from_prover<FF>("perturbator_" + std::to_string(idx)); | ||
} | ||
} | ||
const FF perturbator_challenge = transcript->template get_challenge<FF>("perturbator_challenge"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shuffled things a bit to match prover and paper order better. |
||
|
||
// Combiner quotient round | ||
perturbator_coeffs[0] = accumulator->target_sum; | ||
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> | ||
combiner_quotient_evals; | ||
for (size_t idx = 0; idx < DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH - DeciderVerificationKeys::NUM; idx++) { | ||
combiner_quotient_evals[idx] = transcript->template receive_from_prover<FF>( | ||
"combiner_quotient_" + std::to_string(idx + DeciderVerificationKeys::NUM)); | ||
std::array<FF, BATCHED_EXTENDED_LENGTH - NUM_KEYS> | ||
combiner_quotient_evals; // The degree of the combiner quotient (K in the paper) is dk - k - 1 = k(d - 1) - 1. | ||
// Hence we need k(d - 1) evaluations to represent it. | ||
for (size_t idx = DeciderVerificationKeys::NUM; auto& val : combiner_quotient_evals) { | ||
val = transcript->template receive_from_prover<FF>("combiner_quotient_" + std::to_string(idx++)); | ||
} | ||
const Univariate<FF, DeciderVerificationKeys::BATCHED_EXTENDED_LENGTH, DeciderVerificationKeys::NUM> | ||
combiner_quotient(combiner_quotient_evals); | ||
|
||
// Folding | ||
const FF combiner_challenge = transcript->template get_challenge<FF>("combiner_quotient_challenge"); | ||
const Univariate<FF, BATCHED_EXTENDED_LENGTH, NUM_KEYS> combiner_quotient(combiner_quotient_evals); | ||
const FF combiner_quotient_evaluation = combiner_quotient.evaluate(combiner_challenge); | ||
|
||
constexpr FF inverse_two = FF(2).invert(); | ||
FF vanishing_polynomial_at_challenge; | ||
std::array<FF, DeciderVerificationKeys::NUM> lagranges; | ||
if constexpr (DeciderVerificationKeys::NUM == 2) { | ||
vanishing_polynomial_at_challenge = combiner_challenge * (combiner_challenge - FF(1)); | ||
lagranges = { FF(1) - combiner_challenge, combiner_challenge }; | ||
} else if constexpr (DeciderVerificationKeys::NUM == 3) { | ||
vanishing_polynomial_at_challenge = | ||
combiner_challenge * (combiner_challenge - FF(1)) * (combiner_challenge - FF(2)); | ||
lagranges = { (FF(1) - combiner_challenge) * (FF(2) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (FF(2) - combiner_challenge), | ||
combiner_challenge * (combiner_challenge - FF(1)) * inverse_two }; | ||
} else if constexpr (DeciderVerificationKeys::NUM == 4) { | ||
constexpr FF inverse_six = FF(6).invert(); | ||
vanishing_polynomial_at_challenge = combiner_challenge * (combiner_challenge - FF(1)) * | ||
(combiner_challenge - FF(2)) * (combiner_challenge - FF(3)); | ||
lagranges = { (FF(1) - combiner_challenge) * (FF(2) - combiner_challenge) * (FF(3) - combiner_challenge) * | ||
inverse_six, | ||
combiner_challenge * (FF(2) - combiner_challenge) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (combiner_challenge - FF(2)) * inverse_six }; | ||
} | ||
static_assert(DeciderVerificationKeys::NUM < 5); | ||
|
||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/881): bad pattern | ||
auto next_accumulator = std::make_shared<DeciderVK>(); | ||
next_accumulator->verification_key = std::make_shared<VerificationKey>(*accumulator->verification_key); | ||
next_accumulator->is_accumulator = true; | ||
|
||
size_t commitment_idx = 0; | ||
for (auto& expected_vk : next_accumulator->verification_key->get_all()) { | ||
size_t vk_idx = 0; | ||
expected_vk = Commitment::infinity(); | ||
for (auto& key : keys_to_fold) { | ||
expected_vk = expected_vk + key->verification_key->get_all()[commitment_idx] * lagranges[vk_idx]; | ||
vk_idx++; | ||
} | ||
commitment_idx++; | ||
} | ||
|
||
// Compute next folding parameters | ||
const auto [vanishing_polynomial_at_challenge, lagranges] = | ||
compute_vanishing_polynomial_and_lagrange_evaluations<FF, NUM_KEYS>(combiner_challenge); | ||
next_accumulator->target_sum = | ||
perturbator_evaluation * lagranges[0] + vanishing_polynomial_at_challenge * combiner_quotient_evaluation; | ||
next_accumulator->gate_challenges = | ||
next_accumulator->gate_challenges = // note: known already in previous round | ||
update_gate_challenges(perturbator_challenge, accumulator->gate_challenges, deltas); | ||
|
||
// Compute ϕ | ||
auto& acc_witness_commitments = next_accumulator->witness_commitments; | ||
commitment_idx = 0; | ||
for (auto& comm : acc_witness_commitments.get_all()) { | ||
comm = Commitment::infinity(); | ||
size_t vk_idx = 0; | ||
for (auto& key : keys_to_fold) { | ||
comm = comm + key->witness_commitments.get_all()[commitment_idx] * lagranges[vk_idx]; | ||
vk_idx++; | ||
} | ||
commitment_idx++; | ||
// // Fold the commitments | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor folding to be more succinct, putting grouping logic in the DeciderVerificationKeys object. Ultimately I think this class should go away (you can shove state it in and it's hard to track state in Pg) and these should just be functions, but for now I keep the pattern and put the functions here. |
||
for (auto [combination, to_combine] : | ||
zip_view(next_accumulator->verification_key->get_all(), keys_to_fold.get_precomputed_commitments())) { | ||
combination = batch_mul_native(to_combine, lagranges); | ||
} | ||
for (auto [combination, to_combine] : | ||
zip_view(next_accumulator->witness_commitments.get_all(), keys_to_fold.get_witness_commitments())) { | ||
combination = batch_mul_native(to_combine, lagranges); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels more readable! |
||
size_t alpha_idx = 0; | ||
for (auto& alpha : next_accumulator->alphas) { | ||
alpha = FF(0); | ||
size_t vk_idx = 0; | ||
for (auto& key : keys_to_fold) { | ||
alpha += key->alphas[alpha_idx] * lagranges[vk_idx]; | ||
vk_idx++; | ||
} | ||
alpha_idx++; | ||
// Fold the relation parameters | ||
for (auto [combination, to_combine] : zip_view(next_accumulator->alphas, keys_to_fold.get_alphas())) { | ||
combination = linear_combination(to_combine, lagranges); | ||
} | ||
auto& expected_parameters = next_accumulator->relation_parameters; | ||
for (size_t vk_idx = 0; vk_idx < DeciderVerificationKeys::NUM; vk_idx++) { | ||
auto& key = keys_to_fold[vk_idx]; | ||
expected_parameters.eta += key->relation_parameters.eta * lagranges[vk_idx]; | ||
expected_parameters.eta_two += key->relation_parameters.eta_two * lagranges[vk_idx]; | ||
expected_parameters.eta_three += key->relation_parameters.eta_three * lagranges[vk_idx]; | ||
expected_parameters.beta += key->relation_parameters.beta * lagranges[vk_idx]; | ||
expected_parameters.gamma += key->relation_parameters.gamma * lagranges[vk_idx]; | ||
expected_parameters.public_input_delta += key->relation_parameters.public_input_delta * lagranges[vk_idx]; | ||
expected_parameters.lookup_grand_product_delta += | ||
key->relation_parameters.lookup_grand_product_delta * lagranges[vk_idx]; | ||
for (auto [combination, to_combine] : | ||
zip_view(next_accumulator->relation_parameters.get_to_fold(), keys_to_fold.get_relation_parameters())) { | ||
combination = linear_combination(to_combine, lagranges); | ||
} | ||
|
||
return next_accumulator; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost of including headers in headers is much higher than the cost of including headers in cpp files. He header should be a description of interfaces (this sometimes breaks down/we get lazy around templates) and the dependency structure of those should be exactly what is expressed by the inclusion of headers in headers. We don't achieve this, but we should still not include things in headers when not needed as a best practice (esp when the header to be included contains template definitions!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining this and for the clean-up, will be more careful with the inclusions