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.

Sign up for GitHub

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

Jump to bottom

standard honk on grumpkin #1258

Merged
merged 3 commits into from
Jul 31, 2023
Merged

standard honk on grumpkin #1258

merged 3 commits into from
Jul 31, 2023

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Jul 28, 2023 •
edited
Loading

Description

This PR provides an initial solution for having a complete grumpkin flavor in standard honk and contains the following modifications:

  • the FileCrsFactory now has functionality to produce CRSes for both curves (with their key differences) and I addressed any dependent changes in the codebase
  • the PCSes are now curve agnostic, having the curve set by their parameters and missing tests were added (shlponk\gemini on both grumpkin and bn254 which surfaced a bug in shplonk; unit testing gemini+shplonk+ipa as only the kzg-variant was tested)
  • continued work on enabling field-agnostic gates from (Better templating of gates barretenberg#557, i have to link issues manually)
  • to avoid divison by zero caused by inverting the root of unity in EvaluationDomain<grumpkin::fr> we hardcode the roots of unity to 1 given Grumpkin does not have many roots of unity.

Opens AztecProtocol/barretenberg#635
AztecProtocol/barretenberg#637
AztecProtocol/barretenberg#636,
AztecProtocol/barretenberg#640
for subsequent work.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@maramihali maramihali marked this pull request as draft July 28, 2023 07:58
@maramihali maramihali marked this pull request as ready for review July 28, 2023 08:17
@maramihali maramihali force-pushed the mm/grumpkin-crs branch 2 times, most recently from 4cbf155 to 758d319 Compare July 28, 2023 09:55
@maramihali maramihali force-pushed the mm/grumpkin-crs branch 3 times, most recently from 491a0ab to 443ccb9 Compare July 28, 2023 15:58
@iAmMichaelConnor iAmMichaelConnor added the crypto cryptography label Jul 29, 2023
zac-williamson
zac-williamson requested changes Jul 30, 2023
View reviewed changes
Copy link
Contributor

@zac-williamson zac-williamson left a comment

Choose a reason for hiding this comment

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

Looks good. Code is clean and the targeted changes are easy to understand.

I have some very minor formatting comments but other than that looks good to me 👍

circuits/cpp/barretenberg/cpp/src/barretenberg/srs/global_crs.cpp Outdated Show resolved Hide resolved
circuits/cpp/barretenberg/cpp/src/barretenberg/srs/factories/file_crs_factory.hpp Show resolved Hide resolved
circuits/cpp/barretenberg/cpp/src/barretenberg/srs/global_crs.cpp Outdated Show resolved Hide resolved
circuits/cpp/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/plookup/arithmetic.cpp Outdated Show resolved Hide resolved
circuits/cpp/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/plookup/arithmetic.cpp Outdated Show resolved Hide resolved
@maramihali maramihali force-pushed the mm/grumpkin-crs branch 3 times, most recently from e106375 to 80ba812 Compare July 31, 2023 11:16
zac-williamson
zac-williamson approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@zac-williamson zac-williamson left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@maramihali maramihali merged commit 25e3599 into master Jul 31, 2023
@maramihali maramihali deleted the mm/grumpkin-crs branch July 31, 2023 16:10
@codygunton codygunton mentioned this pull request Aug 17, 2023
Closed
@TomAFrench TomAFrench mentioned this pull request Aug 18, 2023
Merged
4 tasks
kevaundray pushed a commit that referenced this pull request Aug 18, 2023
@TomAFrench
fix: fix paths in barretenberg bootstrap.sh script (#1662)
24bbfd4 
In #1258, we added a
`cd` into the build directory but didn't `cd` back out again. This means
that all the paths for the rest of the script are incorrect.


# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Aug 19, 2023
@TomAFrench @AztecBot
fix: fix paths in barretenberg bootstrap.sh script (#1662)
c8917cd 
In AztecProtocol/aztec-packages#1258, we added a
`cd` into the build directory but didn't `cd` back out again. This means
that all the paths for the rest of the script are incorrect.


# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zac-williamson zac-williamson zac-williamson approved these changes

Assignees

@maramihali maramihali

Labels
crypto cryptography
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maramihali @zac-williamson @codygunton @iAmMichaelConnor