Feat/gkr api #443
Feat/gkr api #443
Conversation
There was a problem hiding this comment.
Mhhh it's a bit hard to review as is, and I think couple of things are needed first.
- lots of pieces should be in
gnark-crypto. I don't understand why we define gates here, nor do we deal with memory pool and worker pool at this level. - in the same spirit the "GkrGateRegistery" and "HashBuilderRegistry" are not documented, not sure I understand the initial objective.
- given that
GKRjust implementAdd/Sub/MulI see very little benefit to be a "fullblown"frontend.API. we could simplify / improve readability quite a lot by having "typed" object inside thegkrpackage instead of dealing withfrontend.Variablethere. - a "end-to-end" how-to-use gkr example for a new comer (in a _test.go file,
ExampleXX(...)would be very useful. That would help understand the proposition here; i.e "here is an example with aMiMCgate (defined in gnark-crypto right?) -> here is how you build the GKR circuit, how you put it in a gnark circuit". And "here is how you would define your own gate or compose more gates... or whatever". -
GKRInfoand other structs inconstraint/package lack documentation; semantically, I don't think aConstraintSystemshould have theAddGkrandGkrInfobits. It would be cleaner if the GKR circuit description ends up in ...constraint/gkrmaybe? I see theconstraint/andConstraintSystemas representing a list of mathematical constraints. The GKR bits seem to add too much here, with gate description and logic that is closer to... the frontend / "how to write a circuit". - some key functions on the
gkr.APIare not documented. When I readgkr := gkr.NewApi(); gkr.Import(...)it's hard to recall whatImportdoes :) - probably taking its roots from
gnark-cryptobut the "proofSerialization" business is a bit odd; in couple of places found someinterface{}and usage ofreflectpackage to optimistically convert tofrontend.Variable. Objects used in test cases should be exported by a gnark-crypto pacakge and defined only once.*big.Intplays well withMarshalJSONso no need to putinterface{}and do hand-made interpretations. (ie the main thought behind that is I find a lot of code is polluting the logic and could go away)
|
Worker and memory pools are large, reusable resources. That's why I thought it made sense for them to be shared between the solver and prover. Maybe the performance benefit is insignificant, I haven't benchmarked with and without yet, but in that case we can just kill it. Otherwise would a more opaque |
|
GKR is built on a circuit representation of computation, rather than linear constraints etc. That's why there are gates on the gnark side. |
|
The HashRegistry and GateRegistry are imperfect solutions to the problem that every specification in GKR world has to be duplicated once in pure Go (prover side) and once in a SNARK (verifier side.) For example, when the API user writes Similarly for hashes, the user specifies to the function Lmk if that makes sense, if so I'll add it to docs. |
If we decide there isn't enough value in being able to pass a GKR API into a gadget, we can kill this (though it would make me very sad, I just think it's a really cool idea.) |
Well, thing is, do we need to? I think it makes more sense to do a minimal stuff with add / mul / sub, a clear way (as simple as possible) to do the original goal (hash (MiMC / Poseidon maybe) computation). Have a small code size footprint, less tests to write, less code & doc to maintain, specially for the upcoming refactoring that PlonK + custom gates may introduce. For the |
|
Yes I think allowing custom gates has a significant performance implication (Will test on MiMC soon.) |
|
|
refactor: gkrAPI is no longer a frontend.API
An API for GKR to make it easy to use end-to-end and usable inside gadgets.