diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index e7fa601fbe3..7ecc4d28032 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1984,14 +1984,7 @@ impl<'a> Context<'a> { if let NumericType::Unsigned { bit_size } = &num_type { // Check for integer overflow - self.check_unsigned_overflow( - result, - *bit_size, - binary.lhs, - binary.rhs, - dfg, - binary.operator, - )?; + self.check_unsigned_overflow(result, *bit_size, binary, dfg)?; } Ok(result) @@ -2002,47 +1995,18 @@ impl<'a> Context<'a> { &mut self, result: AcirVar, bit_size: u32, - lhs: ValueId, - rhs: ValueId, + binary: &Binary, dfg: &DataFlowGraph, - op: BinaryOp, ) -> Result<(), RuntimeError> { - // We try to optimize away operations that are guaranteed not to overflow - let max_lhs_bits = dfg.get_value_max_num_bits(lhs); - let max_rhs_bits = dfg.get_value_max_num_bits(rhs); - - let msg = match op { - BinaryOp::Add => { - if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { - // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. - return Ok(()); - } - "attempt to add with overflow".to_string() - } - BinaryOp::Sub => { - if dfg.is_constant(lhs) && max_lhs_bits > max_rhs_bits { - // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` - // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. - return Ok(()); - } - "attempt to subtract with overflow".to_string() - } - BinaryOp::Mul => { - if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { - // Either performing boolean multiplication (which cannot overflow), - // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. - return Ok(()); - } - "attempt to multiply with overflow".to_string() - } - _ => return Ok(()), + let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) else { + return Ok(()); }; let with_pred = self.acir_context.mul_var(result, self.current_side_effects_enabled_var)?; self.acir_context.range_constrain_var( with_pred, &NumericType::Unsigned { bit_size }, - Some(msg), + Some(msg.to_string()), )?; Ok(()) } @@ -2888,8 +2852,9 @@ mod test { use acvm::{ acir::{ circuit::{ - brillig::BrilligFunctionId, opcodes::AcirFunctionId, ExpressionWidth, Opcode, - OpcodeLocation, + brillig::BrilligFunctionId, + opcodes::{AcirFunctionId, BlackBoxFuncCall}, + ExpressionWidth, Opcode, OpcodeLocation, }, native_types::Witness, }, @@ -2913,6 +2878,8 @@ mod test { }, }; + use super::Ssa; + fn build_basic_foo_with_return( builder: &mut FunctionBuilder, foo_id: FunctionId, @@ -3659,4 +3626,36 @@ mod test { "Should have {expected_num_normal_calls} BrilligCall opcodes to normal Brillig functions but got {num_normal_brillig_calls}" ); } + + #[test] + fn multiply_with_bool_should_not_emit_range_check() { + let src = " + acir(inline) fn main f0 { + b0(v0: bool, v1: u32): + enable_side_effects v0 + v2 = cast v0 as u32 + v3 = mul v2, v1 + return v3 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let (mut acir_functions, _brillig_functions, _, _) = ssa + .into_acir(&brillig, ExpressionWidth::default()) + .expect("Should compile manually written SSA into ACIR"); + + assert_eq!(acir_functions.len(), 1); + + let opcodes = acir_functions[0].take_opcodes(); + + for opcode in opcodes { + if let Opcode::BlackBoxFuncCall(BlackBoxFuncCall::RANGE { input }) = opcode { + assert!( + input.to_witness().0 <= 1, + "only input witnesses should have range checks: {opcode:?}" + ); + } + } + } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 66cc213a986..31c99bf433e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1478,88 +1478,70 @@ impl<'block> BrilligBlock<'block> { is_signed: bool, ) { let bit_size = left.bit_size; - let max_lhs_bits = dfg.get_value_max_num_bits(binary.lhs); - let max_rhs_bits = dfg.get_value_max_num_bits(binary.rhs); - if bit_size == FieldElement::max_num_bits() { + if bit_size == FieldElement::max_num_bits() || is_signed { return; } - match (binary_operation, is_signed) { - (BrilligBinaryOp::Add, false) => { - if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { - // `left` and `right` have both been casted up from smaller types and so cannot overflow. - return; - } - - let condition = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // Check that lhs <= result - self.brillig_context.binary_instruction( - left, - result, - condition, - BrilligBinaryOp::LessThanEquals, - ); - self.brillig_context - .codegen_constrain(condition, Some("attempt to add with overflow".to_string())); - self.brillig_context.deallocate_single_addr(condition); - } - (BrilligBinaryOp::Sub, false) => { - if dfg.is_constant(binary.lhs) && max_lhs_bits > max_rhs_bits { - // `left` is a fixed constant and `right` is restricted such that `left - right > 0` - // Note strict inequality as `right > left` while `max_lhs_bits == max_rhs_bits` is possible. - return; - } - - let condition = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // Check that rhs <= lhs - self.brillig_context.binary_instruction( - right, - left, - condition, - BrilligBinaryOp::LessThanEquals, - ); - self.brillig_context.codegen_constrain( - condition, - Some("attempt to subtract with overflow".to_string()), - ); - self.brillig_context.deallocate_single_addr(condition); - } - (BrilligBinaryOp::Mul, false) => { - if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { - // Either performing boolean multiplication (which cannot overflow), - // or `left` and `right` have both been casted up from smaller types and so cannot overflow. - return; + if let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) { + match binary_operation { + BrilligBinaryOp::Add => { + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // Check that lhs <= result + self.brillig_context.binary_instruction( + left, + result, + condition, + BrilligBinaryOp::LessThanEquals, + ); + self.brillig_context.codegen_constrain(condition, Some(msg.to_string())); + self.brillig_context.deallocate_single_addr(condition); } - - let is_right_zero = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - let zero = self.brillig_context.make_constant_instruction(0_usize.into(), bit_size); - self.brillig_context.binary_instruction( - zero, - right, - is_right_zero, - BrilligBinaryOp::Equals, - ); - self.brillig_context.codegen_if_not(is_right_zero.address, |ctx| { - let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); - let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size); - // Check that result / rhs == lhs - ctx.binary_instruction(result, right, division, BrilligBinaryOp::UnsignedDiv); - ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); - ctx.codegen_constrain( + BrilligBinaryOp::Sub => { + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // Check that rhs <= lhs + self.brillig_context.binary_instruction( + right, + left, condition, - Some("attempt to multiply with overflow".to_string()), + BrilligBinaryOp::LessThanEquals, ); - ctx.deallocate_single_addr(condition); - ctx.deallocate_single_addr(division); - }); - self.brillig_context.deallocate_single_addr(is_right_zero); - self.brillig_context.deallocate_single_addr(zero); + self.brillig_context.codegen_constrain(condition, Some(msg.to_string())); + self.brillig_context.deallocate_single_addr(condition); + } + BrilligBinaryOp::Mul => { + let is_right_zero = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + let zero = + self.brillig_context.make_constant_instruction(0_usize.into(), bit_size); + self.brillig_context.binary_instruction( + zero, + right, + is_right_zero, + BrilligBinaryOp::Equals, + ); + self.brillig_context.codegen_if_not(is_right_zero.address, |ctx| { + let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); + let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size); + // Check that result / rhs == lhs + ctx.binary_instruction( + result, + right, + division, + BrilligBinaryOp::UnsignedDiv, + ); + ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); + ctx.codegen_constrain(condition, Some(msg.to_string())); + ctx.deallocate_single_addr(condition); + ctx.deallocate_single_addr(division); + }); + self.brillig_context.deallocate_single_addr(is_right_zero); + self.brillig_context.deallocate_single_addr(zero); + } + _ => {} } - _ => {} } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index ab37080ac1d..28e58e2cbb1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -319,6 +319,49 @@ impl Binary { }; SimplifyResult::None } + + /// Check if unsigned overflow is possible, and if so return some message to be used if it fails. + pub(crate) fn check_unsigned_overflow_msg( + &self, + dfg: &DataFlowGraph, + bit_size: u32, + ) -> Option<&'static str> { + // We try to optimize away operations that are guaranteed not to overflow + let max_lhs_bits = dfg.get_value_max_num_bits(self.lhs); + let max_rhs_bits = dfg.get_value_max_num_bits(self.rhs); + + let msg = match self.operator { + BinaryOp::Add => { + if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { + // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return None; + } + "attempt to add with overflow" + } + BinaryOp::Sub => { + if dfg.is_constant(self.lhs) && max_lhs_bits > max_rhs_bits { + // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` + // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. + return None; + } + "attempt to subtract with overflow" + } + BinaryOp::Mul => { + if bit_size == 1 + || max_lhs_bits + max_rhs_bits <= bit_size + || max_lhs_bits == 1 + || max_rhs_bits == 1 + { + // Either performing boolean multiplication (which cannot overflow), + // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return None; + } + "attempt to multiply with overflow" + } + _ => return None, + }; + Some(msg) + } } /// Evaluate a binary operation with constant arguments. diff --git a/test_programs/gates_report_brillig_execution.sh b/test_programs/gates_report_brillig_execution.sh index 6a5a699e2d8..219fbb5c11a 100755 --- a/test_programs/gates_report_brillig_execution.sh +++ b/test_programs/gates_report_brillig_execution.sh @@ -3,10 +3,10 @@ set -e # These tests are incompatible with gas reporting excluded_dirs=( - "workspace" - "workspace_default_member" - "double_verify_nested_proof" - "overlapping_dep_and_mod" + "workspace" + "workspace_default_member" + "double_verify_nested_proof" + "overlapping_dep_and_mod" "comptime_println" # bit sizes for bigint operation doesn't match up. "bigint" @@ -33,6 +33,10 @@ for dir in $test_dirs; do continue fi + if [[ ! -f "${base_path}/${dir}/Nargo.toml" ]]; then + continue + fi + echo " \"execution_success/$dir\"," >> Nargo.toml done diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index 651ddbc0b56..8d0fc257e1c 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -257,7 +257,14 @@ fn profile_brillig_execution( initial_witness, &Bn254BlackBoxSolver(pedantic_solving), &mut DefaultForeignCallBuilder::default().build(), - )?; + ) + .map_err(|e| { + CliError::Generic(format!( + "failed to execute '{}': {}", + package.root_dir.to_string_lossy(), + e + )) + })?; let expression_width = get_target_width(package.expression_width, expression_width);