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

Add option for algebraic batching to build DEEP polynomial #357

Merged

Conversation

Al-Kindi-0
Copy link
Contributor

We probably need to sync the next branch so that we can change the base branch for this PR.
Once #356 is merged, I can update the security estimator in this PR.

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline.

Also, shouldn't proof security estimates be affected by the type of batching we do? Or is this too difficult to incorporate in the current structure?

air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/air/mod.rs Outdated Show resolved Hide resolved
air/src/air/mod.rs Outdated Show resolved Hide resolved
air/src/air/mod.rs Outdated Show resolved Hide resolved
prover/benches/lagrange_kernel.rs Outdated Show resolved Hide resolved
@irakliyk
Copy link
Collaborator

Also, let's resolve the merge conflict against the current main.

@Al-Kindi-0
Copy link
Contributor Author

Also, shouldn't proof security estimates be affected by the type of batching we do? Or is this too difficult to incorporate in the current structure?

Exactly! The only thing we need is the the widths of each of the committed traces or the total number of committed columns. If this number is say N, then there will be a (potential) loss of log2(N-1).

@Al-Kindi-0 Al-Kindi-0 force-pushed the al-add-option-algebraic-batching-deep-poly branch from ab9cb5b to b995c7f Compare January 29, 2025 22:29
Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple more comments inline.

air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
@irakliyk
Copy link
Collaborator

Exactly! The only thing we need is the the widths of each of the committed traces or the total number of committed columns. If this number is say N, then there will be a (potential) loss of log2(N-1).

Would we do it in this PR? Or should we leave it for a follow-up?

@Al-Kindi-0
Copy link
Contributor Author

Some clippy warnings related to div_ceil are appearing, think we should just supress them as we are not really doing div_ceil

@Al-Kindi-0
Copy link
Contributor Author

Exactly! The only thing we need is the the widths of each of the committed traces or the total number of committed columns. If this number is say N, then there will be a (potential) loss of log2(N-1).

Would we do it in this PR? Or should we leave it for a follow-up?

I propose we do it in a subsequent PR

@irakliyk
Copy link
Collaborator

Some clippy warnings related to div_ceil are appearing, think we should just supress them as we are not really doing div_ceil

I guess these are functionally equivalent to div_ceil but, yeah - if that's not what we are doing, I'd suppress them.

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline (mostly about rename BatchType into BatchMethod). After these are addressed, we can merge.

winterfell/src/lib.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/options.rs Outdated Show resolved Hide resolved
air/src/air/coefficients.rs Show resolved Hide resolved
@Al-Kindi-0 Al-Kindi-0 marked this pull request as ready for review January 31, 2025 08:17
@irakliyk irakliyk merged commit 89b6d60 into facebook:main Jan 31, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants