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

Marc/lookup prover #3008

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Marc/lookup prover #3008

wants to merge 3 commits into from

Conversation

marcbeunardeau88
Copy link
Contributor

adds a prover for a delayed lookup argument
Proves that we can accumulate some lookup, so that the sum goes from a public input acc_init to a public output acc_final.
A lot of unecessarry cloning is happening, which will be handle in a follow-up.
A lot of vec are used instead of array, which will be handle in a foloow-up.

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 282 lines in your changes missing coverage. Please review.

Project coverage is 76.46%. Comparing base (84a357a) to head (cadf895).
Report is 39 commits behind head on master.

Files with missing lines Patch % Lines
o1vm/src/pickles/lookup_prover.rs 0.00% 200 Missing ⚠️
o1vm/src/pickles/lookup_columns.rs 0.00% 81 Missing ⚠️
utils/src/chunked_polynomial.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3008      +/-   ##
==========================================
- Coverage   77.15%   76.46%   -0.69%     
==========================================
  Files         260      263       +3     
  Lines       61529    62170     +641     
==========================================
+ Hits        47470    47540      +70     
- Misses      14059    14630     +571     

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

use o1_utils::ExtendedDensePolynomial;
use poly_commitment::{commitment::absorb_commitment, ipa::SRS, OpenProof, SRS as _};
//TODO Parralelize
//use rayon::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

Delete dead code.

}
// TODO: I could not find a more elegant solution to map over this struct
impl<X> ColumnEnv<X> {
pub fn my_map<Y, F>(self, f: F) -> ColumnEnv<Y>
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this would be simpler as just a normal map implementation, like

ColumnEnv {
  wires: self.wires.into_iter().map(f).collect(),
...
}

and the known iterator sizes will help the vector allocator to allocate the right amount of memory instead of growing to meet it.

Copy link
Member

Choose a reason for hiding this comment

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

Also, map is probably preferable to my_map as a name.

#[derive(Clone)]
pub struct AllColumns<X> {
pub cols: ColumnEnv<X>,
pub t_shares: Vec<X>,
Copy link
Member

Choose a reason for hiding this comment

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

What is a t_shares? This would benefit from a descriptive name

type IntoIter =
Chain<<ColumnEnv<X> as IntoIterator>::IntoIter, <Vec<X> as IntoIterator>::IntoIter>;
fn into_iter(self) -> Self::IntoIter {
self.cols.into_iter().chain(self.t_shares)
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: here and elsewhere, using a destructuring assignment for self will give you a warning if new fields are ever added

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.

2 participants