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

refactor: allow builder to interact with schema parser #332

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Jun 23, 2022

In the upcoming fake-API for field emulation branch I have implemented a wrapper for frontend.Builder which initialises the variables in the circuit witness. The builder also needs to modify the parsed schema (for example, we need a native element for every limb). For that, the LeafHandler now takes as input *schema.Field which it can modify in-place.

I think this PR may not be optimal. Will try to rebase fake-API branch on top of this one to see how it would be used in practice.

@ivokub ivokub added this to the v0.8.0 milestone Jun 23, 2022
@ivokub ivokub requested a review from gbotrel June 23, 2022 23:11
@ivokub ivokub self-assigned this Jun 23, 2022
Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

looks OK to me, not sure where this is going but doesn't look like to be introducing any regressions 👍

// VariableCount returns the number of native elements required to represent
 	// the given reflected type as a witness.
 	VariableCount(reflect.Type) int

Don't understand why this is in Builder interface.

@ivokub
Copy link
Collaborator Author

ivokub commented Jun 28, 2022

looks OK to me, not sure where this is going but doesn't look like to be introducing any regressions 👍

Just in case on how I would be using it. When I have defined variables in a circuit as type frontend.Variable (for compatibility purposes), but in practice them with nonnative.Elements, then I need to modify the schema field in-place in Builder.Add{Public/Secret}Variable method. See for example https://github.com/ConsenSys/gnark/blob/1fcebc0dcae8a0d7f06c9efe869834141cdc94be/std/math/nonnative/api.go#L62. I'm modifying in-place the schema field from Leaf type to Slice type to account that nonnative element needs several variables for storing the limb values.

// VariableCount returns the number of native elements required to represent
 	// the given reflected type as a witness.
 	VariableCount(reflect.Type) int

Don't understand why this is in Builder interface.

So, if I wrap the native builder (r1cs or scs) for using non-native field emulation (nonnative.Builder), then I need several native elements/circuit variables to represent nonnative elements. Currently compiler runs schema parser in two rounds. In first round to get number of (public/secret) wires and to shift the wire IDs correctly when initialising variable in second round. I didn't see another nice option to count the number of variables properly otherwise.

For example usage, see https://github.com/ConsenSys/gnark/blob/1fcebc0dcae8a0d7f06c9efe869834141cdc94be/std/math/nonnative/api.go#L37

Or actually, maybe I could do without Builder.VariableCount, but then I would have to compute the wire IDs lazily or somehow remove the necessary shifting for private variables?
I can probably figure it out but don't know if it makes sense. What do you think?

@gbotrel
Copy link
Collaborator

gbotrel commented Jun 28, 2022

VariableCount is fine, this is not a user-facing API, so it's OK if we end up refactoring in the future 👍

@ivokub
Copy link
Collaborator Author

ivokub commented Jun 28, 2022

VariableCount is fine, this is not a user-facing API, so it's OK if we end up refactoring in the future 👍

👍. But I slept on it over the weekend and it may not cover all possible use cases in field emulation. I'll try before merging.

@ivokub ivokub merged commit e4b5289 into develop Jul 6, 2022
@ivokub ivokub deleted the refactor/schema-parsing branch July 6, 2022 10:26
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.

2 participants