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: move shared pcs functionality to internal library in solidity and small refactorings in sumcheck #11230

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Jan 15, 2025

  • functionality that is shared in PCS between the ZK and non-ZK contract has been moved to a separate internal library.
  • simplified ZK sumcheck and pcs logic

Copy link
Contributor

@jeanmon jeanmon left a comment

Choose a reason for hiding this comment

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

@maramihali Would you mind updating the template files ?:

  • bb-pilcom/bb-pil-backend/templates/recursive_verifier.cpp.hbs
  • bb-pilcom/bb-pil-backend/templates/verifier.cpp.hbs

Otherwise, let us know if AVM team should do that.

@maramihali maramihali requested a review from Maddiaa0 as a code owner January 15, 2025 11:14
@maramihali maramihali changed the title chore: move shared pcs functionality to internal library in solidity and sumcheck chore: move shared pcs functionality to internal library in solidity and small refactorings in sumcheck Jan 15, 2025
@maramihali
Copy link
Contributor Author

@jeanmon on it :)

@maramihali maramihali requested a review from iakovenkos January 15, 2025 11:48
// For ZK Flavors: the sum of the Libra constant term and Libra univariates evaluated at Sumcheck challenges,
// otherwise remains the default value 0
FF claimed_libra_evaluation = 0;
std::optional<bool> verified = false; // Optional b/c this struct is shared by the Prover/Verifier
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep it std::optional? switching to just bool would reduce noise in our verifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the idea of the std::optional is to make it explicit this value is not present for the prover

Copy link
Contributor

@iakovenkos iakovenkos left a comment

Choose a reason for hiding this comment

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

the changes outside of AVM and sol lgtm, left a couple of comments

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.

4 participants