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

feat: add defer to the Compiler interface #483

Merged
merged 5 commits into from Feb 28, 2023
Merged

feat: add defer to the Compiler interface #483

merged 5 commits into from Feb 28, 2023

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Feb 21, 2023

See discussion in #472. This PR adds PreCompileHook(cb func(API) error) method to the frontend.Compiler. The intent of the method is to register all callback which should be called after the Define method of the circuit is called. These callbacks can be used by gadgets to safely defer any necessary closing tasks (e.g. range checking builds actual constraints, permutation networks build the permutations etc.) without requiring any interaction from the user.

I don't like the implementation though, I think it is really fragile. However, the problem is that I do not want to make frontend.Compile interface any more complex than it barely should, and this also means that there isn't a method to get all the callbacks which the frontend.Compile method can use. That is why I have embedded calling the callback in every builder's Compile method. But this is really really ugly solution.
A better way would definitely be to make list of all callback available to frontend.Compile without making frontend.Compiler interface bigger. One option, for example, would be to use the key-value store with some internal key. For example the implementation for registering callback:

// type CallbackKey is in frontend/internal/XXX

func (b *builder) PreCompileHook(cb func(frontend.API) error) {
    cbs := b.GetKeyValueStore(CallbackKey{})
    b.SetKeyValueStore(callbackKey{}, append(cbs, cb))
}

and then in frontend.Compile:

function Compile() {
// ...
    cbs := builder.GetKeyValue(CallbackKey{})
    // call all callbacks
    builder.Compile()
}

@ivokub ivokub added this to the v0.9.0 milestone Feb 21, 2023
@ivokub ivokub requested a review from gbotrel February 21, 2023 00:35
@ivokub ivokub self-assigned this Feb 21, 2023
@gbotrel
Copy link
Collaborator

gbotrel commented Feb 21, 2023

follow up on our discussion; my vote is to call it api.Compiler().Defer(...) . I think it's OK to assume users calling a method on a compiler object should read the doc (ie that Defer on the compiler object is not limited to the scope of a function like go defer ) .

@ivokub
Copy link
Collaborator Author

ivokub commented Feb 21, 2023

follow up on our discussion; my vote is to call it api.Compiler().Defer(...) . I think it's OK to assume users calling a method on a compiler object should read the doc (ie that Defer on the compiler object is not limited to the scope of a function like go defer ) .

Makes sense. I also think it seems nicer.

I'll also try to append all the deferred methods into the key-value storage to allow frontend.Compile to call the callbacks directly instead of doing in builder.

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.

lgtm, w/ PreCompileHook(..) --> Defer(...)

@ivokub
Copy link
Collaborator Author

ivokub commented Feb 23, 2023

lgtm, w/ PreCompileHook(..) --> Defer(...)

Renamed, but the implementation still doesn't feel right. I'll try with the kvstore to see if it works better. But need to rebase on unmerged PR.

@ivokub ivokub marked this pull request as draft February 23, 2023 00:59
This was used to create API for field emulation. But we removed the fake-API
interface in favor of dedicated emulated field type.
@ivokub ivokub changed the title feat: add PreCompileHook to the Compiler interface feat: add defer to the Compiler interface Feb 23, 2023
@ivokub
Copy link
Collaborator Author

ivokub commented Feb 23, 2023

It is done but rebased on top of #480 (kvstore PR, not yet merged). Imo looks a bit nicer and I think a bit more future-proof.

@ivokub ivokub marked this pull request as ready for review February 23, 2023 22:55
@ivokub ivokub requested a review from gbotrel February 23, 2023 22:55
@gbotrel gbotrel merged commit 458e397 into develop Feb 28, 2023
@gbotrel gbotrel deleted the feat/precompile branch February 28, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants