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

Document/unify approach for division by 0 #2480

Closed
vezenovm opened this issue Aug 29, 2023 · 0 comments · Fixed by #4424
Closed

Document/unify approach for division by 0 #2480

vezenovm opened this issue Aug 29, 2023 · 0 comments · Fixed by #4424
Assignees
Labels
Milestone

Comments

@vezenovm
Copy link
Contributor

Problem

In this PR (#2475) extra checks were added to guarantee that divisions by 0 on fields and integers both resulted in unsatisfied constraints. However, the strategy differs between Field's and integers. We should attempt to unify our strategies for checking against division by 0 or at a minimum clearly document why the strategies are different.

Check PR #2475 and its description for more information as to why the strategies differ for checking division by zero. The gist of it though is that for field division we multiply against the field's inverse while for integer division we use euclidean division. We have a check when computing the inverse which checks that the inverse returned from brillig multiplied by the original value being inverted is equal to one. Doing a Brillig inversion on zero does not panic when executing the Brillig VM and this constraint fails as expected. Now for integer division, which does a euclidean division, we get a panic when executing the Brillig VM. Thus to avoid the panic, we execute checks for the inputs to euclidean division in ACIR gen before executing code for euclidean division.

Happy Case

It should be clear to new contributors why we check for division by zero in this specific manner.

Alternatives Considered

We should most likely have the Brillig VM not panic on an attempted division by zero and rather have the caller (nargo) handle that error. This may lead to clearer errors at least for euclidean division, and we could remove the inputs check before every euclidean division. However, this does still lead to a divergence in strategies as Field division by 0 will still fail with a constraint failed. We could potentially return the same error from the Brillig VM in both these instances in the name of a having a unified strategy.

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Aug 29, 2023
@kevaundray kevaundray added this to the 1.0 milestone Jan 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 26, 2024
# Description

## Problem\*

Resolves #2480

## Summary\*
In BrilligVM, we have now the same behaviour regarding division by 0.
Whether it is a field or integer division, we return 0 when dividing by
zero. Since we have a constraint which checks that the inverse times
itself is 1, this constraint will always fail if we divide by zero
(instead of panic or returning an error).


## Additional Context



## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants