diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 7295e46ea7a..3712634d707 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/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, }; diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 7857a8ddd09..935918c6b7e 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/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, diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index dee10dfbecf..752b2c82d09 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -113,8 +113,8 @@ struct DependencyContext { tainted: BTreeMap, // Map of argument value ids to the Brillig call ids employing them call_arguments: HashMap>, - // Maintains count of calls being tracked - tracking_count: usize, + // The set of calls currently being tracked + tracking: HashSet, // 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>, // Initial result value ids, along with element ids for arrays root_results: HashSet, - // 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,10 +620,10 @@ 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; @@ -635,8 +631,8 @@ impl DependencyContext { 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); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/hint.rs b/compiler/noirc_evaluator/src/ssa/opt/hint.rs index 638da8b7b6e..911554e5d6d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/hint.rs +++ b/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, }; diff --git a/docs/docs/tooling/security.md b/docs/docs/tooling/security.md index e14481efc31..8a09d231a7d 100644 --- a/docs/docs/tooling/security.md +++ b/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 diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 904e404d7c5..b9faf018dfd 100644 --- a/tooling/nargo_cli/build.rs +++ b/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(),