feat: hint name options #666

Tabaie · 2023-04-30T00:50:45Z

This ~~is more of an issue than~~ as of now a real PR! ~~, in that the proposed API modifications are not final and worthy of debate, but for the multi-commit feature I needed something quick and dirty that works.~~

This "PR" enables defining a hint with a user-chosen ID. This is useful in the context of multi-commits when the number of deferred hints is not known at compile time, so no number of placeholder functions can do the job.

ivokub · 2023-05-02T09:47:38Z

Do I understand correctly that the named hints are essentially unnamed hints but we allow to have some additional context within a hint? But right now the implementation doesn't seem to get the context?

Tabaie · 2023-05-02T16:35:06Z

Do I understand correctly that the named hints are essentially unnamed hints but we allow to have some additional context within a hint? But right now the implementation doesn't seem to get the context?

All this does is that it enables the user to give a hint a name (and ID) that is not (derived from) the name of the function used to define the hint. Its motivating use is in multi-commits where there are a number of to-be-replaced hints which is not known at compile time.

p4u · 2023-05-03T09:08:51Z

This is great, we implemented a very similar approach here vocdoni@951aa9e

In my opinion, all hints should be named hints. The use of the reflect package to construct the hintID has two issues:

If the gnark code is forked to another repository, the hints are not compatible among both implementations
Currently, tinygo does not support the Reflect pointer mechanism that is used to extract the package and function name of the hint, so it creates issues when building Gnark for Webassembly

ivokub · 2023-05-03T13:58:37Z

Makes sense. Also @gbotrel is imo right now working on alternative representation for hints. I think there is some overlap, but also some contradictions. There is also another open PR #659 (just FYI, not sure if affects this PR).

But if we start having named hints, then does it make sense to provide solver.Hint function at all to api.NamedHint call? During compile time we mainly used it for the UUID computation and if we give a name, then we don't really need it actually. Imo right now we only need the implementation is for the test engine, but can think about it separately. Otherwise we would pass in some empty stubs (a la like we do for the single-commitment right now before prover monkey-patches the implementation).

I actually really like named hints because it makes clearer the separation that from circuit definition point of view the result can be "anything". Right now because we provide the implementation then a lot of people assume that the hint output is some deterministic value.

Actually, I'm thinking maybe when trying to improve hints, we should have a way to pass some state data between the hints? For example, we could have hints like:

func statefulHint(ctx map[any]any, mod *big.Int, inputs []*big.Int, outputs []*big.Int) error {
   // hint checks if there is anything in ctx. If not, then this is first invocation.
   // after it has performed the computations, this hint stores its state in ctx
   // now, in the second invocation of this hint, this function sees that there already is some context here and uses it to compute the result
}

Tabaie · 2023-05-04T15:48:01Z

In my opinion, all hints should be named hints.

But if we start having named hints, then does it make sense to provide solver.Hint function at all to api.NamedHint call? During compile time we mainly used it for the UUID computation and if we give a name, then we don't really need it actually. Imo right now we only need the implementation is for the test engine, but can think about it separately.

Very much agreed @p4u @ivokub. So how about this instead:

NewHint(id solver.HintID, nbOutputs int, inputs ...Variable) ([]Variable, error)

Actually, I'm thinking maybe when trying to improve hints, we should have a way to pass some state data between the hints?

I like this idea because it might make things like commitments and the GKR widget much simpler (though @gbotrel said for the latter Blueprints would be useful, I don't understand Blueprints well enough yet.) What are your particular use-cases @ivokub? I feel a bit hesitant doing something this radical because of added complexity (both in implementation and use) but if there are many good applications perhaps we can start thinking about it seriously.

ivokub · 2023-05-04T16:23:03Z

Nope, I don't have particular application. I think it could have helped to solve the blowup in the lookup table queries, but this can also be solved using Blueprints.

On a very high level a blueprint is a low-level thingie which takes some inputs and gives some outputs. But it has to figure out its serialisation on its own (it gets and returns a list of uint32 what the solver does and it has to figure out what to do with the uint32 - get some wire values etc.). As such, blueprints are a lot more performance efficient, but I think not that intuitive for the developers.

Tabaie · 2023-05-04T18:23:37Z

Nope, I don't have particular application. I think it could have helped to solve the blowup in the lookup table queries, but this can also be solved using Blueprints.

On a very high level a blueprint is a low-level thingie which takes some inputs and gives some outputs. But it has to figure out its serialisation on its own (it gets and returns a list of uint32 what the solver does and it has to figure out what to do with the uint32 - get some wire values etc.). As such, blueprints are a lot more performance efficient, but I think not that intuitive for the developers.

In that case it seems best to me to use blueprints for our internal use-cases and keep the hint changes minimal. What do you think? Also what do you think of simply replacing the solver.Hint parameter with a solver.HintID?

ivokub · 2023-05-04T22:57:02Z

Nope, I don't have particular application. I think it could have helped to solve the blowup in the lookup table queries, but this can also be solved using Blueprints.
On a very high level a blueprint is a low-level thingie which takes some inputs and gives some outputs. But it has to figure out its serialisation on its own (it gets and returns a list of uint32 what the solver does and it has to figure out what to do with the uint32 - get some wire values etc.). As such, blueprints are a lot more performance efficient, but I think not that intuitive for the developers.

In that case it seems best to me to use blueprints for our internal use-cases and keep the hint changes minimal. What do you think? Also what do you think of simply replacing the solver.Hint parameter with a solver.HintID?

Completely agree. Lets keep the changes minimal. But I would still prefer adding a new method NewNamedHint instead of modifying NewHint. Imo hints are right now quite used, potentially even externally, and I wouldn't want to break interface.

For NewNamedHint we can give as an argument solver.HintID as you suggested. And in that case I'm not sure we need to add options to NewHint and NewNamedHint because it is already captured in solver.HintID, no?

Tabaie · 2023-05-05T14:46:58Z

For NewNamedHint we can give as an argument solver.HintID as you suggested. And in that case I'm not sure we need to add options to NewHint and NewNamedHint because it is already captured in solver.HintID, no?

Yes I think we can get rid of the options 👍

gbotrel

lgtm, but NewNamedHint confuses me a bit as a name.

gbotrel · 2023-05-09T19:18:54Z

constraint/core.go

 	if nbOutput <= 0 {
 		return nil, fmt.Errorf("hint function must return at least one output")
 	}

+	// TODO @Tabaie @gbotrel consider getting rid of hint names entirely


names are good to derive human readable error messages if hints are missing or duplicate, etc ?

gbotrel · 2023-05-09T19:24:15Z

constraint/system.go

@@ -46,7 +46,8 @@ type ConstraintSystem interface {

 	// AddSolverHint adds a hint to the solver such that the output variables will be computed
 	// using a call to output := f(input...) at solve time.
-	AddSolverHint(f solver.Hint, input []LinearExpression, nbOutput int) (internalVariables []int, err error)
+	// Providing the function f is optional.
+	AddSolverHint(f solver.Hint, id solver.HintID, input []LinearExpression, nbOutput int) (internalVariables []int, err error)


"Providing the function f is optional." --> describe a bit more what happens (ie if f == nil, then ... )

gbotrel · 2023-05-09T19:26:47Z

frontend/builder.go

@@ -39,6 +39,7 @@ type Compiler interface {
 	//
 	// If nbOutputs is specified, it must be >= 1 and <= f.NbOutputs
 	NewHint(f solver.Hint, nbOutputs int, inputs ...Variable) ([]Variable, error)
+	NewNamedHint(id solver.HintID, nbOutputs int, inputs ...Variable) ([]Variable, error)


the name of the API confuses me a bit;
it reads NewNamedHint but takes as parameter a HintID

Essentially it says "give me nbOutputs variable, and tell the solver to computes them with a hint that'll be provided at solve time with id == id" right?

Essentially it says "give me nbOutputs variable, and tell the solver to computes them with a hint that'll be provided at solve time with id == id" right?

Exactly.

it reads NewNamedHint but takes as parameter a HintID

You're right about that. Open to suggestions for a better name, couldn't think of one myself. Maybe NewDeferredHint but then again that's a bit misleading since all hints are deferred except for in test.engine

feat: hint name options

fac1c62

Tabaie requested review from gbotrel and ivokub April 30, 2023 00:50

Tabaie added 4 commits May 8, 2023 19:59

refactor: r1cs NewNamedHint not taking hint func

0b4ed13

refactor: NewNamedHint not taking hint function input

7320d89

refactor: remove HintIds struct

59d12aa

Merge branch 'develop' into feat/hint-naming-options

49565ab

Tabaie marked this pull request as ready for review May 9, 2023 16:27

fix: newNamedHint bug

1586fa6

gbotrel approved these changes May 9, 2023

View reviewed changes

Tabaie added 2 commits May 9, 2023 14:57

docs: explain the optionality of f in AddSolverHint

81bc271

refactor: renaming as per robot overlords

d3b97f9

Tabaie merged commit 4d8b283 into develop May 9, 2023
5 checks passed

Tabaie deleted the feat/hint-naming-options branch May 9, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hint name options #666

feat: hint name options #666

Tabaie commented Apr 30, 2023 •

edited

ivokub commented May 2, 2023

Tabaie commented May 2, 2023

p4u commented May 3, 2023

ivokub commented May 3, 2023

Tabaie commented May 4, 2023

ivokub commented May 4, 2023

Tabaie commented May 4, 2023

ivokub commented May 4, 2023

Tabaie commented May 5, 2023

gbotrel left a comment

gbotrel May 9, 2023

gbotrel May 9, 2023

gbotrel May 9, 2023

Tabaie May 9, 2023 •

edited

gbotrel May 9, 2023

feat: hint name options #666

feat: hint name options #666

Conversation

Tabaie commented Apr 30, 2023 • edited

ivokub commented May 2, 2023

Tabaie commented May 2, 2023

p4u commented May 3, 2023

ivokub commented May 3, 2023

Tabaie commented May 4, 2023

ivokub commented May 4, 2023

Tabaie commented May 4, 2023

ivokub commented May 4, 2023

Tabaie commented May 5, 2023

gbotrel left a comment

Choose a reason for hiding this comment

gbotrel May 9, 2023

Choose a reason for hiding this comment

gbotrel May 9, 2023

Choose a reason for hiding this comment

gbotrel May 9, 2023

Choose a reason for hiding this comment

Tabaie May 9, 2023 • edited

Choose a reason for hiding this comment

gbotrel May 9, 2023

Choose a reason for hiding this comment

Tabaie commented Apr 30, 2023 •

edited

Tabaie May 9, 2023 •

edited