rearrange queried_values_by_layer for merkle.

[ilyalesokhin-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: af9ae93	Previous: cd8b37b	Ratio
iffts/simd ifft/22	12738802 ns/iter (± 188241)	6306399 ns/iter (± 210024)	2.02
merkle throughput/simd merkle	32163234 ns/iter (± 407207)	13712527 ns/iter (± 579195)	2.35

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

[ilyalesokhin-starkware]

I passed
test_wide_fib_prove_with_blake

[ilyalesokhin-starkware]

The merkle verifier
takes
queried_values_per_log_size
rather then
queried_values_per_column

I think I can reduce the structure further and just concatenate all the log sizes

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 148 at r1 (raw file):

                                .ok_or(MerkleVerificationError::ColumnValuesTooShort)
                        })
                        .collect::<Result<Vec<_>, _>>()?

The main benefit is coming from removing the reorganization here.

Code quote:

                    layer_queried_values
                        .iter_mut()
                        .map(|(_, ref mut column_queries)| {
                            column_queries
                                .next()
                                .ok_or(MerkleVerificationError::ColumnValuesTooShort)
                        })
                        .collect::<Result<Vec<_>, _>>()?

[ilyalesokhin-starkware]

crates/prover/src/core/fri.rs line 728 at r1 (raw file):

                .flatten()
                .copied()
                .collect();

Is there way to get the decommitment_values in the order that I want?
instead of the reorg below?

Code quote:

            // Prepare values in the structure needed for merkle decommitment.
            let column_decommitment_values: SecureColumnByCoords<CpuBackend> = sparse_evaluation
                .subset_evals
                .iter()
                .flatten()
                .copied()
                .collect();

[andrewmilson]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/fri.rs line 728 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Is there way to get the decommitment_values in the order that I want?
instead of the reorg below?

Yes I think so.

- let mut all_column_decommitment_values = Vec::new();
+ let mut column_decommitment_values = Vec::new();

Then here (Would you consider changing merkle_verifier.verify(...) to take BTreeMap instead of Vec for decommitment column values?)

            // Prepare values in the structure needed for merkle decommitment.
            let column_decommitment_values: SecureColumnByCoords<CpuBackend> = sparse_evaluation
                .subset_evals
                .iter()
                .flatten()
                .copied()
                .collect();

            column_decommitment_values[&column_domain.log_size()].or_default().extend(column_decommitment_values.columns);

[andrewmilson]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/fri.rs line 728 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Yes I think so.

- let mut all_column_decommitment_values = Vec::new();
+ let mut column_decommitment_values = Vec::new();

Then here (Would you consider changing merkle_verifier.verify(...) to take BTreeMap instead of Vec for decommitment column values?)

            // Prepare values in the structure needed for merkle decommitment.
            let column_decommitment_values: SecureColumnByCoords<CpuBackend> = sparse_evaluation
                .subset_evals
                .iter()
                .flatten()
                .copied()
                .collect();

            column_decommitment_values[&column_domain.log_size()].or_default().extend(column_decommitment_values.columns);

my bad:

- let mut all_column_decommitment_values = Vec::new();
+ let mut column_decommitment_values_by_log_size = Vec::new();

[andrewmilson]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

            .into_iter()
            .sorted_by_key(|c| Reverse(c.len()))
            .peekable();

Suggestion:

        let columns = &mut columns
            .into_iter()
            .peekable();

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

            .into_iter()
            .sorted_by_key(|c| Reverse(c.len()))
            .peekable();

Actually, the assertion above is currently not true in general.

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Actually, the assertion above is currently not true in general.

Maybe we can remove the comment on the function "This function will panic if the columns are not sorted in descending order"

---

crates/prover/src/core/vcs/verifier.rs line 19 at r2 (raw file):

impl<H: MerkleHasher> MerkleVerifier<H> {
    pub fn new(root: H::Hash, column_log_sizes: Vec<u32>) -> Self {
        let mut n_columns_per_log_size = BTreeMap::<u32, usize>::new();

nit: types should be implicit

Suggestion:

BTreeMap::new()

---

crates/prover/src/core/vcs/verifier.rs line 23 at r2 (raw file):

            *n_columns_per_log_size.entry(*log_size).or_insert(0) += 1;
        }
        Self {

nit

Suggestion:

        }

        Self {

---

crates/prover/src/core/vcs/prover.rs line 160 at r2 (raw file):

        // Reverse the queried values by layer so that queried_values_by_layer[i]
        // corresponds to the quiries for the layer whose log size is i.

Suggestion:

queries

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/verifier.rs line 149 at r2 (raw file):

                assert_eq!(node_values.len(), n_columns_in_layer);
                if node_values.len() != n_columns_in_layer {

Remove the assert? This branch is unreachable

Code quote:

if node_values.len() != n_columns_in_layer {

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 109 at r2 (raw file):

    query_positions_per_log_size: &BTreeMap<u32, Vec<usize>>,
    mut queried_values_per_layer: TreeVec<Vec<Vec<BaseField>>>,
    mut columns_per_log_size: TreeVec<BTreeMap<u32, usize>>,

Suggestion:

n_columns_per_log_size

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Maybe we can remove the comment on the function "This function will panic if the columns are not sorted in descending order"

lol, didn't see it.

I actually do want to sort the columns in descending order but I'm not sure when to do it.

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 145 at r3 (raw file):

                } else {
                    // Otherwise, read them from the witness.
                    (&mut column_witness).take(n_columns_in_layer).collect_vec()

FYI, this is currently unreachable with n_columns_in_layer != 0 (ignoring tests)

Code quote:

(&mut column_witness).take(n_columns_in_layer).collect_vec()

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 149 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Remove the assert? This branch is unreachable

Take can yield fewer elements than requested.
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.take

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/verifier.rs line 149 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Take can yield fewer elements than requested.
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.take

I mean the assert the line before should be removed

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

lol, didn't see it.

I actually do want to sort the columns in descending order but I'm not sure when to do it.

What are you thinking at this stage?
I thought it's easier to push the responsibility to the caller

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 149 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

I mean the assert the line before should be removed

right, thanks.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.45%. Comparing base (df4a642) to head (af9ae93).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #902      +/-   ##
==========================================
+ Coverage   91.42%   91.45%   +0.02%     
==========================================
  Files          94       94              
  Lines       13581    13544      -37     
  Branches    13581    13544      -37     
==========================================
- Hits        12417    12387      -30     
+ Misses       1049     1042       -7     
  Partials      115      115              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/prover.rs line 160 at r2 (raw file):

        // Reverse the queried values by layer so that queried_values_by_layer[i]
        // corresponds to the quiries for the layer whose log size is i.

Removed.

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/prover.rs line 52 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

What are you thinking at this stage?
I thought it's easier to push the responsibility to the caller

I think we can skip the sorting in the Verifier if we know that the columns are sorted.

I'm not sure we will benefit from moving the sorting elsewhere.

[ilyalesokhin-starkware]

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 109 at r2 (raw file):

    query_positions_per_log_size: &BTreeMap<u32, Vec<usize>>,
    mut queried_values_per_layer: TreeVec<Vec<Vec<BaseField>>>,
    mut columns_per_log_size: TreeVec<BTreeMap<u32, usize>>,

Done.

[andrewmilson]

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 108 at r4 (raw file):

    random_coeff: SecureField,
    query_positions_per_log_size: &BTreeMap<u32, Vec<usize>>,
    queried_values: TreeVec<Vec<BaseField>>,

Previously fri_answers didn't have a notion of trees and just have a big list of columns. Perhaps to be consistent column_log_sizes should be removed and samples should be a TreeVec too? WDYT

Code quote:

TreeVec<

---

crates/prover/src/core/pcs/quotients.rs line 138 at r4 (raw file):

    random_coeff: SecureField,
    query_positions: &[usize],
    queried_values: &mut TreeVec<std::vec::IntoIter<BaseField>>,

Alternatively

Suggestion:

    queried_values: &mut TreeVec<impl Iterator<Item = BaseField>>,

[andrewmilson]

Reviewable status: 0 of 9 files reviewed, 11 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/vcs/verifier.rs line 57 at r4 (raw file):

    /// # Returns
    ///
    /// Returns `Ok(())` if the decommitment is successfully verified.

Can make MerkleProver::decommit link to the function.
Also should "Returns Ok(()) if the decommitment is successfully verified." come before # Arguments?

Suggestion:

    /// * `queried_values` - A vector of queried values according to the order in
    ///   [`MerkleProver::decommit()`].
    /// * `decommitment` - The decommitment object containing the witness and column values.
    ///
    /// # Errors
    ///
    /// Returns an error if any of the following conditions are met:
    ///
    /// * The witness is too long (not fully consumed).
    /// * The witness is too short (missing values).
    /// * Too many queried values (not fully consumed).
    /// * Not enough queried values (missing values).
    /// * The computed root does not match the expected root.
    ///
    /// # Panics
    ///
    /// This function will panic if the `values` vector is not sorted in descending order based on
    /// the `log_size` of the columns.
    ///
    /// # Returns
    ///
    /// Returns `Ok(())` if the decommitment is successfully verified.
    ///
    /// [`MerkleProver::decommit()`]: crate::core::...::MerkleProver::decommit

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 57 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can make MerkleProver::decommit link to the function.
Also should "Returns Ok(()) if the decommitment is successfully verified." come before # Arguments?

I don't know our documentation convention.
here returns is at the end:
https://github.com/starkware-industries/stwo/blob/fee260c03cb8024e318bda6ff43577f3cba6fc1c/crates/prover/src/core/vcs/prover.rs#L75

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 188 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Nit. The standard library uses lowercase without trailing punctuation for error messages. I tried to use the same in FRI. Perhaps we can apply same style to this type to be consistent?

The lack of . is inconsistent with the other errors.

[ilyalesokhin-starkware]

crates/prover/src/core/pcs/quotients.rs line 138 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Alternatively

Why is it better?

[ilyalesokhin-starkware]

crates/prover/src/core/pcs/quotients.rs line 108 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Previously fri_answers didn't have a notion of trees and just have a big list of columns. Perhaps to be consistent column_log_sizes should be removed and samples should be a TreeVec too? WDYT

Then I'd have to count how many samples have the same log size, right?
feels more complicated.

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/verifier.rs line 81 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

nit

done

[ilyalesokhin-starkware]

Reviewable status: 0 of 9 files reviewed, 10 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 109 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Does this need ownership?

Done.

---

crates/prover/src/core/pcs/quotients.rs line 127 at r4 (raw file):

                n_columns_per_log_size
                    .as_mut()
                    .map(|colums_log_sizes| *colums_log_sizes.get(&log_size).unwrap_or(&0)),

Done.

[shaharsamocha7]

Reviewed 2 of 6 files at r2, 7 of 8 files at r4.
Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @andrewmilson and @ilyalesokhin-starkware)

---

crates/prover/src/core/vcs/prover.rs line 157 at r4 (raw file):

            last_layer_queries = layer_total_queries;
        }
        queried_values_by_layer.reverse();

Don't we need the reverse? or this cancels with the reverse in MerkleHasher::verify?

Code quote:

queried_values_by_layer.reverse();

[andrewmilson]

Reviewable status: 6 of 9 files reviewed, 12 unresolved discussions (waiting on @ilyalesokhin-starkware and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 108 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

Then I'd have to count how many samples have the same log size, right?
feels more complicated.

Don't you have to do it anyway? Samples are not ordered by log size globally. samples is currently just a TreeVec of samples flattened

---

crates/prover/src/core/pcs/quotients.rs line 138 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

Why is it better?

Probably worse for the compiler. I just find it more readable and don't have to deal with each types specific iterator struct. Just optional not bothered as is

---

crates/prover/src/core/vcs/verifier.rs line 57 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't know our documentation convention.
here returns is at the end:
https://github.com/starkware-industries/stwo/blob/fee260c03cb8024e318bda6ff43577f3cba6fc1c/crates/prover/src/core/vcs/prover.rs#L75

I don't like returns at the end because IMO what is returned is more relevant than Panic so that's why I prefer at the top. Do you think the function summary already encapsulates Returns Ok(()) if the decommitment is successfully verified.?

We don't have a convention as far as I know but I always try and make my own documentation consistent with Rust's standard library as much as possible. I'm not sure where # Returns came from.

WDYT?

---

crates/prover/src/core/vcs/verifier.rs line 188 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

The lack of . is inconsistent with the other errors.

Can we add a TODO to refactor then plz?

---

crates/prover/src/core/vcs/verifier.rs line 189 at r5 (raw file):

    TooManyQueriedValues,
    #[error("not enough queried values")]
    NotEnoughQueriedValues,

Suggestion:

TooFewQueriedValues

[ilyalesokhin-starkware]

crates/prover/src/core/vcs/prover.rs line 157 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Don't we need the reverse? or this cancels with the reverse in MerkleHasher::verify?

It cancels out with a reverse that I've removed in the Verifier

[ilyalesokhin-starkware]

crates/prover/src/core/pcs/quotients.rs line 108 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Don't you have to do it anyway? Samples are not ordered by log size globally. samples is currently just a TreeVec of samples flattened

I'm sorting the flattened sample list here, instead of sorting each TreeVec independently and taking the right number of sample from each.

[ilyalesokhin-starkware]

Reviewable status: 4 of 9 files reviewed, 11 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/pcs/quotients.rs line 138 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Probably worse for the compiler. I just find it more readable and don't have to deal with each types specific iterator struct. Just optional not bothered as is

Done.

---

crates/prover/src/core/vcs/verifier.rs line 188 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can we add a TODO to refactor then plz?

done

---

crates/prover/src/core/vcs/verifier.rs line 189 at r5 (raw file):

    TooManyQueriedValues,
    #[error("not enough queried values")]
    NotEnoughQueriedValues,

Done.

[ilyalesokhin-starkware]

Reviewable status: 4 of 9 files reviewed, 11 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/core/vcs/verifier.rs line 57 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

I don't like returns at the end because IMO what is returned is more relevant than Panic so that's why I prefer at the top. Do you think the function summary already encapsulates Returns Ok(()) if the decommitment is successfully verified.?

We don't have a convention as far as I know but I always try and make my own documentation consistent with Rust's standard library as much as possible. I'm not sure where # Returns came from.

WDYT?

Done.

[shaharsamocha7]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @andrewmilson)

[andrewmilson]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

[ilyalesokhin-starkware]

Reviewed all commit messages.
Reviewable status: 5 of 11 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

[ilyalesokhin-starkware]

Reviewed 3 of 8 files at r4, 2 of 5 files at r6, 2 of 2 files at r7, 4 of 4 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrewmilson)

[ohad-starkware]

Reviewable status: 8 of 11 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @ilyalesokhin-starkware)