fix lookup keccak rotation to use max 16 limb #1034
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ use multilinear_extensions::{ | |
| Expression, Fixed, Instance, StructuralWitIn, ToExpr, WitIn, WitnessId, rlc_chip_record, | ||
| }; | ||
| use serde::de::DeserializeOwned; | ||
| use std::{collections::HashMap, iter::once, marker::PhantomData}; | ||
|
|
||
| use ff_ext::ExtensionField; | ||
|
|
||
|
|
@@ -782,6 +782,7 @@ impl<'a, E: ExtensionField> CircuitBuilder<'a, E> { | |
| 14 => self.assert_u14(name_fn, expr), | ||
| 8 => self.assert_byte(name_fn, expr), | ||
| 5 => self.assert_u5(name_fn, expr), | ||
| 1 => self.assert_bit(name_fn, expr), | ||
| c => panic!("Unsupported bit range {c}"), | ||
| } | ||
| } | ||
|
|
@@ -1058,8 +1059,29 @@ impl<'a, E: ExtensionField> CircuitBuilder<'a, E> { | |
|
|
||
| // Lookup ranges | ||
| for (i, (size, elem)) in split_rep.iter().enumerate() { | ||
| match *size { | ||
| 32 => (), | ||
| 18 => { | ||
| self.assert_ux::<_, _, 18>(|| format!("{}_{}", name().into(), i), elem.clone())? | ||
| } | ||
| 16 => { | ||
| self.assert_ux::<_, _, 16>(|| format!("{}_{}", name().into(), i), elem.clone())? | ||
| } | ||
| 14 => { | ||
| self.assert_ux::<_, _, 14>(|| format!("{}_{}", name().into(), i), elem.clone())? | ||
| } | ||
| 8 => { | ||
| self.assert_ux::<_, _, 8>(|| format!("{}_{}", name().into(), i), elem.clone())? | ||
| } | ||
| 5 => { | ||
| self.assert_ux::<_, _, 5>(|| format!("{}_{}", name().into(), i), elem.clone())? | ||
| } | ||
| 1 => self.assert_bit(|| format!("{}_{}", name().into(), i), elem.clone())?, | ||
| _ => self.assert_ux_in_u16( | ||
| || format!("{}_{}", name().into(), i), | ||
| *size, | ||
| elem.clone(), | ||
| )?, | ||
|
Comment on lines
+1062
to
+1084
There was a problem hiding this comment. Shall we put this in a helper function? There was a problem hiding this comment. yes will do it in next PR #1036 to fix precompile e2e sound |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -1073,25 +1095,26 @@ impl<'a, E: ExtensionField> CircuitBuilder<'a, E> { | |
| let mut rep_x = rep_x.to_owned(); | ||
| rep_x.rotate_right(chunks_rotation); | ||
|
|
||
| // 64 bits represent in 4 limb, each with 16 bits | ||
| let num_limbs = 4; | ||
| let mut rep_x_iter = rep_x.iter().cloned(); | ||
| for limb_i in 0..num_limbs { | ||
| let lhs = rep8[2 * limb_i..2 * (limb_i + 1)] | ||
| .iter() | ||
| .map(|wit| (8, wit.expr())) | ||
| .collect_vec(); | ||
| let rhs_limbs = take_til_threshold(&mut rep_x_iter, 16, &|limb| limb.0).unwrap(); | ||
|
|
||
| assert_eq!(rhs_limbs.iter().map(|e| e.0).sum::<usize>(), 16); | ||
|
|
||
| self.require_reps_equal::<16, _, _>( | ||
|
There was a problem hiding this comment. key highlights: constrain on 16 bits limb individually instead of 32 bits (limb_16 + (1 << 16( * limb_16) because later are larger than some finite field characteristic, e.g. babybear |
||
| ||format!( | ||
| "rotation internal {}, round {limb_i}, rot: {chunks_rotation}, delta: {delta}, {:?}", | ||
| name().into(), | ||
| sizes | ||
| ), | ||
| &lhs, | ||
| &rhs_limbs, | ||
| )?; | ||
| } | ||
| Ok(()) | ||
|
|
@@ -1113,34 +1136,64 @@ impl<'a, E: ExtensionField> CircuitBuilder<'a, E> { | |
| } | ||
| } | ||
|
|
||
| /// take items from an iterator until the accumulated "weight" (measured by `f`) | ||
| /// reaches exactly `threshold`. | ||
| /// | ||
| /// - `iter`: a mutable iterator to consume items from | ||
| /// - `threshold`: the sum of weights at which to stop and return the group | ||
| /// - `f`: closure that extracts the "weight" (e.g., bit length) from each item | ||
| /// | ||
| /// returns: | ||
| /// - `Some(Vec<T>)` containing the next group of items whose weights sum to `threshold` | ||
| /// - `None` if the iterator is exhausted and no items remain | ||
| /// | ||
| /// panics if the sum of weights ever exceeds `threshold`. | ||
| pub fn take_til_threshold<I, T, F>(iter: &mut I, threshold: usize, f: &F) -> Option<Vec<T>> | ||
| where | ||
| I: Iterator<Item = T>, | ||
| F: Fn(&T) -> usize, | ||
| { | ||
| let mut group = Vec::new(); | ||
| let mut sum = 0; | ||
|
|
||
| for x in iter.by_ref() { | ||
| sum += f(&x); | ||
| group.push(x); | ||
|
|
||
| if sum == threshold { | ||
| return Some(group); | ||
| } else if sum > threshold { | ||
| panic!("sum exceeded threshold!"); | ||
| } | ||
| } | ||
|
|
||
| if group.is_empty() { | ||
| None | ||
| } else { | ||
| Some(group) // leftover if input not perfectly divisible | ||
| } | ||
| } | ||
|
|
||
| /// Compute an adequate split of 64-bits into chunks for performing a rotation | ||
| /// by `delta`. The first element of the return value is the vec of chunk sizes. | ||
| /// The second one is the length of its suffix that needs to be rotated | ||
| pub fn rotation_split(delta: usize) -> (Vec<usize>, usize) { | ||
| if delta == 0 { | ||
| return (vec![16, 16, 16, 16], 0); | ||
| } | ||
|
|
||
| let remainder = delta % 16; | ||
| let split16 = std::iter::repeat_with(|| [16 - remainder, remainder]) | ||
| .flatten() | ||
| .scan(0, |sum, x| { | ||
| if *sum >= 64 { | ||
| None | ||
| } else { | ||
| *sum += x; | ||
| Some(x) | ||
| } | ||
| }) | ||
| .filter(|v| *v > 0) | ||
| .collect_vec(); | ||
|
|
||
| let mut sum = 0; | ||
|
|
@@ -1151,7 +1204,7 @@ pub fn rotation_split(delta: usize) -> (Vec<usize>, usize) { | |
| } | ||
| } | ||
|
|
||
| panic!("delta {:?} split16 {:?}", remainder, split16); | ||
| } | ||
|
|
||
| pub fn expansion_expr<E: ExtensionField, const SIZE: usize>( | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookup_keccakinvolving some deep recursion when build the circuit. I think it's a existing issue, just the new change hit the threshold unluckily. With that, we temporarily increase RUST_MIN_STACK to make tests pass. It doesn't break functional correctness.