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
Show file tree
Hide file tree
Changes from 18 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
18 changes: 14 additions & 4 deletions src/fns/constrained_ops.nr
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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();
Expand All @@ -73,6 +74,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();
Expand Down Expand Up @@ -235,9 +237,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.

}

/**
Expand Down Expand Up @@ -267,7 +276,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;
Expand All @@ -278,12 +286,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
Expand Down
14 changes: 12 additions & 2 deletions src/fns/unconstrained_helpers.nr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
97 changes: 18 additions & 79 deletions src/fns/unconstrained_ops.nr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
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,
};
use crate::params::BigNumParams as P;
use crate::utils::split_bits;
use crate::utils::u60_representation::U60Repr;

/**
* In this file:
Expand Down Expand Up @@ -33,86 +33,24 @@ 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 {
Expand All @@ -124,6 +62,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
}

Expand Down
1 change: 0 additions & 1 deletion src/tests/bignum_test.nr
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading
Loading