-
Notifications
You must be signed in to change notification settings - Fork 36
feat: prove/verify and sync plonky3 #523
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
Conversation
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| let dft = Dft::default(); | ||
| let fri_params = p3_fri::create_benchmark_fri_params(challenge_mmcs); | ||
| let mut fri_params = p3_fri::create_benchmark_fri_params(challenge_mmcs); | ||
| fri_params.log_blowup = 4; // Use a higher blowup than default for degree 9 constraints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fix this now or is there something blocking it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fix this now or is there something blocking it?
I don't think anything is blocking it. There are multiple functions in p3_fri to create params (create_test_fri_params, create_test_fri_params_zk, create_benchmark_fri_params..). Maybe we could add a create_miden_fri_params with the values we want if we know them all.
Here it generates:
FriParameters {
log_blowup: 4,
log_final_poly_len: 0,
num_queries: 100,
proof_of_work_bits: 16,
mmcs,
log_folding_factor: 1,
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, and also a configuration with blow up factor 8 for Miden VM constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, and also a configuration with blow up factor 8 for Miden VM constraints.
Did you mean like so? 0xMiden/Plonky3#19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need log_blowup=4? I thought we only went up until 3 for our constraints, but maybe we're doing 4 for smaller proofs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, log_blowup=3 should be enough for us in this context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated #19 to only have a log_blow_up of 3 and one set of parameters for miden, when targetting it here I have some issues but I think just increasing the number of rows for the proof will be enough, I'll test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also will reduce the constraints degrees for a few of the tests.
|
@Leo-Besancon , should we create an issue in the P3 repo regarding missing support for aux boundary constraints? |
I've added the issue there: 0xMiden/Plonky3#17, would be great if you (or @adr1anh) could check it and add comments if needed! |
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Describe your changes
This PR, based on #461, aims to:
prove/verifyin E2E tests for full workflow #512It may be used in a full E2E workflow.