Skip to content

Commit

Permalink
fix: get_last_bit two errors (#39)
Browse files Browse the repository at this point in the history
2 embarassing errors:
* Witness gen for last bit was wrong (used xor instead of &)
* `ctx.get` was called after `range_check` so it was getting the wrong
  cell
  • Loading branch information
jonathanpwang committed May 23, 2023
1 parent 9298a78 commit 98f8b1d
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 20 deletions.
14 changes: 6 additions & 8 deletions halo2-base/src/gates/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,18 +413,16 @@ pub trait RangeInstructions<F: ScalarField> {
a: AssignedValue<F>,
limb_bits: usize,
) -> AssignedValue<F> {
let a_v = a.value();
let bit_v = {
let a = a_v.get_lower_32();
F::from(a ^ 1 != 0)
};
let a_big = fe_to_biguint(a.value());
let bit_v = F::from(a_big.bit(0));
let two = self.gate().get_field_element(2u64);
let h_v = (*a_v - bit_v) * two.invert().unwrap();
ctx.assign_region([Witness(bit_v), Witness(h_v), Constant(two), Existing(a)], [0]);
let h_v = F::from_bytes_le(&(a_big >> 1usize).to_bytes_le());

ctx.assign_region([Witness(bit_v), Witness(h_v), Constant(two), Existing(a)], [0]);
let half = ctx.get(-3);
self.range_check(ctx, half, limb_bits - 1);
let bit = ctx.get(-4);

self.range_check(ctx, half, limb_bits - 1);
self.gate().assert_bit(ctx, bit);
bit
}
Expand Down
11 changes: 5 additions & 6 deletions halo2-base/src/gates/tests/pos_prop_tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::gates::tests::{flex_gate_tests, range_gate_tests, test_ground_truths::*, Fr};
use crate::utils::bit_length;
use crate::utils::{bit_length, fe_to_biguint};
use crate::{QuantumCell, QuantumCell::Witness};
use proptest::{collection::vec, prelude::*};
//TODO: implement Copy for rand witness and rand fr to allow for array creation
Expand Down Expand Up @@ -285,12 +285,11 @@ proptest! {
prop_assert_eq!(result, ground_truth);
}

// BROKEN
#[test]
fn prop_test_get_last_bit(inputs in (rand_fr().prop_filter("can't be 0", |x| *x != Fr::zero()), 1..=32_usize)) {
let ground_truth = get_last_bit_ground_truth(inputs.0);
let result = range_gate_tests::test_get_last_bit((inputs.0, inputs.1));
println!("result: {result:?}, ground_truth: {ground_truth:?}");
fn prop_test_get_last_bit(input in rand_fr(), pad_bits in 0..10usize) {
let ground_truth = get_last_bit_ground_truth(input);
let bits = fe_to_biguint(&input).bits() as usize + pad_bits;
let result = range_gate_tests::test_get_last_bit((input, bits));
prop_assert_eq!(result, ground_truth);
}

Expand Down
12 changes: 8 additions & 4 deletions halo2-base/src/gates/tests/range_gate_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,17 @@ pub fn test_div_mod<F: ScalarField + BigPrimeField>(
(*a.0.value(), *a.1.value())
}

#[test_case((Fr::from(6), 4) => Fr::one() ; "get_last_bit() pos")]
pub fn test_get_last_bit<F: ScalarField>(inputs: (F, usize)) -> F {
#[test_case((Fr::from(3), 8) => Fr::one() ; "get_last_bit(): 3, 8 bits")]
#[test_case((Fr::from(3), 2) => Fr::one() ; "get_last_bit(): 3, 2 bits")]
#[test_case((Fr::from(0), 2) => Fr::zero() ; "get_last_bit(): 0")]
#[test_case((Fr::from(1), 2) => Fr::one() ; "get_last_bit(): 1")]
#[test_case((Fr::from(2), 2) => Fr::zero() ; "get_last_bit(): 2")]
pub fn test_get_last_bit<F: ScalarField>((a, bits): (F, usize)) -> F {
let mut builder = GateThreadBuilder::mock();
let ctx = builder.main(0);
let chip = RangeChip::default(3);
let a = ctx.assign_witnesses([inputs.0])[0];
let b = chip.get_last_bit(ctx, a, inputs.1);
let a = ctx.load_witness(a);
let b = chip.get_last_bit(ctx, a, bits);
*b.value()
}

Expand Down
3 changes: 1 addition & 2 deletions halo2-base/src/gates/tests/test_ground_truths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ pub fn div_mod_ground_truth<F: ScalarField + BigPrimeField>(inputs: (F, u64)) ->
(biguint_to_fe(&div), biguint_to_fe(&rem))
}

// BROKEN
pub fn get_last_bit_ground_truth<F: ScalarField>(input: F) -> F {
F::from((input.get_lower_128() & ((1 << 127) - 1)) as u64)
F::from(input.get_lower_32() & 1 == 1)
}

0 comments on commit 98f8b1d

Please sign in to comment.