Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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
28 changes: 23 additions & 5 deletions src/fns/constrained_ops.nr
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ pub(crate) fn derive_from_seed<let N: u32, let MOD_BITS: u32, let SeedBytes: u32
params: P<N, MOD_BITS>,
seed: [u8; SeedBytes],
) -> [Field; N] {
// assert(params.MOD_BITS == MOD_BITS);
let mut rolling_seed: [u8; SeedBytes + 1] = [0; SeedBytes + 1];
for i in 0..SeedBytes {
rolling_seed[i] = seed[i];
assert_eq(rolling_seed[i], seed[i]);
}

//the seed is getting passed around ok I checked
let mut hash_buffer: [u8; N * 2 * 15] = [0; N * 2 * 15];

let mut rolling_hash_fields: [Field; (SeedBytes / 31) + 1] = [0; (SeedBytes / 31) + 1];
Expand All @@ -61,10 +62,12 @@ pub(crate) fn derive_from_seed<let N: u32, let MOD_BITS: u32, let SeedBytes: u32
rolling_hash_fields[i] = packed;
}

//the rolling hash fields are getting passed around ok I checked
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;
//the rolling hash is getting passed around ok I checked
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 +76,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 +239,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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 @@ -266,8 +277,13 @@ pub(crate) fn validate_gt<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [
// so we do... p - x - r = 0 and there might be borrow flags
// a - b = r
// p + a - b - r = 0
// for the test, with --force-brillig this outputs
// the result is
// [0x00, 0x00, 0x800]
// and without the flag
// [0x00, 0x00, 0x00]
// the issue is happening here. the result is not being set correctly when the --force-brillig flag is used
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 +294,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
15 changes: 13 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,17 +260,27 @@ 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)) {
Comment thread
TomAFrench marked this conversation as 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);

// assert(modulus[N-1].lt(r[N-1]), "barrett reduction failed");
(q, r)
}

Expand Down
89 changes: 12 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,
Expand Down Expand Up @@ -33,86 +33,20 @@ pub(crate) unconstrained fn __one<let N: u32>() -> [Field; N] {
limbs
}

/**
* @brief Deterministically derives a big_num from a seed value
*
* @description Takes a seed byte array and generates a big_num in the range [0, modulus-1].
* @param params The BigNum parameters containing modulus and reduction info
* @param seed Input seed bytes to derive from
* @returns [Field; N] An array of field elements derived from the seed (the limbs of the big_num)
*/
Comment thread
TomAFrench marked this conversation as resolved.
Outdated
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 +58,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