From 79f895490632a8751cc1ce6e05a862e28810cc3f Mon Sep 17 00:00:00 2001 From: Michael J Klein Date: Tue, 1 Oct 2024 08:14:38 -0400 Subject: [PATCH 1/2] chore: rename `DefinitionKind::GenericType` (#6182) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/6181 ## Summary\* ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- compiler/noirc_frontend/src/elaborator/patterns.rs | 2 +- compiler/noirc_frontend/src/hir/comptime/interpreter.rs | 2 +- compiler/noirc_frontend/src/monomorphization/mod.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index c5e457c405b..c9195fdc267 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -360,7 +360,7 @@ impl<'context> Elaborator<'context> { // Introduce all numeric generics into scope for generic in &all_generics { if let Kind::Numeric(typ) = &generic.kind { - let definition = DefinitionKind::GenericType(generic.type_var.clone()); + let definition = DefinitionKind::NumericGeneric(generic.type_var.clone()); let ident = Ident::new(generic.name.to_string(), generic.span); let hir_ident = self.add_variable_decl( ident, false, // mutable diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 6ed59a61e4e..56b7eb30b3b 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -557,7 +557,7 @@ impl<'context> Elaborator<'context> { self.interner.add_global_reference(global_id, hir_ident.location); } - DefinitionKind::GenericType(_) => { + DefinitionKind::NumericGeneric(_) => { // Initialize numeric generics to a polymorphic integer type in case // they're used in expressions. We must do this here since type_check_variable // does not check definition kinds and otherwise expects parameters to diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index e920073b453..16f154e3ec0 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -582,7 +582,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { Ok(value) } } - DefinitionKind::GenericType(type_variable) => { + DefinitionKind::NumericGeneric(type_variable) => { let value = match &*type_variable.borrow() { TypeBinding::Unbound(_) => None, TypeBinding::Bound(binding) => binding.evaluate_to_u32(), diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 12cc3b55b1f..fea15b26c5b 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -916,7 +916,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Ident(ident) } }, - DefinitionKind::GenericType(type_variable) => { + DefinitionKind::NumericGeneric(type_variable) => { let value = match &*type_variable.borrow() { TypeBinding::Unbound(_) => { unreachable!("Unbound type variable used in expression") diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index a95282a1ec9..8d665ceacbf 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -586,7 +586,7 @@ pub enum DefinitionKind { /// Generic types in functions (T, U in `fn foo(...)` are declared as variables /// in scope in case they resolve to numeric generics later. - GenericType(TypeVariable), + NumericGeneric(TypeVariable), } impl DefinitionKind { @@ -601,7 +601,7 @@ impl DefinitionKind { DefinitionKind::Function(_) => None, DefinitionKind::Global(_) => None, DefinitionKind::Local(id) => *id, - DefinitionKind::GenericType(_) => None, + DefinitionKind::NumericGeneric(_) => None, } } } From b280a79cf8a4fd2a97200e5436e0ec7cb7134711 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:41:43 +0200 Subject: [PATCH 2/2] fix: ensure to_bytes returns the canonical decomposition (#6084) # Description ## Problem\* Resolves #5846 ## Summary\* Enforce canonical decomposition for to_bytes ## Additional Context I also added from_bytes methods since they are often requested ## Documentation\* I updated the comments, I assume the doc is auto generated but I am not sure. Let me know if I need to modify it manually. Check one: - [ ] No documentation needed. - [X] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- noir_stdlib/src/field/mod.nr | 99 +++++++++++++++---- .../execution_success/to_be_bytes/src/main.nr | 1 + .../execution_success/to_le_bytes/src/main.nr | 9 +- 3 files changed, 84 insertions(+), 25 deletions(-) diff --git a/noir_stdlib/src/field/mod.nr b/noir_stdlib/src/field/mod.nr index 93245e18072..d5a6193db3b 100644 --- a/noir_stdlib/src/field/mod.nr +++ b/noir_stdlib/src/field/mod.nr @@ -1,5 +1,6 @@ pub mod bn254; use bn254::lt as bn254_lt; +use crate::runtime::is_unconstrained; impl Field { /// Asserts that `self` can be represented in `bit_size` bits. @@ -49,39 +50,71 @@ impl Field { pub fn to_be_bits(self: Self) -> [u1; N] {} // docs:end:to_be_bits - /// Decomposes `self` into its little endian byte decomposition as a `[u8]` slice of length `byte_size`. - /// This slice will be zero padded should not all bytes be necessary to represent `self`. + /// Decomposes `self` into its little endian byte decomposition as a `[u8;N]` array + /// This array will be zero padded should not all bytes be necessary to represent `self`. /// /// # Failures - /// Causes a constraint failure for `Field` values exceeding `2^{8*byte_size}` as the resulting slice will not - /// be able to represent the original `Field`. + /// The length N of the array must be big enough to contain all the bytes of the 'self', + /// and no more than the number of bytes required to represent the field modulus /// /// # Safety - /// Values of `byte_size` equal to or greater than the number of bytes necessary to represent the `Field` modulus - /// (e.g. 32 for the BN254 field) allow for multiple byte decompositions. This is due to how the `Field` will - /// wrap around due to overflow when verifying the decomposition. + /// The result is ensured to be the canonical decomposition of the field element // docs:start:to_le_bytes pub fn to_le_bytes(self: Self) -> [u8; N] { - self.to_le_radix(256) + // docs:end:to_le_bytes + // Compute the byte decomposition + let bytes = self.to_le_radix(256); + + if !is_unconstrained() { + // Ensure that the byte decomposition does not overflow the modulus + let p = modulus_le_bytes(); + assert(bytes.len() <= p.len()); + let mut ok = bytes.len() != p.len(); + for i in 0..N { + if !ok { + if (bytes[N - 1 - i] != p[N - 1 - i]) { + assert(bytes[N - 1 - i] < p[N - 1 - i]); + ok = true; + } + } + } + assert(ok); + } + bytes } - // docs:end:to_le_bytes - /// Decomposes `self` into its big endian byte decomposition as a `[u8]` slice of length `byte_size`. - /// This slice will be zero padded should not all bytes be necessary to represent `self`. + /// Decomposes `self` into its big endian byte decomposition as a `[u8;N]` array of length required to represent the field modulus + /// This array will be zero padded should not all bytes be necessary to represent `self`. /// /// # Failures - /// Causes a constraint failure for `Field` values exceeding `2^{8*byte_size}` as the resulting slice will not - /// be able to represent the original `Field`. + /// The length N of the array must be big enough to contain all the bytes of the 'self', + /// and no more than the number of bytes required to represent the field modulus /// /// # Safety - /// Values of `byte_size` equal to or greater than the number of bytes necessary to represent the `Field` modulus - /// (e.g. 32 for the BN254 field) allow for multiple byte decompositions. This is due to how the `Field` will - /// wrap around due to overflow when verifying the decomposition. + /// The result is ensured to be the canonical decomposition of the field element // docs:start:to_be_bytes pub fn to_be_bytes(self: Self) -> [u8; N] { - self.to_be_radix(256) + // docs:end:to_be_bytes + // Compute the byte decomposition + let bytes = self.to_be_radix(256); + + if !is_unconstrained() { + // Ensure that the byte decomposition does not overflow the modulus + let p = modulus_be_bytes(); + assert(bytes.len() <= p.len()); + let mut ok = bytes.len() != p.len(); + for i in 0..N { + if !ok { + if (bytes[i] != p[i]) { + assert(bytes[i] < p[i]); + ok = true; + } + } + } + assert(ok); + } + bytes } - // docs:end:to_be_bytes // docs:start:to_le_radix pub fn to_le_radix(self: Self, radix: u32) -> [u8; N] { @@ -130,6 +163,32 @@ impl Field { lt_fallback(self, another) } } + + /// Convert a little endian byte array to a field element. + /// If the provided byte array overflows the field modulus then the Field will silently wrap around. + pub fn from_le_bytes(bytes: [u8; N]) -> Field { + let mut v = 1; + let mut result = 0; + + for i in 0..N { + result += (bytes[i] as Field) * v; + v = v * 256; + } + result + } + + /// Convert a big endian byte array to a field element. + /// If the provided byte array overflows the field modulus then the Field will silently wrap around. + pub fn from_be_bytes(bytes: [u8; N]) -> Field { + let mut v = 1; + let mut result = 0; + + for i in 0..N { + result += (bytes[N-1-i] as Field) * v; + v = v * 256; + } + result + } } #[builtin(modulus_num_bits)] @@ -207,6 +266,7 @@ mod tests { let field = 2; let bits: [u8; 8] = field.to_be_bytes(); assert_eq(bits, [0, 0, 0, 0, 0, 0, 0, 2]); + assert_eq(Field::from_be_bytes::<8>(bits), field); } // docs:end:to_be_bytes_example @@ -216,6 +276,7 @@ mod tests { let field = 2; let bits: [u8; 8] = field.to_le_bytes(); assert_eq(bits, [2, 0, 0, 0, 0, 0, 0, 0]); + assert_eq(Field::from_le_bytes::<8>(bits), field); } // docs:end:to_le_bytes_example @@ -225,6 +286,7 @@ mod tests { let field = 2; let bits: [u8; 8] = field.to_be_radix(256); assert_eq(bits, [0, 0, 0, 0, 0, 0, 0, 2]); + assert_eq(Field::from_be_bytes::<8>(bits), field); } // docs:end:to_be_radix_example @@ -234,6 +296,7 @@ mod tests { let field = 2; let bits: [u8; 8] = field.to_le_radix(256); assert_eq(bits, [2, 0, 0, 0, 0, 0, 0, 0]); + assert_eq(Field::from_le_bytes::<8>(bits), field); } // docs:end:to_le_radix_example } diff --git a/test_programs/execution_success/to_be_bytes/src/main.nr b/test_programs/execution_success/to_be_bytes/src/main.nr index 809d8ad4563..062f9f763d5 100644 --- a/test_programs/execution_success/to_be_bytes/src/main.nr +++ b/test_programs/execution_success/to_be_bytes/src/main.nr @@ -8,5 +8,6 @@ fn main(x: Field) -> pub [u8; 31] { if (bytes[30] != 60) | (bytes[29] != 33) | (bytes[28] != 31) { assert(false); } + assert(Field::from_be_bytes::<31>(bytes) == x); bytes } diff --git a/test_programs/execution_success/to_le_bytes/src/main.nr b/test_programs/execution_success/to_le_bytes/src/main.nr index 4e232b025aa..867551b6dbd 100644 --- a/test_programs/execution_success/to_le_bytes/src/main.nr +++ b/test_programs/execution_success/to_le_bytes/src/main.nr @@ -2,17 +2,12 @@ fn main(x: Field, cond: bool) -> pub [u8; 31] { // The result of this byte array will be little-endian let byte_array: [u8; 31] = x.to_le_bytes(); assert(byte_array.len() == 31); - - let mut bytes = [0; 31]; - for i in 0..31 { - bytes[i] = byte_array[i]; - } - + assert(Field::from_le_bytes::<31>(byte_array) == x); if cond { // We've set x = "2040124" so we shouldn't be able to represent this as a single byte. let bad_byte_array: [u8; 1] = x.to_le_bytes(); assert_eq(bad_byte_array.len(), 1); } - bytes + byte_array }