From 93ec6d0da52721c79fad17f6523e70418b55e5ff Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 31 Jan 2023 19:23:58 +0000 Subject: [PATCH 01/10] feat: add an explicit `return_type` field to ABI --- crates/nargo/src/cli/execute_cmd.rs | 7 +- crates/nargo/src/cli/prove_cmd.rs | 22 +++- crates/nargo/src/cli/verify_cmd.rs | 20 ++-- crates/noirc_abi/src/lib.rs | 101 ++++++++++++++---- crates/noirc_evaluator/src/lib.rs | 10 +- crates/noirc_frontend/src/hir_def/function.rs | 20 ++-- 6 files changed, 132 insertions(+), 48 deletions(-) diff --git a/crates/nargo/src/cli/execute_cmd.rs b/crates/nargo/src/cli/execute_cmd.rs index 492c14529a3..dc52ab4d9f8 100644 --- a/crates/nargo/src/cli/execute_cmd.rs +++ b/crates/nargo/src/cli/execute_cmd.rs @@ -4,7 +4,7 @@ use acvm::acir::native_types::Witness; use acvm::PartialWitnessGenerator; use clap::Args; use noirc_abi::input_parser::{Format, InputValue}; -use noirc_abi::{InputMap, WitnessMap, MAIN_RETURN_NAME}; +use noirc_abi::{InputMap, WitnessMap}; use noirc_driver::CompiledProgram; use super::NargoConfig; @@ -75,8 +75,7 @@ pub(crate) fn execute_program( let solved_witness = solve_witness(compiled_program, inputs_map)?; let public_abi = compiled_program.abi.clone().public_abi(); - let public_inputs = public_abi.decode(&solved_witness)?; - let return_value = public_inputs.get(MAIN_RETURN_NAME).cloned(); + let (_, return_value) = public_abi.decode(&solved_witness)?; Ok((return_value, solved_witness)) } @@ -85,7 +84,7 @@ pub(crate) fn solve_witness( compiled_program: &CompiledProgram, input_map: &InputMap, ) -> Result { - let mut solved_witness = compiled_program.abi.encode(input_map, true)?; + let mut solved_witness = compiled_program.abi.encode(input_map, None)?; let backend = crate::backends::ConcreteBackend; backend.solve(&mut solved_witness, compiled_program.circuit.opcodes.clone())?; diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index f149301e108..0267a8e3869 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -2,7 +2,7 @@ use std::path::{Path, PathBuf}; use acvm::ProofSystemCompiler; use clap::Args; -use noirc_abi::input_parser::Format; +use noirc_abi::{input_parser::Format, MAIN_RETURN_NAME}; use super::{ create_named_dir, fetch_pk_and_vk, read_inputs_from_file, write_inputs_to_file, write_to_file, @@ -92,8 +92,21 @@ pub fn prove_with_path>( // Write public inputs into Verifier.toml let public_abi = compiled_program.abi.clone().public_abi(); - let public_inputs = public_abi.decode(&solved_witness)?; - write_inputs_to_file(&public_inputs, &program_dir, VERIFIER_INPUT_FILE, Format::Toml)?; + let (public_inputs, return_value) = public_abi.decode(&solved_witness)?; + + if let Some(return_value) = return_value.clone() { + // Insert return value into public inputs so it's written to file. + let mut public_inputs_with_return = public_inputs.clone(); + public_inputs_with_return.insert(MAIN_RETURN_NAME.to_owned(), return_value); + write_inputs_to_file( + &public_inputs_with_return, + &program_dir, + VERIFIER_INPUT_FILE, + Format::Toml, + )?; + } else { + write_inputs_to_file(&public_inputs, &program_dir, VERIFIER_INPUT_FILE, Format::Toml)?; + } let backend = crate::backends::ConcreteBackend; let proof = @@ -101,7 +114,8 @@ pub fn prove_with_path>( println!("Proof successfully created"); if check_proof { - let valid_proof = verify_proof(compiled_program, public_inputs, &proof, verification_key)?; + let valid_proof = + verify_proof(compiled_program, public_inputs, return_value, &proof, verification_key)?; println!("Proof verified : {valid_proof}"); if !valid_proof { return Err(CliError::Generic("Could not verify generated proof".to_owned())); diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index 391a384e2c6..55fde3e7f59 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -8,7 +8,8 @@ use crate::{ }; use acvm::{FieldElement, ProofSystemCompiler}; use clap::Args; -use noirc_abi::input_parser::Format; +use noirc_abi::input_parser::{Format, InputValue}; +use noirc_abi::{errors::AbiError, MAIN_RETURN_NAME}; use noirc_driver::CompiledProgram; use std::{collections::BTreeMap, path::Path}; @@ -64,20 +65,22 @@ pub fn verify_with_path>( let (_, verification_key) = fetch_pk_and_vk(compiled_program.circuit.clone(), circuit_build_path, false, true)?; - let mut public_inputs_map: InputMap = BTreeMap::new(); - // Load public inputs (if any) from `VERIFIER_INPUT_FILE`. let public_abi = compiled_program.abi.clone().public_abi(); - let num_pub_params = public_abi.num_parameters(); - if num_pub_params != 0 { + let (public_inputs_map, return_value) = if public_abi.has_public_inputs() { let current_dir = program_dir; - public_inputs_map = + let mut public_inputs_map = read_inputs_from_file(current_dir, VERIFIER_INPUT_FILE, Format::Toml, &public_abi)?; - } + let return_value = public_inputs_map.remove(MAIN_RETURN_NAME); + (public_inputs_map, return_value) + } else { + (BTreeMap::new(), None) + }; let valid_proof = verify_proof( compiled_program, public_inputs_map, + return_value, &load_hex_data(proof_path)?, verification_key, )?; @@ -88,11 +91,12 @@ pub fn verify_with_path>( pub(crate) fn verify_proof( compiled_program: CompiledProgram, public_inputs_map: InputMap, + return_value: Option, proof: &[u8], verification_key: Vec, ) -> Result { let public_abi = compiled_program.abi.public_abi(); - let public_inputs = public_abi.encode(&public_inputs_map, false)?; + let public_inputs = public_abi.encode(&public_inputs_map, return_value)?; let public_inputs_vec: Vec = public_inputs.values().copied().collect(); diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index e3250a5d335..7d545fc373f 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -130,6 +130,8 @@ pub struct Abi { /// A map from the ABI's parameters to the indices they are written to in the [`WitnessMap`]. /// This defines how to convert between the [`InputMap`] and [`WitnessMap`]. pub param_witnesses: BTreeMap>, + pub return_type: Option, + pub return_witnesses: Vec, } impl Abi { @@ -146,6 +148,11 @@ impl Abi { self.parameters.iter().map(|param| param.typ.field_count()).sum() } + /// Returns whether any values are needed to be made public for verification. + pub fn has_public_inputs(&self) -> bool { + self.return_type.is_some() || self.parameters.iter().any(|param| param.is_public()) + } + pub fn to_btree_map(&self) -> BTreeMap { let mut map = BTreeMap::new(); for param in self.parameters.iter() { @@ -164,18 +171,26 @@ impl Abi { .into_iter() .filter(|(param_name, _)| parameters.iter().any(|param| ¶m.name == param_name)) .collect(); - Abi { parameters, param_witnesses } + Abi { + parameters, + param_witnesses, + return_type: self.return_type, + return_witnesses: self.return_witnesses, + } } /// Encode a set of inputs as described in the ABI into a `WitnessMap`. - pub fn encode(&self, input_map: &InputMap, skip_output: bool) -> Result { + pub fn encode( + &self, + input_map: &InputMap, + return_value: Option, + ) -> Result { self.check_for_unexpected_inputs(input_map)?; // First encode each input separately, performing any input validation. let encoded_input_map: BTreeMap> = self .to_btree_map() .into_iter() - .filter(|(param_name, _)| !skip_output || param_name != MAIN_RETURN_NAME) .map(|(param_name, expected_type)| { let value = input_map .get(¶m_name) @@ -197,7 +212,7 @@ impl Abi { .collect::>()?; // Write input field elements into witness indices specified in `self.param_witnesses`. - let witness_map = encoded_input_map + let mut witness_map: WitnessMap = encoded_input_map .iter() .flat_map(|(param_name, encoded_param_fields)| { let param_witness_indices = &self.param_witnesses[param_name]; @@ -208,6 +223,34 @@ impl Abi { }) .collect(); + // The user can optionally provide a return value to be inserted into the witness map. + // This is to be used when encoding public values to be passed to the verifier. + match (&self.return_type, return_value) { + (Some(return_type), Some(return_value)) => { + if !return_value.matches_abi(return_type) { + panic!("Unexpected return value") + } + let encoded_return_fields = Self::encode_value(return_value)?; + let return_witness_indices = &self.return_witnesses; + + // We need to be more careful when writing the return value's witness values. + // This is as it may share witness indices with other public inputs so we must check that when + // this occurs the witness values are consistent with each other. + return_witness_indices.iter().zip(encoded_return_fields.iter()).try_for_each( + |(&witness, &field_element)| match witness_map.insert(witness, field_element) { + Some(existing_value) if existing_value != field_element => { + panic!("Inconsistent return value"); + } + _ => Ok(()), + }, + )?; + } + (None, Some(_)) => panic!("Unexpected return value"), + // We allow not passing a return value despite the circuit defining one + // in order to generate the initial partial witness. + (_, None) => {} + } + Ok(witness_map) } @@ -243,7 +286,10 @@ impl Abi { } /// Decode a `WitnessMap` into the types specified in the ABI. - pub fn decode(&self, witness_map: &WitnessMap) -> Result { + pub fn decode( + &self, + witness_map: &WitnessMap, + ) -> Result<(InputMap, Option), AbiError> { let public_inputs_map = try_btree_map(self.parameters.clone(), |AbiParameter { name, typ, .. }| { let param_witness_values = @@ -261,7 +307,31 @@ impl Abi { .map(|input_value| (name.clone(), input_value)) })?; - Ok(public_inputs_map) + // We also attempt to decode the circuit's return value from `witness_map`. + let return_value = if let Some(return_type) = &self.return_type { + if let Ok(return_witness_values) = + try_vecmap(self.return_witnesses.clone(), |witness_index| { + witness_map + .get(&witness_index) + .ok_or_else(|| AbiError::MissingParamWitnessValue { + name: MAIN_RETURN_NAME.to_string(), + witness_index, + }) + .copied() + }) + { + Some(Self::decode_value(&mut return_witness_values.into_iter(), return_type)?) + } else { + // Unlike for the circuit inputs, we tolerate not being able to find the witness values for the return value. + // This is because the user may be decoding a partial witness map for which is hasn't been calculated yet. + // If a return value is expected, this should be checked for by the user. + None + } + } else { + None + }; + + Ok((public_inputs_map, return_value)) } fn decode_value( @@ -323,10 +393,7 @@ mod test { use acvm::{acir::native_types::Witness, FieldElement}; - use crate::{ - input_parser::InputValue, Abi, AbiParameter, AbiType, AbiVisibility, InputMap, - MAIN_RETURN_NAME, - }; + use crate::{input_parser::InputValue, Abi, AbiParameter, AbiType, AbiVisibility, InputMap}; #[test] fn witness_encoding_roundtrip() { @@ -342,18 +409,14 @@ mod test { typ: AbiType::Field, visibility: AbiVisibility::Public, }, - AbiParameter { - name: MAIN_RETURN_NAME.to_string(), - typ: AbiType::Field, - visibility: AbiVisibility::Public, - }, ], // Note that the return value shares a witness with `thing2` param_witnesses: BTreeMap::from([ ("thing1".to_string(), vec![Witness(1), Witness(2)]), ("thing2".to_string(), vec![Witness(3)]), - (MAIN_RETURN_NAME.to_string(), vec![Witness(3)]), ]), + return_type: Some(AbiType::Field), + return_witnesses: vec![Witness(3)], }; // Note we omit return value from inputs @@ -362,14 +425,14 @@ mod test { ("thing2".to_string(), InputValue::Field(FieldElement::zero())), ]); - let witness_map = abi.encode(&inputs, true).unwrap(); - let reconstructed_inputs = abi.decode(&witness_map).unwrap(); + let witness_map = abi.encode(&inputs, None).unwrap(); + let (reconstructed_inputs, return_value) = abi.decode(&witness_map).unwrap(); for (key, expected_value) in inputs { assert_eq!(reconstructed_inputs[&key], expected_value); } // We also decode the return value (we can do this immediately as we know it shares a witness with an input). - assert_eq!(reconstructed_inputs[MAIN_RETURN_NAME], reconstructed_inputs["thing2"]) + assert_eq!(return_value.unwrap(), reconstructed_inputs["thing2"]) } } diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 9a094661074..9e807154b9d 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -10,7 +10,7 @@ use acvm::{ }; use errors::{RuntimeError, RuntimeErrorKind}; use iter_extended::btree_map; -use noirc_abi::{Abi, AbiType, AbiVisibility}; +use noirc_abi::{Abi, AbiType, AbiVisibility, MAIN_RETURN_NAME}; use noirc_frontend::monomorphization::ast::*; use ssa::{node, ssa_gen::IrGenerator}; use std::collections::{BTreeMap, BTreeSet}; @@ -53,7 +53,13 @@ pub fn create_circuit( let witness_index = evaluator.current_witness_index(); let mut abi = program.abi; - abi.param_witnesses = evaluator.param_witnesses; + + // TODO: remove return value from `param_witnesses` once we track public outputs + // see https://github.com/noir-lang/acvm/pull/56 + let mut param_witnesses = evaluator.param_witnesses; + let return_witnesses = param_witnesses.remove(MAIN_RETURN_NAME).unwrap_or_default(); + abi.param_witnesses = param_witnesses; + abi.return_witnesses = return_witnesses; let public_inputs = evaluator.public_inputs.into_iter().collect(); let optimized_circuit = acvm::compiler::compile( diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 4f295fee90a..85f1e5b3e04 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -1,7 +1,7 @@ use std::collections::BTreeMap; use iter_extended::vecmap; -use noirc_abi::{Abi, AbiParameter, AbiVisibility, MAIN_RETURN_NAME}; +use noirc_abi::{Abi, AbiParameter, AbiVisibility}; use noirc_errors::{Location, Span}; use super::expr::{HirBlockExpression, HirExpression, HirIdent}; @@ -63,7 +63,12 @@ impl Parameters { let as_abi = param.1.as_abi_type(); AbiParameter { name: param_name, typ: as_abi, visibility: param.2 } }); - noirc_abi::Abi { parameters, param_witnesses: BTreeMap::new() } + noirc_abi::Abi { + parameters, + param_witnesses: BTreeMap::new(), + return_type: None, + return_witnesses: Vec::new(), + } } pub fn span(&self) -> Span { @@ -145,15 +150,8 @@ impl FuncMeta { pub fn into_abi(self, interner: &NodeInterner) -> Abi { let return_type = self.return_type().clone(); let mut abi = self.parameters.into_abi(interner); - - if return_type != Type::Unit { - let return_param = AbiParameter { - name: MAIN_RETURN_NAME.into(), - typ: return_type.as_abi_type(), - visibility: self.return_visibility, - }; - abi.parameters.push(return_param); - } + abi.return_type = + if matches!(return_type, Type::Unit) { None } else { Some(return_type.as_abi_type()) }; abi } From 18aa366464c5f3d85b5b0839950f0e0d91815b2a Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 12:07:29 +0000 Subject: [PATCH 02/10] chore: inline `check_for_unexpected_inputs` --- crates/noirc_abi/src/lib.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 7d545fc373f..c8a70254897 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -185,7 +185,13 @@ impl Abi { input_map: &InputMap, return_value: Option, ) -> Result { - self.check_for_unexpected_inputs(input_map)?; + // Check that no extra witness values have been provided. + let param_names = self.parameter_names(); + if param_names.len() < input_map.len() { + let unexpected_params: Vec = + input_map.keys().filter(|param| !param_names.contains(param)).cloned().collect(); + return Err(AbiError::UnexpectedParams(unexpected_params)); + } // First encode each input separately, performing any input validation. let encoded_input_map: BTreeMap> = self @@ -254,18 +260,6 @@ impl Abi { Ok(witness_map) } - /// Checks that no extra witness values have been provided. - fn check_for_unexpected_inputs(&self, inputs: &InputMap) -> Result<(), AbiError> { - let param_names = self.parameter_names(); - if param_names.len() < inputs.len() { - let unexpected_params: Vec = - inputs.keys().filter(|param| !param_names.contains(param)).cloned().collect(); - return Err(AbiError::UnexpectedParams(unexpected_params)); - } - - Ok(()) - } - fn encode_value(value: InputValue) -> Result, AbiError> { let mut encoded_value = Vec::new(); match value { From 2a3153bb0a599a6d53cefe5a8d597d0cc5d0c4b5 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 12:42:10 +0000 Subject: [PATCH 03/10] chore: inline `self.return_witnesses` and remove borrow --- crates/noirc_abi/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index c8a70254897..7a7cecb4566 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -237,12 +237,11 @@ impl Abi { panic!("Unexpected return value") } let encoded_return_fields = Self::encode_value(return_value)?; - let return_witness_indices = &self.return_witnesses; // We need to be more careful when writing the return value's witness values. // This is as it may share witness indices with other public inputs so we must check that when // this occurs the witness values are consistent with each other. - return_witness_indices.iter().zip(encoded_return_fields.iter()).try_for_each( + self.return_witnesses.iter().zip(encoded_return_fields.iter()).try_for_each( |(&witness, &field_element)| match witness_map.insert(witness, field_element) { Some(existing_value) if existing_value != field_element => { panic!("Inconsistent return value"); From 6ad921f976b559b8d63637d766ec1ce4e561e80d Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 12:54:58 +0000 Subject: [PATCH 04/10] chore: add necessary errors --- crates/noirc_abi/src/errors.rs | 2 ++ crates/noirc_abi/src/lib.rs | 13 ++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/noirc_abi/src/errors.rs b/crates/noirc_abi/src/errors.rs index 34ef45a8415..aa48c251fed 100644 --- a/crates/noirc_abi/src/errors.rs +++ b/crates/noirc_abi/src/errors.rs @@ -42,4 +42,6 @@ pub enum AbiError { "Could not read witness value at index {witness_index:?} (required for parameter \"{name}\")" )] MissingParamWitnessValue { name: String, witness_index: Witness }, + #[error("Attempted to write to witness index {0:?} but it is already initialized to a different value")] + InconsistentWitnessAssignment(Witness), } diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 7a7cecb4566..0e6dd0e599f 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -234,7 +234,14 @@ impl Abi { match (&self.return_type, return_value) { (Some(return_type), Some(return_value)) => { if !return_value.matches_abi(return_type) { - panic!("Unexpected return value") + return Err(AbiError::TypeMismatch { + param: AbiParameter { + name: MAIN_RETURN_NAME.to_owned(), + typ: return_type.clone(), + visibility: AbiVisibility::Public, + }, + value: return_value, + }); } let encoded_return_fields = Self::encode_value(return_value)?; @@ -244,13 +251,13 @@ impl Abi { self.return_witnesses.iter().zip(encoded_return_fields.iter()).try_for_each( |(&witness, &field_element)| match witness_map.insert(witness, field_element) { Some(existing_value) if existing_value != field_element => { - panic!("Inconsistent return value"); + Err(AbiError::InconsistentWitnessAssignment(witness)) } _ => Ok(()), }, )?; } - (None, Some(_)) => panic!("Unexpected return value"), + (None, Some(_)) => return Err(AbiError::UnexpectedParams(vec!["return".to_string()])), // We allow not passing a return value despite the circuit defining one // in order to generate the initial partial witness. (_, None) => {} From da36dc385efe6a943f1a0117ad18fd2705f23e94 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 14:17:55 +0000 Subject: [PATCH 05/10] chore: replace `matches` with `match` statement! --- crates/noirc_frontend/src/hir_def/function.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 85f1e5b3e04..1f80cd09df8 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -150,8 +150,10 @@ impl FuncMeta { pub fn into_abi(self, interner: &NodeInterner) -> Abi { let return_type = self.return_type().clone(); let mut abi = self.parameters.into_abi(interner); - abi.return_type = - if matches!(return_type, Type::Unit) { None } else { Some(return_type.as_abi_type()) }; + abi.return_type = match return_type { + Type::Unit => None, + _ => Some(return_type.as_abi_type()), + }; abi } From 4914751328d2f4bbcfb7e3b3200550de2c9c6069 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 14:33:29 +0000 Subject: [PATCH 06/10] chore: clippy --- crates/nargo/src/cli/verify_cmd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index 55fde3e7f59..7ec05783d1e 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -9,7 +9,7 @@ use crate::{ use acvm::{FieldElement, ProofSystemCompiler}; use clap::Args; use noirc_abi::input_parser::{Format, InputValue}; -use noirc_abi::{errors::AbiError, MAIN_RETURN_NAME}; +use noirc_abi::MAIN_RETURN_NAME; use noirc_driver::CompiledProgram; use std::{collections::BTreeMap, path::Path}; From e709c90093dd08ba6d2236ecc4acdc2e916835de Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 14:34:34 +0000 Subject: [PATCH 07/10] chore: remove filtering of return dummy parameter --- crates/noirc_evaluator/src/lib.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 9e807154b9d..14ec26ee5a1 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -297,15 +297,6 @@ impl Evaluator { let main_params = std::mem::take(&mut main.parameters); let abi_params = std::mem::take(&mut ir_gen.program.abi.parameters); - // Remove the return type from the parameters - // Since this is not in the main functions parameters. - // - // TODO(See Issue633) regarding adding a `return_type` field to the ABI struct - let abi_params: Vec<_> = abi_params - .into_iter() - .filter(|param| param.name != noirc_abi::MAIN_RETURN_NAME) - .collect(); - assert_eq!(main_params.len(), abi_params.len()); for ((param_id, _, param_name, _), abi_param) in main_params.iter().zip(abi_params) { From fb3b8b0bf7491c05f1e0c161598461ab97e62d86 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 14:38:09 +0000 Subject: [PATCH 08/10] chore: use `MAIN_RETURN_NAME` instead of "return" --- crates/noirc_abi/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 0e6dd0e599f..24d4880b17a 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -257,7 +257,9 @@ impl Abi { }, )?; } - (None, Some(_)) => return Err(AbiError::UnexpectedParams(vec!["return".to_string()])), + (None, Some(_)) => { + return Err(AbiError::UnexpectedParams(vec![MAIN_RETURN_NAME.to_owned()])) + } // We allow not passing a return value despite the circuit defining one // in order to generate the initial partial witness. (_, None) => {} From f4742552a8b59c81f9c71e9ef530176d6b19a1de Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 15:13:26 +0000 Subject: [PATCH 09/10] chore: add proper error variants for return value issues --- crates/noirc_abi/src/errors.rs | 4 ++++ crates/noirc_abi/src/lib.rs | 12 ++++-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/noirc_abi/src/errors.rs b/crates/noirc_abi/src/errors.rs index aa48c251fed..ce92368462e 100644 --- a/crates/noirc_abi/src/errors.rs +++ b/crates/noirc_abi/src/errors.rs @@ -44,4 +44,8 @@ pub enum AbiError { MissingParamWitnessValue { name: String, witness_index: Witness }, #[error("Attempted to write to witness index {0:?} but it is already initialized to a different value")] InconsistentWitnessAssignment(Witness), + #[error("The return value is expected to be a {return_type:?} but found incompatible value {value:?}")] + ReturnTypeMismatch { return_type: AbiType, value: InputValue }, + #[error("No return value is expected but received {0:?}")] + UnexpectedReturnValue(InputValue), } diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 24d4880b17a..3fa05fa64e3 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -234,12 +234,8 @@ impl Abi { match (&self.return_type, return_value) { (Some(return_type), Some(return_value)) => { if !return_value.matches_abi(return_type) { - return Err(AbiError::TypeMismatch { - param: AbiParameter { - name: MAIN_RETURN_NAME.to_owned(), - typ: return_type.clone(), - visibility: AbiVisibility::Public, - }, + return Err(AbiError::ReturnTypeMismatch { + return_type: return_type.clone(), value: return_value, }); } @@ -257,8 +253,8 @@ impl Abi { }, )?; } - (None, Some(_)) => { - return Err(AbiError::UnexpectedParams(vec![MAIN_RETURN_NAME.to_owned()])) + (None, Some(return_value)) => { + return Err(AbiError::UnexpectedReturnValue(return_value)) } // We allow not passing a return value despite the circuit defining one // in order to generate the initial partial witness. From 6831f8504bf27cea412eb91357bdc3f2c09de665 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 17 Feb 2023 18:27:07 +0000 Subject: [PATCH 10/10] chore: improve comment --- crates/noirc_abi/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index 3fa05fa64e3..8894042d900 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -229,8 +229,8 @@ impl Abi { }) .collect(); - // The user can optionally provide a return value to be inserted into the witness map. - // This is to be used when encoding public values to be passed to the verifier. + // When encoding public inputs to be passed to the verifier, the user can must provide a return value + // to be inserted into the witness map. This is not needed when generating a witness when proving the circuit. match (&self.return_type, return_value) { (Some(return_type), Some(return_value)) => { if !return_value.matches_abi(return_type) {