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

bug: Go 1.22 alternates naming of anonymous functions #1045

Closed
ivokub opened this issue Feb 8, 2024 · 5 comments · Fixed by #1054
Closed

bug: Go 1.22 alternates naming of anonymous functions #1045

ivokub opened this issue Feb 8, 2024 · 5 comments · Fixed by #1054
Assignees
Labels
bug Something isn't working

Comments

@ivokub
Copy link
Collaborator

ivokub commented Feb 8, 2024

Description

For tracking which hints to be used we construct function name and hash it to get a short identifier for the function. However, for anonymous functions var func = func( ...) the naming convention changed from github.com/consensys/gnark/std/...pkg..glob.func1 to github.com/consensys/gnark/std/...pkg.init.func1. As the hint IDs are also embedded inside the compiled circuits then this means that the solver may not resolve the hint implementation.

This issue tracks the process and to let the users know of the potential bugs.

We already have implemented PR #1043 to not use anonymous function as hints. But I guess for more safer approach we could add a rewrite in gnark to rewrite .init.funcN to ..glob.funcN.

@ivokub ivokub added the bug Something isn't working label Feb 8, 2024
@ivokub ivokub self-assigned this Feb 8, 2024
@gbotrel
Copy link
Collaborator

gbotrel commented Feb 13, 2024

@ivokub can you do a quick PR with

.init.funcN to ..glob.funcN

? And a non regression test; don't want to hit the same issue in another deployment.
--> I'm not sure #1043 isn't dangerous; it means circuits compiled before #1043 are not compatible with a process compiled post #1043 right ?

@ivokub
Copy link
Collaborator Author

ivokub commented Feb 13, 2024

@ivokub can you do a quick PR with

.init.funcN to ..glob.funcN

? And a non regression test; don't want to hit the same issue in another deployment. --> I'm not sure #1043 isn't dangerous; it means circuits compiled before #1043 are not compatible with a process compiled post #1043 right ?

Indeed #1043 breaks compatibility between pre- and post 1043 circuits. So if we have a circuit compiled pre-1043, then solver/prover post-1043 fails as the hints are not found.

But still, I think 1043 is better for long-term as ensures more consistency. Instead of reverting I think it would better to add explicit hint overrides (for both Go <1.22 and =1.22 naming convention) to keep the backwards compatibility. I'd be happy to do it if makes sense.

@gbotrel
Copy link
Collaborator

gbotrel commented Feb 13, 2024

Mmhhh no for the hint overhide, I think it's dirty ^^.
For Linea we can just recompile the circuits -- if that's the only change and the actual constraints are unchanged it's not a big issue.

@ivokub
Copy link
Collaborator Author

ivokub commented Feb 13, 2024

Yup, didn't change the constraints. Only changed hints defined as

var Hint = func(mod *big.Int, inputs []*big.Int, outputs []*big.Int) error {
//
}

to

func Hint(mod *big.Int, inputs []*big.Int, outputs []*big.Int) error {
//
}

in std/algebra packages.

@ivokub
Copy link
Collaborator Author

ivokub commented Feb 13, 2024

So to summarize - I'll only add the rewrite from "new-style anonymous functions" to old-style and a regression test which ensures that GetHintName is static for both anonymous and explicit function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants