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

Sha512 cannot hash slices #3687

Closed
Tracked by #3363
dndll opened this issue Dec 5, 2023 · 1 comment · Fixed by #4268
Closed
Tracked by #3363

Sha512 cannot hash slices #3687

dndll opened this issue Dec 5, 2023 · 1 comment · Fixed by #4268
Assignees
Labels
bug Something isn't working
Milestone

Comments

@dndll
Copy link

dndll commented Dec 5, 2023

Aim

Hashing a slice with sha512 in 0.19.2 causes unconstrained comptime issues on noir 0.19.2+47f0130c0d154f1b70eb23f376783beb3f23ad72

Expected Behavior

Hashing slices should be fine, or a code path for hashing unbounded items should be unconstrained.

Bug

Hashing a slice in a test causes:
[simple4x] Testing test_sha512_broken_slices... error: Could not determine loop bound at compile-time

To Reproduce

#[test]
fn test_sha512() {
    let s = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32];
    dep::std::sha512::digest(s);
}

#[test]
fn test_sha512_broken_slices() {
    let s = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32];
    dep::std::sha512::digest(s.as_slice());
}

Installation Method

Compiled from source

Nargo Version

0.19.2+47f0130c0d154f1b70eb23f376783beb3f23ad72

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@dndll dndll added the bug Something isn't working label Dec 5, 2023
@kevaundray kevaundray added this to the 0.24.0 milestone Jan 15, 2024
@vezenovm
Copy link
Contributor

vezenovm commented Feb 5, 2024

Hashing slices should be fine, or a code path for hashing unbounded items should be unconstrained.

What use case do you see for using an unconstrained hash result?

It is expected for this to fail as .len() on a slice is going to use the dynamic array length value which is not a compile-time constant, even when it is obvious for the user as in this case. This error could definitely be more explicit though with suggestions on how to resolve the error for when someone attempts to do slice.len().

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

## Problem\*

Resolves #3687 

## Summary\*

This simply adds a secondary message to clarify that using a slice
length will lead to the `Could not determine loop bound at compile-time`
error.

I was thinking of disabling this earlier during type check but we don't
necessarily know that we will have a slice until codegen as arrays and
slices are polymorphic with one another. I also thought about making a
separate runtime error to check during loop unrolling but we are not
necessarily going to still have the `ArrayLen` call codegen'd in SSA and
will instead have the call's results for the loop condition, so this
isn't a basic check and some more logic would be needed.

The best option for a specific error check seems to be during
`codegen_for` before we codegen the end range. We can check whether we
are making a slice ArrayLen call for the end range. These changes all
felt a bit much as the error is quite clear except for this one error
case and thus a secondary message felt sufficient.

If we would like the error to be more specific for the failure case then
I can look at adding logic that triggers an error for a slice length
loop bound.

An example of the error now:
<img width="1013" alt="Screenshot 2024-02-05 at 5 10 39 PM"
src="https://github.com/noir-lang/noir/assets/43554004/45bc240b-90a7-47b0-974d-d60e7c9a2079">



## Additional Context



## Documentation\*

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

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
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.

4 participants