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: SmallSubgroupIPA tests #11106

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,15 @@ std::vector<typename GeminiProver_<Curve>::Claim> GeminiProver_<Curve>::prove(
}
const Fr r_challenge = transcript->template get_challenge<Fr>("Gemini:r");

const bool gemini_challenge_in_small_subgroup = (has_zk) && (r_challenge.pow(Curve::SUBGROUP_SIZE) == Fr(1));

// If Gemini evaluation challenge lands in the multiplicative subgroup used by SmallSubgroupIPA protocol, the
// evaluations of prover polynomials at this challenge would leak witness data.
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1194). Handle edge cases in PCS
if (gemini_challenge_in_small_subgroup) {
throw_or_abort("Gemini evaluation challenge is in the SmallSubgroup.");
}

std::vector<Claim> claims =
compute_fold_polynomial_evaluations(log_n, std::move(fold_polynomials), r_challenge, std::move(batched_group));

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#pragma once

#include "barretenberg/ecc/curves/bn254/bn254.hpp"
#include "barretenberg/transcript/transcript.hpp"
#include "commitment_key.test.hpp"

namespace bb {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are already in commitment_schemes folder so this file and the other file don't need to be named pcs_*. I would also move them in utils since there are also other test related stuff there. These two Flavors could be called Settings and be made structs since they're just data

Copy link
Contributor

Choose a reason for hiding this comment

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

also will you please make use of these in the other pcs files as well (either in this PR or a follow up PR)

/**
* @brief Mock Flavors to use ZKSumcheckData and SmallSubgroupIPAProver in the PCS tests.
*
*/
class TestBn254Flavor {
public:
using Curve = curve::BN254;
using CommitmentKey = bb::CommitmentKey<curve::BN254>;
using Transcript = NativeTranscript;
using FF = typename Curve::ScalarField;
static constexpr size_t SUBGROUP_SIZE = Curve::SUBGROUP_SIZE;
};

class TestGrumpkinFlavor {
public:
using Curve = curve::Grumpkin;
using CommitmentKey = bb::CommitmentKey<curve::Grumpkin>;
using Transcript = NativeTranscript;
using FF = typename Curve::ScalarField;
static constexpr size_t SUBGROUP_SIZE = Curve::SUBGROUP_SIZE;
};
} // namespace bb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#pragma once
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced this struct to remove redundant pattern in pcs tests where we manually generate polys, commit to them, and evaluate them


#include "barretenberg/ecc/curves/bn254/bn254.hpp"
#include "barretenberg/polynomials/polynomial.hpp"
#include "barretenberg/transcript/transcript.hpp"
#include "commitment_key.test.hpp"

namespace bb {
/**
* @brief Constructs random polynomials, computes commitments and corresponding evaluations.
*
* @tparam Curve
*/
template <typename Curve> struct PCSInstanceWitnessGenerator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove PCS in the name. Also I think this could use the Flavor/Settings defined in the other file

public:
using CommitmentKey = bb::CommitmentKey<Curve>;
using Fr = typename Curve::ScalarField;
using Commitment = typename Curve::AffineElement;
using Polynomial = bb::Polynomial<Fr>;

std::shared_ptr<CommitmentKey> ck;
std::vector<Polynomial> polynomials;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name this unshifted_polynomials and the ones below to_be_shifted_polynomials for clarity

std::vector<Polynomial> shiftable_polynomials;
std::vector<Fr> const_size_mle_opening_point;
std::vector<Commitment> unshifted_commitments;
std::vector<Commitment> shifted_commitments;
std::vector<Fr> unshifted_evals;
std::vector<Fr> shifted_evals;

PCSInstanceWitnessGenerator(const size_t n,
const size_t num_polynomials,
const size_t num_shiftable,
const std::vector<Fr>& mle_opening_point,
std::shared_ptr<CommitmentKey>& commitment_key)
: ck(commitment_key) // Initialize the commitment key
, polynomials(num_polynomials)
, shiftable_polynomials(num_shiftable)

{
construct_instance_and_witnesses(n, mle_opening_point);
}

void construct_instance_and_witnesses(size_t n, const std::vector<Fr>& mle_opening_point)
{

const size_t num_unshifted = polynomials.size() - shiftable_polynomials.size();

// Process polynomials that are not getting shifted
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Process polynomials that are not getting shifted
// Constructs polynomials that are not shifted

for (size_t idx = 0; idx < num_unshifted; idx++) {
polynomials[idx] = Polynomial::random(n);
unshifted_commitments.push_back(ck->commit(polynomials[idx]));
unshifted_evals.push_back(polynomials[idx].evaluate_mle(mle_opening_point));
}

// Process polynomials that are getting shifted
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Process polynomials that are getting shifted
// Constructs polynomials that are being shifted

size_t idx = num_unshifted;
for (auto& poly : shiftable_polynomials) {
poly = Polynomial::random(n, /*shiftable*/ 1);
polynomials[idx] = poly;
auto comm = this->ck->commit(poly);
Copy link
Contributor

Choose a reason for hiding this comment

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

const

unshifted_commitments.push_back(comm);
shifted_commitments.push_back(comm);
Copy link
Contributor

Choose a reason for hiding this comment

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

these are to_be_shifted?

unshifted_evals.push_back(poly.evaluate_mle(mle_opening_point));
shifted_evals.push_back(poly.evaluate_mle(mle_opening_point, true));
idx++;
}
}
};

} // namespace bb
Loading
Loading