From acc99cd633a156df0ccaf96d1a4f62691d4b8175 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:20:41 +0000 Subject: [PATCH 01/36] acir - add public_outputs field to acir structure --- crates/acir/src/circuit/mod.rs | 3 +++ crates/acvm/src/compiler.rs | 3 +++ 2 files changed, 6 insertions(+) diff --git a/crates/acir/src/circuit/mod.rs b/crates/acir/src/circuit/mod.rs index 38b4d14d33d..4df82f83239 100644 --- a/crates/acir/src/circuit/mod.rs +++ b/crates/acir/src/circuit/mod.rs @@ -16,6 +16,7 @@ pub struct Circuit { pub current_witness_index: u32, pub gates: Vec, pub public_inputs: PublicInputs, + pub public_outputs: PublicInputs, } impl Circuit { @@ -86,6 +87,7 @@ mod test { }), ], public_inputs: PublicInputs(vec![Witness(2)]), + public_outputs: PublicInputs(vec![Witness(2)]), }; let json = serde_json::to_string_pretty(&circuit).unwrap(); @@ -114,6 +116,7 @@ mod test { }), ], public_inputs: PublicInputs(vec![Witness(2)]), + public_outputs: PublicInputs(vec![Witness(2)]), }; let bytes = circuit.to_bytes(); diff --git a/crates/acvm/src/compiler.rs b/crates/acvm/src/compiler.rs index 8019ae6b63c..9d75ff8f8c4 100644 --- a/crates/acvm/src/compiler.rs +++ b/crates/acvm/src/compiler.rs @@ -59,6 +59,7 @@ pub fn compile(acir: Circuit, np_language: Language) -> Circuit { current_witness_index, gates: optimised_gates, public_inputs: acir.public_inputs, // The optimiser does not add public inputs + public_outputs: acir.public_outputs, // The optimiser does not add public inputs } } @@ -83,6 +84,7 @@ fn optimise_r1cs(acir: Circuit) -> Circuit { current_witness_index: acir.current_witness_index, gates: optimised_arith_gates, public_inputs: acir.public_inputs, + public_outputs: acir.public_outputs, } } @@ -103,6 +105,7 @@ pub fn fallback(acir: &Circuit, np_language: &Language) -> Circuit { current_witness_index: witness_idx, gates: fallback_gates, public_inputs: acir.public_inputs.clone(), + public_outputs: acir.public_outputs.clone(), } } From 5e3a9c8491e595850429a9d9a8e38350bceed1cf Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:22:47 +0000 Subject: [PATCH 02/36] monomorphisation: - Add a SetPub Expression - Instead of adding a constrain statement and adding a `return` parameter to main, we now specify that the return type should be set as public. This is then dealt with in the ssa pass. --- crates/noirc_frontend/src/monomorphisation/ast.rs | 1 + crates/noirc_frontend/src/monomorphisation/mod.rs | 15 +-------------- .../src/monomorphisation/printer.rs | 4 ++++ 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/crates/noirc_frontend/src/monomorphisation/ast.rs b/crates/noirc_frontend/src/monomorphisation/ast.rs index f43a25d5e7b..dde4c25c9a0 100644 --- a/crates/noirc_frontend/src/monomorphisation/ast.rs +++ b/crates/noirc_frontend/src/monomorphisation/ast.rs @@ -21,6 +21,7 @@ pub enum Expression { Call(Call), CallBuiltin(CallBuiltin), CallLowLevel(CallLowLevel), + SetPub(Box, Location), Let(Let), Constrain(Box, Location), diff --git a/crates/noirc_frontend/src/monomorphisation/mod.rs b/crates/noirc_frontend/src/monomorphisation/mod.rs index 2554ad0bf9a..85899bebfd8 100644 --- a/crates/noirc_frontend/src/monomorphisation/mod.rs +++ b/crates/noirc_frontend/src/monomorphisation/mod.rs @@ -99,21 +99,8 @@ impl Monomorphiser { let main_meta = self.interner.function_meta(&main_id); if main.return_type != ast::Type::Unit { - let id = self.next_definition_id(); - - main.parameters.push((id, false, "return".into(), main.return_type)); - main.return_type = ast::Type::Unit; - - let name = "_".into(); - let typ = Self::convert_type(main_meta.return_type()); - let lhs = - Box::new(ast::Expression::Ident(ast::Ident { id, location: None, name, typ })); - let rhs = Box::new(main.body); - let operator = ast::BinaryOp::Equal; - let eq = ast::Expression::Binary(ast::Binary { operator, lhs, rhs }); - let location = self.interner.function_meta(&main_id).location; - main.body = ast::Expression::Constrain(Box::new(eq), location); + main.body = ast::Expression::SetPub(Box::new(main.body), location); } let abi = main_meta.into_abi(&self.interner); diff --git a/crates/noirc_frontend/src/monomorphisation/printer.rs b/crates/noirc_frontend/src/monomorphisation/printer.rs index fd9f5dfbd8a..c5e138d3094 100644 --- a/crates/noirc_frontend/src/monomorphisation/printer.rs +++ b/crates/noirc_frontend/src/monomorphisation/printer.rs @@ -56,6 +56,10 @@ impl AstPrinter { Expression::Call(call) => self.print_call(call, f), Expression::CallBuiltin(call) => self.print_lowlevel(call, f), Expression::CallLowLevel(call) => self.print_builtin(call, f), + Expression::SetPub(expr, _) => { + write!(f, "setpub ")?; + self.print_expr(expr, f) + } Expression::Let(let_expr) => { write!(f, "let {}${} = ", let_expr.name, let_expr.id.0)?; self.print_expr(&let_expr.expression, f) From 8e53b2ffa471cc013082b0a258890ebd96d06aec Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:25:03 +0000 Subject: [PATCH 03/36] evaluator - acir now has a `public_outputs` field and so we add one into the evaluator --- crates/noirc_evaluator/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index bfe6e78dd60..f5cddd7d950 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -20,6 +20,7 @@ pub struct Evaluator { // to compile wasm64. current_witness_index: u32, public_inputs: Vec, + public_outputs: Vec, gates: Vec, } @@ -45,6 +46,7 @@ pub fn create_circuit( current_witness_index: witness_index, gates: evaluator.gates, public_inputs: PublicInputs(evaluator.public_inputs), + public_outputs: PublicInputs(evaluator.public_outputs), }, np_language, ); @@ -56,6 +58,7 @@ impl Evaluator { fn new() -> Self { Evaluator { public_inputs: Vec::new(), + public_outputs: Vec::new(), // XXX: Barretenberg, reserves the first index to have value 0. // When we increment, we do not use this index at all. // This means that every constraint system at the moment, will either need From d5c5a0b0a61466a83b8582a4d5b609152548de7e Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:26:10 +0000 Subject: [PATCH 04/36] ssa - Add SetPub operation and opcode --- crates/noirc_evaluator/src/ssa/context.rs | 7 +++++++ crates/noirc_evaluator/src/ssa/inline.rs | 2 +- crates/noirc_evaluator/src/ssa/integer.rs | 1 + crates/noirc_evaluator/src/ssa/node.rs | 11 +++++++++++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 28439f05573..e0fc72f0397 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -240,6 +240,13 @@ impl SsaContext { let call = self.node_to_string(*call_instruction); format!("result {} of {}", index, call) } + Operation::SetPub(node_ids, _) => { + let mut s = format!("setpub "); + for node_id in node_ids { + s = format!("{}, {}", s, self.node_to_string(*node_id),); + } + s + } } } diff --git a/crates/noirc_evaluator/src/ssa/inline.rs b/crates/noirc_evaluator/src/ssa/inline.rs index 87ee1914841..77cc3810436 100644 --- a/crates/noirc_evaluator/src/ssa/inline.rs +++ b/crates/noirc_evaluator/src/ssa/inline.rs @@ -408,7 +408,7 @@ impl node::Operation { ) { match self { //default way to handle arrays during inlining; we map arrays using the stack_frame - Operation::Binary(_) | Operation::Constrain(..) | Operation::Intrinsic(_,_) + Operation::Binary(_) | Operation::Constrain(..) | Operation::Intrinsic(_,_) | Operation::SetPub(_,_) => { self.map_id_mut(|id| { if let Some(a) = Memory::deref(ctx, id) { diff --git a/crates/noirc_evaluator/src/ssa/integer.rs b/crates/noirc_evaluator/src/ssa/integer.rs index 6c493562d46..ca9a643dc13 100644 --- a/crates/noirc_evaluator/src/ssa/integer.rs +++ b/crates/noirc_evaluator/src/ssa/integer.rs @@ -496,6 +496,7 @@ fn get_max_value(ins: &Instruction, max_map: &mut HashMap) -> B _ => todo!("max value must be implemented for opcode {} ", opcode), } } + Operation::SetPub(_, _) => BigUint::zero(), }; if ins.res_type == ObjectType::NativeField { diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index c3caccbfd3c..cac56c30d5a 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -384,6 +384,7 @@ impl Instruction { Operation::Call { .. } => false, //return values are in the return statment, should we truncate function arguments? probably but not lhs and rhs anyways. Operation::Return(_) => true, Operation::Result { .. } => false, + Operation::SetPub { .. } => true, } } @@ -511,6 +512,7 @@ pub enum Operation { Not(NodeId), //(!) Bitwise Not Constrain(NodeId, Option), + SetPub(Vec, Option), //control flow Jne(NodeId, BlockId), //jump on not equal @@ -582,6 +584,7 @@ pub enum Opcode { Assign, Cond, Constrain, + SetPub, Cast, //convert type Truncate, //truncate Not, //(!) Bitwise Not @@ -1100,6 +1103,7 @@ impl Operation { Result { call_instruction, index } => { Result { call_instruction: f(*call_instruction), index: *index } } + SetPub(values, loc) => SetPub(vecmap(values.iter().copied(), f), *loc), } } @@ -1154,6 +1158,11 @@ impl Operation { Result { call_instruction, index: _ } => { *call_instruction = f(*call_instruction); } + SetPub(values, _) => { + for value in values { + *value = f(*value); + } + } } } @@ -1195,6 +1204,7 @@ impl Operation { Result { call_instruction, .. } => { f(*call_instruction); } + SetPub(values, _) => values.iter().copied().for_each(f), } } @@ -1217,6 +1227,7 @@ impl Operation { Operation::Store { array_id, .. } => Opcode::Store(*array_id), Operation::Intrinsic(opcode, _) => Opcode::Intrinsic(*opcode), Operation::Nop => Opcode::Nop, + Operation::SetPub(_, _) => Opcode::SetPub, } } } From 8f414562fb5b4b4d92c60d271039a747227ed94e Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:26:36 +0000 Subject: [PATCH 05/36] ssa - codegen for setpub --- crates/noirc_evaluator/src/ssa/code_gen.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index 2dcb026dd49..b00a75e631f 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -367,6 +367,17 @@ impl IRGenerator { Ok(Value::dummy()) } + fn codegen_setpub( + &mut self, + expr: &Expression, + location: noirc_errors::Location, + ) -> Result { + let node_ids = self.codegen_expression(expr)?.to_node_ids(); + let operation = Operation::SetPub(node_ids, Some(location)); + self.context.new_instruction(operation, ObjectType::NotAnObject)?; + Ok(Value::dummy()) + } + /// Bind the given DefinitionId to the given Value. This will flatten the Value as needed, /// expanding each field of the value to a new variable. fn bind_id(&mut self, id: DefinitionId, value: Value, name: &str) -> Result<(), RuntimeError> { @@ -602,6 +613,7 @@ impl IRGenerator { self.codegen_expression(expr.as_ref())?; Ok(Value::dummy()) } + Expression::SetPub(expr, location) => self.codegen_setpub(expr, *location), } } From 96141f7703678d840bd7382e72337250c88b1b6d Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:26:51 +0000 Subject: [PATCH 06/36] ssa - setpub acir_gen --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index f7efc7111de..541099b2cce 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -210,6 +210,15 @@ impl Acir { todo!("dynamic arrays are not implemented yet"); } } + Operation::SetPub(node_ids, _) => { + for node_id in node_ids { + let object = self.substitute(*node_id, evaluator, ctx); + let witness = object.get_or_generate_witness(evaluator); + evaluator.public_outputs.push(witness); + } + + InternalVar::default() + } }; output.id = Some(ins.id); self.arith_cache.insert(ins.id, output); From 2211a935adaf78241833f716c77b8e7cd54d8f06 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:32:20 +0000 Subject: [PATCH 07/36] Overwrite merge commit to revert change to `get_max_value` --- crates/noirc_evaluator/src/ssa/integer.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/noirc_evaluator/src/ssa/integer.rs b/crates/noirc_evaluator/src/ssa/integer.rs index 3efd00e8047..882918e8174 100644 --- a/crates/noirc_evaluator/src/ssa/integer.rs +++ b/crates/noirc_evaluator/src/ssa/integer.rs @@ -482,6 +482,7 @@ fn get_max_value(ins: &Instruction, max_map: &mut HashMap) -> B unreachable!("Functions must have been inlined before checking for overflows") } Operation::Intrinsic(opcode, _) => opcode.get_max_value(), + Operation::SetPub(_, _) => BigUint::zero(), }; if ins.res_type == ObjectType::NativeField { From 387b28e71e3b47b2b580af63719c3de26e135988 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 20:46:16 +0000 Subject: [PATCH 08/36] ssa - acir_gen - `get_or_generate_witness` is not named `generate_witness` --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 875a1e0344b..8d455185ec2 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -232,8 +232,8 @@ impl Acir { } Operation::SetPub(node_ids, _) => { for node_id in node_ids { - let object = self.substitute(*node_id, evaluator, ctx); - let witness = object.get_or_generate_witness(evaluator); + let mut object = self.substitute(*node_id, evaluator, ctx); + let witness = object.generate_witness(evaluator); evaluator.public_outputs.push(witness); } From 1a8fca59dbd913dde7d0a595e6530a980babcca6 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 22:58:39 +0000 Subject: [PATCH 09/36] revert: remove public_outputs field - This can be done as a separate PR. Removing it to make PR easier to review, an issue has been opened up for this --- crates/acir/src/circuit/mod.rs | 3 --- crates/acvm/src/compiler.rs | 3 --- crates/noirc_evaluator/src/lib.rs | 3 --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 2 +- 4 files changed, 1 insertion(+), 10 deletions(-) diff --git a/crates/acir/src/circuit/mod.rs b/crates/acir/src/circuit/mod.rs index 4df82f83239..38b4d14d33d 100644 --- a/crates/acir/src/circuit/mod.rs +++ b/crates/acir/src/circuit/mod.rs @@ -16,7 +16,6 @@ pub struct Circuit { pub current_witness_index: u32, pub gates: Vec, pub public_inputs: PublicInputs, - pub public_outputs: PublicInputs, } impl Circuit { @@ -87,7 +86,6 @@ mod test { }), ], public_inputs: PublicInputs(vec![Witness(2)]), - public_outputs: PublicInputs(vec![Witness(2)]), }; let json = serde_json::to_string_pretty(&circuit).unwrap(); @@ -116,7 +114,6 @@ mod test { }), ], public_inputs: PublicInputs(vec![Witness(2)]), - public_outputs: PublicInputs(vec![Witness(2)]), }; let bytes = circuit.to_bytes(); diff --git a/crates/acvm/src/compiler.rs b/crates/acvm/src/compiler.rs index 9d75ff8f8c4..8019ae6b63c 100644 --- a/crates/acvm/src/compiler.rs +++ b/crates/acvm/src/compiler.rs @@ -59,7 +59,6 @@ pub fn compile(acir: Circuit, np_language: Language) -> Circuit { current_witness_index, gates: optimised_gates, public_inputs: acir.public_inputs, // The optimiser does not add public inputs - public_outputs: acir.public_outputs, // The optimiser does not add public inputs } } @@ -84,7 +83,6 @@ fn optimise_r1cs(acir: Circuit) -> Circuit { current_witness_index: acir.current_witness_index, gates: optimised_arith_gates, public_inputs: acir.public_inputs, - public_outputs: acir.public_outputs, } } @@ -105,7 +103,6 @@ pub fn fallback(acir: &Circuit, np_language: &Language) -> Circuit { current_witness_index: witness_idx, gates: fallback_gates, public_inputs: acir.public_inputs.clone(), - public_outputs: acir.public_outputs.clone(), } } diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index c67c14f9f26..1fd91f1c006 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -20,7 +20,6 @@ pub struct Evaluator { // to compile wasm64. current_witness_index: u32, public_inputs: Vec, - public_outputs: Vec, gates: Vec, } @@ -46,7 +45,6 @@ pub fn create_circuit( current_witness_index: witness_index, gates: evaluator.gates, public_inputs: PublicInputs(evaluator.public_inputs), - public_outputs: PublicInputs(evaluator.public_outputs), }, np_language, ); @@ -58,7 +56,6 @@ impl Evaluator { fn new() -> Self { Evaluator { public_inputs: Vec::new(), - public_outputs: Vec::new(), // XXX: Barretenberg, reserves the first index to have value 0. // When we increment, we do not use this index at all. // This means that every constraint system at the moment, will either need diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 8d455185ec2..dcdbcf5d7fc 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -234,7 +234,7 @@ impl Acir { for node_id in node_ids { let mut object = self.substitute(*node_id, evaluator, ctx); let witness = object.generate_witness(evaluator); - evaluator.public_outputs.push(witness); + evaluator.public_inputs.push(witness); } InternalVar::default() From 8b062778f3ecc0a2d772740756486770e4cb909d Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 23:00:25 +0000 Subject: [PATCH 10/36] remove the return type from the abi when converting abi parameters to noir parameters --- crates/noirc_evaluator/src/lib.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 1fd91f1c006..28421401669 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -233,6 +233,17 @@ impl Evaluator { let main = igen.program.main(); let main_params = std::mem::take(&mut main.parameters); let abi_params = std::mem::take(&mut igen.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 + // TODO(See Issue631) regarding making the return keyword reserved + 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_name1, _), abi_param) in main_params.iter().zip(abi_params) { From 0adbf27f1052dc11aa8e47af96b3019d66dd23f1 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 12 Jan 2023 23:17:27 +0000 Subject: [PATCH 11/36] clippy --- crates/noirc_evaluator/src/ssa/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index e0fc72f0397..af7c7acf5bf 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -241,7 +241,7 @@ impl SsaContext { format!("result {} of {}", index, call) } Operation::SetPub(node_ids, _) => { - let mut s = format!("setpub "); + let mut s = "setpub ".to_string(); for node_id in node_ids { s = format!("{}, {}", s, self.node_to_string(*node_id),); } From fbf4b267a4fdbfc927e4323793ab7a3dbedd8517 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Fri, 13 Jan 2023 00:06:05 +0000 Subject: [PATCH 12/36] ssa - acir_gen : create witnesses for constants generate_witness will panic when trying to generate a witness for a constant --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 9d035a11984..0a11cc76ad0 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -233,7 +233,16 @@ impl Acir { Operation::SetPub(node_ids, _) => { for node_id in node_ids { let mut object = self.substitute(*node_id, evaluator, ctx); - let witness = object.generate_witness(evaluator); + + // TODO(Open issue): We want to semantically specify that + // TODO you cannot return constant values as public outputs. + // TODO: in that case, you should just have a constant. + // TODO: For now, we will support this usecase. + let witness = if object.expression.is_const() { + evaluator.create_intermediate_variable(object.expression) + } else { + object.generate_witness(evaluator) + }; evaluator.public_inputs.push(witness); } From aff8d337208900f47132886eb962583a5b6d3d58 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Fri, 13 Jan 2023 01:09:19 +0000 Subject: [PATCH 13/36] ssa - acir_gen : deref node_id when we have an array codegening an array and calling to_node_ids will produce a single node_id, whereas we want to fetch all elements of the array --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 33 ++++++++++++++-------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 0a11cc76ad0..b2f0d7d1bc6 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -232,18 +232,29 @@ impl Acir { } Operation::SetPub(node_ids, _) => { for node_id in node_ids { - let mut object = self.substitute(*node_id, evaluator, ctx); - - // TODO(Open issue): We want to semantically specify that - // TODO you cannot return constant values as public outputs. - // TODO: in that case, you should just have a constant. - // TODO: For now, we will support this usecase. - let witness = if object.expression.is_const() { - evaluator.create_intermediate_variable(object.expression) - } else { - object.generate_witness(evaluator) + // An array produces a single node_id + // We therefore need to check if the node_id is referring to an array + // and deference to get the elements + let objects = match Memory::deref(ctx, *node_id) { + Some(a) => { + let array = &ctx.mem[a]; + self.load_array(array, false, evaluator) + } + None => vec![self.substitute(*node_id, evaluator, ctx)], }; - evaluator.public_inputs.push(witness); + + for mut object in objects { + // TODO(Open issue): We want to semantically specify that + // TODO you cannot return constant values as public outputs. + // TODO: in that case, you should just have a constant. + // TODO: For now, we will support this usecase. + let witness = if object.expression.is_const() { + evaluator.create_intermediate_variable(object.expression) + } else { + object.generate_witness(evaluator) + }; + evaluator.public_inputs.push(witness); + } } InternalVar::default() From 1fc37dedb7b574d08194c87084e500eb82978487 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 14 Jan 2023 17:45:24 +0000 Subject: [PATCH 14/36] nargo - remove duplicates - When a public input is return as a public output (return value), it is then added into the public input list twice. Using the current code, we need this duplication since we want to write the return values into the toml files. For proving and verifying we want to remove these duplicate values --- crates/nargo/src/cli/prove_cmd.rs | 43 +++++++++++++++++++++++++++++- crates/nargo/src/cli/verify_cmd.rs | 11 ++++++-- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index 5cc5a3d9605..226cbb1e629 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -1,5 +1,7 @@ +use std::collections::HashSet; use std::{collections::BTreeMap, path::PathBuf}; +use acvm::acir::circuit::PublicInputs; use acvm::acir::native_types::Witness; use acvm::FieldElement; use acvm::ProofSystemCompiler; @@ -127,10 +129,18 @@ pub fn prove_with_path>( show_ssa: bool, allow_warnings: bool, ) -> Result { - let (compiled_program, solved_witness) = + let (mut compiled_program, solved_witness) = compile_circuit_and_witness(program_dir, show_ssa, allow_warnings)?; let backend = crate::backends::ConcreteBackend; + + // Since the public outputs are added into the public inputs list + // There can be duplicates. We keep the duplicates for when one is + // encoding the return values into the Verifier.toml + // However, for creating a proof, we remove these duplicates. + compiled_program.circuit.public_inputs = + dedup_public_input_indices(compiled_program.circuit.public_inputs); + let proof = backend.prove_with_meta(compiled_program.circuit, solved_witness); let mut proof_path = create_named_dir(proof_dir.as_ref(), "proof"); @@ -145,3 +155,34 @@ pub fn prove_with_path>( Ok(proof_path) } + +// Removes duplicates from the list of public input witnesses +fn dedup_public_input_indices(indices: PublicInputs) -> PublicInputs { + let duplicates_removed: HashSet<_> = indices.0.into_iter().collect(); + PublicInputs(duplicates_removed.into_iter().collect()) +} + +// Removes duplicates from the list of public input witnesses and the +// associated list of duplicate values. +pub(crate) fn dedup_public_input_indices_values( + indices: PublicInputs, + values: Vec, +) -> (PublicInputs, Vec) { + // Assume that the public input index lists and the values contain duplicates + assert_eq!(indices.0.len(), values.len()); + + let mut public_inputs_without_duplicates = Vec::new(); + let mut already_seen_public_indices = HashSet::new(); + + for (index, value) in indices.0.iter().zip(values) { + if !already_seen_public_indices.contains(&index) { + already_seen_public_indices.insert(index); + public_inputs_without_duplicates.push(value) + } + } + + ( + PublicInputs(already_seen_public_indices.into_iter().cloned().collect()), + public_inputs_without_duplicates, + ) +} diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index 842e608b7d9..38516b1b46a 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -1,4 +1,5 @@ use super::compile_cmd::compile_circuit; +use super::prove_cmd::dedup_public_input_indices_values; use super::{read_inputs_from_file, PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE}; use crate::errors::CliError; use acvm::ProofSystemCompiler; @@ -56,7 +57,7 @@ pub fn verify_with_path>( } fn verify_proof( - compiled_program: CompiledProgram, + mut compiled_program: CompiledProgram, public_inputs: BTreeMap, proof: Vec, ) -> Result { @@ -68,8 +69,14 @@ fn verify_proof( _ => CliError::from(error), })?; + // Similarly to when proving -- we must remove the duplicate public witnesses which + // can be present because a public input can also be added as a public output. + let (dedup_public_indices, dedup_public_values) = + dedup_public_input_indices_values(compiled_program.circuit.public_inputs, public_inputs); + compiled_program.circuit.public_inputs = dedup_public_indices; + let backend = crate::backends::ConcreteBackend; - let valid_proof = backend.verify_from_cs(&proof, public_inputs, compiled_program.circuit); + let valid_proof = backend.verify_from_cs(&proof, dedup_public_values, compiled_program.circuit); Ok(valid_proof) } From a99e485c6e6003091f43f53f24e604c5f7dc9bb2 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 14 Jan 2023 20:19:06 +0000 Subject: [PATCH 15/36] noirc_abi - skip `return` when proving - The prover no longer supplies a `return` field in toml. - The ABI will have a `return` field, so when proving, we use the `skip_output` flag to ignore the `return` field in the ABI - As noted in the comments, this will look cleaner when we add a `public_outputs` field --- crates/noirc_abi/src/lib.rs | 38 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index db9e3d1a1bc..d2bdb04173f 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -141,12 +141,23 @@ impl Abi { pub fn encode( self, inputs: &BTreeMap, - allow_undefined_return: bool, + skip_output: bool, ) -> Result, AbiError> { - let param_names = self.parameter_names(); + // Condition that specifies whether we should filter the "return" + // parameter. We do this in the case that it is not in the `inputs` + // map specified. + // TODO Adding a `public outputs` field into acir and + // TODO the ABI will clean up this logic + // TODO For prosperity; the prover does not know about a `return` value + // TODO so we skip this when encoding the ABI + let return_condition = + |param_name: &&String| !skip_output || (param_name != &MAIN_RETURN_NAME); + + let parameters = self.parameters.iter().filter(|param| return_condition(&¶m.name)); + let param_names: Vec<&String> = parameters.clone().map(|param| ¶m.name).collect(); let mut encoded_inputs = Vec::new(); - for param in self.parameters.iter() { + for param in parameters { let value = inputs .get(¶m.name) .ok_or_else(|| AbiError::MissingParam(param.name.to_owned()))? @@ -156,26 +167,6 @@ impl Abi { return Err(AbiError::TypeMismatch { param: param.to_owned(), value }); } - // As the circuit calculates the return value in the process of calculating rest of the witnesses - // it's not absolutely necessary to provide them as inputs. We then tolerate an undefined value for - // the return value input and just skip it. - if allow_undefined_return - && param.name == MAIN_RETURN_NAME - && matches!(value, InputValue::Undefined) - { - let return_witness_len = param.typ.field_count(); - - // We do not support undefined arrays for now - TODO - if return_witness_len != 1 { - return Err(AbiError::Generic( - "Values of array returned from main must be specified".to_string(), - )); - } else { - // This assumes that the return value is at the end of the ABI, otherwise values will be misaligned. - continue; - } - } - encoded_inputs.extend(Self::encode_value(value, ¶m.name)?); } @@ -221,7 +212,6 @@ impl Abi { let mut index = 0; let mut decoded_inputs = BTreeMap::new(); - for param in &self.parameters { let (next_index, decoded_value) = Self::decode_value(index, encoded_inputs, ¶m.typ)?; From d2a8a7dd03de1103e7267e361ccafaaf603a042e Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 14 Jan 2023 20:20:04 +0000 Subject: [PATCH 16/36] nargo/tests - remove "return" field from toml files --- crates/nargo/tests/test_data/hash_to_field/Prover.toml | 1 - crates/nargo/tests/test_data/main_return/Prover.toml | 1 - crates/nargo/tests/test_data/simple_shield/Prover.toml | 6 +----- crates/nargo/tests/test_data/struct_inputs/Prover.toml | 3 +-- crates/nargo/tests/test_data/to_bytes/Prover.toml | 1 - 5 files changed, 2 insertions(+), 10 deletions(-) diff --git a/crates/nargo/tests/test_data/hash_to_field/Prover.toml b/crates/nargo/tests/test_data/hash_to_field/Prover.toml index 079763a108e..f6597d3f78a 100644 --- a/crates/nargo/tests/test_data/hash_to_field/Prover.toml +++ b/crates/nargo/tests/test_data/hash_to_field/Prover.toml @@ -1,2 +1 @@ input = "1" -return = "0x25cebc29ded2fa515a937e2b5f674e3026c012e5b57f8a48d7dce6b7d274f9d9" diff --git a/crates/nargo/tests/test_data/main_return/Prover.toml b/crates/nargo/tests/test_data/main_return/Prover.toml index ef1ca7bb7e8..63e9878811a 100644 --- a/crates/nargo/tests/test_data/main_return/Prover.toml +++ b/crates/nargo/tests/test_data/main_return/Prover.toml @@ -1,2 +1 @@ -return = "" x = "8" diff --git a/crates/nargo/tests/test_data/simple_shield/Prover.toml b/crates/nargo/tests/test_data/simple_shield/Prover.toml index 4c03ef25a18..67e825f6333 100644 --- a/crates/nargo/tests/test_data/simple_shield/Prover.toml +++ b/crates/nargo/tests/test_data/simple_shield/Prover.toml @@ -5,11 +5,7 @@ index = "0" note_hash_path = [ "0x0000000000000000000000000000000000000000000000000000000000000000", "0x0e4223f3925f98934393c74975142bd73079ab0621f4ee133cee050a3c194f1a", - "0x2fd7bb412155bf8693a3bd2a3e7581a679c95c68a052f835dddca85fa1569a40" + "0x2fd7bb412155bf8693a3bd2a3e7581a679c95c68a052f835dddca85fa1569a40", ] to_pubkey_x = "0x0000000000000000000000000000000000000000000000000000000000000001" to_pubkey_y = "0x0000000000000002cf135e7506a45d632d270d45f1181294833fc48d823f272c" -return = [ - "0x1fa077bf4c787f858e0f3a7fa031ff11f200a0ccc6e549c6af98ec96800af340", - "0x0c25956c07e2277e75f1ca10bae32c29f24b27f25625fe9258cec29dc90651dd" -] \ No newline at end of file diff --git a/crates/nargo/tests/test_data/struct_inputs/Prover.toml b/crates/nargo/tests/test_data/struct_inputs/Prover.toml index e319f65d502..56684459fdc 100644 --- a/crates/nargo/tests/test_data/struct_inputs/Prover.toml +++ b/crates/nargo/tests/test_data/struct_inputs/Prover.toml @@ -1,5 +1,4 @@ x = "5" -return = "" [y] foo = "5" @@ -12,4 +11,4 @@ array = [0, 1] [a] [a.bar_struct] val = "1" -array = [0, 1] \ No newline at end of file +array = [0, 1] diff --git a/crates/nargo/tests/test_data/to_bytes/Prover.toml b/crates/nargo/tests/test_data/to_bytes/Prover.toml index 54cfe16a841..07fe857ac7c 100644 --- a/crates/nargo/tests/test_data/to_bytes/Prover.toml +++ b/crates/nargo/tests/test_data/to_bytes/Prover.toml @@ -1,2 +1 @@ x = "2040124" -return = [0x3c, 0x21, 0x1f, 0x00] From a0a543d8cfa8f18817304f3bcdbd4e35eef3fcef Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Sat, 14 Jan 2023 20:33:31 +0000 Subject: [PATCH 17/36] cargo fmt imports --- crates/nargo/src/cli/prove_cmd.rs | 23 +++++++++++------------ crates/nargo/src/cli/verify_cmd.rs | 21 +++++++++++++-------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index 226cbb1e629..db367d4ba60 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -1,22 +1,21 @@ -use std::collections::HashSet; -use std::{collections::BTreeMap, path::PathBuf}; - -use acvm::acir::circuit::PublicInputs; -use acvm::acir::native_types::Witness; -use acvm::FieldElement; -use acvm::ProofSystemCompiler; -use acvm::{GateResolution, PartialWitnessGenerator}; use clap::ArgMatches; -use noirc_abi::errors::AbiError; -use noirc_abi::input_parser::{Format, InputValue}; -use std::path::Path; +use std::{ + collections::{BTreeMap, HashSet}, + path::{Path, PathBuf}, +}; -use crate::errors::CliError; +use acvm::acir::{circuit::PublicInputs, native_types::Witness, FieldElement}; +use acvm::{GateResolution, PartialWitnessGenerator, ProofSystemCompiler}; +use noirc_abi::{ + errors::AbiError, + input_parser::{Format, InputValue}, +}; use super::{ create_named_dir, read_inputs_from_file, write_inputs_to_file, write_to_file, PROOFS_DIR, PROOF_EXT, PROVER_INPUT_FILE, VERIFIER_INPUT_FILE, }; +use crate::errors::CliError; pub(crate) fn run(args: ArgMatches) -> Result<(), CliError> { let args = args.subcommand_matches("prove").unwrap(); diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index 38516b1b46a..eabfe1a4641 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -1,14 +1,19 @@ -use super::compile_cmd::compile_circuit; -use super::prove_cmd::dedup_public_input_indices_values; -use super::{read_inputs_from_file, PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE}; -use crate::errors::CliError; -use acvm::ProofSystemCompiler; use clap::ArgMatches; -use noirc_abi::errors::AbiError; -use noirc_abi::input_parser::{Format, InputValue}; -use noirc_driver::CompiledProgram; use std::{collections::BTreeMap, path::Path, path::PathBuf}; +use acvm::ProofSystemCompiler; +use noirc_abi::{ + errors::AbiError, + input_parser::{Format, InputValue}, +}; +use noirc_driver::CompiledProgram; + +use super::{ + compile_cmd::compile_circuit, prove_cmd::dedup_public_input_indices_values, + read_inputs_from_file, PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE, +}; +use crate::errors::CliError; + pub(crate) fn run(args: ArgMatches) -> Result<(), CliError> { let args = args.subcommand_matches("verify").unwrap(); From d5bfaf802fcdeec12776b6bb1f36ec341493c147 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 15:55:31 +0000 Subject: [PATCH 18/36] remove SetPub Expression --- crates/noirc_frontend/src/monomorphisation/ast.rs | 1 - crates/noirc_frontend/src/monomorphisation/mod.rs | 9 +-------- crates/noirc_frontend/src/monomorphisation/printer.rs | 4 ---- 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/crates/noirc_frontend/src/monomorphisation/ast.rs b/crates/noirc_frontend/src/monomorphisation/ast.rs index 2d4203247cd..5472e5bd1c0 100644 --- a/crates/noirc_frontend/src/monomorphisation/ast.rs +++ b/crates/noirc_frontend/src/monomorphisation/ast.rs @@ -21,7 +21,6 @@ pub enum Expression { Call(Call), CallBuiltin(CallBuiltin), CallLowLevel(CallLowLevel), - SetPub(Box, Location), Let(Let), Constrain(Box, Location), diff --git a/crates/noirc_frontend/src/monomorphisation/mod.rs b/crates/noirc_frontend/src/monomorphisation/mod.rs index 85899bebfd8..84cee6aa2a3 100644 --- a/crates/noirc_frontend/src/monomorphisation/mod.rs +++ b/crates/noirc_frontend/src/monomorphisation/mod.rs @@ -92,17 +92,10 @@ impl Monomorphiser { self.globals.entry(id).or_default().insert(typ, new_id); } - /// The main function is special, we need to check for a return type and if present, - /// insert an extra constrain on the return value. fn compile_main(&mut self, main_id: node_interner::FuncId) -> Program { - let mut main = self.function(main_id, FuncId(0)); + let main = self.function(main_id, FuncId(0)); let main_meta = self.interner.function_meta(&main_id); - if main.return_type != ast::Type::Unit { - let location = self.interner.function_meta(&main_id).location; - main.body = ast::Expression::SetPub(Box::new(main.body), location); - } - let abi = main_meta.into_abi(&self.interner); Program::new(main, abi) } diff --git a/crates/noirc_frontend/src/monomorphisation/printer.rs b/crates/noirc_frontend/src/monomorphisation/printer.rs index 86b489eee2f..567def2ec34 100644 --- a/crates/noirc_frontend/src/monomorphisation/printer.rs +++ b/crates/noirc_frontend/src/monomorphisation/printer.rs @@ -56,10 +56,6 @@ impl AstPrinter { Expression::Call(call) => self.print_call(call, f), Expression::CallBuiltin(call) => self.print_lowlevel(call, f), Expression::CallLowLevel(call) => self.print_builtin(call, f), - Expression::SetPub(expr, _) => { - write!(f, "setpub ")?; - self.print_expr(expr, f) - } Expression::Let(let_expr) => { write!(f, "let {}${} = ", let_expr.name, let_expr.id.0)?; self.print_expr(&let_expr.expression, f) From d670b8df89291b6d8e280fbdfdc6617c22b112d7 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 15:56:08 +0000 Subject: [PATCH 19/36] ssa - remove SetPub operation --- crates/noirc_evaluator/src/ssa/code_gen.rs | 17 ++++------------- crates/noirc_evaluator/src/ssa/context.rs | 7 ------- crates/noirc_evaluator/src/ssa/inline.rs | 2 +- crates/noirc_evaluator/src/ssa/integer.rs | 1 - crates/noirc_evaluator/src/ssa/node.rs | 10 ---------- 5 files changed, 5 insertions(+), 32 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index d62fc22bf4a..42d54046f17 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -127,7 +127,10 @@ impl IRGenerator { pub fn codegen_main(&mut self) -> Result<(), RuntimeError> { let main_body = self.program.take_main_body(); - self.codegen_expression(&main_body)?; + let value = self.codegen_expression(&main_body)?; + let node_ids = value.to_node_ids(); + + self.context.new_instruction(Operation::Return(node_ids), ObjectType::NotAnObject)?; Ok(()) } @@ -366,17 +369,6 @@ impl IRGenerator { Ok(Value::dummy()) } - fn codegen_setpub( - &mut self, - expr: &Expression, - location: noirc_errors::Location, - ) -> Result { - let node_ids = self.codegen_expression(expr)?.to_node_ids(); - let operation = Operation::SetPub(node_ids, Some(location)); - self.context.new_instruction(operation, ObjectType::NotAnObject)?; - Ok(Value::dummy()) - } - /// Bind the given DefinitionId to the given Value. This will flatten the Value as needed, /// expanding each field of the value to a new variable. fn bind_id(&mut self, id: DefinitionId, value: Value, name: &str) -> Result<(), RuntimeError> { @@ -612,7 +604,6 @@ impl IRGenerator { self.codegen_expression(expr.as_ref())?; Ok(Value::dummy()) } - Expression::SetPub(expr, location) => self.codegen_setpub(expr, *location), } } diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 98a27a329ea..9782c64f3ff 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -236,13 +236,6 @@ impl SsaContext { let call = self.node_to_string(*call_instruction); format!("result {index} of {call}") } - Operation::SetPub(node_ids, _) => { - let mut s = "setpub ".to_string(); - for node_id in node_ids { - s = format!("{}, {}", s, self.node_to_string(*node_id),); - } - s - } } } diff --git a/crates/noirc_evaluator/src/ssa/inline.rs b/crates/noirc_evaluator/src/ssa/inline.rs index 77cc3810436..87ee1914841 100644 --- a/crates/noirc_evaluator/src/ssa/inline.rs +++ b/crates/noirc_evaluator/src/ssa/inline.rs @@ -408,7 +408,7 @@ impl node::Operation { ) { match self { //default way to handle arrays during inlining; we map arrays using the stack_frame - Operation::Binary(_) | Operation::Constrain(..) | Operation::Intrinsic(_,_) | Operation::SetPub(_,_) + Operation::Binary(_) | Operation::Constrain(..) | Operation::Intrinsic(_,_) => { self.map_id_mut(|id| { if let Some(a) = Memory::deref(ctx, id) { diff --git a/crates/noirc_evaluator/src/ssa/integer.rs b/crates/noirc_evaluator/src/ssa/integer.rs index 98a48314831..03450c36072 100644 --- a/crates/noirc_evaluator/src/ssa/integer.rs +++ b/crates/noirc_evaluator/src/ssa/integer.rs @@ -482,7 +482,6 @@ fn get_max_value(ins: &Instruction, max_map: &mut HashMap) -> B unreachable!("Functions must have been inlined before checking for overflows") } Operation::Intrinsic(opcode, _) => opcode.get_max_value(), - Operation::SetPub(_, _) => BigUint::zero(), }; if ins.res_type == ObjectType::NativeField { diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index 81c8a45600b..d7d01babba8 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -383,7 +383,6 @@ impl Instruction { Operation::Call { .. } => false, //return values are in the return statment, should we truncate function arguments? probably but not lhs and rhs anyways. Operation::Return(_) => true, Operation::Result { .. } => false, - Operation::SetPub { .. } => true, } } @@ -511,7 +510,6 @@ pub enum Operation { Not(NodeId), //(!) Bitwise Not Constrain(NodeId, Option), - SetPub(Vec, Option), //control flow Jne(NodeId, BlockId), //jump on not equal @@ -1102,7 +1100,6 @@ impl Operation { Result { call_instruction, index } => { Result { call_instruction: f(*call_instruction), index: *index } } - SetPub(values, loc) => SetPub(vecmap(values.iter().copied(), f), *loc), } } @@ -1157,11 +1154,6 @@ impl Operation { Result { call_instruction, index: _ } => { *call_instruction = f(*call_instruction); } - SetPub(values, _) => { - for value in values { - *value = f(*value); - } - } } } @@ -1203,7 +1195,6 @@ impl Operation { Result { call_instruction, .. } => { f(*call_instruction); } - SetPub(values, _) => values.iter().copied().for_each(f), } } @@ -1226,7 +1217,6 @@ impl Operation { Operation::Store { array_id, .. } => Opcode::Store(*array_id), Operation::Intrinsic(opcode, _) => Opcode::Intrinsic(*opcode), Operation::Nop => Opcode::Nop, - Operation::SetPub(_, _) => Opcode::SetPub, } } } From 4d6f86cbc98663b8b362fcfb167d2d361b6f4cbd Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 15:57:20 +0000 Subject: [PATCH 20/36] evaluator - add a method which tells you whether a witness index corresponds to private input from the ABI (main parameters) --- crates/noirc_evaluator/src/lib.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 5208cc032e3..e8021313bf1 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -19,6 +19,9 @@ pub struct Evaluator { // so it is safer to use a u64, at least until clang is changed // to compile wasm64. current_witness_index: u32, + // This is the number of witnesses indices used when + // creating the private/public inputs of the ABI. + num_witnesses_abi_len: usize, public_inputs: Vec, gates: Vec, } @@ -56,6 +59,7 @@ impl Evaluator { fn new() -> Self { Evaluator { public_inputs: Vec::new(), + num_witnesses_abi_len: 0, // XXX: Barretenberg, reserves the first index to have value 0. // When we increment, we do not use this index at all. // This means that every constraint system at the moment, will either need @@ -69,6 +73,23 @@ impl Evaluator { } } + // Returns true if the `witness_index` + // was created in the ABI as a private input. + // + // Note: This method is used so that we don't convert private + // ABI inputs into public outputs. + fn is_private_abi_input(&self, witness_index: Witness) -> bool { + // If the `witness_index` is more than the `num_witnesses_abi_len` + // then it was created after the ABI was processed and is therefore + // an intermediate variable. + let is_intermediate_variable = + witness_index.as_usize() > self.num_witnesses_abi_len as usize; + + let is_public_input = self.public_inputs.contains(&witness_index); + + !is_intermediate_variable && !is_public_input + } + // Creates a new Witness index fn add_witness_to_cs(&mut self) -> Witness { self.current_witness_index += 1; From e4a97886b16633e199d7bac01993a2e6128baef5 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 15:58:38 +0000 Subject: [PATCH 21/36] - use Operation::Return to set the values returned from main as public. - Additionally, private ABI inputs are not allowed --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 68 ++++++++++++---------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index b2f0d7d1bc6..cb832b051f3 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -177,7 +177,44 @@ impl Acir { InternalVar::from(v) } Operation::Call { .. } => unreachable!("call instruction should have been inlined"), - Operation::Return(_) => todo!(), //return from main + Operation::Return(node_ids) => { + // This can only ever be called in the main context. + // In all other context's, the return operation is transformed. + + for node_id in node_ids { + // An array produces a single node_id + // We therefore need to check if the node_id is referring to an array + // and deference to get the elements + let objects = match Memory::deref(ctx, *node_id) { + Some(a) => { + let array = &ctx.mem[a]; + self.load_array(array, false, evaluator) + } + None => vec![self.substitute(*node_id, evaluator, ctx)], + }; + + for mut object in objects { + // TODO(Open issue): We want to semantically specify that + // TODO you cannot return constant values as public outputs. + // TODO: in that case, you should just have a constant. + // TODO: For now, we will support this usecase. + let witness = if object.expression.is_const() { + evaluator.create_intermediate_variable(object.expression) + } else { + object.generate_witness(evaluator) + }; + + // Before pushing to the public inputs, we need to check that + // it was not a private ABI input + if evaluator.is_private_abi_input(witness) { + todo!("we do not allow private ABI inputs to be used as public outputs") + } + evaluator.public_inputs.push(witness); + } + } + + InternalVar::default() + } Operation::Cond { condition, val_true: lhs, val_false: rhs } => { let cond = self.substitute(*condition, evaluator, ctx); let l_c = self.substitute(*lhs, evaluator, ctx); @@ -230,35 +267,6 @@ impl Acir { todo!("dynamic arrays are not implemented yet"); } } - Operation::SetPub(node_ids, _) => { - for node_id in node_ids { - // An array produces a single node_id - // We therefore need to check if the node_id is referring to an array - // and deference to get the elements - let objects = match Memory::deref(ctx, *node_id) { - Some(a) => { - let array = &ctx.mem[a]; - self.load_array(array, false, evaluator) - } - None => vec![self.substitute(*node_id, evaluator, ctx)], - }; - - for mut object in objects { - // TODO(Open issue): We want to semantically specify that - // TODO you cannot return constant values as public outputs. - // TODO: in that case, you should just have a constant. - // TODO: For now, we will support this usecase. - let witness = if object.expression.is_const() { - evaluator.create_intermediate_variable(object.expression) - } else { - object.generate_witness(evaluator) - }; - evaluator.public_inputs.push(witness); - } - } - - InternalVar::default() - } }; output.id = Some(ins.id); self.arith_cache.insert(ins.id, output); From 05564f24074a4c31ad05309d641f9cdd206c1bbc Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 17:43:00 +0000 Subject: [PATCH 22/36] fix clippy --- crates/noirc_evaluator/src/lib.rs | 3 +-- crates/noirc_evaluator/src/ssa/node.rs | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index e8021313bf1..ef05f6a0b18 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -82,8 +82,7 @@ impl Evaluator { // If the `witness_index` is more than the `num_witnesses_abi_len` // then it was created after the ABI was processed and is therefore // an intermediate variable. - let is_intermediate_variable = - witness_index.as_usize() > self.num_witnesses_abi_len as usize; + let is_intermediate_variable = witness_index.as_usize() > self.num_witnesses_abi_len; let is_public_input = self.public_inputs.contains(&witness_index); diff --git a/crates/noirc_evaluator/src/ssa/node.rs b/crates/noirc_evaluator/src/ssa/node.rs index d7d01babba8..2adcca1a734 100644 --- a/crates/noirc_evaluator/src/ssa/node.rs +++ b/crates/noirc_evaluator/src/ssa/node.rs @@ -581,7 +581,6 @@ pub enum Opcode { Assign, Cond, Constrain, - SetPub, Cast, //convert type Truncate, //truncate Not, //(!) Bitwise Not From bd9e90941d8b42350e3bde2221dd98792e687227 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Mon, 16 Jan 2023 22:29:33 +0000 Subject: [PATCH 23/36] ssa-integer: do not skip the return operation during integer overflow - Only create the Return Operation if the return type from main is not Unit --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 4 ++++ crates/noirc_evaluator/src/ssa/code_gen.rs | 4 +++- crates/noirc_evaluator/src/ssa/integer.rs | 7 ++----- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index cb832b051f3..46f725c70c1 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -178,6 +178,10 @@ impl Acir { } Operation::Call { .. } => unreachable!("call instruction should have been inlined"), Operation::Return(node_ids) => { + // XXX: When we return a node_id that was created from + // the UnitType, there is a witness associated with it + // Ideally no witnesses are created for such types. + // This can only ever be called in the main context. // In all other context's, the return operation is transformed. diff --git a/crates/noirc_evaluator/src/ssa/code_gen.rs b/crates/noirc_evaluator/src/ssa/code_gen.rs index 42d54046f17..0eca6bfeda2 100644 --- a/crates/noirc_evaluator/src/ssa/code_gen.rs +++ b/crates/noirc_evaluator/src/ssa/code_gen.rs @@ -130,7 +130,9 @@ impl IRGenerator { let value = self.codegen_expression(&main_body)?; let node_ids = value.to_node_ids(); - self.context.new_instruction(Operation::Return(node_ids), ObjectType::NotAnObject)?; + if self.program.main().return_type != Type::Unit { + self.context.new_instruction(Operation::Return(node_ids), ObjectType::NotAnObject)?; + } Ok(()) } diff --git a/crates/noirc_evaluator/src/ssa/integer.rs b/crates/noirc_evaluator/src/ssa/integer.rs index 03450c36072..c95316666db 100644 --- a/crates/noirc_evaluator/src/ssa/integer.rs +++ b/crates/noirc_evaluator/src/ssa/integer.rs @@ -245,10 +245,7 @@ fn block_overflow( for mut ins in instructions { if matches!( ins.operation, - Operation::Nop - | Operation::Call { .. } - | Operation::Result { .. } - | Operation::Return(_) + Operation::Nop | Operation::Call { .. } | Operation::Result { .. } ) { //For now we skip completely functions from overflow; that means arguments are NOT truncated. //The reasoning is that this is handled by doing the overflow strategy after the function has been inlined @@ -477,7 +474,7 @@ fn get_max_value(ins: &Instruction, max_map: &mut HashMap) -> B Operation::Load { .. } => unreachable!(), Operation::Store { .. } => BigUint::zero(), Operation::Call { .. } => ins.res_type.max_size(), //n.b. functions should have been inlined - Operation::Return(_) => todo!(), + Operation::Return(_) => ins.res_type.max_size(), Operation::Result { .. } => { unreachable!("Functions must have been inlined before checking for overflows") } From fb7c6a3066e0b6c2dfa6cb6674c2fe699d09307c Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Tue, 17 Jan 2023 10:51:13 +0000 Subject: [PATCH 24/36] ssa-acir_gen : replace todo with Err Result variant --- crates/noirc_evaluator/src/ssa/acir_gen.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 8138311c8d1..792e8e5639d 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -211,7 +211,9 @@ impl Acir { // Before pushing to the public inputs, we need to check that // it was not a private ABI input if evaluator.is_private_abi_input(witness) { - todo!("we do not allow private ABI inputs to be used as public outputs") + return Err(RuntimeErrorKind::Spanless(String::from( + "we do not allow private ABI inputs to be returned as public outputs", + ))); } evaluator.public_inputs.push(witness); } From 21c0b2f48ea8b03681d17bdd37f07dc60c76ea0e Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Tue, 17 Jan 2023 17:20:29 +0000 Subject: [PATCH 25/36] remove TODOs --- crates/noirc_abi/src/lib.rs | 9 +++++---- crates/noirc_evaluator/src/ssa/acir_gen.rs | 4 ---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index d2bdb04173f..3d706ea7b91 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -146,10 +146,11 @@ impl Abi { // Condition that specifies whether we should filter the "return" // parameter. We do this in the case that it is not in the `inputs` // map specified. - // TODO Adding a `public outputs` field into acir and - // TODO the ABI will clean up this logic - // TODO For prosperity; the prover does not know about a `return` value - // TODO so we skip this when encoding the ABI + // + // See Issue #645 : Adding a `public outputs` field into acir and + // the ABI will clean up this logic + // For prosperity; the prover does not know about a `return` value + // so we skip this when encoding the ABI let return_condition = |param_name: &&String| !skip_output || (param_name != &MAIN_RETURN_NAME); diff --git a/crates/noirc_evaluator/src/ssa/acir_gen.rs b/crates/noirc_evaluator/src/ssa/acir_gen.rs index 15184847591..21999005ba5 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen.rs @@ -198,10 +198,6 @@ impl Acir { }; for mut object in objects { - // TODO(Open issue): We want to semantically specify that - // TODO you cannot return constant values as public outputs. - // TODO: in that case, you should just have a constant. - // TODO: For now, we will support this usecase. let witness = if object.expression.is_const() { evaluator.create_intermediate_variable(object.expression) } else { From 8cd6874d16768465be144ae9c26ef976187b5256 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 1 Feb 2023 20:45:45 +0000 Subject: [PATCH 26/36] remove returns --- .../higher-order-functions/Prover.toml | 2 +- .../nargo/tests/test_data/modulus/Prover.toml | 293 +++++++++++++++++- 2 files changed, 291 insertions(+), 4 deletions(-) diff --git a/crates/nargo/tests/test_data/higher-order-functions/Prover.toml b/crates/nargo/tests/test_data/higher-order-functions/Prover.toml index d3504ebe83f..8b137891791 100644 --- a/crates/nargo/tests/test_data/higher-order-functions/Prover.toml +++ b/crates/nargo/tests/test_data/higher-order-functions/Prover.toml @@ -1 +1 @@ -return = "5" \ No newline at end of file + diff --git a/crates/nargo/tests/test_data/modulus/Prover.toml b/crates/nargo/tests/test_data/modulus/Prover.toml index d46300b8a5b..d435609bb1a 100644 --- a/crates/nargo/tests/test_data/modulus/Prover.toml +++ b/crates/nargo/tests/test_data/modulus/Prover.toml @@ -1,3 +1,290 @@ -bn254_modulus_be_bytes = [48, 100, 78, 114, 225, 49, 160, 41, 184, 80, 69, 182, 129, 129, 88, 93, 40, 51, 232, 72, 121, 185, 112, 145, 67, 225, 245, 147, 240, 0, 0, 1] -bn254_modulus_be_bits = [1, 1, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 0, 1, 1, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1, 0, 1, 1, 0, 1, 1, 0, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 1, 1, 0, 0, 0, 0, 1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 1, 1, 1, 0, 1, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 1, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 0, 1, 0, 1, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1] -return = "" \ No newline at end of file +bn254_modulus_be_bytes = [ + 48, + 100, + 78, + 114, + 225, + 49, + 160, + 41, + 184, + 80, + 69, + 182, + 129, + 129, + 88, + 93, + 40, + 51, + 232, + 72, + 121, + 185, + 112, + 145, + 67, + 225, + 245, + 147, + 240, + 0, + 0, + 1, +] +bn254_modulus_be_bits = [ + 1, + 1, + 0, + 0, + 0, + 0, + 0, + 1, + 1, + 0, + 0, + 1, + 0, + 0, + 0, + 1, + 0, + 0, + 1, + 1, + 1, + 0, + 0, + 1, + 1, + 1, + 0, + 0, + 1, + 0, + 1, + 1, + 1, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 1, + 1, + 0, + 0, + 0, + 1, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 1, + 0, + 0, + 1, + 1, + 0, + 1, + 1, + 1, + 0, + 0, + 0, + 0, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 0, + 1, + 0, + 1, + 1, + 0, + 1, + 1, + 0, + 1, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 1, + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 1, + 0, + 1, + 1, + 0, + 0, + 0, + 0, + 1, + 0, + 1, + 1, + 1, + 0, + 1, + 0, + 0, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 0, + 1, + 1, + 0, + 0, + 1, + 1, + 1, + 1, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 1, + 0, + 0, + 0, + 0, + 1, + 1, + 1, + 1, + 0, + 0, + 1, + 1, + 0, + 1, + 1, + 1, + 0, + 0, + 1, + 0, + 1, + 1, + 1, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 1, + 0, + 0, + 0, + 1, + 0, + 1, + 0, + 0, + 0, + 0, + 1, + 1, + 1, + 1, + 1, + 0, + 0, + 0, + 0, + 1, + 1, + 1, + 1, + 1, + 0, + 1, + 0, + 1, + 1, + 0, + 0, + 1, + 0, + 0, + 1, + 1, + 1, + 1, + 1, + 1, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 1, +] From 80cefdaddac5146e63e2f82a66fa63a4620376ea Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 1 Feb 2023 21:27:01 +0000 Subject: [PATCH 27/36] delete unused `to_bytes` --- crates/nargo/tests/test_data/to_bytes/Prover.toml | 1 - 1 file changed, 1 deletion(-) delete mode 100644 crates/nargo/tests/test_data/to_bytes/Prover.toml diff --git a/crates/nargo/tests/test_data/to_bytes/Prover.toml b/crates/nargo/tests/test_data/to_bytes/Prover.toml deleted file mode 100644 index 07fe857ac7c..00000000000 --- a/crates/nargo/tests/test_data/to_bytes/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -x = "2040124" From deae405c365435bb66155ec45df7d02b1a9d9e3e Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 1 Feb 2023 21:27:20 +0000 Subject: [PATCH 28/36] remove `return` from `to_le_bytes` example --- crates/nargo/tests/test_data/to_le_bytes/Prover.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/nargo/tests/test_data/to_le_bytes/Prover.toml b/crates/nargo/tests/test_data/to_le_bytes/Prover.toml index f5f4f367d1c..07fe857ac7c 100644 --- a/crates/nargo/tests/test_data/to_le_bytes/Prover.toml +++ b/crates/nargo/tests/test_data/to_le_bytes/Prover.toml @@ -1,2 +1 @@ x = "2040124" -return = [0x3c, 0x21, 0x1f, 0x00] \ No newline at end of file From 78fc16295eba311fdb36529cfe0af0233103b5cc Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 1 Feb 2023 22:01:26 +0000 Subject: [PATCH 29/36] reduce diff --- crates/nargo/src/cli/prove_cmd.rs | 9 +++++---- crates/nargo/src/cli/verify_cmd.rs | 22 ++++++++++------------ 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index fe2e2dc02f3..f3fe114291a 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -5,16 +5,17 @@ use acvm::PartialWitnessGenerator; use acvm::ProofSystemCompiler; use clap::ArgMatches; use noirc_abi::errors::AbiError; -use noirc_abi::input_parser::Format; -use noirc_abi::input_parser::InputValue; +use noirc_abi::input_parser::{Format, InputValue}; use std::{ collections::{BTreeMap, HashSet}, path::{Path, PathBuf}, }; use super::{create_named_dir, read_inputs_from_file, write_inputs_to_file, write_to_file}; -use crate::constants::{PROOFS_DIR, PROOF_EXT, PROVER_INPUT_FILE, VERIFIER_INPUT_FILE}; -use crate::errors::CliError; +use crate::{ + constants::{PROOFS_DIR, PROOF_EXT, PROVER_INPUT_FILE, VERIFIER_INPUT_FILE}, + errors::CliError, +}; pub(crate) fn run(args: ArgMatches) -> Result<(), CliError> { let args = args.subcommand_matches("prove").unwrap(); diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index 22ad4aa3216..a77cd69b9b7 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -1,19 +1,17 @@ -use crate::errors::CliError; -use acvm::ProofSystemCompiler; -use clap::ArgMatches; -use std::{collections::BTreeMap, path::Path, path::PathBuf}; - -use noirc_abi::{ - errors::AbiError, - input_parser::{Format, InputValue}, -}; -use noirc_driver::CompiledProgram; - use super::{ compile_cmd::compile_circuit, prove_cmd::dedup_public_input_indices_values, read_inputs_from_file, }; -use crate::constants::{PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE}; +use crate::{ + constants::{PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE}, + errors::CliError, +}; +use acvm::ProofSystemCompiler; +use clap::ArgMatches; +use noirc_abi::errors::AbiError; +use noirc_abi::input_parser::{Format, InputValue}; +use noirc_driver::CompiledProgram; +use std::{collections::BTreeMap, path::Path, path::PathBuf}; pub(crate) fn run(args: ArgMatches) -> Result<(), CliError> { let args = args.subcommand_matches("verify").unwrap(); From 4a72ccf7300cd24b6c9b14c0a3ab8d914a1c0e14 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 1 Feb 2023 22:05:22 +0000 Subject: [PATCH 30/36] move dedup methods to mod.rs file --- crates/nargo/src/cli/mod.rs | 34 ++++++++++++++++++++++++++++- crates/nargo/src/cli/prove_cmd.rs | 35 ++---------------------------- crates/nargo/src/cli/verify_cmd.rs | 3 +-- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/crates/nargo/src/cli/mod.rs b/crates/nargo/src/cli/mod.rs index 269acbfbf6a..442e8270658 100644 --- a/crates/nargo/src/cli/mod.rs +++ b/crates/nargo/src/cli/mod.rs @@ -1,3 +1,4 @@ +use acvm::{acir::circuit::PublicInputs, FieldElement}; pub use check_cmd::check_from_path; use clap::{App, AppSettings, Arg}; use const_format::formatcp; @@ -9,7 +10,7 @@ use noirc_abi::{ use noirc_driver::Driver; use noirc_frontend::graph::{CrateName, CrateType}; use std::{ - collections::BTreeMap, + collections::{BTreeMap, HashSet}, fs::File, io::Write, path::{Path, PathBuf}, @@ -206,6 +207,37 @@ fn path_to_stdlib() -> PathBuf { dirs::config_dir().unwrap().join("noir-lang").join("std/src") } +// Removes duplicates from the list of public input witnesses +fn dedup_public_input_indices(indices: PublicInputs) -> PublicInputs { + let duplicates_removed: HashSet<_> = indices.0.into_iter().collect(); + PublicInputs(duplicates_removed.into_iter().collect()) +} + +// Removes duplicates from the list of public input witnesses and the +// associated list of duplicate values. +pub(crate) fn dedup_public_input_indices_values( + indices: PublicInputs, + values: Vec, +) -> (PublicInputs, Vec) { + // Assume that the public input index lists and the values contain duplicates + assert_eq!(indices.0.len(), values.len()); + + let mut public_inputs_without_duplicates = Vec::new(); + let mut already_seen_public_indices = HashSet::new(); + + for (index, value) in indices.0.iter().zip(values) { + if !already_seen_public_indices.contains(&index) { + already_seen_public_indices.insert(index); + public_inputs_without_duplicates.push(value) + } + } + + ( + PublicInputs(already_seen_public_indices.into_iter().cloned().collect()), + public_inputs_without_duplicates, + ) +} + // FIXME: I not sure that this is the right place for this tests. #[cfg(test)] mod tests { diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index f3fe114291a..2d824a8ddd3 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -1,4 +1,3 @@ -use acvm::acir::circuit::PublicInputs; use acvm::acir::native_types::Witness; use acvm::FieldElement; use acvm::PartialWitnessGenerator; @@ -7,11 +6,12 @@ use clap::ArgMatches; use noirc_abi::errors::AbiError; use noirc_abi::input_parser::{Format, InputValue}; use std::{ - collections::{BTreeMap, HashSet}, + collections::BTreeMap, path::{Path, PathBuf}, }; use super::{create_named_dir, read_inputs_from_file, write_inputs_to_file, write_to_file}; +use crate::cli::dedup_public_input_indices; use crate::{ constants::{PROOFS_DIR, PROOF_EXT, PROVER_INPUT_FILE, VERIFIER_INPUT_FILE}, errors::CliError, @@ -158,34 +158,3 @@ fn save_proof_to_dir>( Ok(proof_path) } - -// Removes duplicates from the list of public input witnesses -fn dedup_public_input_indices(indices: PublicInputs) -> PublicInputs { - let duplicates_removed: HashSet<_> = indices.0.into_iter().collect(); - PublicInputs(duplicates_removed.into_iter().collect()) -} - -// Removes duplicates from the list of public input witnesses and the -// associated list of duplicate values. -pub(crate) fn dedup_public_input_indices_values( - indices: PublicInputs, - values: Vec, -) -> (PublicInputs, Vec) { - // Assume that the public input index lists and the values contain duplicates - assert_eq!(indices.0.len(), values.len()); - - let mut public_inputs_without_duplicates = Vec::new(); - let mut already_seen_public_indices = HashSet::new(); - - for (index, value) in indices.0.iter().zip(values) { - if !already_seen_public_indices.contains(&index) { - already_seen_public_indices.insert(index); - public_inputs_without_duplicates.push(value) - } - } - - ( - PublicInputs(already_seen_public_indices.into_iter().cloned().collect()), - public_inputs_without_duplicates, - ) -} diff --git a/crates/nargo/src/cli/verify_cmd.rs b/crates/nargo/src/cli/verify_cmd.rs index a77cd69b9b7..8ec38531102 100644 --- a/crates/nargo/src/cli/verify_cmd.rs +++ b/crates/nargo/src/cli/verify_cmd.rs @@ -1,6 +1,5 @@ use super::{ - compile_cmd::compile_circuit, prove_cmd::dedup_public_input_indices_values, - read_inputs_from_file, + compile_cmd::compile_circuit, dedup_public_input_indices_values, read_inputs_from_file, }; use crate::{ constants::{PROOFS_DIR, PROOF_EXT, VERIFIER_INPUT_FILE}, From c77853d5c5e5161d8b3bb8d0a75147cc68a430e7 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 16:11:39 +0000 Subject: [PATCH 31/36] remove TODO on return type keyword --- crates/noirc_evaluator/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 87b4ccbc78f..7b5bb264ce0 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -286,7 +286,6 @@ impl Evaluator { // Since this is not in the main functions parameters. // // TODO(See Issue633) regarding adding a `return_type` field to the ABI struct - // TODO(See Issue631) regarding making the return keyword reserved let abi_params: Vec<_> = abi_params .into_iter() .filter(|param| param.name != noirc_abi::MAIN_RETURN_NAME) From c0146b67f5631f6e228d3dc8575757275ab5249c Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 17:11:50 +0000 Subject: [PATCH 32/36] move deduplication code into `compile_circuit_and_witness` --- crates/nargo/src/cli/prove_cmd.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/nargo/src/cli/prove_cmd.rs b/crates/nargo/src/cli/prove_cmd.rs index 2d824a8ddd3..a1fda13b3db 100644 --- a/crates/nargo/src/cli/prove_cmd.rs +++ b/crates/nargo/src/cli/prove_cmd.rs @@ -48,16 +48,9 @@ pub fn prove_with_path>( show_ssa: bool, allow_warnings: bool, ) -> Result, CliError> { - let (mut compiled_program, solved_witness) = + let (compiled_program, solved_witness) = compile_circuit_and_witness(program_dir, show_ssa, allow_warnings)?; - // Since the public outputs are added into the public inputs list - // There can be duplicates. We keep the duplicates for when one is - // encoding the return values into the Verifier.toml - // However, for creating a proof, we remove these duplicates. - compiled_program.circuit.public_inputs = - dedup_public_input_indices(compiled_program.circuit.public_inputs); - let backend = crate::backends::ConcreteBackend; let proof = backend.prove_with_meta(compiled_program.circuit, solved_witness); @@ -78,12 +71,20 @@ pub fn compile_circuit_and_witness>( show_ssa: bool, allow_unused_variables: bool, ) -> Result<(noirc_driver::CompiledProgram, BTreeMap), CliError> { - let compiled_program = super::compile_cmd::compile_circuit( + let mut compiled_program = super::compile_cmd::compile_circuit( program_dir.as_ref(), show_ssa, allow_unused_variables, )?; let solved_witness = parse_and_solve_witness(program_dir, &compiled_program)?; + + // Since the public outputs are added into the public inputs list + // There can be duplicates. We keep the duplicates for when one is + // encoding the return values into the Verifier.toml + // However, for creating a proof, we remove these duplicates. + compiled_program.circuit.public_inputs = + dedup_public_input_indices(compiled_program.circuit.public_inputs); + Ok((compiled_program, solved_witness)) } From ba2e28b09470af820af778bee929f8d754c3a488 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 20:49:55 +0000 Subject: [PATCH 33/36] update `num_witnesses_abi_len` parameter --- crates/noirc_evaluator/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/noirc_evaluator/src/lib.rs b/crates/noirc_evaluator/src/lib.rs index 7b5bb264ce0..3e07c6ae7c1 100644 --- a/crates/noirc_evaluator/src/lib.rs +++ b/crates/noirc_evaluator/src/lib.rs @@ -299,5 +299,9 @@ impl Evaluator { self.param_to_var(param_name, def, &abi_param.typ, &abi_param.visibility, igen) .unwrap(); } + + // Store the number of witnesses used to represent the types + // in the ABI + self.num_witnesses_abi_len = self.current_witness_index as usize; } } From bb95372621fe8e18cb4ac221462b4cfaf0e80b63 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 21:53:19 +0000 Subject: [PATCH 34/36] fix example: private values in the ABI cannot be returned as public --- crates/nargo/tests/test_data/main_return/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nargo/tests/test_data/main_return/src/main.nr b/crates/nargo/tests/test_data/main_return/src/main.nr index aefa2532731..06347eb0919 100644 --- a/crates/nargo/tests/test_data/main_return/src/main.nr +++ b/crates/nargo/tests/test_data/main_return/src/main.nr @@ -1,3 +1,3 @@ -fn main(x: Field) -> pub Field { +fn main(x: pub Field) -> pub Field { x } From 5c24c3688bcba0699879f8d24d7d4bf3375ec602 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 22:27:53 +0000 Subject: [PATCH 35/36] use HashMap and check for previous insertion value --- crates/nargo/src/cli/mod.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/nargo/src/cli/mod.rs b/crates/nargo/src/cli/mod.rs index 442e8270658..1f82b40d4bd 100644 --- a/crates/nargo/src/cli/mod.rs +++ b/crates/nargo/src/cli/mod.rs @@ -10,7 +10,7 @@ use noirc_abi::{ use noirc_driver::Driver; use noirc_frontend::graph::{CrateName, CrateType}; use std::{ - collections::{BTreeMap, HashSet}, + collections::{BTreeMap, HashMap, HashSet}, fs::File, io::Write, path::{Path, PathBuf}, @@ -223,17 +223,24 @@ pub(crate) fn dedup_public_input_indices_values( assert_eq!(indices.0.len(), values.len()); let mut public_inputs_without_duplicates = Vec::new(); - let mut already_seen_public_indices = HashSet::new(); + let mut already_seen_public_indices = HashMap::new(); for (index, value) in indices.0.iter().zip(values) { - if !already_seen_public_indices.contains(&index) { - already_seen_public_indices.insert(index); - public_inputs_without_duplicates.push(value) + match already_seen_public_indices.get(index) { + Some(expected_value) => { + // The index has already been added + // so lets check that the values already inserted is equal to the value, we wish to insert + assert_eq!(*expected_value, value, "witness index {:?} does not have a canonical map. The expected value is {expected_value}, the received value is {value}.", index) + } + None => { + already_seen_public_indices.insert(*index, value); + public_inputs_without_duplicates.push(value) + } } } ( - PublicInputs(already_seen_public_indices.into_iter().cloned().collect()), + PublicInputs(already_seen_public_indices.keys().copied().collect()), public_inputs_without_duplicates, ) } From 5ffaad60e9af79f4114ddd224f082919f65f64eb Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 2 Feb 2023 22:42:08 +0000 Subject: [PATCH 36/36] fix clippy --- crates/nargo/src/cli/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nargo/src/cli/mod.rs b/crates/nargo/src/cli/mod.rs index 1f82b40d4bd..336dffecc09 100644 --- a/crates/nargo/src/cli/mod.rs +++ b/crates/nargo/src/cli/mod.rs @@ -230,7 +230,7 @@ pub(crate) fn dedup_public_input_indices_values( Some(expected_value) => { // The index has already been added // so lets check that the values already inserted is equal to the value, we wish to insert - assert_eq!(*expected_value, value, "witness index {:?} does not have a canonical map. The expected value is {expected_value}, the received value is {value}.", index) + assert_eq!(*expected_value, value, "witness index {index:?} does not have a canonical map. The expected value is {expected_value}, the received value is {value}.") } None => { already_seen_public_indices.insert(*index, value);