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

fix: fix broken tests in runtime_bignum_test.nr #39

Merged
merged 21 commits into from
Nov 7, 2024
Merged
Changes from 15 commits
Commits
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
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
target
target
.vscode/launch.json
21 changes: 16 additions & 5 deletions src/fns/constrained_ops.nr
Original file line number Diff line number Diff line change
@@ -5,7 +5,8 @@ use crate::fns::{
unconstrained_helpers::{
__add_with_flags, __neg_with_flags, __sub_with_flags, __validate_gt_remainder,
__validate_in_field_compute_borrow_flags,
}, unconstrained_ops::{__div, __mul, __udiv_mod},
},
unconstrained_ops::{__div, __mul, __udiv_mod},
};

/**
@@ -64,7 +65,8 @@ pub(crate) fn derive_from_seed<let N: u32, let MOD_BITS: u32, let SeedBytes: u32
let compressed =
std::hash::poseidon2::Poseidon2::hash(rolling_hash_fields, (SeedBytes / 31) + 1);
let mut rolling_hash: [Field; 2] = [compressed, 0];
let num_hashes = (30 * N) / 32 + (((30 * N) % 32) != 0) as u32;

let num_hashes = (240 * N) / 254 + (((30 * N) % 32) != 0) as u32;
for i in 0..num_hashes - 1 {
let hash: Field = std::hash::poseidon2::Poseidon2::hash(rolling_hash, 2);
let hash: [u8; 32] = hash.to_le_bytes();
@@ -73,6 +75,7 @@ pub(crate) fn derive_from_seed<let N: u32, let MOD_BITS: u32, let SeedBytes: u32
}
rolling_hash[1] += 1;
}

{
let hash: Field = std::hash::poseidon2::Poseidon2::hash(rolling_hash, 2);
let hash: [u8; 32] = hash.to_le_bytes();
@@ -235,9 +238,16 @@ pub(crate) fn validate_in_field<let N: u32, let MOD_BITS: u32>(
**/
pub(crate) fn validate_in_range<let N: u32, let MOD_BITS: u32>(limbs: [Field; N]) {
for i in 0..(N - 1) {
limbs[i].assert_max_bit_size::<120>();
// this is a hack, the assert_max_bit_size is not working when limbs[i] is not a constant
// limbs[i].assert_max_bit_size::<120>();
assert(limbs[i].lt(2.pow_32(120)), "call to assert_max_bit_size 2");
}
limbs[N - 1].assert_max_bit_size::<MOD_BITS - ((N - 1) * 120)>();
// this is a hack, the assert_max_bit_size is not working when limbs[i] is not a constant
// limbs[N - 1].assert_max_bit_size::<MOD_BITS - ((N - 1) * 120)>();
assert(
limbs[N - 1].lt(2.pow_32((MOD_BITS as Field) - ((N - 1) * 120) as Field)),
"call to assert_max_bit_size",
);
Copy link
Member

Choose a reason for hiding this comment

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

The tests are all passing with the commented out range checks. Is this still an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the asserts that I've added are doing the range check if I'm not making a mistake.
The issue was that as limb[N-1] is not a constant, the range check was happening for the bit size of the native field rather than the actual input.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I agree that the added asserts are going a less efficient range check.

The issue was that as limb[N-1] is not a constant, the range check was happening for the bit size of the native field rather than the actual input.

Why would a non-constant field result in us not performing a check whether limb[N-1] fits within MOD_BITS - (N-1)*120 bits?

Copy link
Member

Choose a reason for hiding this comment

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

Did you verify the output in the ACIR? If we're producing incorrect output here we shouldn't just hide that by rewriting the Noir.

Copy link
Member

Choose a reason for hiding this comment

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

To be 100% clear, I'd like to restore the original calls to assert_max_bit_size so I'm keen to know why they were replaced and if it's due to a correctness issue in the compiler then we need to fix that.

}

/**
@@ -267,7 +277,6 @@ pub(crate) fn validate_gt<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [
// a - b = r
// p + a - b - r = 0
let (result, carry_flags, borrow_flags) = unsafe { __validate_gt_remainder(lhs, rhs) };

validate_in_range::<_, MOD_BITS>(result);

let borrow_shift = 0x1000000000000000000000000000000;
@@ -278,12 +287,14 @@ pub(crate) fn validate_gt<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [
+ (borrow_flags[0] as Field * borrow_shift)
- (carry_flags[0] as Field * carry_shift);
assert(result_limb == 0);

for i in 1..N - 1 {
let result_limb = lhs[i] - rhs[i] + addend[i] - result[i] - borrow_flags[i - 1] as Field
+ carry_flags[i - 1] as Field
+ ((borrow_flags[i] as Field - carry_flags[i] as Field) * borrow_shift);
assert(result_limb == 0);
}

let result_limb = lhs[N - 1] - rhs[N - 1] + addend[N - 1]
- result[N - 1]
- borrow_flags[N - 2] as Field
14 changes: 12 additions & 2 deletions src/fns/unconstrained_helpers.nr
Original file line number Diff line number Diff line change
@@ -68,7 +68,6 @@ pub(crate) unconstrained fn __validate_gt_remainder<let N: u32>(
borrow_flags[i / 2] = borrow as bool;
}
}

let result = U60Repr::into(result_u60);
(result, carry_flags, borrow_flags)
}
@@ -208,12 +207,14 @@ pub(crate) unconstrained fn __barrett_reduction<let N: u32>(
modulus: [Field; N],
modulus_u60: U60Repr<N, 4>,
) -> ([Field; N], [Field; N]) {
// for each i in 0..(N + N), adds x[i] * redc_param[j] to mulout[i + j] for each j in 0..N
let mut mulout: [Field; 3 * N] = [0; 3 * N];
for i in 0..(N + N) {
for j in 0..N {
mulout[i + j] += x[i] * redc_param[j];
}
}

mulout = split_bits::__normalize_limbs(mulout, 3 * N - 1);
let mulout_u60: U60Repr<N, 6> = U60Repr::new(mulout);

@@ -259,13 +260,22 @@ pub(crate) unconstrained fn __barrett_reduction<let N: u32>(
}
let quotient_mul_modulus_u60: U60Repr<N, 4> = U60Repr::new(quotient_mul_modulus_normalized);

// convert the input into U60Repr
let x_u60: U60Repr<N, 4> = U60Repr::new(x);
let mut remainder_u60 = x_u60 - quotient_mul_modulus_u60;

// barrett reduction is quircky so might need to remove a few modulus_u60 from the remainder
if (remainder_u60.gte(modulus_u60)) {
remainder_u60 = remainder_u60 - modulus_u60;
quotient_u60.increment();
} else {}
if (remainder_u60.gte(modulus_u60)) {
remainder_u60 = remainder_u60 - modulus_u60;
quotient_u60.increment();
}
if (remainder_u60.gte(modulus_u60)) {
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
remainder_u60 = remainder_u60 - modulus_u60;
quotient_u60.increment();
}

let q: [Field; N] = U60Repr::into(quotient_u60);
let r: [Field; N] = U60Repr::into(remainder_u60);
94 changes: 17 additions & 77 deletions src/fns/unconstrained_ops.nr
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::utils::split_bits;
use crate::utils::u60_representation::U60Repr;

use crate::fns::constrained_ops::derive_from_seed;
use crate::fns::unconstrained_helpers::{
__barrett_reduction, __multiplicative_generator, __primitive_root_log_size,
__tonelli_shanks_sqrt_inner_loop_check,
@@ -33,86 +33,25 @@ pub(crate) unconstrained fn __one<let N: u32>() -> [Field; N] {
limbs
}


/// Deterministically derives a big_num from a seed value.
///
/// Takes a seed byte array and generates a big_num in the range [0, modulus-1].
///
/// ## Value Parameters
///
/// - `params`: The BigNum parameters containing modulus and reduction info
/// - `seed`: Input seed bytes to derive from.
///
/// ## Returns
///
/// An array of field elements derived from the seed (the limbs of the big_num)
pub(crate) unconstrained fn __derive_from_seed<let N: u32, let MOD_BITS: u32, let SeedBytes: u32>(
params: P<N, MOD_BITS>,
seed: [u8; SeedBytes],
) -> [Field; N] {
let mut rolling_hash_fields: [Field; (SeedBytes / 31) + 1] = [0; (SeedBytes / 31) + 1];
let mut seed_ptr = 0;
for i in 0..(SeedBytes / 31) + 1 {
let mut packed: Field = 0;
for _ in 0..31 {
if (seed_ptr < SeedBytes) {
packed *= 256;
packed += seed[seed_ptr] as Field;
seed_ptr += 1;
}
}
rolling_hash_fields[i] = packed;
}
let compressed =
std::hash::poseidon2::Poseidon2::hash(rolling_hash_fields, (SeedBytes / 31) + 1);
let mut rolling_hash: [Field; 2] = [compressed, 0];

let mut to_reduce: [Field; 2 * N] = [0; 2 * N];

let mut double_modulus_bits = MOD_BITS * 2;
let mut double_modulus_bytes =
(double_modulus_bits) / 8 + (double_modulus_bits % 8 != 0) as u32;

let mut last_limb_bytes = double_modulus_bytes % 15;
if (last_limb_bytes == 0) {
last_limb_bytes = 15;
}
let mut last_limb_bits = double_modulus_bits % 8;
if (last_limb_bits == 0) {
last_limb_bits = 8;
}

for i in 0..(N - 1) {
let hash = std::hash::poseidon2::Poseidon2::hash(rolling_hash, 2);
let hash: [u8; 30] = hash.to_le_bytes();
let mut lo: Field = 0;
let mut hi: Field = 0;
for j in 0..15 {
hi *= 256;
lo *= 256;

if (i < 2 * N - 2) {
lo += hash[j + 15] as Field;
hi += hash[j] as Field;
}
}
to_reduce[2 * i] = lo;
to_reduce[2 * i + 1] = hi;
rolling_hash[1] += 1;
}

{
let hash = std::hash::poseidon2::Poseidon2::hash(rolling_hash, 2);
let hash: [u8; 30] = hash.to_le_bytes();
let mut hi: Field = 0;
for j in 0..(last_limb_bytes - 1) {
hi *= 256;
hi += hash[j] as Field;
}
hi *= 256;
let last_byte = hash[last_limb_bytes - 1];
let mask = (1 as u64 << (last_limb_bits) as u8) - 1;
let last_bits = last_byte as u64 & mask;
hi += last_bits as Field;
to_reduce[2 * N - 2] = hi;
}

let (_, remainder) = __barrett_reduction(
to_reduce,
params.redc_param,
MOD_BITS,
params.modulus,
params.modulus_u60_x4,
);
let result = remainder;
result
let out = derive_from_seed::<N, MOD_BITS, SeedBytes>(params, seed);
out
}

pub(crate) unconstrained fn __eq<let N: u32>(lhs: [Field; N], rhs: [Field; N]) -> bool {
@@ -124,6 +63,7 @@ pub(crate) unconstrained fn __is_zero<let N: u32>(limbs: [Field; N]) -> bool {
for i in 0..N {
result = result & (limbs[i] == 0);
}

result
}

1 change: 0 additions & 1 deletion src/tests/bignum_test.nr
Original file line number Diff line number Diff line change
@@ -624,7 +624,6 @@ type U256 = BN256;
fn test_udiv_mod_U256() {
let a: U256 = unsafe { BigNum::__derive_from_seed([1, 2, 3, 4]) };
let b: U256 = BigNum::from_array([12, 0, 0]);

let (q, r) = a.udiv_mod(b);

// let qb = q.__mul(b);
Loading