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

Make CommitmentSchemeProver::prove_values take ownership #852

Open
wants to merge 1 commit into
base: 09-15-Add_arithmetic_op_counts_to_InfoEvaluator
Choose a base branch
from

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Sep 24, 2024

Simplifies the implementation of quotient commitment tree in following PR

This change is Reviewable

Copy link
Contributor Author

andrewmilson commented Sep 24, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @andrewmilson and the rest of your teammates on Graphite Graphite

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 15 lines in your changes missing coverage. Please review.

Project coverage is 91.95%. Comparing base (a46c994) to head (64b9479).

Files with missing lines Patch % Lines
crates/prover/src/core/prover/mod.rs 50.00% 11 Missing and 4 partials ⚠️
Additional details and impacted files
@@                                 Coverage Diff                                 @@
##           09-15-Add_arithmetic_op_counts_to_InfoEvaluator     #852      +/-   ##
===================================================================================
+ Coverage                                            91.93%   91.95%   +0.02%     
===================================================================================
  Files                                                   90       90              
  Lines                                                12548    12544       -4     
  Branches                                             12548    12544       -4     
===================================================================================
- Hits                                                 11536    11535       -1     
+ Misses                                                 901      898       -3     
  Partials                                               111      111              
Flag Coverage Δ
91.95% <58.33%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

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

:lgtm: up to stylistic question.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson and @shaharsamocha7)


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

    fn extract_composition_oods_eval(&self) -> Result<SecureField, InvalidOodsSampleStructure> {
        // TODO(andrew): `[.., composition_mask, _quotients_mask]` when add quotients commitment.
        let [_traces_mask @ .., composition_mask] = &**self.sampled_values else {

Why are you binding traces_mask?

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: all files reviewed, 1 unresolved discussion (waiting on @Alon-Ti and @shaharsamocha7)


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

Previously, Alon-Ti wrote…

Why are you binding traces_mask?

I would have done _ @ .. but it errors strangely.

OHHH. Wow didn't realise you can just do .. SMH, tyty

@andrewmilson andrewmilson force-pushed the 09-15-Add_arithmetic_op_counts_to_InfoEvaluator branch from 798f58a to a46c994 Compare September 25, 2024 21:22
@andrewmilson andrewmilson force-pushed the 09-23-Make_CommitmentSchemeProver_prove_values_take_ownership branch from 4de2301 to 64b9479 Compare September 25, 2024 21:22
Copy link
Contributor

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 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 3 of 9 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

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