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.

Sign up for GitHub

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

Jump to bottom

o1vm/pickles: Add the verifier #2694

Merged
merged 30 commits into from
Oct 31, 2024
Merged

o1vm/pickles: Add the verifier #2694

merged 30 commits into from
Oct 31, 2024

Conversation

Fizzixnerd
Copy link
Contributor

No description provided.

@Fizzixnerd Fizzixnerd self-assigned this Oct 9, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.31818% with 20 lines in your changes missing coverage. Please review.

Project coverage is 73.21%. Comparing base (ba2d00c) to head (0d8e46d).
Report is 37 commits behind head on master.

Files with missing lines Patch % Lines
o1vm/src/pickles/main.rs 0.00% 13 Missing ⚠️
o1vm/src/pickles/prover.rs 96.87% 2 Missing ⚠️
o1vm/src/pickles/tests.rs 95.74% 2 Missing ⚠️
o1vm/src/pickles/verifier.rs 99.03% 2 Missing ⚠️
o1vm/src/pickles/proof.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
+ Coverage   72.50%   73.21%   +0.70%     
==========================================
  Files         247      248       +1     
  Lines       57706    58017     +311     
==========================================
+ Hits        41842    42475     +633     
+ Misses      15864    15542     -322

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

dannywillems
dannywillems requested changes Oct 9, 2024
View reviewed changes
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

I would simplify the interface of the verifier, and try to "plug it" in the main.rs to check that the interface maps one usable.

@dannywillems dannywillems marked this pull request as draft October 14, 2024 09:13
@dannywillems
Copy link
Member

Converting into a draft as it doesn't seem to be ready.

@Fizzixnerd
Copy link
Contributor Author

Need some help testing this. It compiles now, but I'm pretty sure I forgot stuff and it's not correct.

Fizzixnerd
Fizzixnerd commented Oct 16, 2024
View reviewed changes
Copy link
Contributor Author

@Fizzixnerd Fizzixnerd left a comment

Choose a reason for hiding this comment

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

Some reminders for what to start on tomorrow.

o1vm/src/pickles/column_env.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/column_env.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/prover.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/verifier.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/verifier.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/verifier.rs Outdated Show resolved Hide resolved
o1vm/src/pickles/verifier.rs Outdated Show resolved Hide resolved
@Fizzixnerd Fizzixnerd force-pushed the fizzixnerd/marc/o1vm/verif branch 2 times, most recently from 83562bf to 5f845ee Compare October 17, 2024 01:57
@Fizzixnerd Fizzixnerd marked this pull request as ready for review October 17, 2024 01:58
marcbeunardeau88
marcbeunardeau88 reviewed Oct 17, 2024
View reviewed changes
o1vm/src/pickles/verifier.rs
EFqSponge: Clone + FqSponge<G::BaseField, G, G::ScalarField>,
EFrSponge: FrSponge<G::ScalarField>,
>(
domain: EvaluationDomains<G::ScalarField>,
Copy link
Contributor

Choose a reason for hiding this comment

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

in a follow up we should replace the domain by omega and the domain size.
I do not like having something of linear size as the input of the verifier

@marcbeunardeau88
Copy link
Contributor

Beside changing the assert for the quotient and adding a test, LGTM

@Fizzixnerd Fizzixnerd force-pushed the fizzixnerd/marc/o1vm/verif branch 3 times, most recently from 6db3291 to c04003a Compare October 18, 2024 13:52
@Fizzixnerd
Copy link
Contributor Author

Ready for final review.

@Fizzixnerd Fizzixnerd force-pushed the fizzixnerd/marc/o1vm/verif branch 3 times, most recently from 2ef606e to 7930244 Compare October 18, 2024 14:12
dannywillems
dannywillems reviewed Oct 24, 2024
View reviewed changes
o1vm/src/pickles/tests.rs
}
/* expr *= Expr::cell(Column::DynamicSelector(0), CurrOrNext::Curr); */
let mut rng = make_test_rng(None);
type BaseSponge = DefaultFqSponge<PallasParameters, PlonkSpongeConstantsKimchi>;
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this in mod.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would disagree. These are only used in the tests, and so belong here.

Copy link
Member

Choose a reason for hiding this comment

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

It is not used only in tests. There are also the same definitions in mod.rs for main.rs.

Fizzixnerd and others added 18 commits October 28, 2024 18:06
@Fizzixnerd @marcbeunardeau88
Revert "msm: make pub(crate) fields pub"
28d2091 
This reverts commit 2839921.
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Make prover and verifier compile
031acd5
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Format
b817d5a
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Fix various FIXMEs from review
5c8184d
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Fix various FIXMEs from review [continued]
e7c5b47
@Fizzixnerd @marcbeunardeau88
Format and remove FIXMEs
c70228b
@marcbeunardeau88
o1vm/pickles/verif: add a fixme
34d3b90
@marcbeunardeau88
o1vm/pickles/prover: improve error message
0ca024e
@marcbeunardeau88
WIP : add test
1c570a2
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Fix typo for vanishing poly
a0f4823
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Format
944c700
@Fizzixnerd @marcbeunardeau88
o1vm/pickles: Fixup for tests
0366845
@marcbeunardeau88
o1vm/pickles/test: simplify zero_to_n_minus_1 func
897a501
@marcbeunardeau88
o1vm/pickles/test: simplify error creation
d61efb3
@marcbeunardeau88
o1vm/pickles: fmt
05260c9
@marcbeunardeau88
o1vm/pickles/test : fix it
1c9014c
@marcbeunardeau88
o1vm/pickles/ add verifier to the main
0e84168
@marcbeunardeau88
o1vm/pickles/prover: use commit with fixed blinder
940d2b2
@Fizzixnerd
Copy link
Contributor Author

Note to future viewers of this commit: Consider my commits verified; marc rebased our changes and that's why they aren't signed anymore.

@Fizzixnerd
Copy link
Contributor Author

@dannywillems Let me know if you want more changes or can approve. Should be good, pending CI now.

dannywillems
dannywillems approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

LGTM: added #2738 on top.

@dannywillems dannywillems merged commit 6ce40b0 into master Oct 31, 2024
8 checks passed
@dannywillems dannywillems deleted the fizzixnerd/marc/o1vm/verif branch October 31, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcbeunardeau88 marcbeunardeau88 marcbeunardeau88 approved these changes

@dannywillems dannywillems dannywillems approved these changes

Assignees

@Fizzixnerd Fizzixnerd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Fizzixnerd @dannywillems @marcbeunardeau88