privacy-scaling-explorations / halo2

https://privacy-scaling-explorations.github.io/halo2/
Other
207 stars 129 forks source link

feat!: remove the `compress_selectors` field from `VerifyingKey` #310

Closed duguorong009 closed 6 months ago

duguorong009 commented 6 months ago

Description

Remove the compress_selectors field from VerifyingKey

Related issues

-

Changes

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

duguorong009 commented 6 months ago

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 commented 6 months ago

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:

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

duguorong009 commented 6 months ago

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 https://github.com/privacy-scaling-explorations/halo2/pull/310/commits/56f10ac782277a87bc38ba187774dfabcae96779