feat: GKR Add Instance

[Tabaie]

• Change the GKR API so that defining the circuit and assigning instances are separated. It removes the necessity of defining hints for the output and deferred "finalize" functions.

• Remove the Println function in favor of GetValue.

• gkrgates.Register now checks if a gate of the same name has already been registered. If so, it compares the gates and returns an error if they are not equal. If they are equal, it returns false, nil.

• Remove all logic pertaining to reordering of wires and instances.

• Unless the user explicitly provides their own initial challenge, automatically use all input and output of the circuit as initial Fiat-Shamir challenge.

• Rename GkrCompressions to GkrPermutations to correctly reflect its function. (Note: a future PR should implement compressions instead and revert the name change)

• A benchmark for gkr-poseidon2

[ivokub]

I think the idea is good, makes it a bit simpler to use in some cases. But I would leave the current way to import variables for backwards compatibility to avoid breaking all downstream GKR usage. We could implement the changes in Linea ourselves, but gnark is also used elsewhere and breaking API changes essentially prevent our users to upgrade gnark easily. It also creates some issues when we want to backport some hotfixes for Linea as we would need to start maintaining separate branch preceeding the API breakage.

In general, I would recommend adding a lot more method documentation what some methods do, what are the inputs, what are the outputs and what are the assumptions which may cause the methods to return errors. Right now many methods lack documentation so it is trial and error to figure out how to use them correctly.

Also keep in mind that try to keep type visibility consistent. It goes particularly for options -- when you have an option type and methods which return options then both the option type and these methods should be public. A la

type cfg struct {} // dont make it public unless you need to use configuration in other package

type Option func(*cfg) error // make Option type public, as it then also shows it in package documentation and groups all methods returning options together. NB! Don't forget to add documentation what the options are for, where you can use it etc.

func WithDoThis() Option { // function should be public so that can see in the pkg documentation. NB! Don't forget to add documentation what the method does, what is the behaviour if it is not set and what are conflicting options
    return func(conf *cfg) error {
        // now check here if this option is not conflicting with some existing configuration. A la when cfg has fields `doThis, doThat bool` then you should check that `doThat` is not already set and if it is then return an error
       if conf.doThat {
           return errors.New("WithDoThat option already set")
       }
   }
}

I reviewed mostly the coding part - it has become quite difficult to understand how the full GKR implementation works end to end and if it is sound, as there are already quite a lot of changes compared to the audited version. I'd add soundness tests i.e. what happens if you try to explicitly modify GKR outputs so that they are not consistent with the expected output. Or what happens if you overwrite GKR hints to modify the results.

[ivokub]

It would be nice to make registerOption public. Otherwise it is a bit strange that you have a public method which returns instance of private type. It works but not imo idiomatic Go.

[ivokub]

And it usually is better to make registerOption as func(*registerSettings) error to return errors. It seems that some options are incompatible with each other and it would be good to detect during option parsing.

[ivokub]

For gate degree testing - do we need an option at all? Cannot we just take some "good" modulus and test over that field. For me it makes a bit confusing that the user can choose the curve, but then needs to know the relation between the gate degree and modulus.

[ivokub]

Why do we need to return boolean to indicate if the gate was registered or not? When you return an error, then it is always false. And when the gate is already registered with same properties, then it is a no-op.

It seems you only use it for testing, I think it doesn't make sense to break APIs and add extra return values you don't use outside of tests.

[ivokub]

I think it is not idiomatic Go to compare errors by messages. If you really want to ensure that some error is what you expect, then define a new error type

var errSomeKnownError = errors.New("error msg")
///

err := functionThatReturnsErrors()
errors.Is(err, errSomeKnownError)

or if you want to add context to error then implement error interface

type parametricError struct { name string }
func (pe parametricError) Error() string {
    return fmt.Sprintf("error with parameter: %s", pe.name)
}

func functionThatReturnsErrors() error {
    return &parametricError{name: "aaaa"}
}

err := functionThatReturnsErrors()
var wantedError *parametricError
if errors.As(err, &parametricError) {
    // error is *parametricError
}

[ivokub]

Should be public type if With... options are public. Also function should return errors and you should check that the options are not incompatible with each other.

[ivokub]

I would keep Import method for backwards compatibility. Otherwise you really break all downstream usage of GKR and do not give good migration path for upgrading gnark as the only option is to rewrite all GKR circuits. And I think keeping Import is trivial as you could just do loop of AddInstance inside.

[ivokub]

Check if the cursor comment makes sense. I do not follow why you are comparing against len(c.ins)

[ivokub]

Imo it is difficult to follow what is happening here. I need to know what MapRange and slicePtrAt does, but neither have documentation and slicePtrAt returns a function. I really recommend just doing small loops inline, otherwise I need to jump several times to see differeunt function definitions to understand what is going on and why.

[ivokub]

So, for example, here what you do I think is:

forSnarkSorted := make([]*gkrtypes.Wire, len(c.ToStore.Circuit))
for i := range forSnarkSorted {
    forSnarkSorted[i] = &forSnark.circuit[i]
}

and for me it is a lot more understandable and explicit. Now, you only use utils.MapRange and slicePtrAt only once, exactly here. So instead of four lines of explicit implementation there are several function definitions and more difficult reviewing.

[Tabaie]

Changed to one (documented) utility function rather than two.

[cursor]

Bug: Empty Slice Padding Causes Out-of-Bounds Access

The ExtendRepeatLast function, WireAssignment.RepeatUntilEnd method, and the inline padding logic in NewSolvingData all share a bug. When the slice or segment being padded is empty, their loops attempt to access an element at index j-1 where j is 0, leading to an out-of-bounds panic.

Additional Locations (2)

• internal/gkr/bls12-377/solver_hints.go#L74-L77
• internal/utils/slices.go#L24-L31