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

Remove dependency to common ConstraintSystem in the backend #290

Merged
merged 28 commits into from
Mar 18, 2024

Conversation

ed255
Copy link
Member

@ed255 ed255 commented Feb 28, 2024

After the frontend-backend split, both the backend and frontend were using the same ConstraintSystem internally. This has the following implication

  • The shared ConstraintSystem uses Expressions that can have selectors, but the backend shouldn't have any code related to selectors

This PR does:

  • Create a generic Expression type in middleware that can be easily customized for the backend and frontend needs. The point of this is to reuse some code. Then we have the following configurations:
    • Expression without selectors, without query indices: the one used by the ConstraintSystemMid, which is part of the API to pass a compiled circuit to the backend.
    • Expression without selectors, with query indices: the one used by the ConstraintSystemBack. This is an internal type of the backend and is generated during the setup. All the queries in expressions are indexed so that evaluations can be done fast.
    • NOT DONE: Expression with selectors, without query indices: this would be used by ConstraintSystem in the frontend. This would be a frontend type and would be used as part of the API to build the polynomial identities, and also would be used by the MockProver to evaluate the the circuit on a given witness. This is not done in this PR: I don't know if I should do this because it's a significant breaking change. Here are the pros and cons:
      • Pros: Code reuse (notice how the new Expression in middleware copies a lot of code of the Expression in common)
      • Cons: Breaks the frontend API because now the Expression enum type is different
  • BREAKING: Remove the dependency of the Circuit trait in the backend. This changes the serialization and deserialization format of keys. Previously this serialization was implemented as a generic method using the Circuit trait; it would configure the circuit to extract its configuration, and from it read the keys. Now it receives a ConstraintSystemBackend (which is the output of the configuration, or also part of the output of circuit compilation).
  • Rename permutation::ArgumentV2 to permutation::ArgumentMid
  • Rename ConstraintSystemV2Backend to ConstraintSystemMid
  • Rename GateV2Backend to GateMid
  • Rename lookup::ArgumentV2 to lookup::ArgumentMid
  • Rename shuffle::ArgumentV2 to shuffle::ArgumentMid
  • Move the following from halo2_common to halo_frontend (as they are no longer used in halo_backend):
    • Error
    • Expression
    • sealed::Phase
    • Circuit
    • Chip
    • layouter
    • Selector
    • {Advice,Fixed,Instance}Query
    • TableColumn
    • Assignment
    • Gate
    • ConstraintSystem
    • And many smaller types used as auxiliary types by some of the previous types/functions
  • Add legacy wrappers to read/write setup keys that work with the Circuit trait: pk_read and vk_read in halo2_proofs
  • Split the implementation of converting selectors to fixed columns into two parts:
    • One part just converts the ConstraintSystem, transforming the selectors into fixed columns (compressed and direct versions)
    • The other part transforms the assignments of selector columns into assignments of fixed columns based on the mappings calculated in part one.
    • Add a flag in ConstraintSystem that marks when the transformation of selectors to fixed has been done, so that it panics if the transformation is attempted a second time.

Related issue: #266

NOTE: I'm not marking as breaking the changes that affect the frontend-backend split, only the changes that affect the legacy API, because we still haven't made any release with the frontend-backend split.

Blocked by #289

- Rewrite VerifyingKey and ProvingKey methods in terms of the
  ConstraintSystemBack so that the backend becomes independent of the
  Circuit trait (which belongs to the frontend)
- Add `vk_read` and `pk_read` legacy functions in halo2_proofs for
  compatiblity.
- Split the implementation of converting selectors to fixed columns into
  two parts:
  - One part just converts the ConstraintSystem, transforming the
    selectors into fixed columns (compressed and direct versions)
  - The other part transforms the assignments of selector columns into
    assignments of fixed columns based on the mappings calculated in
    part one.
…value, add safety check

In selectors_to_fixed_compressed and selectors_to_fixed_direct add
safety check via the ConstraintSystem.selectors_to_fixed status field
such that the methods can only be called once.  Calling them multiple
times would lead to an invalid ConstraintSystem with unused duplicated
fixed columns.
@ed255 ed255 marked this pull request as ready for review March 8, 2024 12:23
@ed255 ed255 changed the title [WIP] Remove dependency to common ConstraintSystem in the backend Remove dependency to common ConstraintSystem in the backend Mar 8, 2024
@ed255 ed255 requested a review from CPerezz March 11, 2024 16:53
Copy link
Member

@adria0 adria0 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 quick review, added some comments.

halo2_backend/src/plonk.rs Show resolved Hide resolved
halo2_backend/src/plonk/circuit.rs Outdated Show resolved Hide resolved
halo2_backend/src/plonk/keygen.rs Outdated Show resolved Hide resolved
halo2_frontend/src/circuit.rs Show resolved Hide resolved
halo2_middleware/src/expression.rs Outdated Show resolved Hide resolved
Copy link

@duguorong009 duguorong009 left a comment

Choose a reason for hiding this comment

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

Great work! LGTM! 🚀

I have just one comment.
I think that the remaining modules(helpers, multicore, plonk) of halo2_common can be moved to halo2_backend. right? @ed255
Maybe, it could be better to move all of those in this PR. And remove halo2_common.

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.

Awesome work!!!!!

@ed255
Copy link
Member Author

ed255 commented Mar 18, 2024

Great work! LGTM! 🚀

I have just one comment. I think that the remaining modules(helpers, multicore, plonk) of halo2_common can be moved to halo2_backend. right? @ed255 Maybe, it could be better to move all of those in this PR. And remove halo2_common.

I didn't check in detail but I had the impression that the remaining work regarding halo2_common was small. I would prefer doing that work in a future PR just because this one touches many lines and it would be great to merge it as soon as possible to allow other work to make progress without facing conflicts.

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.

4 participants