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

Sumcheck #18

Merged
merged 26 commits into from
Aug 3, 2023
Merged

Sumcheck #18

merged 26 commits into from
Aug 3, 2023

Conversation

mpenciak
Copy link
Contributor

@mpenciak mpenciak commented Jun 28, 2023

  • Adds the Sumcheck verifier protocol
  • Adds supporting polynomial libraries
  • Adds the sumcheck data contract generated by a python script
  • Adds unit tests for the polynomial libraries
  • Adds end to end tests for sumcheck in the initial part of the Spartan verifier

Note: The data contract has data for all of the outer, inner, and batched sumchecks, though only the outer data is used in the primary and secondary sumcheck tests. Once computational commitments are implemented, we can write end to end tests that will use the inner and batched data.

src/sumcheck.sol Outdated Show resolved Hide resolved
@storojs72
Copy link
Member

As far as I see, new file src/sumcheck.sol is being added and also there are some changes on KeccakTranscript. How do you debug and test your implementation? And what part of Nova sumcheck protocol is being implemented? Just for pointer - we expect this one.

@storojs72
Copy link
Member

@mpenciak , I added basic unit-testing for primary part of sumcheck computation. There were some issues with using the right modulus and negation. Having this as an example, it should be easier to implement secondary part and update data contract as well as Python generator script.

@mpenciak mpenciak marked this pull request as ready for review July 26, 2023 01:29
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mpenciak !

Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

I like the idea of having bunch of absorb functions, each taking its own type of input at a KeccakTranscript level. It seems to be a bit more ergonomic than what I used in pp-spartan branch, where I have similar bunch of toTranscriptBytes functions and I have to introduce a redundant intermediate uint8[] memory variable all the time, when I need to invoke absorb.

At the same time, after taking draft code from early version of this PR and adapting it for usage in pp-spartan branch, I have couple of remarks:

I think a lot of Sumcheck's logic can be implemented in a more generic way: in some places we can pass modulus as a uint256 variable (either Vesta or Pallas), also we can use passing function as a parameter if we need to use particular negation. Also, structures that operates with just uint256 can be defined only once.

It looks like you implemented some more types of evaluations, however, from my experience, the pp-spartan verification uses only sumcheck over UniPoly polynomials (this implementation is sufficient for e2e pp-spartan verification). Though I didn't check if other polynomials are used while IPA (as it is going to be replaced with KZG/Zeromorph eventually). Anyway, if these logic is useful and we want to keep it as a building block (yet unused in high-level verification), it needs to be somehow tested.

src/verifier/step4/KeccakTranscript.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/sumcheck-data-contract-gen.py Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

LGTM now! Thanks!

src/verifier/step4/SumcheckLogic.sol Outdated Show resolved Hide resolved
@mpenciak mpenciak merged commit 845c64e into main Aug 3, 2023
2 checks passed
@mpenciak mpenciak deleted the mp/sumcheck branch August 3, 2023 17:58
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.

3 participants