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

Sudoku example fix #1287

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Sudoku example fix #1287

merged 5 commits into from
Apr 11, 2023

Conversation

Turupawn
Copy link
Contributor

The problem

the logic behind countDuplicates is invalid because it's doing this:

(incorrect)

if(duplicates + e11 == e21)
  duplicates = 1;
else
  duplicates = 0;

Instead of this:
(correct)

if(e11 == e21)
  duplicates = duplicates + 1;
else
  duplicates = duplicates;

The solution

I'm using an auxiliary variable to fix this

Alternative solution

We can also fix it with an array and a loop. But I went for the auxiliary variable for no particular reason.

def countDuplicates(field e11, field e12, field e21, field e22) -> u32 {
    u32[6] mut duplicates = [0,0,0,0,0,0];
    duplicates[0] = e11 == e12 ? 1 : 0;
    duplicates[1] = e11 == e21 ? 1 : 0;
    duplicates[2] = e11 == e22 ? 1 : 0;
    duplicates[3] = e12 == e21 ? 1 : 0;
    duplicates[4] = e12 == e22 ? 1 : 0;
    duplicates[5] = e21 == e22 ? 1 : 0;

    u32 mut count = 0;
    for u32 i in 0..5 {
        count = count + duplicates[i];
    }

    return count;
}

And btw, I also added an assert to make sure we check for this validation. I've tested this on the playground and on chain on goerli and with this changes it works correctly.

Thanks for putting adding the examples. It has helped me a lot.
Cheers!

**Problem**

the logic behind `countDuplicates` is invalid because it's doing this:
(incorrect)
```
if(duplicates + e11 == e21)
  duplicates = 1;
else
  duplicates = 0;
```

Instead of this:
(correct)
```
if(e11 == e21)
  duplicates = duplicates + 1;
else
  duplicates = duplicates;
```

**Solution**

I'm using an auxiliary variable to fix this

**Alternative solution**

We can also fix it with an array and a loop. But I went for the auxiliary variable for no particular reason.

```
def countDuplicates(field e11, field e12, field e21, field e22) -> u32 {
    u32[6] mut duplicates = [0,0,0,0,0,0];
    duplicates[0] = e11 == e12 ? 1 : 0;
    duplicates[1] = e11 == e21 ? 1 : 0;
    duplicates[2] = e11 == e22 ? 1 : 0;
    duplicates[3] = e12 == e21 ? 1 : 0;
    duplicates[4] = e12 == e22 ? 1 : 0;
    duplicates[5] = e21 == e22 ? 1 : 0;

    u32 mut count = 0;
    for u32 i in 0..5 {
        count = count + duplicates[i];
    }

    return count;
}
```

And btw, I also added an assert to make sure we check for this validation. I've tested this on the playground and on chain on goerli and with this changes it works correctly.
@Schaeff
Copy link
Member

Schaeff commented Mar 20, 2023

Oh good catch, seems like a precedence issue, would a smaller change like the following work?

-   duplicates = duplicates + e11 == e21 ? 1 : 0;
+   duplicates = duplicates + (e11 == e21 ? 1 : 0);

@@ -74,5 +81,6 @@ def main(field a21, field b11, field b22, field c11, field c22, field d21, priva
duplicates = duplicates + countDuplicates(b12, b22, d12, d22);

// the solution is correct if and only if there are no duplicates
assert(duplicates == 0);
Copy link
Member

Choose a reason for hiding this comment

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

This is fine but then let's return nothing

@Turupawn
Copy link
Contributor Author

The smaller change worked! I tested it on the playground and works well 👍
I'm also returning nothing on the main function.
Thanks for reviewing this @Schaeff !

@Turupawn Turupawn closed this Mar 21, 2023
@Turupawn Turupawn reopened this Mar 21, 2023
@Turupawn
Copy link
Contributor Author

Woops, closed this by accident. Just reopened

@Turupawn
Copy link
Contributor Author

After further thought, I went back returning a bool instead of nothing. I think it makes more sense. Specially for newcomers like myself.
So in this PR I'm only fixing the precedence issue.

@Turupawn
Copy link
Contributor Author

Hey guys, I wanted to know if you guys think something else should be done on this PR. I would greatly appreciate if we merge this because I'm doing educational content and it would be nice to point to this official repo instead of my own.
This is the first piece of documentation for reference, but I plan on doing more:
https://dev.to/turupawn/circuitos-zksnarks-paso-a-paso-4i6n
It rewards the players with tokens when they submit a valid proof that they solved the sudoku.
Cheers!

@Schaeff
Copy link
Member

Schaeff commented Apr 10, 2023

Yes we can merge it soon. Can you please just add a changelog at changelogs/unreleased/1287-Turupawn? Thanks!

@Turupawn
Copy link
Contributor Author

Added the changelog @Schaeff
Very appreciated 🙏

@Schaeff Schaeff merged commit a895509 into Zokrates:develop Apr 11, 2023
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