Skip to content

Preliminary absorb and final squeeze#4

Closed
vezenovm wants to merge 24 commits intoabsorbfrom
mv/squeeze
Closed

Preliminary absorb and final squeeze#4
vezenovm wants to merge 24 commits intoabsorbfrom
mv/squeeze

Conversation

@vezenovm
Copy link
Collaborator

@vezenovm vezenovm commented Nov 4, 2022

This PR adds usage of the final keccak permutation functions into the absorb function.

These changes successfully build however there are some errors with regards when it comes to compilation. It may be due to something inside of Noir though, further debugging needs to be done to determine the error.

Copy link
Owner

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

If I try to build this I end up with the error below:

error: Expected an end of input but found #[builtin(arraylen)]
  ┌─ /home/tom/.config/noir-lang/std/array.nr:7:1
  │
7 │ #[builtin(arraylen)]
  │ --------------------

error: could not resolve path 'array::len'
   ┌─ /home/tom/.config/noir-lang/std/hash.nr:23:24
   │
23 │     for i in 1..crate::array::len(constants) {
   │                        ----------
   │                               │
   │                               could not resolve `len` in path

error: could not resolve path 'array::len'
    ┌─ /home/tom/.config/noir-lang/std/hash.nr:130:24
    │
130 │     for i in 0..crate::array::len(x) {
    │                        ----------
    │                               │
    │                               could not resolve `len` in path

error: could not resolve path 'array::len'
   ┌─ /home/tom/.config/noir-lang/std/merkle.nr:14:20
   │
14 │     let n = crate::array::len(hash_path);
   │                    ----------
   │                           │
   │                           could not resolve `len` in path

* master:
  refactor: split logic into a lib crate to allow testing
@vezenovm
Copy link
Collaborator Author

vezenovm commented Nov 5, 2022

Cheers!

If I try to build this I end up with the error below:

error: Expected an end of input but found #[builtin(arraylen)]
  ┌─ /home/tom/.config/noir-lang/std/array.nr:7:1
  │
7 │ #[builtin(arraylen)]
  │ --------------------

error: could not resolve path 'array::len'
   ┌─ /home/tom/.config/noir-lang/std/hash.nr:23:24
   │
23 │     for i in 1..crate::array::len(constants) {
   │                        ----------
   │                               │
   │                               could not resolve `len` in path

error: could not resolve path 'array::len'
    ┌─ /home/tom/.config/noir-lang/std/hash.nr:130:24
    │
130 │     for i in 0..crate::array::len(x) {
    │                        ----------
    │                               │
    │                               could not resolve `len` in path

error: could not resolve path 'array::len'
   ┌─ /home/tom/.config/noir-lang/std/merkle.nr:14:20
   │
14 │     let n = crate::array::len(hash_path);
   │                    ----------
   │                           │
   │                           could not resolve `len` in path

When building nargo on Noir master? I get Constraint system successfully built!

I then fail on nargo prove with this:
Screen Shot 2022-11-04 at 8 31 49 PM

Will get the chance to look more into why tomorrow

@TomAFrench
Copy link
Owner

TomAFrench commented Nov 5, 2022

Looks like my version of std lib wasn't being updated by noirup and so I had a const type in there. I can build now and get the same error as you.

TomAFrench and others added 10 commits November 9, 2022 22:04
refactor: use global values for array sizes
* master:
  fix: remove some usages of NUM_LANES which isn't supported
  feat: use constant for lengths of lane offset values
  refactor: use global values for array sizes
Consolidate constants to a single file
* master:
  fix: add missing import to padding test
  fix: remove some usages of NUM_LANES which isn't supported
  refactor: move constants to a single file
  feat: use constant for lengths of lane offset values
  refactor: split logic into a lib crate to allow testing
  refactor: use global values for array sizes
@TomAFrench
Copy link
Owner

I had a go proving this with using noir-lang/noir#437, judging by how long it takes to throw an error we seem to make it further through the proving process but we eventually error out with

thread 'main' panicked at 'internal error: entered unreachable code: Can only multiply linear terms', crates/noirc_evaluator/src/ssa/acir_gen.rs:1020:9

@vezenovm
Copy link
Collaborator Author

vezenovm commented Nov 10, 2022

I tried proving with the same PR and ran into the same error. From some brief debugging it looks like the XOR operations with the state arrays are causing the error. Trying to drill down more exactly what is causing the terms to be non-linear. It looks to possibly be related to the state arrays being parameters to the function where the XOR is being performed.

@vezenovm
Copy link
Collaborator Author

vezenovm commented Nov 10, 2022

I tried proving with the same PR and ran into the same error. From some brief debugging it looks like the XOR operations with the state arrays are causing the error. Trying to drill down more exactly what is causing the terms to be non-linear. It looks to possibly be related to the state arrays being parameters to the function where the XOR is being performed.

I found what was causing the panic which is referenced in PR #6

* master:
  refactor: remove noop renamings of imports (#7)
@TomAFrench
Copy link
Owner

I found what was causing the panic which is referenced in PR #6

Perhaps with this change, as well as the heavy number of additional constraints from working with bit arrays, it may be worth it to consider switching to using u64s sooner rather than later. However, rather than making large changes it may be worth it to just merge this up, test further with the current code, and make sure it correct. Then can worry about optimizing the algorithm later.

Thanks for the help with this. :)

I'm inclined to say it's best that we make the push to switch over sooner rather than later. Doing any meaningful testing using bit arrays will be like pulling teeth (just testing the padding function takes the best part of a minute each time). Tests should easier to write as short arrays of hex are easier to manipulate than massive bit arrays and they'll run faster due to the smaller circuit sizes.

I'll just keep this PR open in the meantime and I can just pull across the code and modify it to handle u64s once the core functions are ready.

@TomAFrench
Copy link
Owner

Closing this as I've integrated the changes into #8. Thanks again!

@TomAFrench TomAFrench closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants