-
Notifications
You must be signed in to change notification settings - Fork 504
feat: Enable multiple GKR sub-circuits #1661
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
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ivokub
left a comment
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.
I still think the automated gate degree testing during registration makes the implementation a bit too complex. Seems ok implementation wise, but I'm not fully confident.
And imo restricting the gates being only used when compiled on particular curves adds also unnnecessary complexity (and most of the times we provide gnark.Curves() everywhere). For example in Poseidon2 package we correctly error out if someone wants to call the circuit on mismatching curve, so the assertion is already done.
For me gate registration is still way too verbose. Instead of
gkrgates.Register(extKeyGate(&p.RoundKeys[round][varIndex]), 2, gkrgates.WithUnverifiedDegree(1), gkrgates.WithUnverifiedSolvableVar(0), gkrgates.WithName(gateNames.linear(varIndex, round)), gkrgates.WithCurves(ecc.BLS12_377))
imo it makes so much more sense if I could do
gkrgates.Register(xtKeyGate(&p.RoundKeys[round][varIndex]), 2 /*inputs*/, 1 /*degree*/, "NAME-R1-V2")
And the one-by-one instance addition. Makes sense, but have you checked the profiles how does it affect circuit compile/solve time? I can imagine when doing one by one there is overhead?
I can try working on the PR tomorrow to wrap it finally up.
Note: deserialization tests are failing due to a change in the structure of constraint systems. Rather than a single "gkr info" sub-object, we now have a slice of them.
Note
Enables multiple GKR sub-circuits per constraint system and modernizes the GKR pipeline (hints, registry, and solving/proving) with clearer APIs and per-curve support.
GkrInfowith[]*gkrinfo.StoringInfo; addSystem.NewGkr()returning a*StoringInfoand sub-circuit index; update all systems and builders accordinglygkrtypes.NewSolvingInfoand per-curveNewSolvingData; override hints viagkrhints.TestEngineHints(GetAssignment,Solve,Prove) instead of stored IDs; pad instances to power-of-two and adjust hint I/O formatsGetAssignmentHint, refactorSolveHint/ProveHintto accept multiple circuits, remove worker pool, addrepeatUntilEnd, and derive Fiat–Shamir challenges with canonical byte parsingRegisteroptions, curve-allowlist support, function equality checks, and stricter duplicate/name/degree handling; exposeGetand refactor verifier APINewCircuit/NewSolvingInfo, gateSupportsCurve, and per-curve defaults; remove dependency/print machinery; tweak circuit metadata (setNbUniqueOutputs)gkr.ComputeLogNbInstances, fixMulAccto multiply, add utils (ExtendRepeatLast, slice refs), update generator templates and testsWritten by Cursor Bugbot for commit 9227b0c. This will update automatically on new commits. Configure here.