From 6154ee35066985114f4343ca78375a2336103ea9 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 00:26:56 +0000 Subject: [PATCH 1/7] Fix optimizer to keep track of changing opcode locations --- acvm-repo/acvm/src/compiler/mod.rs | 71 ++++++++++--------- acvm-repo/acvm/src/compiler/optimizers/mod.rs | 17 +++-- 2 files changed, 49 insertions(+), 39 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 4ad4952c5cc..1f5dbdf8ccd 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -79,42 +79,47 @@ fn transform_assert_messages( /// /// Runs multiple passes until the output stabilizes. pub fn compile( - acir: Circuit, + mut acir: Circuit, expression_width: ExpressionWidth, ) -> (Circuit, AcirTransformationMap) { - 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); - } - let mut pass = 0; - let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); - let mut prev_acir = acir; - - // 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); - - // 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) { - break (acir, acir_opcode_positions); - } - - let (acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions); - - let opcodes_hash = fxhash::hash64(&acir.opcodes); - - // Stop if the output hasn't change in this loop or we went too long. - if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { - break (acir, acir_opcode_positions); - } + let mut acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); + + if MAX_OPTIMIZER_PASSES > 0 { + 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 (new_acir, new_acir_opcode_positions) = loop { + 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) { + break (acir, acir_opcode_positions); + } + + let (acir, acir_opcode_positions) = + transform_internal(acir, expression_width, acir_opcode_positions); + + let opcodes_hash = fxhash::hash64(&acir.opcodes); + + // Stop if the output hasn't change in this loop or we went too long. + if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { + break (acir, acir_opcode_positions); + } + + pass += 1; + prev_acir = acir; + prev_opcodes_hash = opcodes_hash; + prev_acir_opcode_positions = acir_opcode_positions; + }; - pass += 1; - prev_acir = acir; - prev_opcodes_hash = opcodes_hash; - }; + acir = new_acir; + acir_opcode_positions = new_acir_opcode_positions; + } let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); 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); From 8e97fbf438aabac1d218dc076a1948e94faafa1d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 01:02:49 +0000 Subject: [PATCH 2/7] Early return on 0 --- acvm-repo/acvm/src/compiler/mod.rs | 73 +++++++++++++++--------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 1f5dbdf8ccd..daedd77c4a0 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -79,48 +79,47 @@ fn transform_assert_messages( /// /// Runs multiple passes until the output stabilizes. pub fn compile( - mut acir: Circuit, + acir: Circuit, expression_width: ExpressionWidth, ) -> (Circuit, AcirTransformationMap) { - let mut acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - - if MAX_OPTIMIZER_PASSES > 0 { - 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 (new_acir, new_acir_opcode_positions) = loop { - 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) { - break (acir, acir_opcode_positions); - } - - let (acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions); - - let opcodes_hash = fxhash::hash64(&acir.opcodes); - - // Stop if the output hasn't change in this loop or we went too long. - if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { - break (acir, acir_opcode_positions); - } - - pass += 1; - prev_acir = acir; - prev_opcodes_hash = opcodes_hash; - prev_acir_opcode_positions = acir_opcode_positions; - }; + let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - acir = new_acir; - acir_opcode_positions = new_acir_opcode_positions; + if MAX_OPTIMIZER_PASSES == 0 { + 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, 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) { + break (acir, acir_opcode_positions); + } + + let (acir, acir_opcode_positions) = + transform_internal(acir, expression_width, acir_opcode_positions); + + let opcodes_hash = fxhash::hash64(&acir.opcodes); + + // Stop if the output hasn't change in this loop or we went too long. + if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { + break (acir, acir_opcode_positions); + } + + 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); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); From 359a3c92085d5b50de1c423d73509fe0ffcd501c Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 12:04:29 +0000 Subject: [PATCH 3/7] Expose the error in an integration tests --- acvm-repo/acvm/src/compiler/mod.rs | 2 ++ test_programs/noir_test_success/assert_message/Nargo.toml | 7 +++++++ test_programs/noir_test_success/assert_message/src/main.nr | 7 +++++++ tooling/nargo/src/ops/test.rs | 6 +++++- 4 files changed, 21 insertions(+), 1 deletion(-) 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 daedd77c4a0..36001024453 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -96,6 +96,8 @@ pub fn compile( // 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 { + prev_acir_opcode_positions = (0..prev_acir.opcodes.len()).rev().collect::>(); + let (acir, acir_opcode_positions) = optimize_internal(prev_acir, prev_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..e11210d4f21 --- /dev/null +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -0,0 +1,7 @@ +#[test(should_fail_with = "first")] +fn test_assert_message_preserved_during_optimization(a: u64, b: u64, c: u64) { + 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"); +} diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index e258627b522..e05d2af0b6a 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; @@ -60,6 +60,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. From 8c8b03ed5e26c709049fcf6af718edcb2ce2f21d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 12:39:34 +0000 Subject: [PATCH 4/7] Update the fuzzer to handle expected failures --- .../assert_message/src/main.nr | 15 +++++--- tooling/nargo/src/ops/test.rs | 36 ++++++++++++++----- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/test_programs/noir_test_success/assert_message/src/main.nr b/test_programs/noir_test_success/assert_message/src/main.nr index e11210d4f21..128325114ee 100644 --- a/test_programs/noir_test_success/assert_message/src/main.nr +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -1,7 +1,12 @@ #[test(should_fail_with = "first")] -fn test_assert_message_preserved_during_optimization(a: u64, b: u64, c: u64) { - 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"); +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/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index e05d2af0b6a..c218a6d3604 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -27,6 +27,7 @@ use crate::{ use super::execute_program; +#[derive(Debug)] pub enum TestStatus { Pass, Fail { message: String, error_diagnostic: Option }, @@ -83,9 +84,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 = @@ -120,11 +121,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, @@ -134,9 +138,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(); @@ -176,9 +194,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 From 0514339ea3024b94d47727bc1c92efabc3a7865a Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 12:40:40 +0000 Subject: [PATCH 5/7] Fix the canary bug --- acvm-repo/acvm/src/compiler/mod.rs | 2 -- test_programs/noir_test_success/assert_message/src/main.nr | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 36001024453..daedd77c4a0 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -96,8 +96,6 @@ pub fn compile( // 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 { - prev_acir_opcode_positions = (0..prev_acir.opcodes.len()).rev().collect::>(); - let (acir, acir_opcode_positions) = optimize_internal(prev_acir, prev_acir_opcode_positions); diff --git a/test_programs/noir_test_success/assert_message/src/main.nr b/test_programs/noir_test_success/assert_message/src/main.nr index 128325114ee..f746774de2a 100644 --- a/test_programs/noir_test_success/assert_message/src/main.nr +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -1,3 +1,4 @@ +// Have to use inputs otherwise the ACIR gets optimized away due to constants. #[test(should_fail_with = "first")] fn test_assert_message_preserved_during_optimization(a: Field, b: Field, c: Field) { if (a + b) * (a - b) != c { From f30484448c2ac15bb81bc398c5a1d1afacd8ed63 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 12:47:45 +0000 Subject: [PATCH 6/7] Fix(?) another fuzzer test --- test_programs/noir_test_success/fuzzer_checks/src/main.nr | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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"); + } } From a3b66ae8e512810f6950a64d930d1be07d457c12 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 12:52:42 +0000 Subject: [PATCH 7/7] Fix clippy --- test_programs/noir_test_success/assert_message/src/main.nr | 2 ++ tooling/nargo/src/ops/test.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test_programs/noir_test_success/assert_message/src/main.nr b/test_programs/noir_test_success/assert_message/src/main.nr index f746774de2a..073e42daf4a 100644 --- a/test_programs/noir_test_success/assert_message/src/main.nr +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -1,4 +1,6 @@ // 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 { diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 1122e4a8c49..e3a264359e4 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -218,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 {