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

Conversation

iakovenkos
Copy link
Contributor

@iakovenkos iakovenkos commented Jan 8, 2025

This PR is a follow-up to #10773

@iakovenkos iakovenkos self-assigned this Jan 8, 2025
@iakovenkos iakovenkos added the crypto cryptography label Jan 8, 2025
@@ -0,0 +1,64 @@
#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

@iakovenkos iakovenkos marked this pull request as ready for review January 8, 2025 14:40
#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)

*
* @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


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

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

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

std::shared_ptr<typename TypeParam::CommitmentKey> ck;

auto prover_transcript = TypeParam::Transcript::prover_init_empty();
ck = CreateCommitmentKey<typename TypeParam::CommitmentKey>();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto to what I noted in the other test file

SmallSubgroupIPA small_subgroup_ipa_prover =
SmallSubgroupIPA(zk_sumcheck_data, multivariate_challenge, claimed_inner_product, prover_transcript, ck);

auto batched_polynomial = small_subgroup_ipa_prover.get_batched_polynomial();
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid auto if posiblle and i think these are consts?

TYPED_TEST_SUITE(SmallSubgroupIPATest, TestFlavors);

// Implement the tests
TYPED_TEST(SmallSubgroupIPATest, ProverComputationsCorrectness)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you add comments to what each TYPED_TEST is doing, please?

}
};

// Register the flavors for the test suite
Copy link
Contributor

Choose a reason for hiding this comment

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

comment not needed (ditto above) on line 60


// Check that batched polynomial is divisible by Z_H(X)
bool ipa_claim_consistency = true;
for (size_t idx = 0; idx < SUBGROUP_SIZE; idx++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

range loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto cryptography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants