From 982aefeb523eab563d91f592e61b06f00e3d91aa Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 31 Jul 2023 19:06:24 +0000 Subject: [PATCH 1/4] update black box opcodes to accept multiple variables inputs and variable outputs, add RecursiveAggregation opcode --- crates/nargo_cli/tests/test_data/config.toml | 2 +- .../acir_gen/acir_ir/acir_variable.rs | 15 +- .../acir_gen/acir_ir/generated_acir.rs | 155 ++++++++++++++---- .../src/ssa_refactor/acir_gen/mod.rs | 6 +- noir_stdlib/src/lib.nr | 4 +- 5 files changed, 136 insertions(+), 46 deletions(-) diff --git a/crates/nargo_cli/tests/test_data/config.toml b/crates/nargo_cli/tests/test_data/config.toml index 88776ed03d2..4bcb7d5eab8 100644 --- a/crates/nargo_cli/tests/test_data/config.toml +++ b/crates/nargo_cli/tests/test_data/config.toml @@ -2,4 +2,4 @@ exclude = [] # List of tests (as their directory name) expecting to fail: if the test pass, we report an error. -fail = ["brillig_assert_fail", "dep_impl_primitive"] +fail = ["brillig_assert_fail", "dep_impl_primitive", "recursive_aggregation"] diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 8c7fe1e9b6a..5216c3c5225 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -240,7 +240,7 @@ impl AcirContext { typ: AcirType, ) -> Result { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs)?; + let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs, 1)?; Ok(outputs[0]) } @@ -252,7 +252,7 @@ impl AcirContext { typ: AcirType, ) -> Result { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1)?; Ok(outputs[0]) } @@ -279,7 +279,7 @@ impl AcirContext { let a = self.sub_var(max, lhs)?; let b = self.sub_var(max, rhs)?; let inputs = vec![AcirValue::Var(a, typ.clone()), AcirValue::Var(b, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::AND, inputs)?; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1)?; self.sub_var(max, outputs[0]) } } @@ -653,6 +653,7 @@ impl AcirContext { &mut self, name: BlackBoxFunc, mut inputs: Vec, + output_count: usize, ) -> Result, AcirGenError> { // Separate out any arguments that should be constants let constants = match name { @@ -674,7 +675,7 @@ impl AcirContext { let inputs = self.prepare_inputs_for_black_box_func_call(inputs)?; // Call Black box with `FunctionInput` - let outputs = self.acir_ir.call_black_box(name, inputs, constants); + let outputs = self.acir_ir.call_black_box(name, &inputs, constants, output_count); // Convert `Witness` values which are now constrained to be the output of the // black box function call into `AcirVar`s. @@ -690,9 +691,10 @@ impl AcirContext { fn prepare_inputs_for_black_box_func_call( &mut self, inputs: Vec, - ) -> Result, AcirGenError> { + ) -> Result>, AcirGenError> { let mut witnesses = Vec::new(); for input in inputs { + let mut single_val_witnesses = Vec::new(); for (input, typ) in input.flatten() { let var_data = &self.vars[&input]; @@ -702,8 +704,9 @@ impl AcirContext { let expr = var_data.to_expression(); let witness = self.acir_ir.get_or_create_witness(&expr); let num_bits = typ.bit_size(); - witnesses.push(FunctionInput { witness, num_bits }); + single_val_witnesses.push(FunctionInput { witness, num_bits }); } + witnesses.push(single_val_witnesses); } Ok(witnesses) } diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs index d80537a074a..5d9849ac317 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs @@ -117,12 +117,14 @@ impl GeneratedAcir { pub(crate) fn call_black_box( &mut self, func_name: BlackBoxFunc, - mut inputs: Vec, + inputs: &[Vec], constants: Vec, + output_count: usize, ) -> Vec { - intrinsics_check_inputs(func_name, &inputs); + let input_count = inputs.iter().fold(0usize, |sum, val| sum + val.len()); + intrinsics_check_inputs(func_name, input_count); + intrinsics_check_outputs(func_name, output_count); - let output_count = black_box_expected_output_size(func_name); let outputs = vecmap(0..output_count, |_| self.next_witness_index()); // clone is needed since outputs is moved when used in blackbox function. @@ -130,61 +132,89 @@ impl GeneratedAcir { let black_box_func_call = match func_name { BlackBoxFunc::AND => { - BlackBoxFuncCall::AND { lhs: inputs[0], rhs: inputs[1], output: outputs[0] } + BlackBoxFuncCall::AND { lhs: inputs[0][0], rhs: inputs[1][0], output: outputs[0] } } BlackBoxFunc::XOR => { - BlackBoxFuncCall::XOR { lhs: inputs[0], rhs: inputs[1], output: outputs[0] } + BlackBoxFuncCall::XOR { lhs: inputs[0][0], rhs: inputs[1][0], output: outputs[0] } } - BlackBoxFunc::RANGE => BlackBoxFuncCall::RANGE { input: inputs[0] }, - BlackBoxFunc::SHA256 => BlackBoxFuncCall::SHA256 { inputs, outputs }, - BlackBoxFunc::Blake2s => BlackBoxFuncCall::Blake2s { inputs, outputs }, - BlackBoxFunc::HashToField128Security => { - BlackBoxFuncCall::HashToField128Security { inputs, output: outputs[0] } + BlackBoxFunc::RANGE => BlackBoxFuncCall::RANGE { input: inputs[0][0] }, + BlackBoxFunc::SHA256 => BlackBoxFuncCall::SHA256 { inputs: inputs[0].clone(), outputs }, + BlackBoxFunc::Blake2s => { + BlackBoxFuncCall::Blake2s { inputs: inputs[0].clone(), outputs } } + BlackBoxFunc::HashToField128Security => BlackBoxFuncCall::HashToField128Security { + inputs: inputs[0].clone(), + output: outputs[0], + }, BlackBoxFunc::SchnorrVerify => BlackBoxFuncCall::SchnorrVerify { - public_key_x: inputs[0], - public_key_y: inputs[1], + public_key_x: inputs[0][0], + public_key_y: inputs[1][0], // Schnorr signature is an r & s, 32 bytes each - signature: inputs[2..66].to_vec(), - message: inputs[66..].to_vec(), + signature: inputs[2].clone(), + message: inputs[3].clone(), output: outputs[0], }, BlackBoxFunc::Pedersen => BlackBoxFuncCall::Pedersen { - inputs, + inputs: inputs[0].clone(), outputs: (outputs[0], outputs[1]), domain_separator: constants[0].to_u128() as u32, }, BlackBoxFunc::EcdsaSecp256k1 => BlackBoxFuncCall::EcdsaSecp256k1 { // 32 bytes for each public key co-ordinate - public_key_x: inputs[0..32].to_vec(), - public_key_y: inputs[32..64].to_vec(), + public_key_x: inputs[0].clone(), + public_key_y: inputs[1].clone(), // (r,s) are both 32 bytes each, so signature // takes up 64 bytes - signature: inputs[64..128].to_vec(), - hashed_message: inputs[128..].to_vec(), + signature: inputs[2].clone(), + hashed_message: inputs[3].clone(), output: outputs[0], }, BlackBoxFunc::EcdsaSecp256r1 => BlackBoxFuncCall::EcdsaSecp256r1 { // 32 bytes for each public key co-ordinate - public_key_x: inputs[0..32].to_vec(), - public_key_y: inputs[32..64].to_vec(), + public_key_x: inputs[0].clone(), + public_key_y: inputs[1].clone(), // (r,s) are both 32 bytes each, so signature // takes up 64 bytes - signature: inputs[64..128].to_vec(), - hashed_message: inputs[128..].to_vec(), + signature: inputs[2].clone(), + hashed_message: inputs[3].clone(), output: outputs[0], }, BlackBoxFunc::FixedBaseScalarMul => BlackBoxFuncCall::FixedBaseScalarMul { - input: inputs[0], + input: inputs[0][0], outputs: (outputs[0], outputs[1]), }, BlackBoxFunc::Keccak256 => { - let var_message_size = inputs.pop().expect("ICE: Missing message_size arg"); - BlackBoxFuncCall::Keccak256VariableLength { inputs, var_message_size, outputs } + let var_message_size = + inputs.to_vec().pop().expect("ICE: Missing message_size arg")[0]; + BlackBoxFuncCall::Keccak256VariableLength { + inputs: inputs[0].clone(), + var_message_size, + outputs, + } } // TODO(#1570): Generate ACIR for recursive aggregation BlackBoxFunc::RecursiveAggregation => { - panic!("ICE: Cannot generate ACIR for recursive aggregation") + let has_previous_aggregation = self.opcodes.iter().any(|op| { + matches!( + op, + AcirOpcode::BlackBoxFuncCall(BlackBoxFuncCall::RecursiveAggregation { .. }) + ) + }); + + let input_aggregation_object = if !has_previous_aggregation { + None + } else { + Some(inputs[4].clone()) + }; + + BlackBoxFuncCall::RecursiveAggregation { + verification_key: inputs[0].clone(), + proof: inputs[1].clone(), + public_inputs: inputs[2].clone(), + key_hash: inputs[3][0], + input_aggregation_object, + output_aggregation_object: outputs, + } } }; @@ -806,12 +836,8 @@ fn black_box_func_expected_input_size(name: BlackBoxFunc) -> Option { // Inputs for fixed based scalar multiplication // is just a scalar BlackBoxFunc::FixedBaseScalarMul => Some(1), - // TODO(#1570): Generate ACIR for recursive aggregation - // RecursiveAggregation has variable inputs and we could return `None` here, - // but as it is not fully implemented we panic for now - BlackBoxFunc::RecursiveAggregation => { - panic!("ICE: Cannot generate ACIR for recursive aggregation") - } + // Recursive aggregation has a variable number of inputs + BlackBoxFunc::RecursiveAggregation => None, } } @@ -845,6 +871,34 @@ fn black_box_expected_output_size(name: BlackBoxFunc) -> u32 { } } +/// This function will return the number of outputs that a blackbox function +/// expects. Returning `None` if there is no expectation. +fn black_box_expected_output_size_new(name: BlackBoxFunc) -> Option { + match name { + // Bitwise opcodes will return 1 parameter which is the output + // or the operation. + BlackBoxFunc::AND | BlackBoxFunc::XOR => Some(1), + // 32 byte hash algorithms + BlackBoxFunc::Keccak256 | BlackBoxFunc::SHA256 | BlackBoxFunc::Blake2s => Some(32), + // Hash to field returns a field element + BlackBoxFunc::HashToField128Security => Some(1), + // Pedersen returns a point + BlackBoxFunc::Pedersen => Some(2), + // Can only apply a range constraint to one + // witness at a time. + BlackBoxFunc::RANGE => Some(0), + // Signature verification algorithms will return a boolean + BlackBoxFunc::SchnorrVerify + | BlackBoxFunc::EcdsaSecp256k1 + | BlackBoxFunc::EcdsaSecp256r1 => Some(1), + // Output of fixed based scalar mul over the embedded curve + // will be 2 field elements representing the point. + BlackBoxFunc::FixedBaseScalarMul => Some(2), + // Recursive aggregation has a variable number of outputs + BlackBoxFunc::RecursiveAggregation => None, + } +} + /// Checks that the number of inputs being used to call the blackbox function /// is correct according to the function definition. /// @@ -860,12 +914,41 @@ fn black_box_expected_output_size(name: BlackBoxFunc) -> u32 { /// #[foreign(sha256)] /// fn sha256(_input : [u8; N]) -> [u8; 32] {} /// `` -fn intrinsics_check_inputs(name: BlackBoxFunc, inputs: &[FunctionInput]) { +fn intrinsics_check_inputs(name: BlackBoxFunc, input_count: usize) { let expected_num_inputs = match black_box_func_expected_input_size(name) { Some(expected_num_inputs) => expected_num_inputs, None => return, }; - let got_num_inputs = inputs.len(); - assert_eq!(expected_num_inputs,inputs.len(),"Tried to call black box function {name} with {got_num_inputs} inputs, but this function's definition requires {expected_num_inputs} inputs"); + assert_eq!(expected_num_inputs,input_count,"Tried to call black box function {name} with {input_count} inputs, but this function's definition requires {expected_num_inputs} inputs"); +} + +/// Checks that the number of outputs being used to call the blackbox function +/// is correct according to the function definition. +/// +/// Some functions expect a variable number of outputs and in such a case, +/// this method will do nothing. An example of this is recursive aggregation. +/// In that case, this function will not check anything. +/// +/// Since we expect black box functions to be called behind a Noir shim function, +/// we trigger a compiler error if the inputs do not match. +/// +/// An example of Noir shim function is the following: +/// `` +/// #[foreign(sha256)] +/// fn verify_proof( +/// _verification_key : [Field], +/// _proof : [Field], +/// _public_inputs : [Field], +/// _key_hash : Field, +/// _input_aggregation_object : [Field; N] +/// ) -> [Field; N] {} +/// `` +fn intrinsics_check_outputs(name: BlackBoxFunc, output_count: usize) { + let expected_num_outputs = match black_box_expected_output_size_new(name) { + Some(expected_num_inputs) => expected_num_inputs, + None => return, + }; + + assert_eq!(expected_num_outputs,output_count,"Tried to call black box function {name} with {output_count} inputs, but this function's definition requires {expected_num_outputs} inputs"); } diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 3bf18a2d86a..13edcc7eac4 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -880,7 +880,11 @@ impl Context { Intrinsic::BlackBox(black_box) => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - let vars = self.acir_context.black_box_function(black_box, inputs)?; + let output_count = result_ids.iter().fold(0usize, |sum, result_id| { + sum + dfg.try_get_array_length(*result_id).unwrap_or(1) + }); + + let vars = self.acir_context.black_box_function(black_box, inputs, output_count)?; Ok(Self::convert_vars_to_values(vars, dfg, result_ids)) } diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index f6c01ecdfaa..e654a20b1d8 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -18,11 +18,11 @@ mod compat; // Oracle calls are required to be wrapped in an unconstrained function // Thus, the only argument to the `println` oracle is expected to always be an ident #[oracle(println)] -unconstrained fn println_oracle(_input: T) {} +unconstrained fn println_oracle(_input: T) {} unconstrained fn println(input: T) { println_oracle(input); } #[foreign(recursive_aggregation)] -fn verify_proof(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field]) -> [Field] {} +fn verify_proof(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field; N]) -> [Field; N] {} From 8f6f962d8b1c3682656403b811187689265b9308 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 31 Jul 2023 19:14:44 +0000 Subject: [PATCH 2/4] remove old method and comment --- .../acir_gen/acir_ir/generated_acir.rs | 35 ++----------------- 1 file changed, 2 insertions(+), 33 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs index 5d9849ac317..0a7206fe421 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/generated_acir.rs @@ -192,7 +192,6 @@ impl GeneratedAcir { outputs, } } - // TODO(#1570): Generate ACIR for recursive aggregation BlackBoxFunc::RecursiveAggregation => { let has_previous_aggregation = self.opcodes.iter().any(|op| { matches!( @@ -843,37 +842,7 @@ fn black_box_func_expected_input_size(name: BlackBoxFunc) -> Option { /// This function will return the number of outputs that a blackbox function /// expects. Returning `None` if there is no expectation. -fn black_box_expected_output_size(name: BlackBoxFunc) -> u32 { - match name { - // Bitwise opcodes will return 1 parameter which is the output - // or the operation. - BlackBoxFunc::AND | BlackBoxFunc::XOR => 1, - // 32 byte hash algorithms - BlackBoxFunc::Keccak256 | BlackBoxFunc::SHA256 | BlackBoxFunc::Blake2s => 32, - // Hash to field returns a field element - BlackBoxFunc::HashToField128Security => 1, - // Pedersen returns a point - BlackBoxFunc::Pedersen => 2, - // Can only apply a range constraint to one - // witness at a time. - BlackBoxFunc::RANGE => 0, - // Signature verification algorithms will return a boolean - BlackBoxFunc::SchnorrVerify - | BlackBoxFunc::EcdsaSecp256k1 - | BlackBoxFunc::EcdsaSecp256r1 => 1, - // Output of fixed based scalar mul over the embedded curve - // will be 2 field elements representing the point. - BlackBoxFunc::FixedBaseScalarMul => 2, - // TODO(#1570): Generate ACIR for recursive aggregation - BlackBoxFunc::RecursiveAggregation => { - panic!("ICE: Cannot generate ACIR for recursive aggregation") - } - } -} - -/// This function will return the number of outputs that a blackbox function -/// expects. Returning `None` if there is no expectation. -fn black_box_expected_output_size_new(name: BlackBoxFunc) -> Option { +fn black_box_expected_output_size(name: BlackBoxFunc) -> Option { match name { // Bitwise opcodes will return 1 parameter which is the output // or the operation. @@ -945,7 +914,7 @@ fn intrinsics_check_inputs(name: BlackBoxFunc, input_count: usize) { /// ) -> [Field; N] {} /// `` fn intrinsics_check_outputs(name: BlackBoxFunc, output_count: usize) { - let expected_num_outputs = match black_box_expected_output_size_new(name) { + let expected_num_outputs = match black_box_expected_output_size(name) { Some(expected_num_inputs) => expected_num_inputs, None => return, }; From 93a844272a21c984d77c0ff784c3fc3258473b0c Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 31 Jul 2023 19:17:13 +0000 Subject: [PATCH 3/4] remove config change --- crates/nargo_cli/tests/test_data/config.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nargo_cli/tests/test_data/config.toml b/crates/nargo_cli/tests/test_data/config.toml index 4bcb7d5eab8..88776ed03d2 100644 --- a/crates/nargo_cli/tests/test_data/config.toml +++ b/crates/nargo_cli/tests/test_data/config.toml @@ -2,4 +2,4 @@ exclude = [] # List of tests (as their directory name) expecting to fail: if the test pass, we report an error. -fail = ["brillig_assert_fail", "dep_impl_primitive", "recursive_aggregation"] +fail = ["brillig_assert_fail", "dep_impl_primitive"] From c0b4c967838d2a7e8397bb6d039ad2a58c6d80cc Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 31 Jul 2023 19:32:55 +0000 Subject: [PATCH 4/4] remove NotImplemented InternalError --- crates/noirc_evaluator/src/errors.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index 6d53668d7cb..27a87ccce36 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -44,8 +44,6 @@ pub enum InternalError { MissingArg { name: String, arg: String, location: Option }, #[error("ICE: {name:?} should be a constant")] NotAConstant { name: String, location: Option }, - #[error("{name:?} is not implemented yet")] - NotImplemented { name: String, location: Option }, #[error("ICE: Undeclared AcirVar")] UndeclaredAcirVar { location: Option }, #[error("ICE: Expected {expected:?}, found {found:?}")] @@ -61,7 +59,6 @@ impl From for FileDiagnostic { | InternalError::General { location, .. } | InternalError::MissingArg { location, .. } | InternalError::NotAConstant { location, .. } - | InternalError::NotImplemented { location, .. } | InternalError::UndeclaredAcirVar { location } | InternalError::UnExpected { location, .. } => { let file_id = location.map(|loc| loc.file).unwrap();