diff --git a/Cargo.lock b/Cargo.lock index f5a360067ce..f1162d2bcf6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3454,6 +3454,7 @@ name = "noirc_printable_type" version = "1.0.0-beta.2" dependencies = [ "acvm", + "proptest", "serde", ] diff --git a/compiler/noirc_evaluator/proptest-regressions/acir/acir_variable.txt b/compiler/noirc_evaluator/proptest-regressions/acir/acir_variable.txt new file mode 100644 index 00000000000..5b56172668b --- /dev/null +++ b/compiler/noirc_evaluator/proptest-regressions/acir/acir_variable.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 5c73714aa60966f1e26e79441f4a3102fe768cc6c87c9421d44cbc4b9ffa8d66 # shrinks to bit_size = 0 diff --git a/compiler/noirc_evaluator/proptest-regressions/ssa/ir/instruction/binary.txt b/compiler/noirc_evaluator/proptest-regressions/ssa/ir/instruction/binary.txt new file mode 100644 index 00000000000..9da471b5f2c --- /dev/null +++ b/compiler/noirc_evaluator/proptest-regressions/ssa/ir/instruction/binary.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 696c50cf99099048e0e1ea8d77561cc6cf49a71b480a05e0ef437d57f97c454a # shrinks to input = 0, bit_size = 0 diff --git a/compiler/noirc_evaluator/src/acir/acir_variable.rs b/compiler/noirc_evaluator/src/acir/acir_variable.rs index 6e1c2c450de..a4e81de3d5c 100644 --- a/compiler/noirc_evaluator/src/acir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/acir/acir_variable.rs @@ -1026,8 +1026,7 @@ impl> AcirContext { leading: AcirVar, max_bit_size: u32, ) -> Result { - let max_power_of_two = - self.add_constant(F::from(2_u128).pow(&F::from(max_bit_size as u128 - 1))); + let max_power_of_two = self.add_constant(power_of_two::(max_bit_size - 1)); let intermediate = self.sub_var(max_power_of_two, lhs)?; let intermediate = self.mul_var(intermediate, leading)?; @@ -1057,8 +1056,7 @@ impl> AcirContext { assert_ne!(bit_size, 0, "signed integer should have at least one bit"); // 2^{max_bit size-1} - let max_power_of_two = - self.add_constant(F::from(2_u128).pow(&F::from(bit_size as u128 - 1))); + let max_power_of_two = self.add_constant(power_of_two::(bit_size - 1)); let zero = self.add_constant(F::zero()); let one = self.add_constant(F::one()); @@ -1164,7 +1162,7 @@ impl> AcirContext { max_bit_size: u32, ) -> Result { // 2^{rhs} - let divisor = self.add_constant(F::from(2_u128).pow(&F::from(rhs as u128))); + let divisor = self.add_constant(power_of_two::(rhs)); let one = self.add_constant(F::one()); // Computes lhs = 2^{rhs} * q + r @@ -1257,7 +1255,7 @@ impl> AcirContext { // TODO: perhaps this should be a user error, instead of an assert assert!(max_bits + 1 < F::max_num_bits()); - let two_max_bits = self.add_constant(F::from(2_u128).pow(&F::from(max_bits as u128))); + let two_max_bits = self.add_constant(power_of_two::(max_bits)); let diff = self.sub_var(lhs, rhs)?; let comparison_evaluation = self.add_var(diff, two_max_bits)?; @@ -1568,6 +1566,36 @@ impl> AcirContext { } } +/// Returns an `F` representing the value `2**power` +/// +/// # Panics +/// +/// Panics if `2**power` exceeds `F::modulus()`. +pub(super) fn power_of_two(power: u32) -> F { + if power >= F::max_num_bits() { + panic!("Field cannot represent this power of two"); + } + let full_bytes = power / 8; + let extra_bits = power % 8; + let most_significant_byte: u8 = match extra_bits % 8 { + 0 => 0x01, + 1 => 0x02, + 2 => 0x04, + 3 => 0x08, + 4 => 0x10, + 5 => 0x20, + 6 => 0x40, + 7 => 0x80, + _ => unreachable!("We cover the full range of x % 8"), + }; + + let bytes_be: Vec = std::iter::once(most_significant_byte) + .chain(std::iter::repeat(0).take(full_bytes as usize)) + .collect(); + + F::from_be_bytes_reduce(&bytes_be) +} + /// Enum representing the possible values that a /// Variable can be given. #[derive(Debug, Eq, Clone)] @@ -1656,3 +1684,29 @@ fn fits_in_one_identity(expr: &Expression, width: ExpressionWid /// A Reference to an `AcirVarData` #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub(crate) struct AcirVar(usize); + +#[cfg(test)] +mod test { + use acvm::{AcirField, FieldElement}; + use proptest::prelude::*; + + use super::power_of_two; + + #[test] + #[should_panic = "Field cannot represent this power of two"] + fn power_of_two_panics_on_overflow() { + power_of_two::(FieldElement::max_num_bits()); + } + + proptest! { + #[test] + fn power_of_two_agrees_with_generic_impl(bit_size in (0..=128u32)) { + let power_of_two_general = + FieldElement::from(2_u128).pow(&FieldElement::from(bit_size)); + let power_of_two_opt: FieldElement = power_of_two(bit_size); + + prop_assert_eq!(power_of_two_opt, power_of_two_general); + } + + } +} diff --git a/compiler/noirc_evaluator/src/acir/generated_acir.rs b/compiler/noirc_evaluator/src/acir/generated_acir.rs index 7b997eb6d1f..141c9f367c2 100644 --- a/compiler/noirc_evaluator/src/acir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/acir/generated_acir.rs @@ -356,13 +356,13 @@ impl GeneratedAcir { limb_count: u32, bit_size: u32, ) -> Result, RuntimeError> { - let radix_big = BigUint::from(radix); - let radix_range = BigUint::from(2u128)..=BigUint::from(256u128); + let radix_range = 2..=256; assert!( - radix_range.contains(&radix_big), + radix_range.contains(&radix), "ICE: Radix must be in the range 2..=256, but found: {:?}", radix ); + let radix_big = BigUint::from(radix); assert_eq!( BigUint::from(2u128).pow(bit_size), radix_big, diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 46c08242ce6..dc6b3826b60 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -51,7 +51,7 @@ use crate::ssa::{ }, ssa_gen::Ssa, }; -use acir_variable::{AcirContext, AcirType, AcirVar}; +use acir_variable::{power_of_two, AcirContext, AcirType, AcirVar}; use generated_acir::BrilligStdlibFunc; pub(crate) use generated_acir::GeneratedAcir; use noirc_frontend::hir_def::types::Type as HirType; @@ -2122,7 +2122,8 @@ impl<'a> Context<'a> { ) { // Subtractions must first have the integer modulus added before truncation can be // applied. This is done in order to prevent underflow. - let integer_modulus = self.acir_context.add_constant(2_u128.pow(bit_size)); + let integer_modulus = power_of_two::(bit_size); + let integer_modulus = self.acir_context.add_constant(integer_modulus); var = self.acir_context.add_var(var, integer_modulus)?; } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 27c060cb1b5..178e15e4e32 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,3 +1,4 @@ +use binary::truncate_field; use serde::{Deserialize, Serialize}; use std::hash::{Hash, Hasher}; @@ -968,9 +969,8 @@ impl Instruction { return SimplifiedTo(*value); } if let Some((numeric_constant, typ)) = dfg.get_numeric_constant_with_type(*value) { - let integer_modulus = 2_u128.pow(*bit_size); - let truncated = numeric_constant.to_u128() % integer_modulus; - SimplifiedTo(dfg.make_constant(truncated.into(), typ)) + let truncated_field = truncate_field(numeric_constant, *bit_size); + SimplifiedTo(dfg.make_constant(truncated_field, typ)) } else if let Value::Instruction { instruction, .. } = &dfg[dfg.resolve(*value)] { match &dfg[*instruction] { Instruction::Truncate { bit_size: src_bit_size, .. } => { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 217ffd35e02..1550c9ea050 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -441,7 +441,7 @@ pub(crate) fn eval_constant_binary_op( } let result = function(lhs, rhs)?; // Check for overflow - if result >= 1 << bit_size { + if result != 0 && result.ilog2() >= bit_size { return None; } result.into() @@ -489,6 +489,7 @@ fn try_convert_field_element_to_signed_integer(field: FieldElement, bit_size: u3 let signed_int = if is_positive { unsigned_int as i128 } else { + assert!(bit_size < 128); let x = (1u128 << bit_size) - unsigned_int; -(x as i128) }; @@ -501,14 +502,45 @@ fn convert_signed_integer_to_field_element(int: i128, bit_size: u32) -> FieldEle FieldElement::from(int) } else { // We add an offset of `bit_size` bits to shift the negative values into the range [2^(bitsize-1), 2^bitsize) + assert!(bit_size < 128); let offset_int = (1i128 << bit_size) + int; FieldElement::from(offset_int) } } +/// Truncates `int` to fit within `bit_size` bits. fn truncate(int: u128, bit_size: u32) -> u128 { - let max = 1 << bit_size; - int % max + if bit_size == 128 { + int + } else { + let max = 1 << bit_size; + int % max + } +} + +pub(crate) fn truncate_field(int: F, bit_size: u32) -> F { + if bit_size == 0 { + return F::zero(); + } + let num_bytes = bit_size.div_ceil(8); + let mut be_bytes: Vec = + int.to_be_bytes().into_iter().rev().take(num_bytes as usize).rev().collect(); + + // We need to apply a mask to the largest byte to handle non-divisible bit sizes. + let mask = match bit_size % 8 { + 0 => 0xff, + 1 => 0x01, + 2 => 0x03, + 3 => 0x07, + 4 => 0x0f, + 5 => 0x1f, + 6 => 0x3f, + 7 => 0x7f, + _ => unreachable!("We cover the full range of x % 8"), + }; + be_bytes[0] &= mask; + + F::from_be_bytes_reduce(&be_bytes) } impl BinaryOp { @@ -588,8 +620,12 @@ mod test { use proptest::prelude::*; use super::{ - convert_signed_integer_to_field_element, try_convert_field_element_to_signed_integer, + convert_signed_integer_to_field_element, truncate_field, + try_convert_field_element_to_signed_integer, }; + use acvm::{AcirField, FieldElement}; + use num_bigint::BigUint; + use num_traits::One; proptest! { #[test] @@ -601,5 +637,18 @@ mod test { prop_assert_eq!(int, recovered_int); } + + #[test] + fn truncate_field_agrees_with_bigint_modulo(input: u128, bit_size in (0..=253u32)) { + let field = FieldElement::from(input); + let truncated_as_field = truncate_field(field, bit_size); + + let integer_modulus = BigUint::from(2_u128).pow(bit_size); + let truncated_as_bigint = BigUint::from(input) + .modpow(&BigUint::one(), &integer_modulus); + let truncated_as_bigint = FieldElement::from_be_bytes_reduce(&truncated_as_bigint.to_bytes_be()); + prop_assert_eq!(truncated_as_field, truncated_as_bigint); + } + } } diff --git a/compiler/noirc_printable_type/Cargo.toml b/compiler/noirc_printable_type/Cargo.toml index a1eae750b1f..d9ca50c8d51 100644 --- a/compiler/noirc_printable_type/Cargo.toml +++ b/compiler/noirc_printable_type/Cargo.toml @@ -16,3 +16,4 @@ acvm.workspace = true serde.workspace = true [dev-dependencies] +proptest.workspace = true diff --git a/compiler/noirc_printable_type/src/lib.rs b/compiler/noirc_printable_type/src/lib.rs index 1831180d0ab..6ff211cac2a 100644 --- a/compiler/noirc_printable_type/src/lib.rs +++ b/compiler/noirc_printable_type/src/lib.rs @@ -97,7 +97,13 @@ fn to_string(value: &PrintableValue, typ: &PrintableType) -> Op output.push_str(&format_field_string(*f)); } (PrintableValue::Field(f), PrintableType::UnsignedInteger { width }) => { - let uint_cast = f.to_u128() & ((1 << width) - 1); // Retain the lower 'width' bits + // Retain the lower 'width' bits + debug_assert!(*width <= 128, "We don't currently support uints larger than u128"); + let mut uint_cast = f.to_u128(); + if *width != 128 { + uint_cast &= (1 << width) - 1; + }; + output.push_str(&uint_cast.to_string()); } (PrintableValue::Field(f), PrintableType::SignedInteger { width }) => { @@ -267,6 +273,10 @@ fn format_field_string(field: F) -> String { mod tests { use acvm::FieldElement; + use proptest::prelude::*; + + use crate::to_string; + use super::{PrintableType, PrintableValue, PrintableValueDisplay}; #[test] @@ -299,4 +309,16 @@ mod tests { PrintableValueDisplay::::FmtString(template.to_string(), values); assert_eq!(display.to_string(), expected); } + + proptest! { + #[test] + fn handles_decoding_u128_values(uint_value: u128) { + let value = PrintableValue::Field(FieldElement::from(uint_value)); + let typ = PrintableType::UnsignedInteger { width: 128 }; + + let value_as_string = to_string(&value, &typ).unwrap(); + // We want to match rust's stringification. + prop_assert_eq!(value_as_string, uint_value.to_string()); + } + } }