chore(ssa): Turn the Brillig constraints check back on by default

compiler/noirc_driver/src/lib.rs

@@ -136,25 +136,21 @@
     #[arg(long)]
     pub skip_underconstrained_check: bool,
 
-    /// Flag to turn on the compiler check for missing Brillig call constraints.
-    /// Warning: This can degrade compilation speed but will also find some correctness errors.
+    /// Flag to turn off the compiler check for missing Brillig call constraints.
+    /// Warning: This can improve compilation speed but can also lead to correctness errors.
     /// This check should always be run on production code.
     #[arg(long)]
-    pub enable_brillig_constraints_check: bool,
+    pub skip_brillig_constraints_check: bool,
 
     /// Flag to turn on extra Brillig bytecode to be generated to guard against invalid states in testing.
     #[arg(long, hide = true)]
     pub enable_brillig_debug_assertions: bool,
 
-    /// Hidden Brillig call check flag to maintain CI compatibility (currently ignored)
-    #[arg(long, hide = true)]
-    pub skip_brillig_constraints_check: bool,
-
     /// Flag to turn on the lookback feature of the Brillig call constraints
     /// check, allowing tracking argument values before the call happens preventing
     /// certain rare false positives (leads to a slowdown on large rollout functions)
     #[arg(long)]
     pub enable_brillig_constraints_check_lookback: bool,
 
     /// Setting to decide on an inlining strategy for Brillig functions.
     /// A more aggressive inliner should generate larger programs but more optimized
@@ -697,9 +693,9 @@
         },
         emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None },
         skip_underconstrained_check: options.skip_underconstrained_check,
         enable_brillig_constraints_check_lookback: options
             .enable_brillig_constraints_check_lookback,
-        enable_brillig_constraints_check: options.enable_brillig_constraints_check,
+        skip_brillig_constraints_check: options.skip_brillig_constraints_check,
         inliner_aggressiveness: options.inliner_aggressiveness,
         max_bytecode_increase_percent: options.max_bytecode_increase_percent,
     };

compiler/noirc_evaluator/src/ssa.rs

@@ -72,13 +72,13 @@
     /// Skip the check for under constrained values
     pub skip_underconstrained_check: bool,
 
-    /// Enable the missing Brillig call constraints check
-    pub enable_brillig_constraints_check: bool,
+    /// Skip the missing Brillig call constraints check
+    pub skip_brillig_constraints_check: bool,
 
     /// Enable the lookback feature of the Brillig call constraints
     /// check (prevents some rare false positives, leads to a slowdown
     /// on large rollout functions)
     pub enable_brillig_constraints_check_lookback: bool,
 
     /// The higher the value, the more inlined Brillig functions will be.
     pub inliner_aggressiveness: i64,
@@ -143,13 +143,13 @@
         ));
     }
 
-    if options.enable_brillig_constraints_check {
+    if !options.skip_brillig_constraints_check {
         ssa_level_warnings.extend(time(
             "After Check for Missing Brillig Call Constraints",
             options.print_codegen_timings,
             || {
                 ssa.check_for_missing_brillig_constraints(
                     options.enable_brillig_constraints_check_lookback,
                 )
             },
         ));

compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs

@@ -41,7 +41,7 @@
     /// and return a vector of bug reports if any have been found
     pub(crate) fn check_for_missing_brillig_constraints(
         &mut self,
         enable_lookback: bool,
     ) -> Vec<SsaReport> {
         // Skip the check if there are no Brillig functions involved
         if !self.functions.values().any(|func| func.runtime().is_brillig()) {
@@ -57,7 +57,7 @@
                 match function_to_process.runtime() {
                     RuntimeType::Acir { .. } => {
                         let mut context =
                             DependencyContext { enable_lookback, ..Default::default() };
                         context.build(function_to_process, &self.functions);
                         context.collect_warnings(function_to_process)
                     }
@@ -110,12 +110,12 @@
     side_effects_condition: Option<ValueId>,
     // Map of Brillig call ids to sets of the value ids descending
     // from their arguments and results
-    tainted: BTreeMap<InstructionId, BrilligTaintedIds>,
+    tainted: BTreeMap<(FunctionId, InstructionId), BrilligTaintedIds>,
     // Map of argument value ids to the Brillig call ids employing them
     call_arguments: HashMap<ValueId, Vec<InstructionId>>,
-    // Maintains count of calls being tracked
-    tracking_count: usize,
+    // The set of calls currently being tracked
+    tracking: HashSet<(FunctionId, InstructionId)>,
     // Opt-in to use the lookback feature (tracking the argument values
     // of a Brillig call before the call happens if their usage precedes
     // it). Can prevent certain false positives, at the cost of
     // slowing down checking large functions considerably
@@ -138,8 +138,6 @@
     array_elements: HashMap<ValueId, Vec<usize>>,
     // Initial result value ids, along with element ids for arrays
     root_results: HashSet<ValueId>,
-    // The flag signaling that the call should be now tracked
-    tracking: bool,
 }
 
 #[derive(Clone, Debug)]
@@ -195,7 +193,6 @@
             results: results_status,
             array_elements,
             root_results: HashSet::from_iter(results.iter().copied()),
-            tracking: false,
         }
     }
 
@@ -358,7 +355,7 @@
 
                             if !wrapped_call_found {
                                 // Record the current call, remember the argument values involved
-                                self.tainted.insert(*instruction, current_tainted);
+                                self.tainted.insert((function.id(), *instruction), current_tainted);
                                 arguments.iter().for_each(|value| {
                                     self.call_arguments
                                         .entry(*value)
@@ -394,23 +391,19 @@
                 for argument in &arguments {
                     if let Some(calls) = self.call_arguments.get(argument) {
                         for call in calls {
-                            if let Some(tainted_ids) = self.tainted.get_mut(call) {
-                                tainted_ids.tracking = true;
-                                self.tracking_count += 1;
+                            if self.tainted.get(&(function.id(), *call)).is_some() {
+                                self.tracking.insert((function.id(), *call));
                             }
                         }
                     }
                 }
             }
-            if let Some(tainted_ids) = self.tainted.get_mut(instruction) {
-                if !tainted_ids.tracking {
-                    tainted_ids.tracking = true;
-                    self.tracking_count += 1;
-                }
+            if self.tainted.get(&(function.id(), *instruction)).is_some() {
+                self.tracking.insert((function.id(), *instruction));
             }
 
             // We can skip over instructions while nothing is being tracked
-            if self.tracking_count > 0 {
+            if !self.tracking.is_empty() {
                 let mut results = Vec::new();
 
                 // Collect non-constant instruction results
@@ -519,7 +512,7 @@
                     // results involving the array in question, to properly
                     // populate the array element tainted sets
                     Instruction::ArrayGet { array, index } => {
-                        self.process_array_get(function, *array, *index, &results);
+                        self.process_array_get(*array, *index, &results, function);
                         // Record all the used arguments as parents of the results
                         self.update_children(&arguments, &results);
                     }
@@ -557,8 +550,12 @@
         let warnings: Vec<SsaReport> = self
             .tainted
             .keys()
-            .map(|brillig_call| {
-                trace!("tainted structure for {}: {:?}", brillig_call, self.tainted[brillig_call]);
+            .map(|(_, brillig_call)| {
+                trace!(
+                    "tainted structure for {:?}: {:?}",
+                    brillig_call,
+                    self.tainted[&(function.id(), *brillig_call)]
+                );
                 SsaReport::Bug(InternalBug::UncheckedBrilligCall {
                     call_stack: function.dfg.get_instruction_call_stack(*brillig_call),
                 })
@@ -582,8 +579,8 @@
         self.side_effects_condition.map(|v| parents.insert(v));
 
         // Don't update sets for the calls not yet being tracked
-        for (_, tainted_ids) in self.tainted.iter_mut() {
-            if tainted_ids.tracking {
+        for (call, tainted_ids) in self.tainted.iter_mut() {
+            if self.tracking.contains(call) {
                 tainted_ids.update_children(&parents, children);
             }
         }
@@ -600,15 +597,15 @@
             .collect();
 
         // Skip untracked calls
-        for (_, tainted_ids) in self.tainted.iter_mut() {
-            if tainted_ids.tracking {
+        for (call, tainted_ids) in self.tainted.iter_mut() {
+            if self.tracking.contains(call) {
                 tainted_ids.store_partial_constraints(&constrained_values);
             }
         }
 
-        self.tainted.retain(|_, tainted_ids| {
+        self.tainted.retain(|call, tainted_ids| {
             if tainted_ids.check_constrained() {
-                self.tracking_count -= 1;
+                self.tracking.remove(call);
                 false
             } else {
                 true
@@ -619,19 +616,19 @@
     /// Process ArrayGet instruction for tracked Brillig calls
     fn process_array_get(
         &mut self,
-        function: &Function,
         array: ValueId,
         index: ValueId,
         element_results: &[ValueId],
+        function: &Function,
     ) {
         use acvm::acir::AcirField;
 
         // Only allow numeric constant indices
         if let Some(value) = function.dfg.get_numeric_constant(index) {
             if let Some(index) = value.try_to_u32() {
                 // Skip untracked calls
-                for (_, tainted_ids) in self.tainted.iter_mut() {
-                    if tainted_ids.tracking {
+                for (call, tainted_ids) in self.tainted.iter_mut() {
+                    if self.tracking.contains(call) {
                         tainted_ids.process_array_get(array, index as usize, element_results);
                     }
                 }

compiler/noirc_evaluator/src/ssa/opt/hint.rs

@@ -20,7 +20,7 @@ mod tests {
             emit_ssa: None,
             skip_underconstrained_check: true,
             enable_brillig_constraints_check_lookback: false,
-            enable_brillig_constraints_check: false,
+            skip_brillig_constraints_check: true,
             inliner_aggressiveness: 0,
             max_bytecode_increase_percent: None,
         };

tooling/nargo_cli/build.rs

@@ -433,7 +433,6 @@ fn generate_compile_success_no_bug_tests(test_file: &mut File, test_data_dir: &P
             &test_dir,
             "compile",
             r#"
-                nargo.arg("--enable-brillig-constraints-check");
                 nargo.assert().success().stderr(predicate::str::contains("bug:").not());
             "#,
             &MatrixConfig::default(),
@@ -463,7 +462,6 @@ fn generate_compile_success_with_bug_tests(test_file: &mut File, test_data_dir:
             &test_dir,
             "compile",
             r#"
-                nargo.arg("--enable-brillig-constraints-check");
                 nargo.assert().success().stderr(predicate::str::contains("bug:"));
             "#,
             &MatrixConfig::default(),