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: pcs tests refactor #11188

Merged
merged 12 commits into from
Jan 29, 2025
Merged

chore: pcs tests refactor #11188

merged 12 commits into from
Jan 29, 2025

Conversation

iakovenkos
Copy link
Contributor

@iakovenkos iakovenkos commented Jan 13, 2025

Cleaned up Gemini, IPA, and KZG tests

@iakovenkos iakovenkos self-assigned this Jan 28, 2025
@iakovenkos iakovenkos marked this pull request as ready for review January 28, 2025 13:44
@@ -12,24 +13,27 @@ template <class Curve> class GeminiTest : public CommitmentTest<Curve> {
using Commitment = typename Curve::AffineElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleaned up witness and instance generation, also switched to creating a ck of the size controlled by the test suite inputs

#include "./mock_transcript.hpp"
#include "barretenberg/commitment_schemes/commitment_key.test.hpp"
#include "barretenberg/commitment_schemes/shplonk/shplemini.hpp"
#include "barretenberg/commitment_schemes/utils/instance_witness_generator.hpp"
using namespace bb;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed manual witness/instance generation in integration tests/ some clean-up in unit tests, trying to make the test suites more uniform

unshifted_polynomials[idx] = Polynomial::random(n);
unshifted_commitments.push_back(ck->commit(unshifted_polynomials[idx]));
unshifted_evals.push_back(unshifted_polynomials[idx].evaluate_mle(mle_opening_point));
if (!unshifted_polynomials.empty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for edge case testing - needed to test the situation when Gemini opens only shifts

size_t idx = num_unshifted;

// Add unshifted evaluations of shiftable polynomials
if (!unshifted_polynomials.empty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above - to support 0 unshifted polynomials as input

@iakovenkos iakovenkos changed the title chore: pcs tests refactor [wip] chore: pcs tests refactor Jan 28, 2025
Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

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

Excellent! Just a few minor comments

@@ -19,12 +19,12 @@ template <typename Curve> struct InstanceWitnessGenerator {
using Polynomial = bb::Polynomial<Fr>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I found the name of this class to be a little bit confusing. Maybe "instance" is a term that's used to refer to witness commitments in PG or something? Even if so, we don't use that term elsewhere in the code AFAIK. Also, I think its always ideal to have Test or Mock in the name of a class like this since its purely a test class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then --> MockWitnessGenerator
I wanted to emphasize that we also produce the evaluations and commitments in this class, and they define an instance for the PCS opening protocol. but I guess it's a bit redundant in the context of the PCS test suites

@iakovenkos iakovenkos merged commit c4892c6 into master Jan 29, 2025
24 checks passed
@iakovenkos iakovenkos deleted the si/pcs-tests-clean-up branch January 29, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants