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

Compilation fails if looping on slices... Only in libraries #3883

Closed
signorecello opened this issue Dec 19, 2023 · 5 comments
Closed

Compilation fails if looping on slices... Only in libraries #3883

signorecello opened this issue Dec 19, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@signorecello
Copy link
Contributor

Aim

This came to me through @madztheo, so I can't provide much context. But I can reproduce this.

The aim is to compile a circuit that loops on slices, in a library

// lib.nr
pub fn dependency(x: [Field], y: [Field]) {
    for i in 0..x.len() {
        assert(y[i] == x[i]);
    }
}

// main.nr
use dep::dependency::dependency;

fn main(x: [Field; 3], y: [Field; 3]) {
    dependency(x.as_slice(), y.as_slice());
}

Expected Behavior

Expected both tests and proofs to work when this dependency is imported into a bin file.

Bug

Instead, it fails with Could not determine loop bound at compile-time even though the loop bound is indeed known at compile-time. So much that the code runs if I remove the library altogether:

fn main(x: [Field; 3], y: [Field; 3]) {
    for i in 0..x.len() {
        assert(y[i] == x[i]);
    }
}

Tests also pass in both lib and bin

To Reproduce

  1. Clone my MRE repository
  2. cd into project
  3. Run nargo test or nargo compile or nargo prove, etc
  4. cd into dependency
  5. Run nargo test to see it pass
  6. Modify the code according to above and repeat. It should work.

Installation Method

Binary

Nargo Version

nargo version = 0.21.0 noirc version = 0.21.0+e7fb36bfe2f9971eef7129d26680cccf5fbffff4

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@signorecello signorecello added the bug Something isn't working label Dec 19, 2023
@vezenovm
Copy link
Contributor

vezenovm commented Jan 9, 2024

Instead, it fails with Could not determine loop bound at compile-time even though the loop bound is indeed known at compile-time. So much that the code runs if I remove the library altogether:

This is not the same as the library code though. You are accepting two arrays to main which have a constant length, and thus the loop bound is known at compile-time.

Our check in SSA disallows

pub fn dependency(x: [Field], y: [Field]) {
    for i in 0..x.len() {
        assert(y[i] == x[i]);
    }
}

as the value returned from slice.len() will be a dynamic value. If it comes from a library the slices x and y could differ based upon the runtime arguments to the program.

Until we enable direct comparison of slices through ==, slices will have to be converted to arrays for comparison.

@madztheo
Copy link

madztheo commented Jan 9, 2024

Instead, it fails with Could not determine loop bound at compile-time even though the loop bound is indeed known at compile-time. So much that the code runs if I remove the library altogether:

This is not the same as the library code though. You are accepting two arrays to main which have a constant length, and thus the loop bound is known at compile-time.

Our check in SSA disallows

pub fn dependency(x: [Field], y: [Field]) {
    for i in 0..x.len() {
        assert(y[i] == x[i]);
    }
}

as the value returned from slice.len() will be a dynamic value. If it comes from a library the slices x and y could differ based upon the runtime arguments to the program.

Until we enable direct comparison of slices through ==, slices will have to be converted to arrays for comparison.

To complete what @signorecello said, I encountered this error with two functions of my date library https://github.com/madztheo/noir-date

    fn get_duration_between_months(self: Self, other: Self) -> i32 {
        assert(self.month >= 1 & self.month <= 12);
        assert(other.month >= 1 & other.month <= 12);
        let mut duration: i32 = 0;
        if(self.month < other.month) {
            for month in self.month..other.month {
                duration -= other.get_days_in_month(month) as i32;
            }
        } else {
            for month in other.month..self.month {
                duration += self.get_days_in_month(month) as i32;
            }
        }
        duration
    }

    fn get_duration_between_years(self: Self, other: Self) -> i32 {
        let mut duration: i32 = 0;
        if(self.year < other.year) {
            for year in self.year..other.year {
                if Date::new(year as u16, 1, 1).is_leap_year() {
                    duration -= 366;
                } else {
                    duration -= 365;
                }
            }
        } else {
            for year in other.year..self.year {
                if Date::new(year as u16, 1, 1).is_leap_year() {
                    duration += 366;
                } else {
                    duration += 365;
                }
            }
        }
        duration
    }

When I declared this function directly in a bin file it worked but when I used it in another project by importing the library that contains it then I got the Could not determine loop bound at compile-time error. Also when I run the tests on the library, they all pass, with no errors. You can try to clone the library and run nargo test with version 0.22.0 of nargo.

Screenshot 2024-01-09 at 21 47 04

So that's why I'm a bit confused, if the size of the loop cannot be determined at compile time, shouldn't it fail in all cases?

In the meantime, I've made a temporary fix for my library on this branch https://github.com/madztheo/noir-date/tree/compile-time-var-temp-fix

@vezenovm
Copy link
Contributor

vezenovm commented Jan 9, 2024

Also when I run the tests on the library, they all pass, with no errors.

Ah, I think this is going to be because of this issue (#2128). Tests are going to run with all inputs constant. Also in your library can you point me to where you are looping over slices? In the get_duration_between_months function you posted the month and year fields used in the loops are u8.

Do you also have an example of what you tried to do? It will then be easier to tell what is different between your bin project and when you import it into a library.

@madztheo
Copy link

Also when I run the tests on the library, they all pass, with no errors.

Ah, I think this is going to be because of this issue (#2128). Tests are going to run with all inputs constant. Also in your library can you point me to where you are looping over slices? In the get_duration_between_months function you posted the month and year fields used in the loops are u8.

Do you also have an example of what you tried to do? It will then be easier to tell what is different between your bin project and when you import it into a library.

Actually, in my case, unlike the example of @signorecello, I'm not looping over slices but only on the month and year fields that are just unsigned integers as you've noticed. He ended up using a different example to showcase the problem.

I've just tried to declare the function in a bin file again, and it looks like I get the error this time with the latest nightly version. So only when I run the test it doesn't work. But I guess if all inputs are considered constants in the tests then it would make sense why it doesn't fail in that case. In the case of a bin file or when I import it into another project, the value of the date is determined by an argument to the proof, which makes it non-deterministic.

I guess I'll just merge my fix on the main branch of my library, it works fine but just slightly less flexible for the years. Although I may be able to find a way to do it without loops for the years.

@vezenovm
Copy link
Contributor

I guess I'll just merge my fix on the main branch of my library, it works fine but just slightly less flexible for the years. Although I may be able to find a way to do it without loops for the years.

You can still use a loop if you need it but you are just going to need a constant loop bound. I'm going to close this issue for now, as it is the expected functionality.

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

No branches or pull requests

3 participants