-
Notifications
You must be signed in to change notification settings - Fork 131
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
Frontend-Backend split #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tmp removal of the thread-safe-region
makes sense.
We should anyways fill an issue to re-address it after this is merged.
I created issues for all the tasks that need to be done after this is merged, I hope I didn't forget anything! This is the issue for the |
Thanks for putting all of these together!!! |
4fa7f4e
to
4db6980
Compare
4db6980
to
fb2e556
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ed255 Why we need middleware plonk module if is not used by CompiledCircuit? ( AFAIU the main goal of middleware is to hold a CompiledCircuit
)
The halo2/halo2_backend/src/plonk/prover.rs Lines 258 to 267 in 25666a4
The reason for having Update: I've removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
Awesome work mate!!! <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job, @ed255 !
Simplify the prover API by receiving the witness as Vec<F> instead of Vec<Assigned<F>>. This moves the batch inverstion from the backend to the frontend, so that the backend directly receives the final witness values. This change requires updating the batch_invert_assigned method to work on Vec instead of Polynomial.
This PR builds on top of #243 which now has become outdated. I haven't removed the original PR yet because it can be useful for reviewing as it contains a much smaller diff.
The main change in this PR from the previous one is moving all the code into different crates:
name = "halo2_frontend"
: the library to write circuits, calculate witness and MockProvername = "halo2_backend"
: the library that implements the proof systemname = "halo2_middleware"
: the traits and structs used between frontend and backendname = "halo2_common"
: shared parts between frontend and backendname = "halo2_proofs"
: lightweight layer + re-exports to make it easy to update a halo2 dependency to this new version, without any API changeIdeally the
common
crate shouldn't exist; but there are many parts of the halo2 implementation that share structs in the frontend and backend. For example, the Polynomial type is used to calculate commitments in the backend, but also to hold the witness assignment during witness generation in the frontend. The plan is to move as many things as possible fromcommon
to eitherfrontend
orbackend
, ideally reaching a point wherecommon
disappears. But that takes some effort so I would request to leavecommon
in this PR and iterate it further in other PRs.Big changes that are not just moving code around in this PR:
permutation::keygen::Assembly
calledAssemblyFront
incommon/src/plonk/permutation.rs
to be used by the frontend. Related to [Preview - historical] Frontend-Backend split #243 (comment) . This type now just holds a vector of pairs of cells that need to be copied, without any further processing. All the mappings and cycles are calculated by the backend. This also required a change in the MockProver, as it now has direct access to pairs of copy cells, simplifying the error checking.Cargo.toml
. Should we also add some entry to include the community?thread-safe-region
permutation Assembly inbackend/src/plonk/permutation/keygen.rs
. I plan to bring this back later. See [post frontend-backend split] Bring back thread-safe-region permutation Assembly #258ChallengeMid
inmiddleware/src/circuit.rs
and moved originalChallenge
tocommon/src/plonk/circuit.rs
. This is becauseChallenge
has a public API which returnsExpression
is is not middleware; soChallengeMid
is a lightweight version ofChallenge
that can be converted toChallenge
to be used in the frontend/backend.ColumnMid
inmiddleware/src/circuit.rs
which follows the same reasoning asChallengeMid
, to not depend onExpression
in middleware.Note: I'm not sure what's the best way to review this PR, but I certainly think that looking at the entire diff is not the best approach! The majority of the changes are just moving things around, the actual changes can be reviewed in #243
Missing things
Cargo.toml
halo2_proofs_rm
. This is just a copy of halo2 before the crate split, which I'm using for reference while I'm working on this.allow(unused_code)
and such. I added these lines to make progress faster; once everything builds and all tests pass, I'll work on thatpub(crate)
where necessary. Since some types are now imported from a different crate, they have becomepub
frompub(crate)
. To make my life easier I made a lot of stuff justpub
while moving things around. Once types and functions are settled, I need to addpub(crate)
where necessary. [post frontend-backend split] Review pub types / fields #259halo2_proofs
for the public facing crates:pub
to make progress faster while I was figuring out where to put things; but the end goal is that user facing types don't expose internals if possible (following the original design of halo2) [post frontend-backend split] Review pub types / fields #259pub(crate)
where appropriate, write the documentation for the new public functions/types that don't have it. [post frontend-backend split] Review pub types / fields #259use
imports will be broken)poly
to backend [post frontend-backend split] Move poly to backend #257thread-safe-region
permutation Assembly [post frontend-backend split] Bring back thread-safe-region permutation Assembly #258