From 3bb2ef2e71b5b62a7795692f1ba8e081db77cc59 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 22 Nov 2024 11:36:44 +0000 Subject: [PATCH 1/7] chore: remove temporary allocations from `num_bits` --- acvm-repo/acir_field/src/field_element.rs | 39 +++++++++++------------ 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index 47ceb903111..173851949a5 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -158,23 +158,6 @@ impl FieldElement { let fr = F::from_str(input).ok()?; Some(FieldElement(fr)) } - - fn bits(&self) -> Vec { - fn byte_to_bit(byte: u8) -> Vec { - let mut bits = Vec::with_capacity(8); - for index in (0..=7).rev() { - bits.push((byte & (1 << index)) >> index == 1); - } - bits - } - - let bytes = self.to_be_bytes(); - let mut bits = Vec::with_capacity(bytes.len() * 8); - for byte in bytes { - bits.extend(byte_to_bit(byte)); - } - bits - } } impl AcirField for FieldElement { @@ -224,12 +207,26 @@ impl AcirField for FieldElement { /// This is the number of bits required to represent this specific field element fn num_bits(&self) -> u32 { - let bits = self.bits(); - // Iterate the number of bits and pop off all leading zeroes - let iter = bits.iter().skip_while(|x| !(**x)); + let bytes = self.to_be_bytes(); + + // Iterate through the byte decomposition and pop off all leading zeroes + let mut iter = bytes.iter().skip_while(|x| (**x) == 0); + + // The first non-zero byte in the decomposition may have some leading zero-bits. + let Some(head_byte) = iter.next() else { + // If we don't have a non-zero byte then the field element is zero, + // which we consider to require a single bit to represent. + return 1; + }; + let num_bits_for_head_byte = head_byte.ilog2(); + + // Each remaining byte in the byte decomposition requires 8 bits. + // // Note: count will panic if it goes over usize::MAX. // This may not be suitable for devices whose usize < u16 - iter.count() as u32 + let tail_length = iter.count() as u32; + + 8 * tail_length + num_bits_for_head_byte } fn to_u128(self) -> u128 { From 0d8ed0210f08bbb3163ee04425e2cea5f21f222f Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 22 Nov 2024 12:04:53 +0000 Subject: [PATCH 2/7] chore: add proptest for `num_bits` matching `ilog2` --- acvm-repo/acir_field/src/field_element.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index 173851949a5..dfe3382a95a 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -371,6 +371,14 @@ mod tests { use super::{AcirField, FieldElement}; use proptest::prelude::*; + proptest! { + #[test] + fn num_bits_agrees_with_ilog2(num: u128) { + let field = FieldElement::::from(num); + prop_assert_eq!(field.num_bits(), num.ilog2()); + } + } + #[test] fn serialize_fixed_test_vectors() { // Serialized field elements from of 0, -1, -2, -3 From 11643612f5c65660168ed7c88ba1ef2c2c5f9f2e Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 22 Nov 2024 12:06:50 +0000 Subject: [PATCH 3/7] chore: reject zero inputs --- acvm-repo/acir_field/src/field_element.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index dfe3382a95a..cb307cd9294 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -373,7 +373,7 @@ mod tests { proptest! { #[test] - fn num_bits_agrees_with_ilog2(num: u128) { + fn num_bits_agrees_with_ilog2(num in 1u128..) { let field = FieldElement::::from(num); prop_assert_eq!(field.num_bits(), num.ilog2()); } From 59b71c83987970d864183fdd7b02d8be0ece5863 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 22 Nov 2024 12:34:02 +0000 Subject: [PATCH 4/7] off by 1 error --- acvm-repo/acir_field/src/field_element.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index cb307cd9294..a17e6fe63e0 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -213,11 +213,7 @@ impl AcirField for FieldElement { let mut iter = bytes.iter().skip_while(|x| (**x) == 0); // The first non-zero byte in the decomposition may have some leading zero-bits. - let Some(head_byte) = iter.next() else { - // If we don't have a non-zero byte then the field element is zero, - // which we consider to require a single bit to represent. - return 1; - }; + let head_byte = iter.next().copied().unwrap_or(0); let num_bits_for_head_byte = head_byte.ilog2(); // Each remaining byte in the byte decomposition requires 8 bits. @@ -226,7 +222,7 @@ impl AcirField for FieldElement { // This may not be suitable for devices whose usize < u16 let tail_length = iter.count() as u32; - 8 * tail_length + num_bits_for_head_byte + 8 * tail_length + num_bits_for_head_byte + 1 } fn to_u128(self) -> u128 { @@ -375,10 +371,20 @@ mod tests { #[test] fn num_bits_agrees_with_ilog2(num in 1u128..) { let field = FieldElement::::from(num); - prop_assert_eq!(field.num_bits(), num.ilog2()); + prop_assert_eq!(field.num_bits(), num.ilog2() + 1); } } + #[test] + fn test_fits_in_u128() { + let field = FieldElement::::from(u128::MAX); + assert_eq!(field.num_bits(), 128); + assert!(field.fits_in_u128()); + let big_field = field + FieldElement::one(); + assert_eq!(big_field.num_bits(), 129); + assert!(!big_field.fits_in_u128()); + } + #[test] fn serialize_fixed_test_vectors() { // Serialized field elements from of 0, -1, -2, -3 From 98676cc6269ef3e5ae67e5f4fe2a8bb1d5af8ff2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 22 Nov 2024 12:42:33 +0000 Subject: [PATCH 5/7] . --- acvm-repo/acir_field/src/field_element.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index a17e6fe63e0..add388073cf 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -213,7 +213,11 @@ impl AcirField for FieldElement { let mut iter = bytes.iter().skip_while(|x| (**x) == 0); // The first non-zero byte in the decomposition may have some leading zero-bits. - let head_byte = iter.next().copied().unwrap_or(0); + let Some(head_byte) = iter.next() else { + // If we don't have a non-zero byte then the field element is zero, + // which we consider to require a single bit to represent. + return 1; + }; let num_bits_for_head_byte = head_byte.ilog2(); // Each remaining byte in the byte decomposition requires 8 bits. From 569c76e326b5de388e2cb338aa686ebc4e8ff5c2 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:55:52 +0000 Subject: [PATCH 6/7] Update acvm-repo/acir_field/src/field_element.rs --- acvm-repo/acir_field/src/field_element.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index add388073cf..b14659f85b4 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -371,6 +371,12 @@ mod tests { use super::{AcirField, FieldElement}; use proptest::prelude::*; + #[test] + fn requires_one_bit_to_hold_zero() { + let field = FieldElement::::zero(); + assert_eq!(field.num_bits(), 1); + } + proptest! { #[test] fn num_bits_agrees_with_ilog2(num in 1u128..) { From f65808af7885f842f6ca0cccb1c56f186dfe1692 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 25 Nov 2024 14:04:24 +0000 Subject: [PATCH 7/7] . --- acvm-repo/acir_field/src/field_element.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm-repo/acir_field/src/field_element.rs b/acvm-repo/acir_field/src/field_element.rs index b14659f85b4..01b9bf8881d 100644 --- a/acvm-repo/acir_field/src/field_element.rs +++ b/acvm-repo/acir_field/src/field_element.rs @@ -376,7 +376,7 @@ mod tests { let field = FieldElement::::zero(); assert_eq!(field.num_bits(), 1); } - + proptest! { #[test] fn num_bits_agrees_with_ilog2(num in 1u128..) {