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

fix: gkr add gate evaluate #490

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

BeratOz01
Copy link
Contributor

Description

This PR addresses a bug in the gkr package related to the Evaluate function of the AddGate. The existing logic within the AddGate evaluation function had an error that caused incorrect addition behavior under certain input scenarios. In addition to fixing the bug, this PR includes related tests as well.

func (g AddGate) Evaluate(x ...fr.Element) (res fr.Element) {
switch len(x) {
case 0:
// set zero
case 1:
res.Set(&x[0])
case 2:
res.Add(&x[0], &x[1])
for i := 2; i < len(x); i++ {
res.Add(&res, &x[2])
}
}
return
}

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

How has this been benchmarked?

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ivokub ivokub self-requested a review March 4, 2024 23:23
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Good catch, thanks! However, it seems the same issue is for the MulGate (we multply always x[2] when more than two inputs). Can you also fix it?

Also, would it be possible to add regressions tests, i.e. tests where the AddGate takes three inputs and MulGate takes three inputs?

@ivokub
Copy link
Collaborator

ivokub commented Mar 5, 2024

Also, would it be possible to add regressions tests, i.e. tests where the AddGate takes three inputs and MulGate takes three inputs?

Hmm, looking at it, I'm not sure it is worth adding regression tests -- the prover and verifier use the same definition of the gates and it is difficult to test regression (without defining the correct AddGate for the verifier).

@ivokub ivokub self-requested a review March 5, 2024 09:55
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

I implemented fix for MulGate directly in your branch.

Thanks for the contribution, I'll merge it currently as is. Let us know if there is anything else you notice!

@ivokub ivokub merged commit b961fba into Consensys:master Mar 5, 2024
4 of 5 checks passed
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