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

Native BigInt division doesn't yield the correct result #4578

Closed
madztheo opened this issue Mar 19, 2024 · 0 comments · Fixed by #4802
Closed

Native BigInt division doesn't yield the correct result #4578

madztheo opened this issue Mar 19, 2024 · 0 comments · Fixed by #4802
Assignees
Labels
bug Something isn't working

Comments

@madztheo
Copy link

Aim

I'm currently working with the newly released native BigInt and when I divide two BigInts the result is incorrect. I've tried with Secpk1Fq and Bn254Fr, and also directly with the BigInt struct (by implementing the different traits myself).

Expected Behavior

The division between two BigInts should yield the correct result.

Side note, there should be a clearer separation between BigField and BigInt, with the first doing field division and the latter actual euclidean division.

Bug

For example, the test below fails, while it shouldn't since 10 / 5 = 2

#[test]
fn test_div() {
    let a = Secpk1Fq::from_le_bytes([10]);
    let b = Secpk1Fq::from_le_bytes([5]);
    let res = a / b;
    assert(res == Secpk1Fq::from_le_bytes([2]));
}

But the test below succeeds, while it shouldn't

#[test]
fn test_div() {
    let a = Secpk1Fq::from_le_bytes([10]);
    let b = Secpk1Fq::from_le_bytes([5]);
    let res = a / b;
    assert(res == Secpk1Fq::from_le_bytes([10]));
}

In general, the result of the division of two BigInts is equal to the dividend of the division, so in this case a. Swapping Secpk1Fq with Bn254Fr leads to the same behavior.

I also created a custom implementation of the BigInt struct with my own modulo (2^256) and implemented the necessary traits similarly to Secpk1Fq and Bn254Fr. In this case, the behavior when dividing two instances of the struct is different but still yields incorrect results. For example, the following test passes just fine as expected:

#[test]
fn test_div() {
    let a = BigInt256::from_le_bytes([10]);
    let b = BigInt256::from_le_bytes([5]);
    let res = a / b;
    assert(res == BigInt256::from_le_bytes([2]));
}

However, for some other numbers, the result is incorrect as the division just yields 0 as in this case:

#[test]
fn test_div() {
    let a = BigInt256::from_le_bytes([128]);
    let b = BigInt256::from_le_bytes([2]);
    let res = a / b;
    assert(res == BigInt256::from_le_bytes([64]));
}

To Reproduce

  1. Initiate two instances of Secpk1Fq
  2. Divide them
  3. Check if the result is the one you would expect

Project Impact

Blocker

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

Binary (noirup default)

Nargo Version

nargo version = 0.25.0 noirc version = 0.25.0+6a9ea35c4f1578058179aa08eedf44eb18bad4a1

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@madztheo madztheo added the bug Something isn't working label Mar 19, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Mar 19, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 15, 2024
# Description

## Problem\*

Resolves #4578. The fix for issue #4530 has also being merged back so I
also activate the related test case.

## Summary\*

Field inversion for bigints had a typo.

## Additional Context

I did not consider biguint256 since it was decided not to merge the PR.

## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** 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 Apr 15, 2024
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
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants