Redshift recursion #16
Conversation
There was a problem hiding this comment.
There are a lot of assert!s here. If panic behavior is acceptable for each of them, then it's ok.
However, it is important to see why each assert! is placed into the code.
static &strs can be helpful to provide panic annotations and not to maintain long messages scattered around.
Such strings can be moved to another module and each assert! may refer to some specific panic circumstances description.
Also, do not forget to run cargo fmt and cargo clippy before each commit.
|
|
||
|
|
||
| pub fn log2_floor(num: usize) -> usize { | ||
| assert!(num > 0); |
There was a problem hiding this comment.
Does the assert indicate a bug? If not, consider using Result. If yes, an error message describing the panic circumstances may be helpful.
| fn rescue_alpha<CS : ConstraintSystem<E>>(elem: &Num<E>, mut cs: CS) -> Result<Num<E>, SynthesisError> { | ||
|
|
||
| #![allow(non_snake_case)] | ||
| let RESCUE_ALPHA : u64 = 5; |
| fn rescue_inalpha<CS : ConstraintSystem<E>>(elem: &Num<E>, mut cs: CS) -> Result<Num<E>, SynthesisError> { | ||
|
|
||
| #![allow(non_snake_case)] | ||
| let RESCUE_INALPHA : [u64; 4] = [14981214993055009997, 6006880321387387405, 10624953561019755799, 2789598613442376532]; |
| // L_k(omega) = 1 = L_0(omega * omega^-k) | ||
| // L_k(z) = L_0(z * omega^-k) = (1/n) * (z^n - 1)/(z * omega^{-k} - 1) | ||
|
|
||
| // TODO: I think the code above has a bug - the scale coefficient should be 1/(n-1) instead of n |
| pow | ||
| } | ||
|
|
||
| // TODO: better replace by tag = ENUM |
| let t_high_com : &AllocatedNum<E> = find_by_label("t_high", &proof.commitments)?; | ||
| channel.consume(t_high_com.clone(), unnamed(cs))?; | ||
|
|
||
| // TODO: we should be very carefule in choosing z! |
| // res.mul_assign(&inverse_vanishing_at_z); | ||
|
|
||
| // res | ||
| // }; |
There was a problem hiding this comment.
Consider moving the code to another function or annotating why is it commented out.
|
|
||
| pub struct FriVerifierGadget<E: Engine, I: OracleGadget<E>> { | ||
| pub collapsing_factor : usize, | ||
| //number of iterations done during FRI query phase |
There was a problem hiding this comment.
Please, change such comments to doc ones:
/// number of iterations done during FRI query phase
|
|
||
| use super::data_structs::*; | ||
|
|
||
| // TODO: FriParams are copyable - stop clonning! |
| // variable: CS::one() | ||
| // } | ||
| // } | ||
|
|
There was a problem hiding this comment.
Please, annotate why is it commented out.
| @@ -610,6 +693,37 @@ impl<E: Engine> AllocatedNum<E> { | |||
| pub fn get_variable(&self) -> Variable { | |||
| self.variable | |||
| } | |||
|
|
|||
| // Montgomery double and add algorithm | |||
| @@ -0,0 +1,21 @@ | |||
| // we prefer to make it modular and generic as we have to use sha256-based channel instead of rescue_channel in future releases | |||
| let mut cur_height = top_leve_height - fri_params.collapsing_factor; | ||
| let mut commitments = Vec::with_capacity(num_of_iters); | ||
|
|
||
| for _ in 0..num_of_iters { |
There was a problem hiding this comment.
Taking into account the comment above, 1..num_of_iters-1 is probably preferred or more clear.
| where CS: ConstraintSystem<E>, I : Iterator<Item = &'a Boolean>, | ||
| { | ||
|
|
||
| assert_eq!(coset.len(), self.wrapping_factor); |
There was a problem hiding this comment.
Please, annotate which bug does the assert indicate.
| } | ||
| } | ||
| // We've already squeezed out all available elements | ||
| unreachable!("Sponge number is too small"); |
There was a problem hiding this comment.
Perhaps, it is worth refactor this to avoid such statements.
| pow += 1; | ||
| } | ||
| pow | ||
| } |
There was a problem hiding this comment.
This same function occurs very frequently throughout the code. It is worth moving such math utilities to another crate or module.
Do not merge!