From 73e23c8d2b30175a9baa1baa5d0ea9fb99e395b5 Mon Sep 17 00:00:00 2001 From: Aztec Bot <49558828+AztecBot@users.noreply.github.com> Date: Wed, 24 Apr 2024 16:12:27 +0100 Subject: [PATCH] feat: Sync from aztec-packages (#4902) Automated pull of Noir development from [aztec-packages](https://github.com/AztecProtocol/aztec-packages). BEGIN_COMMIT_OVERRIDE feat: Sync from noir (https://github.com/AztecProtocol/aztec-packages/pull/5999) refactor: using poseidon2 when computing a nullifier (https://github.com/AztecProtocol/aztec-packages/pull/5906) feat: Sync from noir (https://github.com/AztecProtocol/aztec-packages/pull/5955) feat: Sync from noir (https://github.com/AztecProtocol/aztec-packages/pull/5935) chore: redo typo PR by dockercui (https://github.com/AztecProtocol/aztec-packages/pull/5930) chore: Improve `compute_note_hash_and_nullifier` autogeneration and `NoteProcessor` warnings (https://github.com/AztecProtocol/aztec-packages/pull/5838) END_COMMIT_OVERRIDE --------- Co-authored-by: Tom French Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .aztec-sync-commit | 2 +- .github/actions/install-playwright/action.yml | 22 ++--- acvm-repo/acvm_js/build.sh | 4 +- aztec_macros/src/lib.rs | 13 +-- .../compute_note_hash_and_nullifier.rs | 94 ++++++++++++++++--- aztec_macros/src/transforms/functions.rs | 19 ++-- aztec_macros/src/transforms/note_interface.rs | 6 +- aztec_macros/src/transforms/storage.rs | 60 ++++-------- aztec_macros/src/utils/hir_utils.rs | 79 ++++++++++++++++ tooling/nargo/src/ops/execute.rs | 2 +- tooling/noirc_abi_wasm/build.sh | 1 + 11 files changed, 213 insertions(+), 89 deletions(-) diff --git a/.aztec-sync-commit b/.aztec-sync-commit index 1cb130e7c58..a159698f8a8 100644 --- a/.aztec-sync-commit +++ b/.aztec-sync-commit @@ -1 +1 @@ -beab8c93857536e07fa37994213fc664a5864013 +2e64428af9525bd8c390931061505f7b48d729a4 diff --git a/.github/actions/install-playwright/action.yml b/.github/actions/install-playwright/action.yml index a70c45d152a..0bd61b38c49 100644 --- a/.github/actions/install-playwright/action.yml +++ b/.github/actions/install-playwright/action.yml @@ -4,21 +4,21 @@ description: Installs Playwright and its dependencies and caches them. runs: using: composite steps: - - name: Query playwright version - shell: bash - run: echo "PLAYWRIGHT_VERSION=$(yarn workspace @noir-lang/noirc_abi info @web/test-runner-playwright --json | jq .children.Version | tr -d '"')" >> $GITHUB_ENV + # - name: Query playwright version + # shell: bash + # run: echo "PLAYWRIGHT_VERSION=$(yarn workspace @noir-lang/noirc_abi info @web/test-runner-playwright --json | jq .children.Version | tr -d '"')" >> $GITHUB_ENV - - name: Cache playwright binaries - uses: actions/cache@v4 - id: playwright-cache - with: - path: | - ~/.cache/ms-playwright - key: ${{ runner.os }}-playwright-${{ env.PLAYWRIGHT_VERSION }} + # - name: Cache playwright binaries + # uses: actions/cache@v4 + # id: playwright-cache + # with: + # path: | + # ~/.cache/ms-playwright + # key: ${{ runner.os }}-playwright-${{ env.PLAYWRIGHT_VERSION }} - name: Install playwright deps shell: bash - if: steps.playwright-cache.outputs.cache-hit != 'true' + # if: steps.playwright-cache.outputs.cache-hit != 'true' run: | # Workaround: https://github.com/microsoft/playwright/issues/30503#issuecomment-2074783821 sudo rm /etc/apt/sources.list.d/microsoft-prod.list diff --git a/acvm-repo/acvm_js/build.sh b/acvm-repo/acvm_js/build.sh index 4486a214c9c..16fb26e55db 100755 --- a/acvm-repo/acvm_js/build.sh +++ b/acvm-repo/acvm_js/build.sh @@ -49,5 +49,5 @@ BROWSER_WASM=${BROWSER_DIR}/${pname}_bg.wasm run_or_fail cargo build --lib --release --target $TARGET --package ${pname} run_or_fail wasm-bindgen $WASM_BINARY --out-dir $NODE_DIR --typescript --target nodejs run_or_fail wasm-bindgen $WASM_BINARY --out-dir $BROWSER_DIR --typescript --target web -run_or_fail wasm-opt $NODE_WASM -o $NODE_WASM -O -run_or_fail wasm-opt $BROWSER_WASM -o $BROWSER_WASM -O +run_if_available wasm-opt $NODE_WASM -o $NODE_WASM -O +run_if_available wasm-opt $BROWSER_WASM -o $BROWSER_WASM -O diff --git a/aztec_macros/src/lib.rs b/aztec_macros/src/lib.rs index dff3193a327..17ae999fb8f 100644 --- a/aztec_macros/src/lib.rs +++ b/aztec_macros/src/lib.rs @@ -93,17 +93,18 @@ fn transform_module( // Check for a user defined storage struct let maybe_storage_struct_name = check_for_storage_definition(module)?; + let storage_defined = maybe_storage_struct_name.is_some(); - if let Some(storage_struct_name) = maybe_storage_struct_name { - if !check_for_storage_implementation(module, &storage_struct_name) { - generate_storage_implementation(module, &storage_struct_name)?; + if let Some(ref storage_struct_name) = maybe_storage_struct_name { + if !check_for_storage_implementation(module, storage_struct_name) { + generate_storage_implementation(module, storage_struct_name)?; } // Make sure we're only generating the storage layout for the root crate // In case we got a contract importing other contracts for their interface, we // don't want to generate the storage layout for them if crate_id == context.root_crate_id() { - generate_storage_layout(module, storage_struct_name)?; + generate_storage_layout(module, storage_struct_name.clone())?; } } @@ -164,14 +165,14 @@ fn transform_module( transform_function( fn_type, func, - storage_defined, + maybe_storage_struct_name.clone(), is_initializer, insert_init_check, is_internal, )?; has_transformed_module = true; } else if storage_defined && func.def.is_unconstrained { - transform_unconstrained(func); + transform_unconstrained(func, maybe_storage_struct_name.clone().unwrap()); has_transformed_module = true; } } diff --git a/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs b/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs index 70d05e5c59e..f624cde9969 100644 --- a/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs +++ b/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs @@ -8,7 +8,10 @@ use noirc_frontend::{ use crate::utils::{ errors::AztecMacroError, - hir_utils::{collect_crate_functions, fetch_notes, get_contract_module_data, inject_fn}, + hir_utils::{ + collect_crate_functions, collect_traits, fetch_notes, get_contract_module_data, + get_global_numberic_const, get_serialized_length, inject_fn, + }, }; // Check if "compute_note_hash_and_nullifier(AztecAddress,Field,Field,Field,[Field; N]) -> [Field; 4]" is defined @@ -60,13 +63,68 @@ pub fn inject_compute_note_hash_and_nullifier( return Ok(()); } + let traits: Vec<_> = collect_traits(context); + + // Get MAX_NOTE_FIELDS_LENGTH global to check if the notes in our contract are too long. + let max_note_length_const = get_global_numberic_const(context, "MAX_NOTE_FIELDS_LENGTH") + .map_err(|err| { + ( + AztecMacroError::CouldNotImplementComputeNoteHashAndNullifier { + secondary_message: Some(err.primary_message), + }, + file_id, + ) + })?; + // In order to implement compute_note_hash_and_nullifier, we need to know all of the different note types the - // contract might use. These are the types that are marked as #[aztec(note)]. + // contract might use and their serialized lengths. These are the types that are marked as #[aztec(note)]. + let mut notes_and_lengths = vec![]; + + for (path, typ) in fetch_notes(context) { + let serialized_len: u128 = get_serialized_length( + &traits, + "NoteInterface", + &Type::Struct(typ.clone(), vec![]), + &context.def_interner, + ) + .map_err(|_err| { + ( + AztecMacroError::CouldNotImplementComputeNoteHashAndNullifier { + secondary_message: Some(format!( + "Failed to get serialized length for note type {}", + path + )), + }, + file_id, + ) + })? + .into(); + + if serialized_len > max_note_length_const { + return Err(( + AztecMacroError::CouldNotImplementComputeNoteHashAndNullifier { + secondary_message: Some(format!( + "Note type {} as {} fields, which is more than the maximum allowed length of {}.", + path, + serialized_len, + max_note_length_const + )), + }, + file_id, + )); + } + + notes_and_lengths.push((path.to_string(), serialized_len)); + } + + let max_note_length: u128 = + *notes_and_lengths.iter().map(|(_, serialized_len)| serialized_len).max().unwrap_or(&0); + let note_types = - fetch_notes(context).iter().map(|(path, _)| path.to_string()).collect::>(); + notes_and_lengths.iter().map(|(note_type, _)| note_type.clone()).collect::>(); // We can now generate a version of compute_note_hash_and_nullifier tailored for the contract in this crate. - let func = generate_compute_note_hash_and_nullifier(¬e_types); + let func = generate_compute_note_hash_and_nullifier(¬e_types, max_note_length); // And inject the newly created function into the contract. @@ -86,8 +144,12 @@ pub fn inject_compute_note_hash_and_nullifier( Ok(()) } -fn generate_compute_note_hash_and_nullifier(note_types: &[String]) -> NoirFunction { - let function_source = generate_compute_note_hash_and_nullifier_source(note_types); +fn generate_compute_note_hash_and_nullifier( + note_types: &[String], + max_note_length: u128, +) -> NoirFunction { + let function_source = + generate_compute_note_hash_and_nullifier_source(note_types, max_note_length); let (function_ast, errors) = parse_program(&function_source); if !errors.is_empty() { @@ -99,25 +161,30 @@ fn generate_compute_note_hash_and_nullifier(note_types: &[String]) -> NoirFuncti function_ast.functions.remove(0) } -fn generate_compute_note_hash_and_nullifier_source(note_types: &[String]) -> String { +fn generate_compute_note_hash_and_nullifier_source( + note_types: &[String], + max_note_length: u128, +) -> String { // TODO(#4649): The serialized_note parameter is a fixed-size array, but we don't know what length it should have. // For now we hardcode it to 20, which is the same as MAX_NOTE_FIELDS_LENGTH. if note_types.is_empty() { // 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. - " + format!( + " unconstrained fn compute_note_hash_and_nullifier( contract_address: dep::aztec::protocol_types::address::AztecAddress, nonce: Field, storage_slot: Field, note_type_id: Field, - serialized_note: [Field; 20] - ) -> pub [Field; 4] { + serialized_note: [Field; {}] + ) -> pub [Field; 4] {{ assert(false, \"This contract does not use private notes\"); [0, 0, 0, 0] - }" - .to_string() + }}", + max_note_length + ) } else { // For contracts that include notes we do a simple if-else chain comparing note_type_id with the different // get_note_type_id of each of the note types. @@ -142,12 +209,13 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &[String]) -> Str nonce: Field, storage_slot: Field, note_type_id: Field, - serialized_note: [Field; 20] + serialized_note: [Field; {}] ) -> pub [Field; 4] {{ let note_header = dep::aztec::prelude::NoteHeader::new(contract_address, nonce, storage_slot); {} }}", + max_note_length, full_if_statement ) } diff --git a/aztec_macros/src/transforms/functions.rs b/aztec_macros/src/transforms/functions.rs index dd7a416b941..24cad05b866 100644 --- a/aztec_macros/src/transforms/functions.rs +++ b/aztec_macros/src/transforms/functions.rs @@ -29,7 +29,7 @@ use crate::{ pub fn transform_function( ty: &str, func: &mut NoirFunction, - storage_defined: bool, + storage_struct_name: Option, is_initializer: bool, insert_init_check: bool, is_internal: bool, @@ -57,8 +57,8 @@ pub fn transform_function( } // Add access to the storage struct - if storage_defined { - let storage_def = abstract_storage(&ty.to_lowercase(), false); + if let Some(storage_struct_name) = storage_struct_name { + let storage_def = abstract_storage(storage_struct_name, &ty.to_lowercase(), false); func.def.body.statements.insert(0, storage_def); } @@ -209,8 +209,11 @@ pub fn export_fn_abi( /// ``` /// /// This will allow developers to access their contract' storage struct in unconstrained functions -pub fn transform_unconstrained(func: &mut NoirFunction) { - func.def.body.statements.insert(0, abstract_storage("Unconstrained", true)); +pub fn transform_unconstrained(func: &mut NoirFunction, storage_struct_name: String) { + func.def + .body + .statements + .insert(0, abstract_storage(storage_struct_name, "Unconstrained", true)); } /// Helper function that returns what the private context would look like in the ast @@ -575,7 +578,7 @@ fn abstract_return_values(func: &NoirFunction) -> Result>, /// unconstrained fn lol() { /// let storage = Storage::init(Context::none()); /// } -fn abstract_storage(typ: &str, unconstrained: bool) -> Statement { +fn abstract_storage(storage_struct_name: String, typ: &str, unconstrained: bool) -> Statement { let init_context_call = if unconstrained { call( variable_path(chained_dep!("aztec", "context", "Context", "none")), // Path @@ -591,8 +594,8 @@ fn abstract_storage(typ: &str, unconstrained: bool) -> Statement { assignment( "storage", // Assigned to call( - variable_path(chained_path!("Storage", "init")), // Path - vec![init_context_call], // args + variable_path(chained_path!(storage_struct_name.as_str(), "init")), // Path + vec![init_context_call], // args ), ) } diff --git a/aztec_macros/src/transforms/note_interface.rs b/aztec_macros/src/transforms/note_interface.rs index 70db1ebd336..f183c69b27a 100644 --- a/aztec_macros/src/transforms/note_interface.rs +++ b/aztec_macros/src/transforms/note_interface.rs @@ -418,8 +418,7 @@ fn generate_note_properties_fn( // Automatically generate the method to compute the note's content hash as: // fn compute_note_content_hash(self: NoteType) -> Field { -// // TODO(#1205) Should use a non-zero generator index. -// dep::aztec::hash::pedersen_hash(self.serialize_content(), 0) +// dep::aztec::hash::pedersen_hash(self.serialize_content(), dep::aztec::protocol_types::constants::GENERATOR_INDEX__NOTE_CONTENT_HASH) // } // fn generate_compute_note_content_hash( @@ -429,8 +428,7 @@ fn generate_compute_note_content_hash( let function_source = format!( " fn compute_note_content_hash(self: {}) -> Field {{ - // TODO(#1205) Should use a non-zero generator index. - dep::aztec::hash::pedersen_hash(self.serialize_content(), 0) + dep::aztec::hash::pedersen_hash(self.serialize_content(), dep::aztec::protocol_types::constants::GENERATOR_INDEX__NOTE_CONTENT_HASH) }} ", note_type diff --git a/aztec_macros/src/transforms/storage.rs b/aztec_macros/src/transforms/storage.rs index 49fcee9fe29..1e3cc011715 100644 --- a/aztec_macros/src/transforms/storage.rs +++ b/aztec_macros/src/transforms/storage.rs @@ -1,5 +1,3 @@ -use std::borrow::Borrow; - use noirc_errors::Span; use noirc_frontend::ast::{ BlockExpression, Expression, ExpressionKind, FunctionDefinition, Ident, Literal, NoirFunction, @@ -10,7 +8,7 @@ use noirc_frontend::{ macros_api::{ FieldElement, FileId, HirContext, HirExpression, HirLiteral, HirStatement, NodeInterner, }, - node_interner::{TraitId, TraitImplKind}, + node_interner::TraitId, parse_program, parser::SortedModule, token::SecondaryAttribute, @@ -25,7 +23,9 @@ use crate::{ make_type, pattern, return_type, variable, variable_path, }, errors::AztecMacroError, - hir_utils::{collect_crate_structs, collect_traits, get_contract_module_data}, + hir_utils::{ + collect_crate_structs, collect_traits, get_contract_module_data, get_serialized_length, + }, }, }; @@ -198,7 +198,7 @@ pub fn generate_storage_implementation( } /// Obtains the serialized length of a type that implements the Serialize trait. -fn get_serialized_length( +pub fn get_storage_serialized_length( traits: &[TraitId], typ: &Type, interner: &NodeInterner, @@ -216,48 +216,22 @@ fn get_serialized_length( secondary_message: Some("State storage variable must be generic".to_string()), })?; - let is_note = traits.iter().any(|&trait_id| { - let r#trait = interner.get_trait(trait_id); - r#trait.name.0.contents == "NoteInterface" - && !interner.lookup_all_trait_implementations(stored_in_state, trait_id).is_empty() - }); + let is_note = match stored_in_state { + Type::Struct(typ, _) => interner + .struct_attributes(&typ.borrow().id) + .iter() + .any(|attr| is_custom_attribute(attr, "aztec(note)")), + _ => false, + }; // Maps and (private) Notes always occupy a single slot. Someone could store a Note in PublicMutable for whatever reason though. if struct_name == "Map" || (is_note && struct_name != "PublicMutable") { return Ok(1); } - let serialized_trait_impl_kind = traits - .iter() - .find_map(|&trait_id| { - let r#trait = interner.get_trait(trait_id); - if r#trait.borrow().name.0.contents == "Serialize" - && r#trait.borrow().generics.len() == 1 - { - interner - .lookup_all_trait_implementations(stored_in_state, trait_id) - .into_iter() - .next() - } else { - None - } - }) - .ok_or(AztecMacroError::CouldNotAssignStorageSlots { - secondary_message: Some("Stored data must implement Serialize trait".to_string()), - })?; - - let serialized_trait_impl_id = match serialized_trait_impl_kind { - TraitImplKind::Normal(trait_impl_id) => Ok(trait_impl_id), - _ => Err(AztecMacroError::CouldNotAssignStorageSlots { secondary_message: None }), - }?; - - let serialized_trait_impl_shared = interner.get_trait_implementation(*serialized_trait_impl_id); - let serialized_trait_impl = serialized_trait_impl_shared.borrow(); - - match serialized_trait_impl.trait_generics.first().unwrap() { - Type::Constant(value) => Ok(*value), - _ => Err(AztecMacroError::CouldNotAssignStorageSlots { secondary_message: None }), - } + get_serialized_length(traits, "Serialize", stored_in_state, interner).map_err(|err| { + AztecMacroError::CouldNotAssignStorageSlots { secondary_message: Some(err.primary_message) } + }) } /// Assigns storage slots to the storage struct fields based on the serialized length of the types. This automatic assignment @@ -438,7 +412,7 @@ pub fn assign_storage_slots( }; let type_serialized_len = - get_serialized_length(&traits, field_type, &context.def_interner) + get_storage_serialized_length(&traits, field_type, &context.def_interner) .map_err(|err| (err, file_id))?; context.def_interner.update_expression(new_call_expression.arguments[1], |expr| { @@ -506,7 +480,7 @@ pub fn generate_storage_layout( let (struct_ast, errors) = parse_program(&storage_fields_source); if !errors.is_empty() { dbg!(errors); - return Err(AztecMacroError::CouldNotImplementNoteInterface { + return Err(AztecMacroError::CouldNotExportStorageLayout { secondary_message: Some("Failed to parse Noir macro code (struct StorageLayout). This is either a bug in the compiler or the Noir macro code".to_string()), span: None }); diff --git a/aztec_macros/src/utils/hir_utils.rs b/aztec_macros/src/utils/hir_utils.rs index cafa6ed38ca..3801ff1cc75 100644 --- a/aztec_macros/src/utils/hir_utils.rs +++ b/aztec_macros/src/utils/hir_utils.rs @@ -1,6 +1,8 @@ use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast; +use noirc_frontend::macros_api::{HirExpression, HirLiteral}; +use noirc_frontend::node_interner::{NodeInterner, TraitImplKind}; use noirc_frontend::{ graph::CrateId, hir::{ @@ -310,3 +312,80 @@ fn find_non_contract_dependencies_bfs( }) }) } + +pub fn get_serialized_length( + traits: &[TraitId], + trait_name: &str, + typ: &Type, + interner: &NodeInterner, +) -> Result { + let serialized_trait_impl_kind = traits + .iter() + .find_map(|&trait_id| { + let r#trait = interner.get_trait(trait_id); + if r#trait.name.0.contents == trait_name && r#trait.generics.len() == 1 { + interner.lookup_all_trait_implementations(typ, trait_id).into_iter().next() + } else { + None + } + }) + .ok_or(MacroError { + primary_message: format!("Type {} must implement {} trait", typ, trait_name), + secondary_message: None, + span: None, + })?; + + let serialized_trait_impl_id = match serialized_trait_impl_kind { + TraitImplKind::Normal(trait_impl_id) => Ok(trait_impl_id), + _ => Err(MacroError { + primary_message: format!("{} trait impl for {} must not be assumed", trait_name, typ), + secondary_message: None, + span: None, + }), + }?; + + let serialized_trait_impl_shared = interner.get_trait_implementation(*serialized_trait_impl_id); + let serialized_trait_impl = serialized_trait_impl_shared.borrow(); + + match serialized_trait_impl.trait_generics.first().unwrap() { + Type::Constant(value) => Ok(*value), + _ => Err(MacroError { + primary_message: format!("{} length for {} must be a constant", trait_name, typ), + secondary_message: None, + span: None, + }), + } +} + +pub fn get_global_numberic_const( + context: &HirContext, + const_name: &str, +) -> Result { + context + .def_interner + .get_all_globals() + .iter() + .find_map(|global_info| { + if global_info.ident.0.contents == const_name { + let stmt = context.def_interner.get_global_let_statement(global_info.id); + if let Some(let_stmt) = stmt { + let expression = context.def_interner.expression(&let_stmt.expression); + match expression { + HirExpression::Literal(HirLiteral::Integer(value, _)) => { + Some(value.to_u128()) + } + _ => None, + } + } else { + None + } + } else { + None + } + }) + .ok_or(MacroError { + primary_message: format!("Could not find {} global constant", const_name), + secondary_message: None, + span: None, + }) +} diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 97584aff150..34755d14ed2 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -106,7 +106,7 @@ impl<'a, B: BlackBoxFunctionSolver, F: ForeignCallExecutor> ProgramExecutor<'a, return Err(NargoError::ExecutionError(match call_stack { Some(call_stack) => { // First check whether we have a runtime assertion message that should be resolved on an ACVM failure - // If we do not have a runtime assertion message, we check wether the error is a brillig error with a user-defined message, + // If we do not have a runtime assertion message, we check whether the error is a brillig error with a user-defined message, // and finally we should check whether the circuit has any hardcoded messages associated with a specific `OpcodeLocation`. // Otherwise return the provided opcode resolution error. if let Some(assert_message) = assert_message { diff --git a/tooling/noirc_abi_wasm/build.sh b/tooling/noirc_abi_wasm/build.sh index 58724dee02c..16fb26e55db 100755 --- a/tooling/noirc_abi_wasm/build.sh +++ b/tooling/noirc_abi_wasm/build.sh @@ -25,6 +25,7 @@ function run_if_available { require_command jq require_command cargo require_command wasm-bindgen +require_command wasm-opt self_path=$(dirname "$(readlink -f "$0")") pname=$(cargo read-manifest | jq -r '.name')