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

feat!: remove the compress_selectors field from VerifyingKey #310

Merged
merged 13 commits into from
Apr 12, 2024

Conversation

duguorong009
Copy link

@duguorong009 duguorong009 commented Apr 8, 2024

Description

Remove the compress_selectors field from VerifyingKey

Related issues

Changes

  • create keygen_pk_custom util
  • add the comments to keygen_pk, keygen_pk_custom, keygen_vk & keygen_vk_custom
  • update the corresponding example & bench codes
  • add compress_selectors field to halo2_proofs::plonk::create_proof func
  • merge the ZAL code (ZAL: ZK Accel Layer #277 )
  • create create_proof_custom_with_engine func for both compatibility & new API

Context

While working in PR #304, I found that compress_selectors field is essentially not needed in VerifyingKey.
It was there because @ed255 didn't find appropriate solution while doing frontend-backend split work.
But, after frontend-backend split, it is clear that compress_selectors is for frontend(compile circuit), and VerifyingKey is only for backend stuff.
Regarding this issue, I had a bit of discussion with @ed255
He gave some good ideas:

  1. Remove compress_selectors from VerifyingKey
  2. Two possible options
    • add compress_selectors param to legacy keygen_pk(safe, but break API)
    • add keygen_pk_custom with assumption of keygen_pk having compress_selectors: true (compatible, but user could misuse)

He recommended to create draft PR for discussing the possible solutions with other members.
This PR serves the purpose of discussing the better solutions regarding the compress_selectors in VerifyingKey

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

From this PR, my only worry is breaking the API for create_proof; I wrote an idea that could resolve that.

@CPerezz CPerezz self-requested a review April 8, 2024 13:22
@duguorong009
Copy link
Author

From this PR, my only worry is breaking the API for create_proof; I wrote an idea that could resolve that.

@ed255
Can you share your idea here? (I cannot see your writing anywhere. 😅 )

From my end, I believe we should break the API for create_proof since it needs the compress_selectors info.
As long as we go for option of removing compress_selectors from VerifyingKey, we should manually feed compress_selectors to the create_proof func.

This is kinda side effect.
I focused on keygen part when starting this draft PR.
But, as we can see, it is closely related to create_proof func, too.

@ed255
Copy link
Member

ed255 commented Apr 9, 2024

From this PR, my only worry is breaking the API for create_proof; I wrote an idea that could resolve that.

@ed255 Can you share your idea here? (I cannot see your writing anywhere. 😅 )

Oh how unfortunate, I had written it as a review comment, but for some reason it was not submitted.

From my end, I believe we should break the API for create_proof since it needs the compress_selectors info. As long as we go for option of removing compress_selectors from VerifyingKey, we should manually feed compress_selectors to the create_proof func.

This is kinda side effect. I focused on keygen part when starting this draft PR. But, as we can see, it is closely related to create_proof func, too.

My idea was to break the create_proof method into two versions: create_proof and create_proof_custom.
Then we'd have the following:

  • methods that assume compress_selectors = true: keygen_pk, keygen_vk, create_proof
  • methods that receive compress_selectors parameter: keygen_pk_custom, keygen_vk_custom, create_proof_custom.

This way we don't break the API, and the resulting API looks consistent.

@duguorong009
Copy link
Author

duguorong009 commented Apr 9, 2024

My idea was to break the create_proof method into two versions: create_proof and create_proof_custom. Then we'd have the following:

  • methods that assume compress_selectors = true: keygen_pk, keygen_vk, create_proof
  • methods that receive compress_selectors parameter: keygen_pk_custom, keygen_vk_custom, create_proof_custom.

This way we don't break the API, and the resulting API looks consistent.

Wow, thanks for the idea! @ed255
I believe it is best idea for both compatibility and new API.

Done 56f10ac

@duguorong009 duguorong009 requested a review from ed255 April 9, 2024 17:58
@duguorong009 duguorong009 requested a review from ed255 April 10, 2024 13:31
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

@duguorong009 duguorong009 force-pushed the draft@remove-compress-selectors branch from 43e2ceb to 8b3bdf5 Compare April 11, 2024 13:49
@duguorong009 duguorong009 requested a review from ed255 April 11, 2024 14:19
@duguorong009 duguorong009 changed the title feat!: remove the compress_selectors field from VerifyingKey [draft] feat!: remove the compress_selectors field from VerifyingKey Apr 12, 2024
@duguorong009 duguorong009 marked this pull request as ready for review April 12, 2024 08:44
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

@duguorong009 duguorong009 merged commit 3600207 into main Apr 12, 2024
16 checks passed
@duguorong009 duguorong009 deleted the draft@remove-compress-selectors branch April 12, 2024 14:41
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