Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit to FRI first layer #897

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Commit to FRI first layer #897

merged 1 commit into from
Nov 25, 2024

Conversation

andrewmilson
Copy link
Contributor

Return opening positions and query positions per column size in FRI

Add FRI layer 0

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

andrewmilson commented Nov 23, 2024

@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch 4 times, most recently from caec233 to e25ce71 Compare November 23, 2024 23:13
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from 16f372f to e5089ad Compare November 23, 2024 23:18
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch 2 times, most recently from 10483ed to 28ecf2c Compare November 23, 2024 23:24
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: 2e1da82 Previous: f6214d1 Ratio
iffts/simd ifft/27 629093148 ns/iter (± 19830439) 312325137 ns/iter (± 5094310) 2.01
merkle throughput/simd merkle 31928540 ns/iter (± 632902) 14690867 ns/iter (± 434150) 2.17

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

CC: @spapinistarkware

@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch 3 times, most recently from 90267f9 to 2e1da82 Compare November 24, 2024 02:00
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from e5089ad to 02fe2b1 Compare November 24, 2024 02:20
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 2e1da82 to 72d0c31 Compare November 24, 2024 02:20
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from 02fe2b1 to 557345d Compare November 24, 2024 03:59
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 72d0c31 to c62e375 Compare November 24, 2024 03:59
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from 557345d to 78be733 Compare November 24, 2024 04:04
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from c62e375 to 8c42d17 Compare November 24, 2024 04:04
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from 78be733 to c46d77b Compare November 24, 2024 04:08
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 8c42d17 to 2955551 Compare November 24, 2024 04:08
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2024

Codecov Report

Attention: Patch coverage is 94.88636% with 27 lines in your changes missing coverage. Please review.

Project coverage is 91.89%. Comparing base (3e5a81d) to head (1be06bd).

Files with missing lines Patch % Lines
crates/prover/src/core/prover/mod.rs 0.00% 14 Missing ⚠️
crates/prover/src/core/fri.rs 97.05% 9 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #897      +/-   ##
==========================================
- Coverage   91.95%   91.89%   -0.06%     
==========================================
  Files          93       93              
  Lines       13308    13251      -57     
  Branches    13308    13251      -57     
==========================================
- Hits        12237    12177      -60     
- Misses        954      961       +7     
+ Partials      117      113       -4     

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


🚨 Try these New Features:

@ilyalesokhin-starkware
Copy link
Collaborator

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

    for &query_position in query_positions {
        let domain_point = commitment_domain.at(bit_reverse_index(query_position, log_size));
        let row = row_index_iter.next().unwrap();

Suggestion:

    for (row, &query_position) in query_positions.iter().enumerate() {
        let domain_point = commitment_domain.at(bit_reverse_index(query_position, log_size));

@ilyalesokhin-starkware
Copy link
Collaborator

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

    /// Stores the evaluations the verifier can't compute but needs in order to perform folding and
    /// verify the merkle decommitment.
    pub missing_evals: Vec<SecureField>,

wdyt?

Suggestion:

    /// values that the verifier needs but cannot deduce from previous computations, in the
    /// order they are needed.
    /// This complements the values that were queried. These must be supplied directly to
    /// the verifier.
    pub fri_witness: Vec<SecureField>,

@ilyalesokhin-starkware
Copy link
Collaborator

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

pub struct FriProver<'a, B: FriOps + MerkleOps<MC::H>, MC: MerkleChannel> {
    config: FriConfig,
    first_layer: FriFirstLayerProver<'a, B, MC::H>,

not sure about the names since it includes all layers.

Code quote:

first_layer: FriFirstLayerProver<'a, B, MC::H>,

@ilyalesokhin-starkware
Copy link
Collaborator

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

            let eval = CircleEvaluation::new(
                domain,
                queried_values.take(domain.size()).copied().collect_vec(),

Here you have 2 eval points per query, right?

How does this work after the change?

Code quote:

 queried_values.take(domain.size()).copied().collect_vec(),

@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from c46d77b to ba15488 Compare November 24, 2024 15:11
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 2955551 to 402c3e3 Compare November 24, 2024 15:11
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, ilyalesokhin-starkware wrote…

wdyt?

LGTM


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

Previously, ilyalesokhin-starkware wrote…

Here you have 2 eval points per query, right?

How does this work after the change?

That was the case previously.
Now only 1 eval per query now with FRI layer 0


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

    for &query_position in query_positions {
        let domain_point = commitment_domain.at(bit_reverse_index(query_position, log_size));
        let row = row_index_iter.next().unwrap();

Done tyty

Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, ilyalesokhin-starkware wrote…

not sure about the names since it includes all layers.

Open to suggestions.

I guess the way I think about it is that the first layer could includes all possible sizes. But all these polynomials are still commited and folded in a single layer (what I currently have called the first layer).

@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from ba15488 to bbe8f96 Compare November 24, 2024 15:22
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 402c3e3 to 5997c7c Compare November 24, 2024 15:22
Base automatically changed from 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership to dev November 24, 2024 15:25
@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch 2 times, most recently from 0b88e46 to 492bb14 Compare November 24, 2024 15:30
@ilyalesokhin-starkware
Copy link
Collaborator

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

Previously, andrewmilson (Andrew Milson) wrote…

Open to suggestions.

I guess the way I think about it is that the first layer could includes all possible sizes. But all these polynomials are still commited and folded in a single layer (what I currently have called the first layer).

Ok, I see, so it's a commitment rather than an evaluation layer.

@ilyalesokhin-starkware
Copy link
Collaborator

crates/prover/src/core/fri.rs line 324 at r3 (raw file):

    // TODO(andrew): Consider making this an `Option` since there may be curcumstances where we
    // don't actually want to commit to the first layer evaluations. This is a simple change. FRI
    // just returns more query positions for the first layer.

I think this might be a per-log-size configuration rather than a global one.

Code quote:

    // TODO(andrew): Consider making this an `Option` since there may be curcumstances where we
    // don't actually want to commit to the first layer evaluations. This is a simple change. FRI
    // just returns more query positions for the first layer.

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 11 files at r1, all commit messages.
Reviewable status: 2 of 11 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from 492bb14 to d01b005 Compare November 24, 2024 17:02
Copy link
Contributor Author

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 11 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @shaharsamocha7)


crates/prover/src/core/fri.rs line 324 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

I think this might be a per-log-size configuration rather than a global one.

Updated the TODO. WDYT?

@andrewmilson andrewmilson force-pushed the Commit_fri_first_layer branch from f8983b3 to 1be06bd Compare November 25, 2024 04:25
Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 6 of 11 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: 8 of 11 files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@andrewmilson andrewmilson merged commit af9250e into dev Nov 25, 2024
18 of 19 checks passed
@andrewmilson andrewmilson deleted the Commit_fri_first_layer branch November 25, 2024 13:30
@ohad-starkware ohad-starkware mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants