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

if-then-else with compare-not-equal modifies value when it shouldn't #5202

Closed
smanilov opened this issue Jun 7, 2024 · 1 comment · Fixed by #5240
Closed

if-then-else with compare-not-equal modifies value when it shouldn't #5202

smanilov opened this issue Jun 7, 2024 · 1 comment · Fixed by #5240
Labels
bug Something isn't working

Comments

@smanilov
Copy link

smanilov commented Jun 7, 2024

Aim

Debug a failing test in a third_party library (resurgencelabs/fraction).

Expected Behavior

Test is failing due to a bug in the library.

Bug

Test is failing due to a bug in the Noir language tools.

To Reproduce

  1. checkout https://github.com/resurgencelabs/fraction
  2. change Nargo.toml to use >=0.25, not exactly 0.25
  3. get nargo v0.30 or later
  4. issue nargo test

Test "test_add_large()" fails with the following message:

[fraction] Testing test_add_large... FAIL
Failed to solve program: 'Cannot satisfy constraint'

error: Failed constraint
    ┌─ /home/stan/code/repos/benchmarking-noir/fraction/src/lib.nr:710:12
    │
710 │     assert(compareFraction(floor(a), MAX) == 2);
    │            -----------------------------------
    │
    = Call stack:
      1. /home/stan/code/repos/benchmarking-noir/fraction/src/lib.nr:710:12

[fraction] 1 test failed

compareFraction returns 1 instead of 2.

Note: this is not a bug in the library, but rather a bug in the Noir implementation. It can be reproduced with the following snippet:

use dep::fraction::{Fraction, MAX, floor, toFraction, addFraction};

fn main() {
    let g1 = toFraction(true, 33333333, 5);
    let g2 = toFraction(true, 500000, 33333333);
    let a = addFraction(g1, g2);

    print("a: ");
    println(a);

    let f1 = floor(a);
    let f2 = MAX;

    // assert(compareFraction(floor(a), MAX) == 2);

    print("1 signs: ");
    print(f1.sign);
    print(" ");
    println(f2.sign);

    let _ = if (f1.sign != f2.sign) {
        if (f1.sign) { 1 } else { 2 }
    } else {
        print("2 signs: ");
        print(f1.sign);
        print(" ");
        println(f2.sign);
        0
    };
}

Note that in this code, "1 signs" and "2 signs" should print the same thing, because f1.sign and f2.sign are not modified in between. The output of nargo execute, however, is this:

a: Fraction { sign: true, num: 666666661, den: 100 }
1 signs: true true
2 signs: false true

Also note: using nargo debug results in the correct expected behavior:

a: Fraction { sign: true, num: 666666661, den: 100 }
1 signs: true true
2 signs: true true

Project Impact

Blocker

Impact Context

This is a serious issue that undermines the credibility of using Noir in mission-critical systems and projects.

Workaround

None

Workaround Description

No response

Additional Context

This was discovered while trying to compile a collection of working programs and libraries written in Noir.

Installation Method

Compiled from source

Nargo Version

nargo version = 0.30.0 noirc version = 0.30.0+af57471035e4fa7eaffa71693219df6d029dbcde

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@smanilov smanilov added the bug Something isn't working label Jun 7, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jun 7, 2024
@jfecher
Copy link
Contributor

jfecher commented Jun 13, 2024

Thanks for the issue - this was caused by a faulty optimization which I've disabled in #5240. Should be fixed once that is merged.

github-merge-queue bot pushed a commit that referenced this issue Jun 13, 2024
# Description

## Problem\*

Resolves #5202 

## Summary\*

The linked issue was coming from our if condition optimization where we
assume the value of an if's condition to be true within the then branch
and false within the else branch. It seems the `map_value` calls from
this weren't being reset properly since the nested `if f1.sign { .. } {
.. }`'s else branch assumed `f1.sign` to be false, which was not reset
properly after the `if`. I've temporarily disabled the optimization
until we can add it back without breaking this test.

## Additional Context



## 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 Jun 13, 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
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants