Skip to content

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 ThomasPiellard and ivokub 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 Copilot and gbotrel March 27, 2025 01:55
@Tabaie Tabaie marked this pull request as ready for review March 27, 2025 01:55
Copy link
Contributor

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 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 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 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 {

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

See the comments. Otherwise LGTM

@ivokub
Copy link
Collaborator

ivokub commented Apr 2, 2025

Also please run gofmt/goimports on the sources. I randomly ran it on std/gkr/registry.go and it changed import order. Otherwise there may be unrelated changes in other PR in the future which make reviews more complicated.

@Tabaie
Copy link
Contributor Author

Tabaie commented Apr 2, 2025

Merging for the Poseidon2 fix. More PR feedback addressed in future PR.

@Tabaie Tabaie merged commit a0e0056 into master Apr 2, 2025
5 checks passed
@Tabaie Tabaie deleted the chore/gkr-gateregistry branch April 2, 2025 20:45
ivokub pushed a commit that referenced this pull request Apr 7, 2025
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.

3 participants