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

Prover: turns the plonk-in-wizard into a query #688

Merged

Conversation

AlexandreBelling
Copy link
Contributor

As part of the limitless prover effort. We need to turn the plonk-in-wizard into a query. Doing so means that instead of directly declaring the Plonk columns in the wizard we instead create a query that we can subsequently compile after the splitting has occurred.

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

@AlexandreBelling AlexandreBelling added the Prover Tag to use for all work impacting the prover label Feb 15, 2025
@AlexandreBelling AlexandreBelling self-assigned this Feb 15, 2025
Copy link
Contributor

@arijitdutta67 arijitdutta67 left a comment

Choose a reason for hiding this comment

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

I had minor suggestions. But broadly LGTM.

Copy link
Contributor

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

In general, looks good to me. However I think we have lost applications of PlonkOption during refactor and they are not applied anymore? See the comment plonkinwizard/compile.go.

And I think it is also a bit inconvenient that now the PlonkOption fields are defined as []any now. It seems due to trying to avoid import cycles as the options refer directly to the CompilationCtx and we cannot import it in dedicated/plonk. But I think we could define Option type in dedicated/plonk and then use implict interfaces to set the options.

So for now not accepting yet as I'm not sure we haven't lost functionality. Could you confirm?

@AlexandreBelling
Copy link
Contributor Author

Yeah we are not loosing functionality in this way. The tricky thing is that the Plonk options that we have today are options for the [PlonkCheck] function which was in protocol/dedicated/plonk but now this part of the code is part of a compiler (i.e. this is very internal: PlonkCheck should not be exposed to the user but the options should.

I'll make a "second" option type in the query package to have a cleaner separation

@AlexandreBelling
Copy link
Contributor Author

Updated it. I also noticed some failed tests and I fixed them. I ended up removing the added constraints on the selector since this was 1) redundant and 2) not backward compatible. Instead of looking at the whole column, the query only looks at positions at the beginning of a Plonk instance. I think this is simpler

@AlexandreBelling AlexandreBelling merged commit fccd354 into prover/limitless-top-level Feb 24, 2025
1 of 5 checks passed
@AlexandreBelling AlexandreBelling deleted the prover/limitless-circuits-as-queries branch February 24, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prover Tag to use for all work impacting the prover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants