feat(avm): fix some Brillig problems (AztecProtocol/aztec-packages#5091)

This PR enables the stack of 6 PRs on top.
\
While working on external calls, we came across several problems with Brillig. I made some changes to fix them. Some Brillig changes:
* **Truncation**: Brillig was using AND of Fields (actually, AND on Ints of 254 bits). This is not supported by the VM. Truncation was changed to be done without ANDing, and using `CAST` instead, which truncates to the required bit size.
* **Array.get**/**Array.set**: Calculation of the `arrayBase+index` was done using field arithmetic (or field sizes). Now it's done using ints.
* **Reference counting**: Checking `refCount==1` was done using field arithmetic (or field sizes). Now it's done with ints.

These changes seem to solve all the comparison w/different bit sizes, and unexpected uses of field arithmetic. (That we've found with the current test contract).

NOTE: I had to recreate the contract snapshots with `yarn workspace @aztec/protocol-contracts test -u`, then modify
* noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr
* noir-projects/noir-contracts/contracts/fpc_contract/src/main.nr

Relates to #4313, #4127.

.aztec-sync-commit

@@ -1 +1 @@
-7ff9b71d8d87fc93ae7dbd8ba63f5176b0cd17be
+07dd8215dffd2c3c6d22e0f430f5072b4ff7c763

Cargo.toml

@@ -26,6 +26,7 @@ members = [
     "tooling/nargo_toml",
     "tooling/noirc_abi",
     "tooling/noirc_abi_wasm",
+    "tooling/acvm_cli",
     # ACVM
     "acvm-repo/acir_field",
     "acvm-repo/acir",
@@ -36,7 +37,7 @@ members = [
     "acvm-repo/blackbox_solver",
     "acvm-repo/bn254_blackbox_solver",
 ]
-default-members = ["tooling/nargo_cli"]
+default-members = ["tooling/nargo_cli", "tooling/acvm_cli"]
 resolver = "2"
 
 [workspace.package]
@@ -78,6 +79,7 @@ noir_lsp = { path = "tooling/lsp" }
 noir_debugger = { path = "tooling/debugger" }
 noirc_abi = { path = "tooling/noirc_abi" }
 bb_abstraction_leaks = { path = "tooling/bb_abstraction_leaks" }
+acvm_cli = { path = "tooling/acvm_cli" }
 
 # LSP
 async-lsp = { version = "0.1.0", default-features = false }

aztec_macros/src/lib.rs

@@ -727,8 +727,14 @@ fn transform_function(
 /// Transform a function to work with AVM bytecode
 fn transform_vm_function(
     func: &mut NoirFunction,
-    _storage_defined: bool,
+    storage_defined: bool,
 ) -> Result<(), AztecMacroError> {
+    // Create access to storage
+    if storage_defined {
+        let storage = abstract_storage("public_vm", true);
+        func.def.body.0.insert(0, storage);
+    }
+
     // Push Avm context creation to the beginning of the function
     let create_context = create_avm_context()?;
     func.def.body.0.insert(0, create_context);
@@ -1869,8 +1875,8 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
     // For now we hardcode it to 20, which is the same as MAX_NOTE_FIELDS_LENGTH.
 
     if note_types.is_empty() {
-        // TODO(#4520): Even if the contract does not include any notes, other parts of the stack expect for this
-        // function to exist, so we include a dummy version. We likely should error out here instead.
+        // Even if the contract does not include any notes, other parts of the stack expect for this function to exist,
+        // so we include a dummy version.
         "
         unconstrained fn compute_note_hash_and_nullifier(
             contract_address: AztecAddress,
@@ -1879,6 +1885,7 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
             note_type_id: Field,
             serialized_note: [Field; 20]
         ) -> pub [Field; 4] {
+            assert(false, \"This contract does not use private notes\");
             [0, 0, 0, 0]
         }"
         .to_string()
@@ -1892,10 +1899,10 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
             }}"
         , note_type)).collect();
 
-        // TODO(#4520): error out on the else instead of returning a zero array
         let full_if_statement = if_statements.join(" else ")
             + "
             else {
+                assert(false, \"Unknown note type ID\");
                 [0, 0, 0, 0]
             }";
 

compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

@@ -831,16 +831,10 @@ impl<'block> BrilligBlock<'block> {
             _ => unreachable!("ICE: array set on non-array"),
         };
 
+        // Here we want to compare the reference count against 1.
         let one = self.brillig_context.make_usize_constant(1_usize.into());
         let condition = self.brillig_context.allocate_register();
-
-        self.brillig_context.binary_instruction(
-            reference_count,
-            one,
-            condition,
-            BrilligBinaryOp::Field { op: BinaryFieldOp::Equals },
-        );
-
+        self.brillig_context.memory_op(reference_count, one, condition, BinaryIntOp::Equals);
         self.brillig_context.branch_instruction(condition, |ctx, cond| {
             if cond {
                 // Reference count is 1, we can mutate the array directly

compiler/noirc_evaluator/src/brillig/brillig_ir.rs

@@ -27,12 +27,11 @@ use acvm::{
     FieldElement,
 };
 use debug_show::DebugShow;
-use num_bigint::BigUint;
 
 /// The Brillig VM does not apply a limit to the memory address space,
 /// As a convention, we take use 64 bits. This means that we assume that
 /// memory has 2^64 memory slots.
-pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 32;
+pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 64;
 
 // Registers reserved in runtime for special purposes.
 pub(crate) enum ReservedRegisters {
@@ -213,13 +212,7 @@ impl BrilligContext {
         self.debug_show.array_get(array_ptr, index, result);
         // Computes array_ptr + index, ie array[index]
         let index_of_element_in_memory = self.allocate_register();
-        self.binary_instruction(
-            array_ptr,
-            index,
-            index_of_element_in_memory,
-            BrilligBinaryOp::Field { op: BinaryFieldOp::Add },
-        );
-
+        self.memory_op(array_ptr, index, index_of_element_in_memory, BinaryIntOp::Add);
         self.load_instruction(result, index_of_element_in_memory);
         // Free up temporary register
         self.deallocate_register(index_of_element_in_memory);
@@ -239,7 +232,10 @@ impl BrilligContext {
             array_ptr,
             index,
             index_of_element_in_memory,
-            BrilligBinaryOp::Field { op: BinaryFieldOp::Add },
+            BrilligBinaryOp::Integer {
+                op: BinaryIntOp::Add,
+                bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
+            },
         );
 
         self.store_instruction(index_of_element_in_memory, value);
@@ -562,6 +558,7 @@ impl BrilligContext {
         bit_size: u32,
     ) {
         self.debug_show.const_instruction(result, constant);
+
         self.push_opcode(BrilligOpcode::Const { destination: result, value: constant, bit_size });
     }
 
@@ -725,6 +722,8 @@ impl BrilligContext {
     /// Instead truncation instructions are emitted as to when a
     /// truncation should be done.
     /// For Brillig, all integer operations will overflow as its cheap.
+    /// We currently use cast to truncate: we cast to the required bit size
+    /// and back to the original bit size.
     pub(crate) fn truncate_instruction(
         &mut self,
         destination_of_truncated_value: SingleAddrVariable,
@@ -743,20 +742,12 @@ impl BrilligContext {
             value_to_truncate.bit_size
         );
 
-        let mask = BigUint::from(2_u32).pow(bit_size) - BigUint::from(1_u32);
-        let mask_constant = self.make_constant(
-            FieldElement::from_be_bytes_reduce(&mask.to_bytes_be()).into(),
-            value_to_truncate.bit_size,
-        );
-
-        self.binary_instruction(
-            value_to_truncate.address,
-            mask_constant,
-            destination_of_truncated_value.address,
-            BrilligBinaryOp::Integer { op: BinaryIntOp::And, bit_size: value_to_truncate.bit_size },
-        );
-
-        self.deallocate_register(mask_constant);
+        // We cast back and forth to ensure that the value is truncated.
+        let intermediate_register =
+            SingleAddrVariable { address: self.allocate_register(), bit_size };
+        self.cast_instruction(intermediate_register, value_to_truncate);
+        self.cast_instruction(destination_of_truncated_value, intermediate_register);
+        self.deallocate_register(intermediate_register.address);
     }
 
     /// Emits a stop instruction

compiler/wasm/test/fixtures/deps/lib-c/src/lib.nr

@@ -1 +1 @@
-mod module;
+mod module;

compiler/wasm/test/fixtures/deps/lib-c/src/module.nr

@@ -1 +1 @@
-mod foo;
+mod foo;

compiler/wasm/test/fixtures/deps/lib-c/src/module/foo.nr

@@ -1,3 +1,3 @@
 pub fn bar(param: Field) -> Field {
-  dep::std::hash::pedersen_hash([param])
+    dep::std::hash::pedersen_hash([param])
 }

compiler/wasm/test/fixtures/noir-contract/src/main.nr

@@ -8,5 +8,4 @@ contract TestContract {
     open fn openFunction() -> pub Field {
         42
     }
-
 }

docs/scripts/codegen_nargo_reference.sh

@@ -30,4 +30,4 @@ sidebar_position: 0
 ---
 " > $NARGO_REFERENCE
 
-cargo run -F codegen-docs -- info >> $NARGO_REFERENCE
+cargo run --bin nargo -F codegen-docs -- info >> $NARGO_REFERENCE

test_programs/compile_failure/assert_msg_runtime/src/main.nr

@@ -4,4 +4,4 @@ fn main(x: Field, y: pub Field) {
     let z = x + y;
     assert(z != y, f"Expected z != y, but got both equal {z}");
     assert_eq(x, y, f"Expected x == y, but x is {x} and y is {y}");
-}
+}

test_programs/compile_failure/brillig_assert_msg_runtime/src/main.nr

@@ -7,4 +7,4 @@ unconstrained fn conditional(x: Field) -> Field {
     assert_eq(z, 25, f"Expected 25 but got {z}");
     assert(x == 10, f"Expected x to equal 10, but got {x}");
     1
-}
+}

test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr

@@ -5,4 +5,4 @@ unconstrained fn mut_ref_identity(value: &mut Field) -> Field {
 fn main(mut x: Field, y: pub Field) {
     let returned_x = mut_ref_identity(&mut x);
     assert(returned_x == x);
-}
+}

test_programs/compile_success_contract/contract_with_impl/src/main.nr

@@ -2,6 +2,6 @@ contract Foo {
     struct T { x: [Field] }
 
     impl T {
-        fn t(self){}
+        fn t(self) {}
     }
 }

test_programs/compile_success_contract/simple_contract/src/main.nr

@@ -13,7 +13,7 @@ contract Foo {
     }
     // Regression for issue #3344
     #[contract_library_method]
-    fn foo(x : u8) -> u8 {
+    fn foo(x: u8) -> u8 {
         x
     }
 }

test_programs/execution_success/brillig_cow_regression/src/main.nr

@@ -8,9 +8,9 @@ global MAX_NEW_CONTRACTS_PER_TX: u64 = 1;
 global NUM_ENCRYPTED_LOGS_HASHES_PER_TX: u64 = 1;
 global NUM_UNENCRYPTED_LOGS_HASHES_PER_TX: u64 = 1;
 global NUM_FIELDS_PER_SHA256 = 2;
-global CALLDATA_HASH_INPUT_SIZE = 169;
-global CALL_DATA_HASH_LOG_FIELDS = 4;
-global CALL_DATA_HASH_FULL_FIELDS = 165;
+global TX_EFFECT_HASH_INPUT_SIZE = 169;
+global TX_EFFECT_HASH_LOG_FIELDS = 4;
+global TX_EFFECT_HASH_FULL_FIELDS = 165;
 
 struct PublicDataUpdateRequest {
     leaf_slot : Field,
@@ -99,7 +99,7 @@ impl U256 {
 }
 
 unconstrained fn main(kernel_data: DataToHash) -> pub [Field; NUM_FIELDS_PER_SHA256] {
-    let mut calldata_hash_inputs = [0; CALLDATA_HASH_INPUT_SIZE];
+    let mut tx_effects_hash_inputs = [0; TX_EFFECT_HASH_INPUT_SIZE];
 
     let new_note_hashes = kernel_data.new_note_hashes;
     let new_nullifiers = kernel_data.new_nullifiers;
@@ -111,65 +111,65 @@ unconstrained fn main(kernel_data: DataToHash) -> pub [Field; NUM_FIELDS_PER_SHA
     let mut offset = 0;
 
     for j in 0..MAX_NEW_NOTE_HASHES_PER_TX {
-        calldata_hash_inputs[offset + j] = new_note_hashes[j];
+        tx_effects_hash_inputs[offset + j] = new_note_hashes[j];
     }
     offset += MAX_NEW_NOTE_HASHES_PER_TX ;
 
     for j in 0..MAX_NEW_NULLIFIERS_PER_TX {
-        calldata_hash_inputs[offset + j] = new_nullifiers[j];
+        tx_effects_hash_inputs[offset + j] = new_nullifiers[j];
     }
     offset += MAX_NEW_NULLIFIERS_PER_TX ;
 
     for j in 0..MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX {
-        calldata_hash_inputs[offset + j * 2] =
+        tx_effects_hash_inputs[offset + j * 2] =
                 public_data_update_requests[j].leaf_slot;
-        calldata_hash_inputs[offset + j * 2 + 1] =
+        tx_effects_hash_inputs[offset + j * 2 + 1] =
                 public_data_update_requests[j].new_value;
     }
     offset += MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX * 2;
 
     for j in 0..MAX_NEW_L2_TO_L1_MSGS_PER_TX {
-        calldata_hash_inputs[offset + j] = newL2ToL1msgs[j];
+        tx_effects_hash_inputs[offset + j] = newL2ToL1msgs[j];
     }
     offset += MAX_NEW_L2_TO_L1_MSGS_PER_TX;
 
     let contract_leaf = kernel_data.new_contracts[0];
-    calldata_hash_inputs[offset] = contract_leaf.hash();
+    tx_effects_hash_inputs[offset] = contract_leaf.hash();
 
     offset += MAX_NEW_CONTRACTS_PER_TX;
 
     let new_contracts = kernel_data.new_contracts;
-    calldata_hash_inputs[offset] = new_contracts[0].contract_address;
+    tx_effects_hash_inputs[offset] = new_contracts[0].contract_address;
 
-    calldata_hash_inputs[offset + 1] = new_contracts[0].portal_contract_address;
+    tx_effects_hash_inputs[offset + 1] = new_contracts[0].portal_contract_address;
 
     offset += MAX_NEW_CONTRACTS_PER_TX * 2;
 
     for j in 0..NUM_FIELDS_PER_SHA256 {
-        calldata_hash_inputs[offset + j] = encryptedLogsHash[j];
+        tx_effects_hash_inputs[offset + j] = encryptedLogsHash[j];
     }
 
     offset += NUM_ENCRYPTED_LOGS_HASHES_PER_TX * NUM_FIELDS_PER_SHA256;
 
     for j in 0..NUM_FIELDS_PER_SHA256 {
-        calldata_hash_inputs[offset + j] = unencryptedLogsHash[j];
+        tx_effects_hash_inputs[offset + j] = unencryptedLogsHash[j];
     }
 
     offset += NUM_UNENCRYPTED_LOGS_HASHES_PER_TX * NUM_FIELDS_PER_SHA256;
-    assert_eq(offset, CALLDATA_HASH_INPUT_SIZE); // Sanity check
+    assert_eq(offset, TX_EFFECT_HASH_INPUT_SIZE); // Sanity check
 
-    let mut hash_input_flattened = [0; CALL_DATA_HASH_FULL_FIELDS * 32 + CALL_DATA_HASH_LOG_FIELDS * 16];
-    for offset in 0..CALL_DATA_HASH_FULL_FIELDS {
-        let input_as_bytes = calldata_hash_inputs[offset].to_be_bytes(32);
+    let mut hash_input_flattened = [0; TX_EFFECT_HASH_FULL_FIELDS * 32 + TX_EFFECT_HASH_LOG_FIELDS * 16];
+    for offset in 0..TX_EFFECT_HASH_FULL_FIELDS {
+        let input_as_bytes = tx_effects_hash_inputs[offset].to_be_bytes(32);
         for byte_index in 0..32 {
             hash_input_flattened[offset * 32 + byte_index] = input_as_bytes[byte_index];
         }
     }
 
-    for log_field_index in 0..CALL_DATA_HASH_LOG_FIELDS {
-        let input_as_bytes = calldata_hash_inputs[CALL_DATA_HASH_FULL_FIELDS + log_field_index].to_be_bytes(16);
+    for log_field_index in 0..TX_EFFECT_HASH_LOG_FIELDS {
+        let input_as_bytes = tx_effects_hash_inputs[TX_EFFECT_HASH_FULL_FIELDS + log_field_index].to_be_bytes(16);
         for byte_index in 0..16 {
-            hash_input_flattened[CALL_DATA_HASH_FULL_FIELDS * 32 + log_field_index * 16 + byte_index] = input_as_bytes[byte_index];
+            hash_input_flattened[TX_EFFECT_HASH_FULL_FIELDS * 32 + log_field_index * 16 + byte_index] = input_as_bytes[byte_index];
         }
     }
 

test_programs/execution_success/double_verify_proof/Nargo.toml

@@ -2,4 +2,6 @@
 name = "double_verify_proof"
 type = "bin"
 authors = [""]
-[dependencies]
+compiler_version = ">=0.24.0"
+
+[dependencies]

test_programs/execution_success/double_verify_proof/src/main.nr

@@ -1,6 +1,5 @@
 use dep::std;
 
-#[recursive]
 fn main(
     verification_key: [Field; 114],
     // This is the proof without public inputs attached.

test_programs/execution_success/double_verify_proof_recursive/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "double_verify_proof_recursive"
+type = "bin"
+authors = [""]
+[dependencies]