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

)

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>

compiler/noirc_driver/src/lib.rs

@@ -133,20 +133,16 @@ pub struct CompileOptions {
     #[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)
@@ -698,7 +694,7 @@ pub fn compile_no_check(
         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,8 +72,8 @@ pub struct SsaEvaluatorOptions {
     /// 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
@@ -143,7 +143,7 @@ pub(crate) fn optimize_into_acir(
         ));
     }
 
-    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,

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

@@ -113,8 +113,8 @@ struct DependencyContext {
     tainted: BTreeMap<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<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
@@ -138,8 +138,6 @@ struct BrilligTaintedIds {
     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 @@ impl BrilligTaintedIds {
             results: results_status,
             array_elements,
             root_results: HashSet::from_iter(results.iter().copied()),
-            tracking: false,
         }
     }
 
@@ -394,23 +391,19 @@ impl DependencyContext {
                 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.contains_key(call) {
+                                self.tracking.insert(*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.contains_key(instruction) {
+                self.tracking.insert(*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
@@ -524,7 +517,7 @@ impl DependencyContext {
                     // 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);
                     }
@@ -563,7 +556,10 @@ impl DependencyContext {
             .tainted
             .keys()
             .map(|brillig_call| {
-                trace!("tainted structure for {}: {:?}", brillig_call, self.tainted[brillig_call]);
+                trace!(
+                    "tainted structure for {:?}: {:?}",
+                    brillig_call, self.tainted[brillig_call]
+                );
                 SsaReport::Bug(InternalBug::UncheckedBrilligCall {
                     call_stack: function.dfg.get_instruction_call_stack(*brillig_call),
                 })
@@ -587,8 +583,8 @@ impl DependencyContext {
         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 in &self.tracking {
+            if let Some(tainted_ids) = self.tainted.get_mut(call) {
                 tainted_ids.update_children(&parents, children);
             }
         }
@@ -605,15 +601,15 @@ impl DependencyContext {
             .collect();
 
         // Skip untracked calls
-        for (_, tainted_ids) in self.tainted.iter_mut() {
-            if tainted_ids.tracking {
+        for call in &self.tracking {
+            if let Some(tainted_ids) = self.tainted.get_mut(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
@@ -624,19 +620,19 @@ impl DependencyContext {
     /// 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 in &self.tracking {
+                    if let Some(tainted_ids) = self.tainted.get_mut(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,
         };

docs/docs/tooling/security.md

@@ -39,7 +39,7 @@ Here, the results of `factor` are two elements of the returned array. The value
 
 This pass checks if the constraint coverage of Brillig calls is sufficient in these terms.
 
-The check is at the moment disabled by default due to performance concerns and can be enabled by passing the `--enable-brillig-constraints-check` option to `nargo`.
+The check is enabled by default and can be disabled by passing the `--skip-brillig-constraints-check` option to `nargo`.
 
 #### Lookback option
 

tooling/nargo_cli/build.rs

@@ -438,7 +438,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(),
@@ -468,7 +467,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(),