-
Notifications
You must be signed in to change notification settings - Fork 578
feat: fixed size decider without padding #16318
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
feat: fixed size decider without padding #16318
Conversation
✅ Deploy Preview for aztec-docs-temp-you-can-delete ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for barretenberg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| std::span<const Fr> multilinear_challenge, | ||
| const Polynomial& A_0); | ||
| const Polynomial& A_0, | ||
| const bool& has_zk = false); |
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.
See the explanation below
| template <typename Curve> | ||
| std::vector<typename GeminiProver_<Curve>::Polynomial> GeminiProver_<Curve>::compute_fold_polynomials( | ||
| const size_t log_n, std::span<const Fr> multilinear_challenge, const Polynomial& A_0) | ||
| const size_t log_n, std::span<const Fr> multilinear_challenge, const Polynomial& A_0, const bool& has_zk) |
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.
the main Gemini changes are here:
the prover computes the folds for the first log_n rounds as before, and either scales the final constant fold during the virtual rounds or places zero polys to the claims. The reason for the latter is that UltraZKVerifiers would still need the log_circuit_size to evaluate the RowDisablingPolynomial in the very end of sumcheck.
Note MegaZKRecrursive verifier can always run for 20 rounds, cause it's used in a single place.
|
|
||
| // Execute Sumcheck Verifier | ||
| const size_t log_circuit_size = numeric::get_msb(circuit_size); | ||
| // const size_t log_circuit_size = numeric::get_msb(circuit_size); |
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.
Beware that we were in the process of making the AVM circuit size non const: #16332
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.
Thanks @fcarreiro! Now the verifier does not need to know the size of the circuit, just an upper bound, which in your case will be given by MAX_AVM_TRACE_LOG_SIZE. The main change is that in "virtual" rounds the prover is sending valid univariates in Sumchecck and valid commitments/evals in the PCS. The overhead is
num virtual rounds * MAX_RELATION_PARTIAL_LENGTH * num of ops to evaluate the entire relation at a given point.
So the Prover's work stays O(max_poly_size).
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.
IIUC if we used MAX_AVM_TRACE_LOG_SIZE then the proof size would still be the correct unpadded one?
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.
Looked at Lucas' PR - I think the avm prover must still use the actual polynomial size when initializing Sumcheck and doing Gemini, cause it saves work and memory + doesn't affect the VK
…l/aztec-packages into si/genuine-sumcheck-padding
|
|
||
| return Utils::scale_and_batch_elements(relation_evaluations, alphas); | ||
| } | ||
|
|
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.
🎉
| std::shared_ptr<Goblin::Transcript> transcript = std::make_shared<Goblin::Transcript>(); | ||
| // Construct Mega proof \pi_M of the AVM recursive verifier circuit | ||
| auto mega_proving_key = std::make_shared<DeciderProvingKey_<MegaFlavor>>(mega_builder); | ||
| // Detect when MEGA_AVM_LOG_N needs to be bumped. |
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.
Seems useful to keep track of this. Currently it's 21.
ledwards2225
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.
Good stuff. Let's get this in
| const Fr u_last = multilinear_challenge[log_n - 1]; | ||
| const Fr final_eval = last.at(0) + u_last * (last.at(1) - last.at(0)); | ||
| Polynomial const_fold(1); | ||
| // Temporary fix: when we're running a zk proof, the verifier uses a `padding_indicator_array`. So the evals in |
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 you see a path to improving this? Worth an issue?
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.
Yeah, I think I know how to remove it
BEGIN_COMMIT_OVERRIDE chore: Clean up public input propagation in `acir_format` (#16411) feat: merge via append ecc ops in hiding kernel (#16338) chore: install clang20, upgrade wasi-sdk to 27 (#16414) chore: make clang20 default and clean up presets (#16306) chore: stdlib sha256 without packed_byte_array (#15912) fix: Revert "chore: stdlib sha256 without packed_byte_array (#15912)" fix: Correct active range for databus (#16381) fix: Revert "fix: Correct active range for databus (#16381)" feat: fixed size decider without padding (#16318) Revert "chore: make clang20 default and clean up presets (#16306)" Revert "chore: install clang20, upgrade wasi-sdk to 27 (#16414)" chore: document the ECCVM builders. (#15913) END_COMMIT_OVERRIDE
Closes AztecProtocol/barretenberg#1158 Closes AztecProtocol/barretenberg#1310 Closes AztecProtocol/barretenberg#1283 --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com> Co-authored-by: Maddiaa <47148561+Maddiaa0@users.noreply.github.com>
Closes AztecProtocol/barretenberg#1158 Closes AztecProtocol/barretenberg#1310 Closes AztecProtocol/barretenberg#1283 --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com> Co-authored-by: Maddiaa <47148561+Maddiaa0@users.noreply.github.com>
Closes AztecProtocol/barretenberg#1158 Closes AztecProtocol/barretenberg#1310 Closes AztecProtocol/barretenberg#1283 --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com> Co-authored-by: Maddiaa <47148561+Maddiaa0@users.noreply.github.com>
Closes AztecProtocol/barretenberg#1158
Closes AztecProtocol/barretenberg#1310
Closes AztecProtocol/barretenberg#1283