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

Include instances in the Circuit trait #255

Open
zemse opened this issue Jan 29, 2024 · 2 comments
Open

Include instances in the Circuit trait #255

zemse opened this issue Jan 29, 2024 · 2 comments

Comments

@zemse
Copy link
Member

zemse commented Jan 29, 2024

Currently, we have to calculate the instances in a separate code. However, it would be great if we required defining instance calculation function in the Circuit trait.

It is already in the snark-verifier SDK in a CircuitExt trait.

https://github.com/privacy-scaling-explorations/snark-verifier/blob/c400ffcd629c337111c4e3cbf95acfe1230b068b/snark-verifier-sdk/src/lib.rs#L111-L115

As well as on the SubCircuit trait in zkevm

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/d97258872bbe14547465b984343b1e359217281b/zkevm-circuits/src/util.rs#L156

I think upstreaming the instances function and num_instances to the pse halo2's Circuit trait can be beneficial to all projects, i.e. organise the code that computes the instance that is to be passed on mock prover or verifier.

But yeah it would be a breaking change and annoying to implement when someone is upgrading to latest halo2 but I believe it can be worth it. Happy to make a PR for this if it is okay.

@zemse zemse changed the title ELI15: Include instances in the Circuit trait Include instances in the Circuit trait Jan 29, 2024
@CPerezz
Copy link
Member

CPerezz commented Jan 30, 2024

I think we can maybe add it with a nightly feature. See how it evolves and if it's supported. And then, we can simply pass it to stable and remove the feature.

WDYT @han0110 @kilic ?

@han0110
Copy link

han0110 commented Feb 8, 2024

I think an approach could be adding the 2 methods in Circuit but with default implementation for them, and don't use them in halo2_proofs crates for now (keep the same API of the other things), then it wouldn't be a breaking change.

And perhaps after the backend-frontend splitting is finished, the new backend can start to user this instance API. Tho not sure if the default implementation would be a footgun or not..

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

No branches or pull requests

3 participants