fix: Fix integration test failures for sync-noir #11294

[aakoshh]

Fixes the regression with the private-kernel-inner-simulated circuit in #11294 by ensuring that inc_rc on array inputs outputs of functions are kept in tact during the preprocessing step.

See #11294 (comment) for more details.

What seems to happen is that normally the DIE pass runs on fully inlined functions, and considers inc_rc on arrays that the function did not mutate as unnecessary. However in this case we consider functions in isolation; in our case the new_from_previous_kernel took an array as input, determined its length, then passed it to BoundedVec::from_parts_unchecked which used the array as storage. Later another function called pop on the vector, which does use array_set, but because new_from_previous_kernel didn't know about it, the inc_rc got removed, and later pop thought it was safe to modify the array, rather than make a copy.

[TomAFrench]

getting test failures on here btw.

[aakoshh]

After fixing the "subtract with overflow" issue, I'm now investigating the contract test failures like so:

Rebuild the contracts:

cd aztec-packages
./noir/bootstrap.sh
./noir-projects/bootstrap.sh
./yarn-project/bootstrap.sh

Start an execution server to serve as an oracle:

export LOG_LEVEL=debug
cd ./yarn-project/txe
yarn start

Then run one of the failing tests from ci-noir-bb:

export NARGO=$(pwd)/noir/noir-repo/target/release/nargo 
cd ./noir-projects/noir-protocol-circuits
$NARGO test --silence-warnings --oracle-resolver http://localhost:8080 --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds 

[aakoshh]

Running the above reveals an ICE:

% $NARGO test --silence-warnings --oracle-resolver http://localhost:8080 --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds   8s ~/aztec-packages/noir-projects/noir-protocol-circuits af/fix-11294+ akosh-box


[private_kernel_lib] Running 1 test function
The application panicked (crashed).
Message:  internal error: entered unreachable code: All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value v39154 = Instruction { instruction: Id(54188), position: 0, typ: Numeric(NativeField) }
Location: compiler/noirc_evaluator/src/ssa/opt/inlining.rs:696

This is a bug. We may have already fixed this in newer versions of Nargo so try searching for similar issues at https://github.com/noir-lang/noir/issues/.
If there isn't an open issue for this bug, consider opening one at https://github.com/noir-lang/noir/issues/new?labels=bug&template=bug_report.yml


[private_kernel_lib] Testing tests::reset_output_validator_builder::validate_sorted_siloed_nullifiers::validate_sorted_siloed_nullifiers_succeeds... FAIL
An unexpected error happened


[private_kernel_lib] Failures:
     tests::reset_output_validator_builder::validate_sorted_siloed_nullifiers::validate_sorted_siloed_nullifiers_succeeds

[private_kernel_lib] 1 test failed

[aakoshh]

The backtrace from $NARGO above is not very revealing, but if we run cargo run in debug mode we get more pointers:

% RUST_BACKTRACE=1 cargo run -p nargo_cli -- --program-dir ../../noir-projects/noir-protocol-circuits test --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds --silence-warnings
    Finished dev [optimized + debuginfo] target(s) in 0.35s
     Running `target/debug/nargo --program-dir ../../noir-projects/noir-protocol-circuits test --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds --silence-warnings`
[private_kernel_lib] Running 1 test function
The application panicked (crashed).
Message:  internal error: entered unreachable code: All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value v39154 = Instruction { instruction: Id(54188), position: 0, typ: Numeric(NativeField) }
Location: compiler/noirc_evaluator/src/ssa/opt/inlining.rs:696

This is a bug. We may have already fixed this in newer versions of Nargo so try searching for similar issues at https://github.com/noir-lang/noir/issues/.
If there isn't an open issue for this bug, consider opening one at https://github.com/noir-lang/noir/issues/new?labels=bug&template=bug_report.yml

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 8 frames hidden ⋮                               
   9: noirc_evaluator::ssa::opt::inlining::PerFunctionContext::translate_value::h2bd733da53be08ad
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:<unknown line>
  10: noirc_evaluator::ssa::opt::inlining::PerFunctionContext::push_instruction::{{closure}}::hb24121387698b9cd
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:925
  11: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once::hb9412fd2666b8142
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:305
  12: core::option::Option<T>::map::h2855516fe7d8affc
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/option.rs:1072
  13: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next::hf47105f97bcacf0b
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/iter/adapters/map.rs:103
  14: <im::vector::Vector<A> as core::iter::traits::collect::FromIterator<A>>::from_iter::h038d7a18c0bb70eb
      at /mnt/user-data/akosh/.cargo/registry/src/index.crates.io-6f17d22bba15001f/im-15.1.0/./src/vector/mod.rs:1895
  15: core::iter::traits::iterator::Iterator::collect::h268bc9dd7b2586c3
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/iter/traits/iterator.rs:2054
  16: noirc_evaluator::ssa::ir::instruction::Instruction::map_values::h8ce745d6c7bc7c2e
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs:719
  17: noirc_evaluator::ssa::opt::inlining::PerFunctionContext::push_instruction::hb94194a12a4f3356
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:925
  18: noirc_evaluator::ssa::opt::inlining::PerFunctionContext::inline_block_instructions::h8867af39938eb21f
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/slice/iter/macros.rs:<unknown line>
  19: noirc_evaluator::ssa::opt::inlining::PerFunctionContext::inline_blocks::h1dc286caa0cf8423
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:798
  20: noirc_evaluator::ssa::opt::inlining::InlineContext::inline_all::h94122beaf37da434
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:601
  21: noirc_evaluator::ssa::opt::inlining::<impl noirc_evaluator::ssa::ir::function::Function>::inlined::h0859e2c5447c17df
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:124
  22: noirc_evaluator::ssa::opt::inlining::<impl noirc_evaluator::ssa::ssa_gen::program::Ssa>::inline_functions_inner::{{closure}}::hb4c02d57ee283f84
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:72
  23: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once::hf80adbe0d51110e2
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:305
  24: core::option::Option<T>::map::h3e0bd637a9029e3f
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/option.rs:1072
  25: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next::h622ad443c92f8109
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/iter/adapters/map.rs:103
  26: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter::h70381a67cb460bc8
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/vec/spec_from_iter_nested.rs:26
  27: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter::h4ebf902c32b7378e
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/vec/spec_from_iter.rs:33
  28: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter::hfdb6f09e75792fec
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/vec/mod.rs:2753
  29: core::iter::traits::iterator::Iterator::collect::hbaae4f8aa0bbb5ab
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/iter/traits/iterator.rs:2054
  30: <alloc::collections::btree::map::BTreeMap<K,V> as core::iter::traits::collect::FromIterator<(K,V)>>::from_iter::h4d5381d8a6406d05
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/collections/btree/map.rs:2221
  31: core::iter::traits::iterator::Iterator::collect::hbeb17e467f2b12a7
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/iter/traits/iterator.rs:2054
  32: iter_extended::btree_map::hca77cb6533776ed2
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/utils/iter-extended/src/lib.rs:33
  33: noirc_evaluator::ssa::opt::inlining::<impl noirc_evaluator::ssa::ssa_gen::program::Ssa>::inline_functions_inner::h620d99ce61d42fe6
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:69
  34: noirc_evaluator::ssa::opt::inlining::<impl noirc_evaluator::ssa::ssa_gen::program::Ssa>::inline_functions_with_no_predicates::h98c8449b8fbd1e2f
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs:56
  35: noirc_evaluator::ssa::optimize_all::{{closure}}::h8ed30f8317d4b9f1
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:186
  36: noirc_evaluator::ssa::SsaBuilder::run_pass::{{closure}}::h67ed4c1684b19e77
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:473
  37: noirc_evaluator::ssa::time::hbf070850f633f5af
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:203
  38: noirc_evaluator::ssa::SsaBuilder::run_pass::he2ecf0b5864c8956
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:473
  39: noirc_evaluator::ssa::optimize_all::hba76e59e0ec26e70
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:152
  40: noirc_evaluator::ssa::optimize_into_acir::h2e40da40ea4d5ed7
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:103
  41: noirc_evaluator::ssa::create_program::hbe72e315e3a1ac54
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs:279
  42: noirc_driver::compile_no_check::h7ef6889e01006e89
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/compiler/noirc_driver/src/lib.rs:687
  43: nargo::ops::test::run_test::had048dd91f852fe4
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/tooling/nargo/src/ops/test.rs:56
  44: nargo::cli::test_cmd::TestRunner::run_test::h79d83cf3d0418589
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs:506
  45: nargo::cli::test_cmd::TestRunner::collect_package_tests::{{closure}}::{{closure}}::h8c256971f701d37a
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs:453
  46: core::ops::function::FnOnce::call_once{{vtable.shim}}::h5b4093e8b78d464c
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:250
  47: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::h4fba8a7b22d2952f
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/boxed.rs:2007
  48: std::panicking::try::do_call::hbbb46094a22eb5c5
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:552
  49: std::panicking::try::h1091ab6c8d4e074f
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:516
  50: std::panic::catch_unwind::hac85c3e54cb8e0cb
      at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panic.rs:142
  51: nargo::cli::test_cmd::TestRunner::run_all_tests::{{closure}}::{{closure}}::h867842b75eb7ab99
      at /mnt/user-data/akosh/aztec-packages/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs:260
                                ⋮ 11 frames hidden ⋮                              
[private_kernel_lib] Testing tests::reset_output_validator_builder::validate_sorted_siloed_nullifiers::validate_sorted_siloed_nullifiers_succeeds... FAIL
An unexpected error happened


[private_kernel_lib] Failures:
     tests::reset_output_validator_builder::validate_sorted_siloed_nullifiers::validate_sorted_siloed_nullifiers_succeeds

[private_kernel_lib] 1 test failed

/ssa.rs:186 indicates that this is the pass where the panic occurs:

[aakoshh]

Random observations:

If I run the following to show the SSA before the 2nd inlining pass, it crashes during the normalisation of IDs:

 cargo run -p nargo_cli -- --program-dir ../../noir-projects/noir-protocol-circuits test --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds --silence-warnings --show-ssa --show-ssa-pass "Mem2Reg (3rd)"

But if I show all SSA passed, then the test passes as well, ie. normalising the IDs after each pass does something to keep the issue from surfacing:

 cargo run -p nargo_cli -- --program-dir ../../noir-projects/noir-protocol-circuits test --package private_kernel_lib validate_sorted_siloed_nullifiers_succeeds --silence-warnings --show-ssa

It is true that if I print the non-normalised SSA, then there is this line in it:

    v81402 = make_array [v39154, v39151] : [Field; 2]

and that is the only occurrence of v39154 in validate_sorted_siloed_nullifiers_succeeds, it's definition is lost.

Using the above command to probe passes, the first one which fails is Loop Invariant Code Motion; at static_assert and assert_constant we're still okay (or at least not crashing).

I can confirm that with .run_pass(Ssa::loop_invariant_code_motion, "Loop Invariant Code Motion") commented out the test passes, and it also passes if ssa.preprocess_functions is not called.

[aakoshh]

Looking the not normalized SSA before the Loop Invariant Code Motion we have this:

  b2377():                                               # predecessor of b2382 
    jmp b2382()
  b2378():                                               # `then` of block b2382 
    jmp b2383(v39151)                                    # jump to the continuation with the result v39151
  b2379():                                               # `else` of block b2382 
    v39168 = make_array [v39154, v39151] : [Field; 2]    # use v39154, which is defined in b2382
    v39169 = call f55(v39168, u32 7) -> Field
    jmp b2383(v39169)                                    # jump to the continuation with the result v39169
  b2382():                                               # predecessor of b2378 and b2379
    v39151 = array_get v29916, index u32 4 -> Field
    v39152 = array_get v29916, index u32 5 -> u32
    v39153 = array_get v29916, index u32 6 -> Field
    v39154 = array_get v29916, index u32 7 -> Field      # define v39154
    v39155 = load v39126 -> u1
    v39156 = eq u32 1, v39097
    v39157 = or v39155, v39156
    store v39157 at v39126
    v39158 = array_get v29917, index u32 1 -> u32
    v39159 = unchecked_mul v39158, u32 4
    v39160 = array_get v29329, index v39159 -> Field
    v39161 = unchecked_add v39159, u32 1
    v39162 = array_get v29329, index v39161 -> u32
    v39163 = unchecked_add v39159, u32 2
    v39164 = array_get v29329, index v39163 -> Field
    v39165 = unchecked_add v39159, u32 3
    v39166 = array_get v29329, index v39165 -> Field
    v39167 = eq v39154, Field 0                          # use v39154 in the `if` to decide if `then` or `else` follows
    jmpif v39167 then: b2378, else: b2379
  b2383(v20465: Field):                                  # continuation

After the Loop Invariant Code Motion pass the same blocks look like this:

  b2377():
    jmp b2382()
  b2378():
    jmp b2383(v39151)
  b2379():
    v51603 = make_array [v39154, v39151] : [Field; 2]          # still using v39154 in the `else`
    v51604 = call f55(v51603, u32 7) -> Field
    jmp b2383(v51604)
  b2382():
    v51605 = array_get v45347, index u32 4 -> Field
    v51606 = array_get v45347, index u32 5 -> u32
    v51607 = array_get v45347, index u32 6 -> Field
    v51608 = array_get v45347, index u32 7 -> Field            # but the ID where it was defined is now v51608
    v51609 = load v51578 -> u1
    v51610 = eq u32 1, v51549
    v51611 = or v51609, v51610
    store v51611 at v51578
    v51612 = array_get v45348, index u32 1 -> u32
    v51613 = unchecked_mul v51612, u32 4
    v51614 = array_get v45339, index v51613 -> Field
    v51615 = unchecked_add v51613, u32 1
    v51616 = array_get v45339, index v51615 -> u32
    v51617 = unchecked_add v51613, u32 2
    v51618 = array_get v45339, index v51617 -> Field
    v51619 = unchecked_add v51613, u32 3
    v51620 = array_get v45339, index v51619 -> Field
    v51621 = eq v51608, Field 0
    jmpif v51621 then: b2378, else: b2379
  b2383(v20465: Field):

Again this is not normalised, but display of values calls resolve, so if there was a forwarding path from v39154 to v51608 it would match.

[aakoshh]

The reason for this error is that this loop processes blocks ordered by ID rather than a topological order of the call graph, and in this case that means processing b2379 before b2382. Then, during push_instruction, a new ID is created for the instruction (which now has its values resolved, even if nothing changed). Because the else was already inserted, it somehow didn't get the memo.

This should be fixed in noir-lang/noir#7140