audit comments.

[ilyalesokhin-starkware]

This change is 

[gilbens-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions

---

stwo_cairo_verifier/crates/verifier_core/src/verifier.cairo line 66 at r1 (raw file):

    // TODO(audit): what is log_size why -2?
    // Read composition polynomial commitment, there are 8 columns, 4 columns for left,
    // and 4 columns for right, where composition(z) = left(z) + pi^{log_size-2}(z)* right(z).

This is resolved, right?

Code quote:

    // TODO(audit): what is log_size why -2?
    // Read composition polynomial commitment, there are 8 columns, 4 columns for left,
    // and 4 columns for right, where composition(z) = left(z) + pi^{log_size-2}(z)* right(z).

---

stwo_cairo_verifier/crates/verifier_core/src/channel/blake2s.cairo line 185 at r1 (raw file):

fn update_digest(ref channel: Blake2sChannel, new_digest: Blake2sHash) {
    channel.digest = new_digest;
    // TODO(audit): consider resetting the counter when mixing a commitment.

It reintroducing the logic we removed, @leo-starkware right?

Code quote:

// TODO(audit): consider resetting the counter when mixing a commitment.

[ilyalesokhin-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @leo-starkware)

---

stwo_cairo_verifier/crates/verifier_core/src/verifier.cairo line 66 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

This is resolved, right?

I think we should improve the documentation.

[leo-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @leo-starkware)

---

stwo_cairo_verifier/crates/verifier_core/src/channel/blake2s.cairo line 185 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

It reintroducing the logic we removed, @leo-starkware right?

I'm not sure I understand the audit comment: the functionupdate_digest does reset the counter, and it's called at the end of every mixing method mix_*

[ilyalesokhin-starkware]

stwo_cairo_verifier/crates/verifier_core/src/channel/blake2s.cairo line 185 at r1 (raw file):

Previously, leo-starkware wrote…

I'm not sure I understand the audit comment: the functionupdate_digest does reset the counter, and it's called at the end of every mixing method mix_*

@yuvalsw said that it is safer not to reset this counter.
I don't have a strong opinion on this.

[ilyalesokhin-starkware]

stwo_cairo_verifier/crates/verifier_core/src/pcs/verifier.cairo line 236 at r1 (raw file):

    let mut log_degree_bounds = array![];
    let mut degree_bound = column_indices_by_log_deg_bound.len();
    while let Some(columns_of_log_degree_bounds) = column_indices_by_deg_bound.pop_back() {

done in
#1510

Code quote:

    // TODO(audit): consider unpacking the tree array.
    let trace_lde_log_size = *commitment_scheme_trees[1].tree_height;
    assert!(trace_lde_log_size == *commitment_scheme_trees[2].tree_height);
    trace_lde_log_size
}

/// Returns all column log bounds sorted in descending order.
#[inline]
fn get_column_log_degree_bounds(
    mut column_indices_by_deg_bound: ColumnsIndicesByDegreeBound,
) -> Array<u32> {
    let mut log_degree_bounds = array![];
    let mut degree_bound = column_indices_by_log_deg_bound.len();
    while let Some(columns_of_log_degree_bounds) = column_indices_by_deg_bound.pop_back() {

[ilyalesokhin-starkware]

stwo_cairo_verifier/crates/verifier_core/src/pcs/verifier.cairo line 144 at r1 (raw file):

        let fri_config = config.fri_config;

	/// TODO(audit): Move next to usage.

done in
#1510

Code quote:

/// TODO(audit): Move next to usage.

[ilyalesokhin-starkware]

stwo_cairo_verifier/crates/verifier_core/src/pcs/verifier.cairo line 81 at r1 (raw file):

        self: @CommitmentSchemeVerifier,
    ) -> ColumnsIndicesPerTreeByLogDegreeBound {
        let mut columns_by_log_deg_bound_per_tree = array![];

done in
#1510

Code quote:

        let mut columns_by_log_deg_bound_per_tree = array![];

[ilyalesokhin-starkware]

Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware, @leo-starkware, and @yuvalsw)

---

stwo_cairo_verifier/crates/verifier_core/src/pcs/verifier.cairo line 150 at r1 (raw file):

	// TODO(Audit): Consider doing this based on the intraction trace.

Fixed the verifier to fail if the bounds computed here are wrong.
in #1504

Code quote:

// TODO(Audit): Consider doing this based on the intraction trace.

[yuvalsw]

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

---

stwo_cairo_verifier/crates/verifier_core/src/channel/blake2s.cairo line 185 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

@yuvalsw said that it is safer not to reset this counter.
I don't have a strong opinion on this.

I don't understand what you gain from resetting - both a redundant operation which, if anything, seems to make things less safe.
The audit comment should say "consider not resetting" :)

---

stwo_cairo_verifier/crates/verifier_core/src/pcs/verifier.cairo line 150 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Fixed the verifier to fail if the bounds computed here are wrong.
in #1504

• Does it ensure a component can't have an empty trace?
• I still miss "preprocessed" in this comment. And it may need to be more elaborated anyway, to convince the next reader not log size is skipped here.
• Isn't it still safer to do this here based on the interaction trace?

---

stwo_cairo_verifier/crates/verifier_core/src/channel/blake2s.cairo line 145 at r1 (raw file):

        let [d0, d1, d2, d3, d4, d5, d6, d7] = self.digest.hash.unbox();
        // Compute `POW_PREFIX || zeros  || digest || n_bits`.
        //          1 u32      || 3 u32s || 8 u32  || 1 u32.

need to complete fixing the doc here. Also - why this way of padding?