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

PolyComm: use method len directly instead of chunks.len #2659

Merged

Conversation

dannywillems
Copy link
Member

len() on PolyComm is exactly implemented as chunks.len() It is part of a work abstracting the structure PolyComm, and remove the access to the field chunks.

@dannywillems dannywillems requested a review from querolita October 4, 2024 13:03
@dannywillems dannywillems force-pushed the dw/poly-comm-rename-elems-in-chunks branch from 86aeee7 to 7f9f96c Compare October 4, 2024 13:10
`len()` on PolyComm is exactly implemented as `chunks.len()`
It is part of a work abstracting the structure `PolyComm`, and remove the access
to the field `chunks`.
@dannywillems dannywillems force-pushed the dw/poly-comm-use-len-instead-of-chunks-dot-length branch from a4840bc to b2a22a7 Compare October 4, 2024 13:11
@@ -239,11 +239,11 @@ where
let alpha = alpha_chal.to_field(endo_r);

//~ 1. Enforce that the length of the $t$ commitment is of size 7.
if self.commitments.t_comm.chunks.len() > chunk_size * 7 {
if self.commitments.t_comm.len() > chunk_size * 7 {
return Err(VerifyError::IncorrectCommitmentLength(
"t",
chunk_size * 7,
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but this 7 should be a PERMUTS or similar

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is because we do have a degree 8, and therefore 7 chunks for the quotient polynomial. It doesn't seem to be related to the permutations (even though, because we do use degree 8 constraints, we do use 7 permutations).

Base automatically changed from dw/poly-comm-rename-elems-in-chunks to master October 4, 2024 14:25
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.97%. Comparing base (7f9f96c) to head (b2a22a7).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
poly-commitment/src/ipa.rs 50.00% 2 Missing ⚠️
ivc/src/plonkish_lang.rs 0.00% 1 Missing ⚠️
kimchi/src/verifier.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2659      +/-   ##
==========================================
- Coverage   72.98%   72.97%   -0.02%     
==========================================
  Files         244      244              
  Lines       57777    57776       -1     
==========================================
- Hits        42170    42163       -7     
- Misses      15607    15613       +6     

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

@dannywillems dannywillems merged commit 3c35d20 into master Oct 4, 2024
6 of 7 checks passed
@dannywillems dannywillems deleted the dw/poly-comm-use-len-instead-of-chunks-dot-length branch October 4, 2024 14:40
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