Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9dcda72
Use BigInt instead of FieldElement in noirc_frontend (except the inte…
asterite Jun 26, 2024
a5ed4b3
Use strip_prefix
asterite Jun 26, 2024
f02176a
Correct way to go from BigInt to FieldElement
asterite Jun 26, 2024
b352725
Move BigInt to create HirLiteral::Integer
asterite Jun 26, 2024
bc65406
Correct way to convert a FieldElement into BigInt
asterite Jun 26, 2024
f1609ce
Use BigInt in HirLiteral
asterite Jun 26, 2024
db8a29b
Merge branch 'master' into ab/use-big-int-instead-of-field-element-fo…
asterite Jun 26, 2024
71e0870
Use BigInt in monomorphization Literal
asterite Jun 26, 2024
ac2b583
field_element_from_big_int -> big_int_to_field_element
asterite Jun 26, 2024
8e0ac47
Improve `value_is_within_limits` by checking min bounds, and signedness
asterite Jun 26, 2024
4ec7a2f
Check that values are within Field bounds
asterite Jun 26, 2024
52b9b65
Add some test programs that check the new overflow rules
asterite Jun 26, 2024
4bae625
clippy, and remove some TODOs
asterite Jun 26, 2024
bf4dd5b
Use `compat::is_bn254` in test program
asterite Jun 26, 2024
c2cb68e
Inline function call that only happens once
asterite Jun 26, 2024
73deab4
clippy
asterite Jun 26, 2024
0cd91fa
Merge branch 'master' into ab/use-big-int-instead-of-field-element-fo…
asterite Jun 27, 2024
f1f9bfc
Merge branch 'master' into ab/use-big-int-instead-of-field-element-fo…
asterite Jun 28, 2024
d0e4a7a
Let `big_int_to_field_element` also produce negative field elements
asterite Jun 28, 2024
5114699
Add a test for truncate_big_int_to_u128
asterite Jun 28, 2024
55e2094
Use workspace version of num-traits
asterite Jun 28, 2024
a099db1
clippy
asterite Jun 28, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions aztec_macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ noirc_errors.workspace = true
iter-extended.workspace = true
convert_case = "0.6.0"
regex = "1.10"
num-bigint.workspace = true
num-traits.workspace = true
tiny-keccak = { version = "2.0.0", features = ["keccak"] }
10 changes: 2 additions & 8 deletions aztec_macros/src/transforms/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use noirc_frontend::ast::{
UnresolvedTypeData, Visibility,
};

use noirc_frontend::{macros_api::FieldElement, parse_program};
use noirc_frontend::parse_program;

use crate::utils::ast_utils::member_access;
use crate::{
Expand Down Expand Up @@ -754,13 +754,7 @@ fn create_loop_over(var: Expression, loop_body: Vec<Statement>) -> Statement {

// `for i in 0..{ident}.len()`
make_statement(StatementKind::For(ForLoopStatement {
range: ForRange::Range(
expression(ExpressionKind::Literal(Literal::Integer(
FieldElement::from(i128::from(0)),
false,
))),
end_range_expression,
),
range: ForRange::Range(expression(ExpressionKind::zero()), end_range_expression),
identifier: ident("i"),
block: for_loop_block,
span,
Expand Down
5 changes: 2 additions & 3 deletions aztec_macros/src/transforms/note_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use noirc_frontend::{
Type,
};

use acvm::AcirField;
use regex::Regex;
// TODO(#7165): nuke the following dependency from here and Cargo.toml
use tiny_keccak::{Hasher, Keccak};
Expand Down Expand Up @@ -747,7 +746,7 @@ pub fn inject_note_exports(
let note_type_id = match note_type_id_statement {
HirStatement::Expression(expression_id) => {
match context.def_interner.expression(&expression_id) {
HirExpression::Literal(HirLiteral::Integer(value, _)) => Ok(value),
HirExpression::Literal(HirLiteral::Integer(value)) => Ok(value),
HirExpression::Literal(_) => Err((
AztecMacroError::CouldNotExportStorageLayout {
span: None,
Expand Down Expand Up @@ -782,7 +781,7 @@ pub fn inject_note_exports(
}?;
let global = generate_note_exports_global(
&note.borrow().name.0.contents,
&note_type_id.to_hex(),
&note_type_id.to_str_radix(16),
)
.map_err(|err| (err, file_id))?;

Expand Down
26 changes: 8 additions & 18 deletions aztec_macros/src/transforms/storage.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
use acvm::acir::AcirField;
use noirc_errors::Span;
use noirc_frontend::ast::{
BlockExpression, Expression, ExpressionKind, FunctionDefinition, Ident, Literal, NoirFunction,
BlockExpression, Expression, ExpressionKind, FunctionDefinition, Ident, NoirFunction,
NoirStruct, Pattern, StatementKind, TypeImpl, UnresolvedType, UnresolvedTypeData,
};
use noirc_frontend::{
graph::CrateId,
macros_api::{
FieldElement, FileId, HirContext, HirExpression, HirLiteral, HirStatement, NodeInterner,
},
macros_api::{FileId, HirContext, HirExpression, HirLiteral, HirStatement, NodeInterner},
node_interner::TraitId,
parse_program,
parser::SortedModule,
Expand Down Expand Up @@ -173,7 +170,7 @@
///
/// To:
///
/// impl<Context> Storage<Contex> {

Check warning on line 173 in aztec_macros/src/transforms/storage.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Contex)
/// fn init(context: Context) -> Self {
/// Storage {
/// a_map: Map::new(context, 0, |context, slot| {
Expand Down Expand Up @@ -201,10 +198,7 @@
.find(|r#struct| r#struct.name.0.contents == *storage_struct_name)
.unwrap();

let slot_zero = expression(ExpressionKind::Literal(Literal::Integer(
FieldElement::from(i128::from(0)),
false,
)));
let slot_zero = expression(ExpressionKind::zero());

let field_constructors = definition
.fields
Expand Down Expand Up @@ -419,7 +413,9 @@
context.def_interner.expression(&new_call_expression.arguments[1]);

let current_storage_slot = match slot_arg_expression {
HirExpression::Literal(HirLiteral::Integer(slot, _)) => Ok(slot.to_u128()),
HirExpression::Literal(HirLiteral::Integer(slot)) => {
Ok(noirc_frontend::utils::truncate_big_int_to_u128(&slot))
}
_ => Err((
AztecMacroError::CouldNotAssignStorageSlots {
secondary_message: Some(
Expand Down Expand Up @@ -475,17 +471,11 @@
.map_err(|err| (err, file_id))?;

context.def_interner.update_expression(new_call_expression.arguments[1], |expr| {
*expr = HirExpression::Literal(HirLiteral::Integer(
FieldElement::from(new_storage_slot),
false,
))
*expr = HirExpression::Literal(HirLiteral::Integer(new_storage_slot.into()))
});

context.def_interner.update_expression(storage_layout_slot_expr_id, |expr| {
*expr = HirExpression::Literal(HirLiteral::Integer(
FieldElement::from(new_storage_slot),
false,
))
*expr = HirExpression::Literal(HirLiteral::Integer(new_storage_slot.into()))
});

storage_slot += type_serialized_len;
Expand Down
5 changes: 2 additions & 3 deletions aztec_macros/src/utils/hir_utils.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use acvm::acir::AcirField;
use iter_extended::vecmap;
use noirc_errors::Location;
use noirc_frontend::ast;
Expand Down Expand Up @@ -359,7 +358,7 @@
}
}

pub fn get_global_numberic_const(

Check warning on line 361 in aztec_macros/src/utils/hir_utils.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (numberic)
context: &HirContext,
const_name: &str,
) -> Result<u128, MacroError> {
Expand All @@ -373,8 +372,8 @@
if let Some(let_stmt) = stmt {
let expression = context.def_interner.expression(&let_stmt.expression);
match expression {
HirExpression::Literal(HirLiteral::Integer(value, _)) => {
Some(value.to_u128())
HirExpression::Literal(HirLiteral::Integer(value)) => {
Some(noirc_frontend::utils::truncate_big_int_to_u128(&value))
}
_ => None,
}
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_driver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ rust-embed.workspace = true
tracing.workspace = true

aztec_macros = { path = "../../aztec_macros" }
num-traits.workspace = true
7 changes: 5 additions & 2 deletions compiler/noirc_driver/src/abi_gen.rs
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,
Expand All @@ -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`.
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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),
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ im.workspace = true
serde.workspace = true
tracing.workspace = true
chrono = "0.4.37"
num-traits.workspace = true
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
//! An Error of the former is a user Error
//!
//! An Error of the latter is an error in the implementation of the compiler
use acvm::FieldElement;
use iter_extended::vecmap;
use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic};
use num_bigint::BigInt;
use thiserror::Error;

use crate::ssa::ir::{dfg::CallStack, types::NumericType};
Expand All @@ -24,7 +24,7 @@ pub enum RuntimeError {
#[error("Range constraint of {num_bits} bits is too large for the Field size")]
InvalidRangeConstraint { num_bits: u32, call_stack: CallStack },
#[error("{value} does not fit within the type bounds for {typ}")]
IntegerOutOfBounds { value: FieldElement, typ: NumericType, call_stack: CallStack },
IntegerOutOfBounds { value: BigInt, typ: NumericType, call_stack: CallStack },
#[error("Expected array index to fit into a u64")]
TypeConversion { from: String, into: String, call_stack: CallStack },
#[error("{name:?} is not initialized")]
Expand Down
71 changes: 65 additions & 6 deletions compiler/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 min and max big ints. BigInt can only be compared with other big ints...

Copy link
Member

Choose a reason for hiding this comment

The 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);

Check warning on line 48 in compiler/noirc_evaluator/src/ssa/ir/types.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (biguint)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 BigInt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a simple global for the time being.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the way to declare global variables in Rust?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const MODULUS: BigInt = blah

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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:

cannot call non-const fn `num_bigint::BigInt::from_biguint` in constants
calls in constants are limited to constant functions, tuple structs and tuple variants


if value.is_positive() {
value < &modulus_big_int
} else {
value >= &(-modulus_big_int)
}
Comment on lines +50 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The canonical field representation is the range [0,p) so it seems odd to allow negative values. Will come back to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 [-p, 0] (or [-p + 1, 0]?). But anything outside [-p, p] is a clear error (maybe?)

}
NumericType::NativeField => true,
}
}
}
Expand Down Expand Up @@ -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);

Check warning on line 261 in compiler/noirc_evaluator/src/ssa/ir/types.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (biguint)

// 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)));
}
}
35 changes: 28 additions & 7 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use noirc_errors::Location;
use noirc_frontend::ast::{BinaryOpKind, Signedness};
use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters};
use noirc_frontend::monomorphization::ast::{FuncId, Program};
use num_bigint::BigInt;

use crate::errors::RuntimeError;
use crate::ssa::function_builder::FunctionBuilder;
Expand All @@ -18,6 +19,7 @@ use crate::ssa::ir::instruction::Instruction;
use crate::ssa::ir::map::AtomicCounter;
use crate::ssa::ir::types::{NumericType, Type};
use crate::ssa::ir::value::ValueId;
use num_traits::sign::Signed;

use super::value::{Tree, Value, Values};
use super::SSA_WORD_SIZE;
Expand Down Expand Up @@ -265,21 +267,40 @@ impl<'a> FunctionContext<'a> {
/// otherwise values like 2^128 can be assigned to a u8 without error or wrapping.
pub(super) fn checked_numeric_constant(
&mut self,
value: impl Into<FieldElement>,
value: &BigInt,
typ: Type,
) -> Result<ValueId, RuntimeError> {
let value = value.into();
let negative = value.is_negative();
let field = noirc_frontend::utils::big_int_to_field_element(value);

if let Type::Numeric(typ) = typ {
if !typ.value_is_within_limits(value) {
if let Type::Numeric(numeric_type) = typ {
if !numeric_type.value_is_within_limits(value) {
let call_stack = self.builder.get_call_stack();
return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack });
return Err(RuntimeError::IntegerOutOfBounds {
value: value.clone(),
typ: numeric_type,
call_stack,
});
}

let field = if negative {
match numeric_type {
NumericType::NativeField => field,
NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => {
let base = 1_u128 << bit_size;
// We do `- (-field)` because `big_int_to_field_element` already made the field
// negative, so here we make it positive to later subtract it from the base.
FieldElement::from(base) - (-field)
}
}
} else {
field
};

Ok(self.builder.numeric_constant(field, typ))
} else {
panic!("Expected type for numeric constant to be a numeric type, found {typ}");
}

Ok(self.builder.numeric_constant(value, typ))
}

/// helper function which add instructions to the block computing the absolute value of the
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl<'a> FunctionContext<'a> {
ast::Literal::Integer(value, typ, location) => {
self.builder.set_location(*location);
let typ = Self::convert_non_tuple_type(typ);
self.checked_numeric_constant(*value, typ).map(Into::into)
self.checked_numeric_constant(value, typ).map(Into::into)
}
ast::Literal::Bool(value) => {
// Don't need to call checked_numeric_constant here since `value` can only be true or false
Expand Down
Loading
Loading