From 56461285ef7904224a2c3b42e0f15b69ccec8de6 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Tue, 27 Feb 2024 15:03:51 +0000 Subject: [PATCH 1/6] fix: Remove old coercions done in brillig gen --- .../src/brillig/brillig_gen/brillig_block.rs | 14 +------------- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 11 ----------- 2 files changed, 1 insertion(+), 24 deletions(-) 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 f01f60252f6..cfc69d41a81 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,9 +1,7 @@ use crate::brillig::brillig_ir::brillig_variable::{ type_to_heap_value_type, BrilligArray, BrilligVariable, BrilligVector, SingleAddrVariable, }; -use crate::brillig::brillig_ir::{ - BrilligBinaryOp, BrilligContext, BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE, -}; +use crate::brillig::brillig_ir::{BrilligBinaryOp, BrilligContext}; use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::instruction::ConstrainError; use crate::ssa::ir::{ @@ -1403,8 +1401,6 @@ impl<'block> BrilligBlock<'block> { } /// Returns the type of the operation considering the types of the operands -/// TODO: SSA issues binary operations between fields and integers. -/// This probably should be explicitly casted in SSA to avoid having to coerce at this level. pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type { match (lhs_type, rhs_type) { (_, Type::Function) | (Type::Function, _) => { @@ -1419,10 +1415,6 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type (_, Type::Slice(..)) | (Type::Slice(..), _) => { unreachable!("Arrays are invalid in binary operations") } - // If either side is a Field constant then, we coerce into the type - // of the other operand - (Type::Numeric(NumericType::NativeField), typ) - | (typ, Type::Numeric(NumericType::NativeField)) => typ.clone(), // If both sides are numeric type, then we expect their types to be // the same. (Type::Numeric(lhs_type), Type::Numeric(rhs_type)) => { @@ -1461,10 +1453,6 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( BinaryOp::Mul => BrilligBinaryOp::Field { op: BinaryFieldOp::Mul }, BinaryOp::Div => BrilligBinaryOp::Field { op: BinaryFieldOp::Div }, BinaryOp::Eq => BrilligBinaryOp::Field { op: BinaryFieldOp::Equals }, - BinaryOp::Lt => BrilligBinaryOp::Integer { - op: BinaryIntOp::LessThan, - bit_size: BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE, - }, _ => unreachable!( "Field type cannot be used with {op}. This should have been caught by the frontend" ), diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 90608974f98..9b140535a98 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -29,17 +29,6 @@ use acvm::{ use debug_show::DebugShow; use num_bigint::BigUint; -/// Integer arithmetic in Brillig is limited to 127 bit -/// integers. -/// -/// We could lift this in the future and have Brillig -/// do big integer arithmetic when it exceeds the field size -/// or we could have users re-implement big integer arithmetic -/// in Brillig. -/// Since constrained functions do not have this property, it -/// would mean that unconstrained functions will differ from -/// constrained functions in terms of syntax compatibility. -pub(crate) const BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE: u32 = 127; /// The Brillig VM does not apply a limit to the memory address space, /// As a convention, we take use 64 bits. This means that we assume that /// memory has 2^64 memory slots. From d77d1e7cc51b5ab535f5e7d0200e44187e88bf02 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 28 Feb 2024 07:50:27 +0000 Subject: [PATCH 2/6] feat: Add underflow and overflow checks in brillig --- .../src/brillig/brillig_gen/brillig_block.rs | 111 +++++++++++++++++- .../noirc_evaluator/src/brillig/brillig_ir.rs | 17 ++- .../brillig_fns_as_values/Prover.toml | 2 +- .../brillig_fns_as_values/src/main.nr | 4 +- 4 files changed, 126 insertions(+), 8 deletions(-) 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 cfc69d41a81..94fde31e998 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1195,15 +1195,116 @@ impl<'block> BrilligBlock<'block> { let left = self.convert_ssa_single_addr_value(binary.lhs, dfg); let right = self.convert_ssa_single_addr_value(binary.rhs, dfg); - let brillig_binary_op = + let (brillig_binary_op, is_signed) = convert_ssa_binary_op_to_brillig_binary_op(binary.operator, &binary_type); + self.add_underflow_check(brillig_binary_op, left, right, is_signed); self.brillig_context.binary_instruction( left.address, right.address, result_variable.address, brillig_binary_op, ); + + self.add_overflow_check(brillig_binary_op, left, right, result_variable, is_signed); + } + + fn add_underflow_check( + &mut self, + binary_operation: BrilligBinaryOp, + left: SingleAddrVariable, + right: SingleAddrVariable, + is_signed: bool, + ) { + let (op, bit_size) = if let BrilligBinaryOp::Integer { op, bit_size } = binary_operation { + (op, bit_size) + } else { + return; + }; + + if let (BinaryIntOp::Sub, false) = (op, is_signed) { + let condition = self.brillig_context.allocate_register(); + // Check that rhs <= lhs + self.brillig_context.binary_instruction( + right.address, + left.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, + ); + self.brillig_context + .constrain_instruction(condition, Some("Underflow sub check failed".to_string())); + self.brillig_context.deallocate_register(condition); + } + } + + fn add_overflow_check( + &mut self, + binary_operation: BrilligBinaryOp, + left: SingleAddrVariable, + right: SingleAddrVariable, + result: SingleAddrVariable, + is_signed: bool, + ) { + let (op, bit_size) = if let BrilligBinaryOp::Integer { op, bit_size } = binary_operation { + (op, bit_size) + } else { + return; + }; + + match (op, is_signed) { + (BinaryIntOp::Add, false) => { + let condition = self.brillig_context.allocate_register(); + // Check that lhs <= result + self.brillig_context.binary_instruction( + left.address, + result.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, + ); + self.brillig_context.constrain_instruction( + condition, + Some("Overflow add check failed".to_string()), + ); + self.brillig_context.deallocate_register(condition); + } + (BinaryIntOp::Mul, false) => { + // Multiplication overflow is only possible for bit sizes > 1 + if bit_size > 1 { + let is_right_zero = self.brillig_context.allocate_register(); + let zero = self.brillig_context.make_constant(0_usize.into(), bit_size); + self.brillig_context.binary_instruction( + zero, + right.address, + is_right_zero, + BrilligBinaryOp::Integer { op: BinaryIntOp::Equals, bit_size }, + ); + self.brillig_context.if_not_instruction(is_right_zero, |ctx| { + let condition = ctx.allocate_register(); + // Check that result / rhs == lhs + ctx.binary_instruction( + result.address, + right.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::UnsignedDiv, bit_size }, + ); + ctx.binary_instruction( + condition, + left.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::Equals, bit_size }, + ); + ctx.constrain_instruction( + condition, + Some("Overflow mul check failed".to_string()), + ); + ctx.deallocate_register(condition); + }); + self.brillig_context.deallocate_register(is_right_zero); + self.brillig_context.deallocate_register(zero); + } + } + _ => {} + } } /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. @@ -1433,7 +1534,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( ssa_op: BinaryOp, typ: &Type, -) -> BrilligBinaryOp { +) -> (BrilligBinaryOp, bool) { // First get the bit size and whether its a signed integer, if it is a numeric type // if it is not,then we return None, indicating that // it is a Field. @@ -1488,7 +1589,9 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( // If bit size is available then it is a binary integer operation match bit_size_signedness { - Some((bit_size, is_signed)) => binary_op_to_int_op(ssa_op, *bit_size, is_signed), - None => binary_op_to_field_op(ssa_op), + Some((bit_size, is_signed)) => { + (binary_op_to_int_op(ssa_op, *bit_size, is_signed), is_signed) + } + None => (binary_op_to_field_op(ssa_op), false), } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 9b140535a98..ed415005a10 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -345,6 +345,21 @@ impl BrilligContext { self.enter_section(end_section); } + /// This instruction issues a branch that jumps over the code generated by the given function if the condition is truthy + pub(crate) fn if_not_instruction( + &mut self, + condition: MemoryAddress, + mut f: impl FnMut(&mut BrilligContext), + ) { + let (end_section, end_label) = self.reserve_next_section_label(); + + self.jump_if_instruction(condition, end_label.clone()); + + f(self); + + self.enter_section(end_section); + } + /// Adds a label to the next opcode pub(crate) fn enter_context(&mut self, label: T) { self.debug_show.enter_context(label.to_string()); @@ -1071,7 +1086,7 @@ impl BrilligContext { } /// Type to encapsulate the binary operation types in Brillig -#[derive(Clone)] +#[derive(Clone, Copy)] pub(crate) enum BrilligBinaryOp { Field { op: BinaryFieldOp }, Integer { op: BinaryIntOp, bit_size: u32 }, diff --git a/test_programs/execution_success/brillig_fns_as_values/Prover.toml b/test_programs/execution_success/brillig_fns_as_values/Prover.toml index 11497a473bc..4dd6b405159 100644 --- a/test_programs/execution_success/brillig_fns_as_values/Prover.toml +++ b/test_programs/execution_success/brillig_fns_as_values/Prover.toml @@ -1 +1 @@ -x = "0" +x = "1" diff --git a/test_programs/execution_success/brillig_fns_as_values/src/main.nr b/test_programs/execution_success/brillig_fns_as_values/src/main.nr index ea3148915b8..9248bff2f4c 100644 --- a/test_programs/execution_success/brillig_fns_as_values/src/main.nr +++ b/test_programs/execution_success/brillig_fns_as_values/src/main.nr @@ -7,9 +7,9 @@ struct MyStruct { fn main(x: u32) { assert(wrapper(increment, x) == x + 1); assert(wrapper(increment_acir, x) == x + 1); - assert(wrapper(decrement, x) == std::wrapping_sub(x, 1)); + assert(wrapper(decrement, x) == x - 1); assert(wrapper_with_struct(MyStruct { operation: increment }, x) == x + 1); - assert(wrapper_with_struct(MyStruct { operation: decrement }, x) == std::wrapping_sub(x, 1)); + assert(wrapper_with_struct(MyStruct { operation: decrement }, x) == x - 1); // https://github.com/noir-lang/noir/issues/1975 assert(increment(x) == x + 1); } From 323c529d88fe8e13ac9dd5eca91fa1b55e177dcf Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 28 Feb 2024 11:13:19 +0000 Subject: [PATCH 3/6] test: Add test for overflows --- .../src/brillig/brillig_gen/brillig_block.rs | 48 +++++++------------ .../brillig_overflow_checks/Nargo.toml | 5 ++ .../brillig_overflow_checks/Prover.toml | 0 .../brillig_overflow_checks/src/main.nr | 23 +++++++++ 4 files changed, 45 insertions(+), 31 deletions(-) create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/Prover.toml create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/src/main.nr 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 94fde31e998..a3ee3e7ca7e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1198,7 +1198,6 @@ impl<'block> BrilligBlock<'block> { let (brillig_binary_op, is_signed) = convert_ssa_binary_op_to_brillig_binary_op(binary.operator, &binary_type); - self.add_underflow_check(brillig_binary_op, left, right, is_signed); self.brillig_context.binary_instruction( left.address, right.address, @@ -1209,34 +1208,6 @@ impl<'block> BrilligBlock<'block> { self.add_overflow_check(brillig_binary_op, left, right, result_variable, is_signed); } - fn add_underflow_check( - &mut self, - binary_operation: BrilligBinaryOp, - left: SingleAddrVariable, - right: SingleAddrVariable, - is_signed: bool, - ) { - let (op, bit_size) = if let BrilligBinaryOp::Integer { op, bit_size } = binary_operation { - (op, bit_size) - } else { - return; - }; - - if let (BinaryIntOp::Sub, false) = (op, is_signed) { - let condition = self.brillig_context.allocate_register(); - // Check that rhs <= lhs - self.brillig_context.binary_instruction( - right.address, - left.address, - condition, - BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, - ); - self.brillig_context - .constrain_instruction(condition, Some("Underflow sub check failed".to_string())); - self.brillig_context.deallocate_register(condition); - } - } - fn add_overflow_check( &mut self, binary_operation: BrilligBinaryOp, @@ -1263,7 +1234,22 @@ impl<'block> BrilligBlock<'block> { ); self.brillig_context.constrain_instruction( condition, - Some("Overflow add check failed".to_string()), + Some("attempt to add with overflow".to_string()), + ); + self.brillig_context.deallocate_register(condition); + } + (BinaryIntOp::Sub, false) => { + let condition = self.brillig_context.allocate_register(); + // Check that rhs <= lhs + self.brillig_context.binary_instruction( + right.address, + left.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, + ); + self.brillig_context.constrain_instruction( + condition, + Some("attempt to subtract with overflow".to_string()), ); self.brillig_context.deallocate_register(condition); } @@ -1295,7 +1281,7 @@ impl<'block> BrilligBlock<'block> { ); ctx.constrain_instruction( condition, - Some("Overflow mul check failed".to_string()), + Some("attempt to multiply with overflow".to_string()), ); ctx.deallocate_register(condition); }); diff --git a/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml b/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml new file mode 100644 index 00000000000..b2d47d258ed --- /dev/null +++ b/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml @@ -0,0 +1,5 @@ +[package] +name = "brillig_overflow_checks" +type = "bin" +authors = [""] +[dependencies] diff --git a/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml b/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr b/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr new file mode 100644 index 00000000000..5d73ef96d49 --- /dev/null +++ b/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr @@ -0,0 +1,23 @@ +use dep::std::field::bn254::{TWO_POW_128, assert_gt}; + +#[test(should_fail_with = "attempt to add with overflow")] +unconstrained fn test_overflow_add() { + let a: u8 = 255; + let b: u8 = 1; + assert_eq(a + b, 0); +} + +#[test(should_fail_with = "attempt to subtract with overflow")] +unconstrained fn test_overflow_sub() { + let a: u8 = 0; + let b: u8 = 1; + assert_eq(a - b, 255); +} + +#[test(should_fail_with = "attempt to multiply with overflow")] +unconstrained fn test_overflow_mul() { + let a: u8 = 128; + let b: u8 = 2; + assert_eq(a * b, 0); +} + From 39b5cdb71bc74dac7058b4703256b465fb2761a8 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 28 Feb 2024 11:24:28 +0000 Subject: [PATCH 4/6] clippy --- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index ed415005a10..674be07cbd0 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -537,7 +537,7 @@ impl BrilligContext { result: MemoryAddress, operation: BrilligBinaryOp, ) { - self.debug_show.binary_instruction(lhs, rhs, result, operation.clone()); + self.debug_show.binary_instruction(lhs, rhs, result, operation); match operation { BrilligBinaryOp::Field { op } => { let opcode = BrilligOpcode::BinaryFieldOp { op, destination: result, lhs, rhs }; From 5932f06aed1c1d8c98725a23285e76e1c1ff65f7 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 29 Feb 2024 09:05:13 +0000 Subject: [PATCH 5/6] test: Add brillig wrapping test --- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 2 +- .../execution_success/brillig_wrapping/Nargo.toml | 6 ++++++ .../execution_success/brillig_wrapping/Prover.toml | 2 ++ .../execution_success/brillig_wrapping/src/main.nr | 8 ++++++++ 4 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 test_programs/execution_success/brillig_wrapping/Nargo.toml create mode 100644 test_programs/execution_success/brillig_wrapping/Prover.toml create mode 100644 test_programs/execution_success/brillig_wrapping/src/main.nr diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 674be07cbd0..aab96d73f9e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -349,7 +349,7 @@ impl BrilligContext { pub(crate) fn if_not_instruction( &mut self, condition: MemoryAddress, - mut f: impl FnMut(&mut BrilligContext), + mut f: impl FnOnce(&mut BrilligContext), ) { let (end_section, end_label) = self.reserve_next_section_label(); diff --git a/test_programs/execution_success/brillig_wrapping/Nargo.toml b/test_programs/execution_success/brillig_wrapping/Nargo.toml new file mode 100644 index 00000000000..a52246ba908 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "brillig_wrapping" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/brillig_wrapping/Prover.toml b/test_programs/execution_success/brillig_wrapping/Prover.toml new file mode 100644 index 00000000000..346fd2764a7 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/Prover.toml @@ -0,0 +1,2 @@ +x = 0 +y = 255 diff --git a/test_programs/execution_success/brillig_wrapping/src/main.nr b/test_programs/execution_success/brillig_wrapping/src/main.nr new file mode 100644 index 00000000000..4153a466057 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/src/main.nr @@ -0,0 +1,8 @@ +use dep::std; + +unconstrained fn main(x: u8, y: u8) { + assert(std::wrapping_sub(x, 1) == y); + assert(std::wrapping_add(y, 1) == x); + assert(std::wrapping_mul(y, y) == 1); +} + From 606571937c23df79b8802cb221032214a291432c Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 29 Feb 2024 09:06:09 +0000 Subject: [PATCH 6/6] clippy --- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index aab96d73f9e..f1a8f24ed03 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -349,7 +349,7 @@ impl BrilligContext { pub(crate) fn if_not_instruction( &mut self, condition: MemoryAddress, - mut f: impl FnOnce(&mut BrilligContext), + f: impl FnOnce(&mut BrilligContext), ) { let (end_section, end_label) = self.reserve_next_section_label();