From 13c41d21f81fb40cdf2a970be119a83d11da9e03 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 15:12:03 +0000 Subject: [PATCH 1/5] fix: optimizer to keep track of changing opcode locations (#6781) --- acvm-repo/acvm/src/compiler/mod.rs | 12 +++-- acvm-repo/acvm/src/compiler/optimizers/mod.rs | 17 ++++--- .../assert_message/Nargo.toml | 7 +++ .../assert_message/src/main.nr | 15 +++++++ .../fuzzer_checks/src/main.nr | 7 ++- tooling/nargo/src/ops/test.rs | 44 ++++++++++++++----- 6 files changed, 79 insertions(+), 23 deletions(-) create mode 100644 test_programs/noir_test_success/assert_message/Nargo.toml create mode 100644 test_programs/noir_test_success/assert_message/src/main.nr diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 4ad4952c5cc..daedd77c4a0 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -82,19 +82,22 @@ pub fn compile( acir: Circuit, expression_width: ExpressionWidth, ) -> (Circuit, AcirTransformationMap) { + let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); + if MAX_OPTIMIZER_PASSES == 0 { - let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); - return (acir, transformation_map); + return (acir, AcirTransformationMap::new(&acir_opcode_positions)); } + let mut pass = 0; let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); let mut prev_acir = acir; + let mut prev_acir_opcode_positions = acir_opcode_positions; // For most test programs it would be enough to only loop `transform_internal`, // but some of them don't stabilize unless we also repeat the backend agnostic optimizations. let (mut acir, acir_opcode_positions) = loop { - let (acir, acir_opcode_positions) = optimize_internal(prev_acir); + let (acir, acir_opcode_positions) = + optimize_internal(prev_acir, prev_acir_opcode_positions); // Stop if we have already done at least one transform and an extra optimization changed nothing. if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) { @@ -114,6 +117,7 @@ pub fn compile( pass += 1; prev_acir = acir; prev_opcodes_hash = opcodes_hash; + prev_acir_opcode_positions = acir_opcode_positions; }; let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); diff --git a/acvm-repo/acvm/src/compiler/optimizers/mod.rs b/acvm-repo/acvm/src/compiler/optimizers/mod.rs index f86bf500998..1b743227acf 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/mod.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/mod.rs @@ -21,7 +21,11 @@ use super::{transform_assert_messages, AcirTransformationMap}; /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`]. pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformationMap) { - let (mut acir, new_opcode_positions) = optimize_internal(acir); + // Track original acir opcode positions throughout the transformation passes of the compilation + // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert) + let acir_opcode_positions = (0..acir.opcodes.len()).collect(); + + let (mut acir, new_opcode_positions) = optimize_internal(acir, acir_opcode_positions); let transformation_map = AcirTransformationMap::new(&new_opcode_positions); @@ -31,12 +35,13 @@ pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformati } /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`]. +/// +/// Accepts an injected `acir_opcode_positions` to allow optimizations to be applied in a loop. #[tracing::instrument(level = "trace", name = "optimize_acir" skip(acir))] -pub(super) fn optimize_internal(acir: Circuit) -> (Circuit, Vec) { - // Track original acir opcode positions throughout the transformation passes of the compilation - // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert) - let acir_opcode_positions = (0..acir.opcodes.len()).collect(); - +pub(super) fn optimize_internal( + acir: Circuit, + acir_opcode_positions: Vec, +) -> (Circuit, Vec) { if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) { info!("Program is fully unconstrained, skipping optimization pass"); return (acir, acir_opcode_positions); diff --git a/test_programs/noir_test_success/assert_message/Nargo.toml b/test_programs/noir_test_success/assert_message/Nargo.toml new file mode 100644 index 00000000000..667035339bd --- /dev/null +++ b/test_programs/noir_test_success/assert_message/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "assert_message" +type = "bin" +authors = [""] +compiler_version = ">=0.23.0" + +[dependencies] diff --git a/test_programs/noir_test_success/assert_message/src/main.nr b/test_programs/noir_test_success/assert_message/src/main.nr new file mode 100644 index 00000000000..073e42daf4a --- /dev/null +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -0,0 +1,15 @@ +// Have to use inputs otherwise the ACIR gets optimized away due to constants. +// With the original ACIR the optimizations can rearrange or merge opcodes, +// which might end up getting out of sync with assertion messages. +#[test(should_fail_with = "first")] +fn test_assert_message_preserved_during_optimization(a: Field, b: Field, c: Field) { + if (a + b) * (a - b) != c { + assert((a + b) * (a - b) == c, "first"); + assert((a - b) * (a + b) == c, "second"); + assert((a + b) * (a - b) == c, "third"); + assert((2 * a + b) * (a - b / 2) == c * c, "fourth"); + } else { + // The fuzzer might have generated a passing test. + assert((a + b) * (a - b) != c, "first"); + } +} diff --git a/test_programs/noir_test_success/fuzzer_checks/src/main.nr b/test_programs/noir_test_success/fuzzer_checks/src/main.nr index 2b928db092e..70e5e29b855 100644 --- a/test_programs/noir_test_success/fuzzer_checks/src/main.nr +++ b/test_programs/noir_test_success/fuzzer_checks/src/main.nr @@ -1,6 +1,9 @@ - #[test(should_fail_with = "42 is not allowed")] fn finds_magic_value(x: u32) { let x = x as u64; - assert(2 * x != 42, "42 is not allowed"); + if x == 21 { + assert(2 * x != 42, "42 is not allowed"); + } else { + assert(2 * x == 42, "42 is not allowed"); + } } diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 7087c0de64e..e3a264359e4 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -9,7 +9,7 @@ use acvm::{ AcirField, BlackBoxFunctionSolver, FieldElement, }; use noirc_abi::Abi; -use noirc_driver::{compile_no_check, CompileError, CompileOptions}; +use noirc_driver::{compile_no_check, CompileError, CompileOptions, DEFAULT_EXPRESSION_WIDTH}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; use noirc_printable_type::ForeignCallError; @@ -29,6 +29,7 @@ use crate::{ use super::execute_program; +#[derive(Debug)] pub enum TestStatus { Pass, Fail { message: String, error_diagnostic: Option }, @@ -62,6 +63,10 @@ pub fn run_test>( match compile_no_check(context, config, test_function.get_id(), None, false) { Ok(compiled_program) => { + // Do the same optimizations as `compile_cmd`. + let target_width = config.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH); + let compiled_program = crate::ops::transform_program(compiled_program, target_width); + if test_function_has_no_arguments { // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, // otherwise constraints involving these expressions will not error. @@ -81,9 +86,9 @@ pub fn run_test>( let status = test_status_program_compile_pass( test_function, - compiled_program.abi, - compiled_program.debug, - circuit_execution, + &compiled_program.abi, + &compiled_program.debug, + &circuit_execution, ); let ignore_foreign_call_failures = @@ -118,11 +123,14 @@ pub fn run_test>( use proptest::test_runner::TestRunner; let runner = TestRunner::default(); + let abi = compiled_program.abi.clone(); + let debug = compiled_program.debug.clone(); + let executor = |program: &Program, initial_witness: WitnessMap| -> Result, String> { - execute_program( + let circuit_execution = execute_program( program, initial_witness, blackbox_solver, @@ -132,9 +140,23 @@ pub fn run_test>( root_path.clone(), package_name.clone(), ), - ) - .map_err(|err| err.to_string()) + ); + + let status = test_status_program_compile_pass( + test_function, + &abi, + &debug, + &circuit_execution, + ); + + if let TestStatus::Fail { message, error_diagnostic: _ } = status { + Err(message) + } else { + // The fuzzer doesn't care about the actual result. + Ok(WitnessStack::default()) + } }; + let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); let result = fuzzer.fuzz(); @@ -174,9 +196,9 @@ fn test_status_program_compile_fail(err: CompileError, test_function: &TestFunct /// passed/failed to determine the test status. fn test_status_program_compile_pass( test_function: &TestFunction, - abi: Abi, - debug: Vec, - circuit_execution: Result, NargoError>, + abi: &Abi, + debug: &[DebugInfo], + circuit_execution: &Result, NargoError>, ) -> TestStatus { let circuit_execution_err = match circuit_execution { // Circuit execution was successful; ie no errors or unsatisfied constraints @@ -196,7 +218,7 @@ fn test_status_program_compile_pass( // If we reach here, then the circuit execution failed. // // Check if the function should have passed - let diagnostic = try_to_diagnose_runtime_error(&circuit_execution_err, &abi, &debug); + let diagnostic = try_to_diagnose_runtime_error(circuit_execution_err, abi, debug); let test_should_have_passed = !test_function.should_fail(); if test_should_have_passed { return TestStatus::Fail { From 655a3d3fdcf4f4cdb4381e3ff47d2f822c4b2276 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 12 Dec 2024 12:24:04 -0300 Subject: [PATCH 2/5] fix: use extension in docs link so it also works on GitHub (#6787) --- docs/docs/getting_started/noir_installation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/getting_started/noir_installation.md b/docs/docs/getting_started/noir_installation.md index a5c7e649278..05f036d4f6d 100644 --- a/docs/docs/getting_started/noir_installation.md +++ b/docs/docs/getting_started/noir_installation.md @@ -93,7 +93,7 @@ step 2: Follow the [Noirup instructions](#installing-noirup). ## Setting up shell completions -Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions). +Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions.md). ## Uninstalling Nargo From bd46bdd7c3b92a29ca2ecba8cd1720cf94c7597e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Dec 2024 10:39:28 -0500 Subject: [PATCH 3/5] chore(ssa): Activate loop invariant code motion on ACIR functions (#6785) --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 87e7f8bcff3..9f07b076e8a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -13,7 +13,7 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::{ ir::{ basic_block::BasicBlockId, - function::{Function, RuntimeType}, + function::Function, function_inserter::FunctionInserter, instruction::{Instruction, InstructionId}, types::Type, @@ -27,12 +27,7 @@ use super::unrolling::{Loop, Loops}; impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa { - let brillig_functions = self - .functions - .iter_mut() - .filter(|(_, func)| matches!(func.runtime(), RuntimeType::Brillig(_))); - - for (_, function) in brillig_functions { + for function in self.functions.values_mut() { function.loop_invariant_code_motion(); } @@ -63,6 +58,7 @@ impl Loops { } context.map_dependent_instructions(); + context.inserter.map_data_bus_in_place(); } } From 5795a099657a268b735a539298dfeefa445db3ff Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 15:46:11 +0000 Subject: [PATCH 4/5] feat(cli): Verify `return` against ABI and `Prover.toml` (#6765) --- .../diamond_deps_0/Prover.toml | 2 +- .../execution_success/return_twice/Nargo.toml | 7 ++++ .../return_twice/Prover.toml | 2 ++ .../return_twice/src/main.nr | 5 +++ tooling/nargo_cli/src/cli/execute_cmd.rs | 34 +++++++++++++++---- tooling/nargo_cli/src/errors.rs | 13 ++++++- 6 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 test_programs/execution_success/return_twice/Nargo.toml create mode 100644 test_programs/execution_success/return_twice/Prover.toml create mode 100644 test_programs/execution_success/return_twice/src/main.nr diff --git a/test_programs/execution_success/diamond_deps_0/Prover.toml b/test_programs/execution_success/diamond_deps_0/Prover.toml index a713241e7dd..2bad103177c 100644 --- a/test_programs/execution_success/diamond_deps_0/Prover.toml +++ b/test_programs/execution_success/diamond_deps_0/Prover.toml @@ -1,3 +1,3 @@ x = 1 y = 1 -return = 5 \ No newline at end of file +return = 7 diff --git a/test_programs/execution_success/return_twice/Nargo.toml b/test_programs/execution_success/return_twice/Nargo.toml new file mode 100644 index 00000000000..defd68f37b8 --- /dev/null +++ b/test_programs/execution_success/return_twice/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "return_twice" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/return_twice/Prover.toml b/test_programs/execution_success/return_twice/Prover.toml new file mode 100644 index 00000000000..b343065a7fc --- /dev/null +++ b/test_programs/execution_success/return_twice/Prover.toml @@ -0,0 +1,2 @@ +return = [100, 100] +in0 = 10 diff --git a/test_programs/execution_success/return_twice/src/main.nr b/test_programs/execution_success/return_twice/src/main.nr new file mode 100644 index 00000000000..68bd5f916ce --- /dev/null +++ b/test_programs/execution_success/return_twice/src/main.nr @@ -0,0 +1,5 @@ +pub fn main(in0: Field) -> pub (Field, Field) { + let out0 = (in0 * in0); + let out1 = (in0 * in0); + (out0, out1) +} diff --git a/tooling/nargo_cli/src/cli/execute_cmd.rs b/tooling/nargo_cli/src/cli/execute_cmd.rs index c0838743d6d..49a23a7ea62 100644 --- a/tooling/nargo_cli/src/cli/execute_cmd.rs +++ b/tooling/nargo_cli/src/cli/execute_cmd.rs @@ -64,8 +64,9 @@ pub(crate) fn run(args: ExecuteCommand, config: NargoConfig) -> Result<(), CliEr let program_artifact_path = workspace.package_build_path(package); let program: CompiledProgram = read_program_from_file(program_artifact_path.clone())?.into(); + let abi = program.abi.clone(); - let (return_value, witness_stack) = execute_program_and_decode( + let results = execute_program_and_decode( program, package, &args.prover_name, @@ -75,14 +76,27 @@ pub(crate) fn run(args: ExecuteCommand, config: NargoConfig) -> Result<(), CliEr )?; println!("[{}] Circuit witness successfully solved", package.name); - if let Some(return_value) = return_value { + if let Some(ref return_value) = results.actual_return { println!("[{}] Circuit output: {return_value:?}", package.name); } let package_name = package.name.clone().into(); let witness_name = args.witness_name.as_ref().unwrap_or(&package_name); - let witness_path = save_witness_to_dir(witness_stack, witness_name, target_dir)?; + let witness_path = save_witness_to_dir(results.witness_stack, witness_name, target_dir)?; println!("[{}] Witness saved to {}", package.name, witness_path.display()); + + // Sanity checks on the return value after the witness has been saved, so it can be inspected if necessary. + if let Some(expected) = results.expected_return { + if results.actual_return.as_ref() != Some(&expected) { + return Err(CliError::UnexpectedReturn { expected, actual: results.actual_return }); + } + } + // We can expect that if the circuit returns something, it should be non-empty after execution. + if let Some(ref expected) = abi.return_type { + if results.actual_return.is_none() { + return Err(CliError::MissingReturn { expected: expected.clone() }); + } + } } Ok(()) } @@ -94,18 +108,24 @@ fn execute_program_and_decode( foreign_call_resolver_url: Option<&str>, root_path: Option, package_name: Option, -) -> Result<(Option, WitnessStack), CliError> { +) -> Result { // Parse the initial witness values from Prover.toml - let (inputs_map, _) = + let (inputs_map, expected_return) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &program.abi)?; let witness_stack = execute_program(&program, &inputs_map, foreign_call_resolver_url, root_path, package_name)?; // Get the entry point witness for the ABI let main_witness = &witness_stack.peek().expect("Should have at least one witness on the stack").witness; - let (_, return_value) = program.abi.decode(main_witness)?; + let (_, actual_return) = program.abi.decode(main_witness)?; + + Ok(ExecutionResults { expected_return, actual_return, witness_stack }) +} - Ok((return_value, witness_stack)) +struct ExecutionResults { + expected_return: Option, + actual_return: Option, + witness_stack: WitnessStack, } pub(crate) fn execute_program( diff --git a/tooling/nargo_cli/src/errors.rs b/tooling/nargo_cli/src/errors.rs index b28012ae7aa..9255d6fc6a6 100644 --- a/tooling/nargo_cli/src/errors.rs +++ b/tooling/nargo_cli/src/errors.rs @@ -2,7 +2,11 @@ use acvm::{acir::native_types::WitnessStackError, FieldElement}; use nargo::{errors::CompileError, NargoError}; use nargo_toml::ManifestError; use noir_debugger::errors::DapError; -use noirc_abi::errors::{AbiError, InputParserError}; +use noirc_abi::{ + errors::{AbiError, InputParserError}, + input_parser::InputValue, + AbiReturnType, +}; use std::path::PathBuf; use thiserror::Error; @@ -32,6 +36,7 @@ pub(crate) enum FilesystemError { pub(crate) enum CliError { #[error("{0}")] Generic(String), + #[error("Error: destination {} already exists", .0.display())] DestinationAlreadyExists(PathBuf), @@ -63,4 +68,10 @@ pub(crate) enum CliError { /// Error from the compilation pipeline #[error(transparent)] CompileError(#[from] CompileError), + + #[error("Unexpected return value: expected {expected:?}; got {actual:?}")] + UnexpectedReturn { expected: InputValue, actual: Option }, + + #[error("Missing return witnesses; expected {expected:?}")] + MissingReturn { expected: AbiReturnType }, } From 68ff7bd85cb17f57afd481ec55318ec282a93aa6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 12 Dec 2024 13:20:49 -0300 Subject: [PATCH 5/5] fix: disable failure persistance in nargo test fuzzing (#6777) --- tooling/nargo/src/ops/test.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index e3a264359e4..1306150518d 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -120,8 +120,11 @@ pub fn run_test>( { use acvm::acir::circuit::Program; use noir_fuzzer::FuzzedExecutor; + use proptest::test_runner::Config; use proptest::test_runner::TestRunner; - let runner = TestRunner::default(); + + let runner = + TestRunner::new(Config { failure_persistence: None, ..Config::default() }); let abi = compiled_program.abi.clone(); let debug = compiled_program.debug.clone();