feat: implement decommitment phase of fri verifier #14
Conversation
jaehunkim
force-pushed
the
dev-fri-verifier3
branch
from
c5abb5f to
3788f92
Compare
jaehunkim
force-pushed
the
dev-fri-verifier3
branch
from
5d15292 to
2491cc0
Compare
jaehunkim
force-pushed
the
dev-fri-verifier3
branch
from
2491cc0 to
f0faed9
Compare
…ev-fri-verifier1
jaehunkim
changed the title
There was a problem hiding this comment.
I find this code somewhat hard to follow:
There is a lot of functions mutating state on the same large object, lots of values which can be either None/Some which are set/unwrapped inside nested loops, lots of indexing. It's really hard to discern which state is being update by what and which state is relevant to a particular function.
Might even make sense to merge the logic across verify_first_layer(), verify_inner_layers(), verify_last_layer(): currently they are transferring state between these functions via the FriVerifier struct anyway and it does not make sense to invoke one without the other, or even invoke them out-of-order: they are really "one transformation" on the object.
Consider adding some comments to the folding/verifier logic.
It also seems to me that it folds the same layer multiple times.
| let mut cumulative_fri_step = 0; | ||
| for i in 0..layer_num { | ||
| cumulative_fri_step += params.fri_step_list[i]; | ||
| } |
There was a problem hiding this comment.
let cumulative_fri_step = params.fri_step_list.take(layer_num).sum()
fri/src/verifier.rs
Outdated
| if self.query_indices_test.is_some() { | ||
| continue; | ||
| } else { | ||
| assert!( | ||
| self.table_verifiers[i] | ||
| .verify_decommitment(&mut self.channel, &to_verify) | ||
| .unwrap(), | ||
| "Layer {} failed decommitment", | ||
| i | ||
| ); | ||
| } |
There was a problem hiding this comment.
It's really hard for me to follow when something is allowed to be a None/Some in this code.
If you expect things to be Some then consider unwrapping them at the top of the function: so that there is a clear set of preconditions for invoking the function.
fri/src/verifier.rs
Outdated
| let mut to_verify = if let Some(test_data) = self.to_verify_test.as_ref() { | ||
| test_data[i].clone() | ||
| } else { | ||
| self.table_verifiers[i] | ||
| .query( | ||
| &mut self.channel, | ||
| &layer_data_queries, | ||
| &layer_integrity_queries, | ||
| ) | ||
| .unwrap() | ||
| }; |
There was a problem hiding this comment.
What does these two paths correspond to?
| query_indices | ||
| } | ||
|
|
||
| pub fn next_layer_data_and_integrity_queries<F: FftField + PrimeField, E: EvaluationDomain<F>>( |
There was a problem hiding this comment.
Maybe a comment explaining what these sets are.
| let coset_row = get_table_prover_row(idx >> cumulative_fri_step, layer_fri_step); | ||
| for coset_col in 0..(1 << layer_fri_step) { | ||
| let query = RowCol::new(coset_row, coset_col); | ||
| if !integrity_queries.contains(&query) { |
There was a problem hiding this comment.
Can integrity_queries.contains(&query) occur?
fri/src/verifier.rs
Outdated
| } | ||
|
|
||
| if self.query_indices_test.is_some() { | ||
| continue; |
There was a problem hiding this comment.
This "continue" does nothing: there is no logic following the clause.
Implement compute_next_fri_layer and add tests
…v-fri-verifier3
No description provided.