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

Implement SolidityGenerator #2

Merged

Conversation

han0110
Copy link
Collaborator

@han0110 han0110 commented Aug 29, 2023

Description

Currently it allows to generate verifier in 2 way:

  • Verifier with verifying key in a single contract.
  • Verifier and verifying key are in different contract, and verifier read k and preprocessed commitments from verifying key so we could reuse verifier with different verifying key as long as the configure is same (with one caveat, see the second bullet in Caveats).

But it assumes:

  • Circuit has exact 1 instance columns
  • Circuit has no rotated query to instance column

Also the transcript behaves exactly same as the EvmTranscript in snark-verifier.

Benchmark

For halo2wrong main gate with range chip, the separated verifier contract has size only 38% of the output of snark-verifier, and surprisingly, the gas cost is 10k less snark-verifier even it has many for-loops.

Caveats

  • It seems option --via-ir is necessary when compiling the generated contract, otherwise it'd cause stack too deep. However, --via-ir is not allowed to be used with --standard-json, not sure how to workaround this.
  • Even the configure is same, the selector compression might lead to different configuration when selector assignments are different. To avoid this we might need to update halo2 to support disabling selector compression.
  • Now it only supports BDFG21 batch open scheme (SHPLONK), GWC19 is not yet implemented.

Acknowledgement

The template is heavily inspired by Aztec's verifier contract https://github.com/AztecProtocol/barretenberg/blob/master/sol/src/ultra/BaseUltraVerifier.sol.

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Did a first review round with some questions. Still need to review half codegen module & the tests and transcript files.

Hope to do this within the next days. This is quite a big PR so sorry if some questions are dumb.

Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/separately.rs Show resolved Hide resolved
examples/separately.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/evm.rs Outdated Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
@CPerezz CPerezz self-requested a review September 12, 2023 15:40
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Didn't mean to approve yet. Fixing this.

Copy link
Collaborator Author

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Thanks for the review and suggestion, commits to resolve the comment have been submitted, hope it's more clear now.

Cargo.toml Outdated Show resolved Hide resolved
examples/separately.rs Show resolved Hide resolved
examples/separately.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/codegen/util.rs Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
src/test.rs Show resolved Hide resolved
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

I'll apporve the PR as I don't want to keep it stucked.
TBH, I;d need more time to be able to sit down and propperly review all the logic. In any case, this is not meant to be used yet. So we can make an audit any week later this year if we feel like deploying this.

I'd add a huge disclaimer mentioning that this should not be used in production as it hasn't been yet been audited nor properly reviewed.

Aside from that, this is an awesome work Han. Amazing effort! 👏 👏

@han0110
Copy link
Collaborator Author

han0110 commented Sep 27, 2023

I'd add a huge disclaimer mentioning that this should not be used in production as it hasn't been yet been audited nor properly reviewed.

Make senses, thanks for reminding! I've added that in c51fe30 and hope it's clear!

@han0110 han0110 merged commit ab4e36b into privacy-scaling-explorations:main Sep 27, 2023
2 checks passed
@han0110 han0110 deleted the feature/solidity-generator branch October 3, 2023 23:59
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