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

GKR Gate Registry #1442

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

GKR Gate Registry #1442

wants to merge 16 commits into from

Conversation

Tabaie
Copy link
Contributor

@Tabaie Tabaie commented Mar 5, 2025

Companion to Consensys/gnark-crypto#652

  • Implement a GKR Gate registry
  • ... which includes automatic degree detection. However this is not perfect, as it always delegates the degree computation to a BN254 instantiation. If for whatever reason a gate polynomial has a leading coefficient multiple of the BN254 scalar field size, the estimated degree will be lower than the actual value. In the future we should probably use the same field as the frontend api (but that will involve a lot of code generation.)
  • Turn gkr.Gate into a struct, rather than an interface.
  • Add "feed forward" to Poseidon2 GKR for it to compute a compression function, not just a permutation.

@Tabaie Tabaie requested review from ivokub and ThomasPiellard March 5, 2025 22:22
@Tabaie Tabaie marked this pull request as draft March 6, 2025 03:42
@Tabaie Tabaie marked this pull request as ready for review March 11, 2025 15:35
@Tabaie Tabaie marked this pull request as draft March 27, 2025 00:21
@Tabaie Tabaie requested review from gbotrel and Copilot March 27, 2025 01:55
@Tabaie Tabaie marked this pull request as ready for review March 27, 2025 01:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a GKR Gate registry with automatic degree detection and updates to the GKR API. Key changes include:

  • Introducing a new settings struct for gate registration with options for degree and solvable variable verification.
  • Refactoring the bn254 wrapper API and updating tests and benchmarks to use MustSetRandom.
  • Adjusting API methods (Add, Sub, Mul) to remove varargs and use new gate names (e.g. "add2", "mul2") and registering core gates in gkr/gkr.go.

Reviewed Changes

Copilot reviewed 25 out of 27 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
std/gkr/registry.go Implements the gate registry with settings and degree detection.
std/gkr/internal/bn254_wrapper_api.go Updates the BN254 wrapper with additional API methods and error handling.
std/gkr/api_test.go Registers MiMC gates using the new registry and updates tests.
std/gkr/api.go Refactors API methods to remove varargs and use new registered gate names.
std/gkr/gkr.go Converts Gate from an interface to a struct and registers basic gates.
internal/tinyfield/vector.go & element.go Updates SetRandom usages to MustSetRandom to panic on error.
Various constraint and test files Replace direct map lookups with GetGate calls for consistency.
Files not reviewed (2)
  • go.mod: Language not supported
  • internal/generator/backend/template/representations/gkr.go.tmpl: Language not supported
Comments suppressed due to low confidence (1)

std/gkr/registry.go:82

  • Using panic for invalid settings in RegisterGate could cause abrupt termination in production. Consider returning an error instead of panicking to handle misconfigurations more gracefully.
if s.noDegreeVerification { panic("invalid settings") }

@Tabaie Tabaie requested a review from Copilot March 27, 2025 02:27
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the GKR gate handling by introducing a registry mechanism, updating the gate API from an interface to a struct, and improving the degree/solvability settings for gates. It also updates downstream code to use the new registry and replaces direct random-setting methods with panicking variants for consistency.

  • Implements a thread-safe GKR gate registry with configuration options
  • Refactors gate handling in multiple files (registry, api, tests, and constraints)
  • Replaces legacy methods with updated versions and updates calls accordingly

Reviewed Changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
std/gkr/registry.go Implements a new registry and RegisterGate function with settings
std/gkr/internal/bn254_wrapper_api.go Wraps BN254 operations using a frontend.API and adds field operation stubs
std/gkr/gkr_test.go Updates gate lookups and removes legacy gate map usage
std/gkr/gkr.go Refactors gate handling via GetGate and removes unused legacy gate types
std/gkr/compile.go Adjusts gate lookups to use GetGate with new registration
std/gkr/api.go Updates NamedGate and arithmetic function calls to use new GateName
internal/tinyfield/* Replaces SetRandom() calls with MustSetRandom() for consistency
constraint/*/gkr.go Converts legacy gate map lookups to using GetGate consistently
std/gkr/api_test.go Refactors mimc gate registration to use RegisterGate
Files not reviewed (2)
  • go.mod: Language not supported
  • internal/generator/backend/template/representations/gkr.go.tmpl: Language not supported
Comments suppressed due to low confidence (2)

std/gkr/internal/bn254_wrapper_api.go:158

  • Indexing the fr.Element pointer (x[i]) for printing may not yield the complete or intended representation of the element. Consider using a dedicated string conversion method (e.g., x.String()) to output the full element value.
toPrint[i] = x[i]

std/gkr/api_test.go:436

  • [nitpick] Multiple registrations for the 'mimc' gate are occurring in the test, which may lead to one registration overriding the other. It is recommended to consolidate these registrations to avoid potential conflicts.
panicIfError(RegisterGate("mimc", func(api frontend.API, input ...frontend.Variable) frontend.Variable {

@Tabaie Tabaie requested a review from Copilot March 27, 2025 02:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a GKR gate registry to support automatic degree detection and converts Gate from an interface to a struct. The changes include updating gate registration and retrieval mechanisms (using GetGate and GateName), modifying test cases and API functions to align with the new registry design, and adding utility functions for randomness in the tinyfield package.

Reviewed Changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
std/gkr/registry.go Implements the gate registry, degree detection, and configurable registration options.
std/gkr/internal/bn254_wrapper_api.go Wraps BN254 field arithmetic into a frontend API to support gate function conversion.
std/gkr/gkr_test.go Updates tests to use the new GetGate API and registration methods.
std/gkr/gkr.go Refactors Gate from an interface to a struct and updates gate retrieval in circuit construction.
std/gkr/compile.go & std/gkr/api.go Adapts gate naming and retrieval to use GateName rather than raw strings.
internal/tinyfield/* Introduces MustSetRandom methods to enforce error‐free random initialization.
constraint/*/gkr.go Updates circuit conversion functions to replace direct map access with GetGate calls.
Files not reviewed (2)
  • go.mod: Language not supported
  • internal/generator/backend/template/representations/gkr.go.tmpl: Language not supported
Comments suppressed due to low confidence (1)

std/gkr/gkr.go:370

  • [nitpick] For consistency and to leverage type safety, use the Identity constant (of type GateName) instead of a hard-coded string literal "identity".
c[i].Gate = GetGate("identity")

@Tabaie Tabaie requested a review from Copilot March 27, 2025 02:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new GKR Gate registry along with several updates:

  • Implements a new registry mechanism for GKR Gates including options for degree and solvability verification.
  • Refactors the Gate abstraction from an interface to a struct and updates the related API functions.
  • Updates test and internal helper functions (e.g. random element allocation) to use the new MustSetRandom variant.

Reviewed Changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
std/gkr/registry.go Implements the new gate registration API with degree options.
std/gkr/api_test.go Replaces the old MiMC gate registration with two mimc registrations.
constraint/*/gkr.go Standardizes gate lookup by refactoring direct map access to GetGate.
internal/tinyfield/vector.go Adds vector-level MustSetRandom and SetRandom helper functions.
std/gkr/gkr.go Refactors Wire and Gate types; converts Identity assignment.
std/gkr/api.go Updates API methods to use GateName instead of raw strings.
std/gkr/compile.go Updates circuit data construction to use the new gate registry.
internal/tinyfield/element_test.go Updates benchmarking code to use the new MustSetRandom functions.
internal/tinyfield/vector_test.go Updates random generation calls to use MustSetRandom.
Files not reviewed (2)
  • go.mod: Language not supported
  • internal/generator/backend/template/representations/gkr.go.tmpl: Language not supported
Comments suppressed due to low confidence (1)

std/gkr/api_test.go:437

  • The gate 'mimc' is being registered twice in this file, which may lead to unexpected behavior as the second registration will override the first. Consider using distinct names for the different mimc implementations or merging their functionality into a single registration.
panicIfError(RegisterGate("mimc", func(api frontend.API, input ...frontend.Variable) frontend.Variable {

@@ -223,7 +223,7 @@ func newCircuitDataForSnark(info constraint.GkrInfo, assignment assignment) circ
for i := range circuit {
w := info.Circuit[i]
circuit[i] = Wire{
Gate: ite(w.IsInput(), Gates[w.Gate], Gate(IdentityGate{})),
Gate: GetGate(ite(w.IsInput(), GateName(w.Gate), Identity)),
Copy link
Preview

Copilot AI Mar 27, 2025

Choose a reason for hiding this comment

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

The conversion 'GateName(w.Gate)' assumes that w.Gate is a string, but Wire.Gate has been refactored to a pointer to Gate. This may lead to a type error; consider updating the conversion logic to extract the appropriate gate name from the Gate pointer.

Suggested change
Gate: GetGate(ite(w.IsInput(), GateName(w.Gate), Identity)),
Gate: GetGate(ite(w.IsInput(), GateName(*w.Gate), Identity)),

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

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.

1 participant