-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: Use BigInt instead of FieldElement in the compiler to be able to detect overflows/underflows #5338
feat!: Use BigInt instead of FieldElement in the compiler to be able to detect overflows/underflows #5338
Changes from 14 commits
9dcda72
a5ed4b3
f02176a
b352725
bc65406
f1609ce
db8a29b
71e0870
ac2b583
8e0ac47
4ec7a2f
52b9b65
4bae625
bf4dd5b
c2cb68e
73deab4
0cd91fa
f1f9bfc
d0e4a7a
5114699
55e2094
a099db1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
use std::collections::BTreeMap; | ||
|
||
use acvm::acir::circuit::ErrorSelector; | ||
use acvm::AcirField; | ||
use iter_extended::vecmap; | ||
use noirc_abi::{ | ||
Abi, AbiErrorType, AbiParameter, AbiReturnType, AbiType, AbiValue, AbiVisibility, Sign, | ||
|
@@ -14,6 +13,7 @@ use noirc_frontend::{ | |
node_interner::{FuncId, NodeInterner}, | ||
}; | ||
use noirc_frontend::{TypeBinding, TypeVariableKind}; | ||
use num_traits::sign::Signed; | ||
|
||
/// Arranges a function signature and a generated circuit's return witnesses into a | ||
/// `noirc_abi::Abi`. | ||
|
@@ -198,7 +198,10 @@ pub(super) fn value_from_hir_expression(context: &Context, expression: HirExpres | |
}, | ||
HirLiteral::Bool(value) => AbiValue::Boolean { value }, | ||
HirLiteral::Str(value) => AbiValue::String { value }, | ||
HirLiteral::Integer(field, sign) => AbiValue::Integer { value: field.to_hex(), sign }, | ||
HirLiteral::Integer(value) => { | ||
let str = value.abs().to_str_radix(16); | ||
AbiValue::Integer { value: str, sign: value.is_negative() } | ||
Comment on lines
+202
to
+203
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Is this correct? |
||
} | ||
_ => unreachable!("Literal cannot be used in the abi"), | ||
}, | ||
_ => unreachable!("Type cannot be used in the abi {:?}", expression), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,3 +21,4 @@ im.workspace = true | |
serde.workspace = true | ||
tracing.workspace = true | ||
chrono = "0.4.37" | ||
num-traits = "0.2.19" |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,8 +2,10 @@ | |||
|
||||
use acvm::{acir::AcirField, FieldElement}; | ||||
use iter_extended::vecmap; | ||||
use num_bigint::{BigInt, Sign}; | ||||
|
||||
use crate::ssa::ssa_gen::SSA_WORD_SIZE; | ||||
use num_traits::{FromPrimitive, Signed}; | ||||
|
||||
/// A numeric type in the Intermediate representation | ||||
/// Note: we class NativeField as a numeric type | ||||
|
@@ -29,15 +31,28 @@ | |||
} | ||||
} | ||||
|
||||
/// Returns true if the given Field value is within the numeric limits | ||||
/// Returns true if the given BigInt value is within the numeric limits | ||||
/// for the current NumericType. | ||||
pub(crate) fn value_is_within_limits(self, field: FieldElement) -> bool { | ||||
pub(crate) fn value_is_within_limits(self, value: &BigInt) -> bool { | ||||
match self { | ||||
NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => { | ||||
let max = 2u128.pow(bit_size) - 1; | ||||
field <= max.into() | ||||
NumericType::Signed { bit_size } => { | ||||
let min: BigInt = -BigInt::from_i128(1_i128 << (bit_size - 1)).unwrap(); | ||||
let max: BigInt = BigInt::from_i128(1_i128 << (bit_size - 1)).unwrap() - 1; | ||||
min <= *value && *value <= max | ||||
Comment on lines
+39
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 I wonder if there's a way to do this without allocating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One quick solution would be to use a OnceCell so that we only need to allocate a single bigint and reuse that for all other integers of the same bitsize |
||||
} | ||||
NumericType::Unsigned { bit_size } => { | ||||
!value.is_negative() && value.bits() <= bit_size.into() | ||||
} | ||||
NumericType::NativeField => { | ||||
let modulus = FieldElement::modulus(); | ||||
let modulus_big_int = BigInt::from_biguint(Sign::Plus, modulus); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I wonder if there's a way to avoid creating this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the way to declare global variables in Rust? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I tried it: const MODULUS: BigInt = BigInt::from_biguint(Sign::Plus, FieldElement::modulus()); but I get this error:
|
||||
|
||||
if value.is_positive() { | ||||
value < &modulus_big_int | ||||
} else { | ||||
value >= &(-modulus_big_int) | ||||
} | ||||
Comment on lines
+50
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The canonical field representation is the range There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. However, there's some code that uses negative field values, like this one:
So I was thinking we can at least allow negative values if they are in the range |
||||
} | ||||
NumericType::NativeField => true, | ||||
} | ||||
} | ||||
} | ||||
|
@@ -210,3 +225,47 @@ | |||
} | ||||
} | ||||
} | ||||
|
||||
#[cfg(test)] | ||||
mod tests { | ||||
use super::*; | ||||
use num_bigint::Sign; | ||||
use num_traits::FromPrimitive; | ||||
use num_traits::Zero; | ||||
|
||||
#[test] | ||||
fn test_u8_is_within_limits() { | ||||
let u8 = NumericType::Unsigned { bit_size: 8 }; | ||||
assert!(!u8.value_is_within_limits(&BigInt::from_i32(-1).unwrap())); | ||||
assert!(u8.value_is_within_limits(&BigInt::from_i32(0).unwrap())); | ||||
assert!(u8.value_is_within_limits(&BigInt::from_i32(255).unwrap())); | ||||
assert!(!u8.value_is_within_limits(&BigInt::from_i32(256).unwrap())); | ||||
} | ||||
|
||||
#[test] | ||||
fn test_i8_is_within_limits() { | ||||
let i8 = NumericType::Signed { bit_size: 8 }; | ||||
assert!(!i8.value_is_within_limits(&BigInt::from_i32(-129).unwrap())); | ||||
assert!(i8.value_is_within_limits(&BigInt::from_i32(-128).unwrap())); | ||||
assert!(i8.value_is_within_limits(&BigInt::from_i32(0).unwrap())); | ||||
assert!(i8.value_is_within_limits(&BigInt::from_i32(127).unwrap())); | ||||
assert!(!i8.value_is_within_limits(&BigInt::from_i32(128).unwrap())); | ||||
} | ||||
|
||||
#[test] | ||||
fn test_native_field_is_within_limits() { | ||||
let field = NumericType::NativeField; | ||||
assert!(field.value_is_within_limits(&BigInt::zero())); | ||||
|
||||
let modulus = FieldElement::modulus(); | ||||
let modulus_big_int = BigInt::from_biguint(Sign::Plus, modulus); | ||||
|
||||
// For positive values, check that right before modulus it's fine, but at modulus it's not | ||||
assert!(field.value_is_within_limits(&(&modulus_big_int - 1))); | ||||
assert!(!field.value_is_within_limits(&modulus_big_int)); | ||||
|
||||
// For negative values, check that right at the (negative) modulus it's fine, but before that it's not | ||||
assert!(field.value_is_within_limits(&(-&modulus_big_int))); | ||||
assert!(!field.value_is_within_limits(&(-&modulus_big_int - 1))); | ||||
} | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've defined a version to use across the workspace in the root
Cargo.toml
. Can you replace all the other instances with this?