Skip to content

Commit

Permalink
response to code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@maramihali
maramihali committed Feb 2, 2024
1 parent d6365c5 commit 2d39bbc
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 43 deletions.
2 changes: 0 additions & 2 deletions barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_prover.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,6 @@ std::shared_ptr<typename ProverInstances::Instance> ProtoGalaxyProver_<ProverIns
FF& challenge,
const FF& compressed_perturbator)
{
static_cast<void>(challenge);

auto combiner_quotient_at_challenge = combiner_quotient.evaluate(challenge);

// Given the challenge \gamma, compute Z(\gamma) and {L_0(\gamma),L_1(\gamma)}
Expand Down
15 changes: 8 additions & 7 deletions barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_prover.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ template <class ProverInstances_> class ProtoGalaxyProver_ {

// Sum relation evaluations, batched by their corresponding relation separator challenge, to get the value
// of the full honk relation at a specific row
Utils::scale_and_batch_elements_without_linear_contributions(
Utils::scale_and_batch_elements(
relation_evaluations, alpha, running_challenge, output, linearly_dependent_contribution);

full_honk_evaluations[row] = output;
Expand All @@ -170,9 +170,9 @@ template <class ProverInstances_> class ProtoGalaxyProver_ {
}

/**
* @brief Recursively compute the parent nodes of each level in there, starting from the leaves. Note that at each
* level, the resulting parent nodes will be polynomials of degree (level+1) because we multiply by an additional.
* factor of X.
* @brief Recursively compute the parent nodes of each level in the tree, starting from the leaves. Note that at
* each level, the resulting parent nodes will be polynomials of degree (level+1) because we multiply by an
* additional factor of X.
*/
static std::vector<FF> construct_coefficients_tree(const std::vector<FF>& betas,
const std::vector<FF>& deltas,
Expand Down Expand Up @@ -319,8 +319,8 @@ template <class ProverInstances_> class ProtoGalaxyProver_ {
FF pow_challenge = pow_betas[idx];

// Accumulate the i-th row's univariate contribution. Note that the relation parameters passed to this
// function have already been folded
// This will not add pow stuff to relation dependent stuff
// function have already been folded. Moreover, linear-dependent relations that act over the entire
// execution trace rather than on rows, will not be multiplied by the pow challenge.
accumulate_relation_univariates(
thread_univariate_accumulators[thread_idx],
extended_univariates[thread_idx],
Expand Down Expand Up @@ -430,7 +430,8 @@ template <class ProverInstances_> class ProtoGalaxyProver_ {
}

/**
* @brief Compute the next accumulator (ϕ*, ω*\vec{\beta*}, e*), send the public data ϕ* and the folding parameters
* @brief Compute the next accumulator (ϕ*, ω*, \vec{\beta*}, e*), send the public data ϕ* and the folding
* parameters
* (\vec{\beta*}, e*) to the verifier and return the complete accumulator
*
* @details At this stage, we assume that the instances have the same size and the same number of public parameter.s
Expand Down
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void ProtoGalaxyVerifier_<VerifierInstances>::receive_and_finalise_instance(cons
witness_commitments.w_o = transcript->template receive_from_prover<Commitment>(domain_separator + "_" + labels.w_o);

if constexpr (IsGoblinFlavor<Flavor>) {
// Get commitments to the ECC wire polynomials
// Get commitments to the ECC wire polynomials and databus polynomials
witness_commitments.ecc_op_wire_1 =
transcript->template receive_from_prover<Commitment>(domain_separator + "_" + labels.ecc_op_wire_1);
witness_commitments.ecc_op_wire_2 =
Expand Down
30 changes: 17 additions & 13 deletions barretenberg/cpp/src/barretenberg/relations/utils.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,7 @@ template <typename Flavor> class RelationUtils {
template <typename... T>
static constexpr void add_tuples(std::tuple<T...>& tuple_1, const std::tuple<T...>& tuple_2)
{
auto add_tuples_helper = [&]<std::size_t... I>(std::index_sequence<I...>)
{
auto add_tuples_helper = [&]<std::size_t... I>(std::index_sequence<I...>) {
((std::get<I>(tuple_1) += std::get<I>(tuple_2)), ...);
};

Expand Down Expand Up @@ -184,7 +183,7 @@ template <typename Flavor> class RelationUtils {
};

/**
* @brief Scale elements, representing evaluations of subrelations, by separate challenges and afterwards sum them
* @brief Scale elements, representing evaluations of subrelations, by separate challenges then sum them
* @param challenges Array of NUM_SUBRELATIONS - 1 challenges (because the first subrelation does not need to be
* scaled)
* @param result Batched result
Expand All @@ -197,9 +196,7 @@ template <typename Flavor> class RelationUtils {
{
size_t idx = 0;
std::array<FF, NUM_SUBRELATIONS> tmp{ current_scalar };

std::copy(challenges.begin(), challenges.end(), tmp.begin() + 1);

auto scale_by_challenges_and_accumulate = [&](auto& element) {
for (auto& entry : element) {
result += entry * tmp[idx];
Expand All @@ -224,11 +221,11 @@ template <typename Flavor> class RelationUtils {
* @param result
* @param linearly_dependent_contribution
*/
static void scale_and_batch_elements_without_linear_contributions(auto& tuple,
const RelationSeparator& challenges,
FF current_scalar,
FF& result,
FF& linearly_dependent_contribution)
static void scale_and_batch_elements(auto& tuple,
const RelationSeparator& challenges,
FF current_scalar,
FF& result,
FF& linearly_dependent_contribution)
requires bb::IsFoldingFlavor<Flavor>
{
size_t idx = 0;
Expand Down Expand Up @@ -285,18 +282,25 @@ template <typename Flavor> class RelationUtils {
}
}

// Recursive template function to apply a specific operation on each element of several arrays in a tuple
/**
* @brief Recursive template function to apply a specific operation on each element of several arrays in a tuple
*
* @details We need this method in addition to the apply_to_tuple_of_arrays when we aim to perform different
* operations depending on the array element. More explicitly, in our codebase this method is used when the elements
* of array are values of subrelations and we want to accumulate some of these values separately (the linearly
* dependent contribution when we compute the evaluation of full rel_U(G)H at particular row.)
*/
template <size_t outer_idx = 0, size_t inner_idx = 0, typename Operation, typename... Ts>
static void apply_to_tuple_of_arrays_elements(Operation&& operation, std::tuple<Ts...>& tuple)
{
using Relation = typename std::tuple_element_t<outer_idx, Relations>;
const auto subrel = Relation::SUBRELATION_PARTIAL_LENGTHS.size();
const auto subrelation_length = Relation::SUBRELATION_PARTIAL_LENGTHS.size();
auto& element = std::get<outer_idx>(tuple);

// Invoke the operation with outer_idx (array index) and inner_idx (element index) as template arguments
operation.template operator()<outer_idx, inner_idx>(element[inner_idx]);

if constexpr (inner_idx + 1 < subrel) {
if constexpr (inner_idx + 1 < subrelation_length) {
// Recursively call for the next element within the same array
apply_to_tuple_of_arrays_elements<outer_idx, inner_idx + 1, Operation>(std::forward<Operation>(operation),
tuple);
Expand Down
70 changes: 50 additions & 20 deletions barretenberg/cpp/src/barretenberg/ultra_honk/protogalaxy.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {

static void SetUpTestSuite() { bb::srs::init_crs_factory("../srs_db/ignition"); }

// TODO(https://github.com/AztecProtocol/barretenberg/issues/744): make testing utility with functionality shared
// amongst test files in the proof system
static bb::Polynomial<FF> get_random_polynomial(size_t size)
{
auto poly = bb::Polynomial<FF>(size);
for (auto& coeff : poly) {
coeff = FF::random_element();
}
return poly;
}

static void construct_circuit(Builder& builder)
{
if constexpr (IsGoblinFlavor<Flavor>) {
GoblinMockCircuits::construct_goblin_ecc_op_circuit(builder);
GoblinMockCircuits::construct_arithmetic_circuit(builder);
GoblinMockCircuits::construct_goblin_ecc_op_circuit(builder);

} else {
FF a = FF::random_element();
Expand Down Expand Up @@ -97,20 +86,25 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
for (size_t i = 0; i < instance_size; i++) {
expected_target_sum += expected_honk_evals[i] * expected_pows[i];
}
info("expected_target_sum", expected_target_sum);

EXPECT_EQ(accumulator->target_sum == expected_target_sum, expected_result);
}

static void decide_and_verify(std::shared_ptr<Instance>& accumulator, Composer& composer, bool expected_result)
{
auto decider_prover = composer.create_decider_prover(accumulator);
auto decider_verifier = composer.create_decider_verifier(accumulator);
auto decision = decider_prover.construct_proof();
auto verified = decider_verifier.verify_proof(decision);
auto decider_proof = decider_prover.construct_proof();
auto verified = decider_verifier.verify_proof(decider_proof);
EXPECT_EQ(verified, expected_result);
}

static void full_honk_evaluations_valid()
/**
* @brief For a valid circuit, ensures that computing the value of the full UH/UGH relation at each row in its
* execution trace (with the contribution of the linearly dependent one added tot he first row, in case of Goblin)
* will be 0.
*
*/
static void test_full_honk_evaluations_valid_circuit()
{
auto builder = typename Flavor::CircuitBuilder();
construct_circuit(builder);
Expand Down Expand Up @@ -140,6 +134,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
}
}

/**
* @brief Check the coefficients of the perturbator computed from dummy \vec{β}, \vec{δ} and f_i(ω) will be the same
* as if computed manually.
*
*/
static void test_pertubator_coefficients()
{
std::vector<FF> betas = { FF(5), FF(8), FF(11) };
Expand All @@ -153,14 +152,19 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
}
}

/**
* @brief Create a dummy accumulator and ensure coefficient 0 of the computed perturbator is the same as the
* accumulator's target sum.
*
*/
static void test_pertubator_polynomial()
{
using RelationSeparator = typename Flavor::RelationSeparator;
const size_t log_instance_size(3);
const size_t instance_size(1 << log_instance_size);
std::array<bb::Polynomial<FF>, Flavor::NUM_ALL_ENTITIES> random_polynomials;
for (auto& poly : random_polynomials) {
poly = get_random_polynomial(instance_size);
poly = bb::Polynomial<FF>::random(instance_size);
}
auto full_polynomials = construct_full_prover_polynomials(random_polynomials);
auto relation_parameters = bb::RelationParameters<FF>::get_random();
Expand Down Expand Up @@ -200,6 +204,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
EXPECT_EQ(perturbator[0], target_sum);
}

/**
* @brief Manually compute the expected evaluations of the combiner quotient, given evaluations of the combiner and
* check them against the evaluations returned by the function.
*
*/
static void test_combiner_quotient()
{
auto compressed_perturbator = FF(2); // F(\alpha) in the paper
Expand Down Expand Up @@ -228,6 +237,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
}
}

/**
* @brief For two dummy instances with their relation parameter η set, check that combining them in a univariate,
* barycentrially extended to the desired number of evaluations, is performed correctly.
*
*/
static void test_combine_relation_parameters()
{
using Instances = ProverInstances_<Flavor, 2>;
Expand All @@ -249,6 +263,10 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
EXPECT_EQ(instances.relation_parameters.eta, expected_eta);
}

/**
* @brief Given two dummy instances with the batching challenges alphas set (one for each subrelation) ensure
* combining them in a univariate of desired length works as expected.
*/
static void test_combine_alpha()
{
using Instances = ProverInstances_<Flavor, 2>;
Expand All @@ -272,6 +290,10 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
}
}

/**
* @brief Testing two valid rounds of folding followed by the decider.
*
*/
static void test_full_protogalaxy()
{
auto composer = Composer();
Expand Down Expand Up @@ -300,6 +322,10 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
decide_and_verify(first_accumulator, composer, true);
}

/**
* @brief Ensure tampering a commitment and then calling the decider causes the decider verification to fail.
*
*/
static void test_tampered_commitment()
{
auto composer = Composer();
Expand Down Expand Up @@ -331,6 +357,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
decide_and_verify(second_accumulator, composer, false);
}

/**
* @brief Ensure tampering an accumulator and then calling fold again causes both the folding verification and
* decider verification to fail.
*
*/
static void test_tampered_accumulator_polynomial()
{
auto composer = Composer();
Expand Down Expand Up @@ -372,7 +403,7 @@ TYPED_TEST(ProtoGalaxyTests, PerturbatorCoefficients)

TYPED_TEST(ProtoGalaxyTests, FullHonkEvaluationsValidCircuit)
{
TestFixture::full_honk_evaluations_valid();
TestFixture::test_full_honk_evaluations_valid_circuit();
}

TYPED_TEST(ProtoGalaxyTests, PerturbatorPolynomial)
Expand All @@ -395,7 +426,6 @@ TYPED_TEST(ProtoGalaxyTests, CombineAlpha)
TestFixture::test_combine_alpha();
}

// Check both manually and using the protocol two rounds of folding
TYPED_TEST(ProtoGalaxyTests, FullProtogalaxyTest)
{
TestFixture::test_full_protogalaxy();
Expand Down

0 comments on commit 2d39bbc

Please sign in to comment.