-
Notifications
You must be signed in to change notification settings - Fork 259
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: IPA documentation #4924
feat: IPA documentation #4924
Conversation
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contracts are deployed in the tx.
Transaction processing duration by data writes.
|
# Add doxygen build command | ||
find_package(Doxygen) | ||
if (DOXYGEN_FOUND) | ||
add_custom_target(build_docs |
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.
This adds a command, so that it's easier to rebuild docs
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.
could you clarify more on how to use 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.
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.
Added information to the readme
/** | ||
* @brief IPA (inner-product argument) commitment scheme class. Conforms to the specification | ||
* https://hackmd.io/q-A8y6aITWyWJrvsGGMWNA?view. | ||
* @brief IPA (inner product argument) commitment scheme class. |
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.
@@ -33,31 +83,67 @@ template <typename Curve> class IPA { | |||
* @param polynomial The witness polynomial whose opening proof needs to be computed | |||
* @param transcript Prover transcript | |||
* https://github.com/AztecProtocol/aztec-packages/pull/3434 | |||
* | |||
*@details For a vector \f$\vec{v}=(v_0,v_1,...,v_{2n-1})\f$ of length \f$2n\f$ we'll denote |
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.
* | ||
* @return true/false depending on if the proof verifies | ||
* | ||
* @details The procedure runs as follows: |
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.
288ba3d
to
c947f64
Compare
// Run scalar product in parallel | ||
run_loop_in_parallel_if_effective_with_index( | ||
// Run scalar products in parallel | ||
run_loop_in_parallel_if_effective( |
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.
We are not accessing a vector any more, so we don't need the thread index
* b_zero = g(evaluation) = ∏_{i ∈ [k]} (1 + u_{k-i}^{-1}. (evaluation)^{2^{i-1}}) | ||
*/ | ||
// Step 6. | ||
// Compute b_zero where b_zero can be computed using the polynomial: |
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.
"/**
*
*/" Comments go into docs :(
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.
hm, that's unfortunate
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 for the documentation and the comments! Left some requests for small changes and clarifications.
@@ -1,19 +1,2738 @@ | |||
# Minimal Doxyfile. See https://www.doxygen.nl/manual/config.html | |||
# Doxyfile 1.9.6 |
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.
where was this copied from?
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.
From Cody's branch :). I made some adjustments to parallelize the build somewhat, but it only took off 5 seconds
# Add doxygen build command | ||
find_package(Doxygen) | ||
if (DOXYGEN_FOUND) | ||
add_custom_target(build_docs |
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.
could you clarify more on how to use it?
*1. Originally we have two vectors \f$\vec{a}\f$ and \f$\vec{b}\f$, which the product of which we want to prove, but | ||
*the prover can't just send vector \f$\vec{a}\f$ to the verifier, it can only provide a commitment | ||
\f$\langle\vec{a},\vec{G}\rangle\f$ | ||
*2. The verifier computes the \f$C'=C+f(\beta)\cdot U\f$ to "bind" together the |
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.
C should be defined
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.
and U too
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.
you're also mixing the notation using f vs the notation using a and b which is confusing
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.
*/ | ||
static void compute_opening_proof(const std::shared_ptr<CK>& ck, | ||
const OpeningPair<Curve>& opening_pair, | ||
const Polynomial& polynomial, | ||
const std::shared_ptr<NativeTranscript>& transcript) | ||
{ | ||
ASSERT(opening_pair.challenge != 0 && "The challenge point should not be zero"); | ||
auto poly_degree = static_cast<size_t>(polynomial.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.
how about poly_length instead of poly_degree_plus_1? just a suggestion.
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.
Left the same in the manifest, but changed the name of the variable
&inner_prod_L, | ||
&inner_prod_R | ||
#ifndef NO_MULTITHREADING | ||
, |
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.
can't this comma be put on the previous line?
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.
No, build starts failing
partial_inner_prod_R[workload_index] = current_inner_prod_R; | ||
// Update the accumulated results thread-safely | ||
{ | ||
#ifndef NO_MULTITHREADING |
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.
what's the performance impact from switching to this?
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.
* b_zero = g(evaluation) = ∏_{i ∈ [k]} (1 + u_{k-i}^{-1}. (evaluation)^{2^{i-1}}) | ||
*/ | ||
// Step 6. | ||
// Compute b_zero where b_zero can be computed using the polynomial: |
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.
hm, that's unfortunate
// L_i = < a_vec_lo, G_vec_hi > + inner_prod_L * aux_generator | ||
L_elements[i] = bb::scalar_multiplication::pippenger_without_endomorphism_basis_points<Curve>( |
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.
why did we store these before?
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.
No idea
std::mutex inner_product_accumulation_mutex; | ||
#endif | ||
// Step 6. | ||
// Perform IPA reduction rounds | ||
for (size_t i = 0; i < log_poly_degree; i++) { |
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.
could flip this backwards to align with documentation
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.
Forgot to do that. One of the reasons I got rid of the vectors
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.
Actually, the index in the manifest is correct. I'm just iterating like this for simplicity, so I'd prefer not to change 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.
Looks good! Left minor remark regarding step numbering.
std::string index = std::to_string(i); | ||
transcript->send_to_verifier("IPA:L_" + index, Commitment(L_elements[i])); | ||
transcript->send_to_verifier("IPA:R_" + index, Commitment(R_elements[i])); | ||
// Step 6.3 |
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.
can these change to 6a, b, c, d, e like the documentation?
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.
Yes
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.26.6</summary> ## [0.26.6](aztec-package-v0.26.5...aztec-package-v0.26.6) (2024-03-08) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.26.6</summary> ## [0.26.6](barretenberg.js-v0.26.5...barretenberg.js-v0.26.6) (2024-03-08) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.26.6</summary> ## [0.26.6](aztec-cli-v0.26.5...aztec-cli-v0.26.6) (2024-03-08) ### Features * Show bytecode size per function in CLI inspect-contract ([#5059](#5059)) ([cb9fdc6](cb9fdc6)) </details> <details><summary>aztec-packages: 0.26.6</summary> ## [0.26.6](aztec-packages-v0.26.5...aztec-packages-v0.26.6) (2024-03-08) ### Features * Basic public reverts ([#4870](#4870)) ([5cccc78](5cccc78)) * Deploying new inbox ([#5036](#5036)) ([fed729d](fed729d)) * Detect unknown note type ids in compute_note_hash ([#5086](#5086)) ([6206bec](6206bec)) * Easy deployment of protocol contracts in e2e ([#4983](#4983)) ([480161f](480161f)) * IPA documentation ([#4924](#4924)) ([48bd22e](48bd22e)) * Nullifier read requests in public kernel ([#4910](#4910)) ([0e44247](0e44247)) * Show bytecode size per function in CLI inspect-contract ([#5059](#5059)) ([cb9fdc6](cb9fdc6)) * Updating an SMT solver class ([#4981](#4981)) ([4b94d58](4b94d58)) ### Bug Fixes * Canonical contract address ([#5030](#5030)) ([b2af880](b2af880)) * Flaky deployment test ([#5035](#5035)) ([039eafc](039eafc)) * Pull the correct platform image for noir ([#5097](#5097)) ([3342371](3342371)) * Sleep function memory leak ([#5023](#5023)) ([a72cfea](a72cfea)), closes [#4817](#4817) * Storage v2 ([#5027](#5027)) ([fe3190e](fe3190e)) * Update protogalaxy cmake dependencies ([#5066](#5066)) ([507c374](507c374)) ### Miscellaneous * Address warnings in noir test suite ([#4966](#4966)) ([7ef4ef5](7ef4ef5)) * Bootstrap noir natively if nargo is invalid ([#5034](#5034)) ([df089de](df089de)) * Build avm transpiler if we are on mac ([#5039](#5039)) ([c2966b9](c2966b9)) * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](#5065)) ([58e1ff4](58e1ff4)) * Cleanup of prover and verifier instances ([#4959](#4959)) ([f2fdefd](f2fdefd)) * Delete bootstrap scripts from `noir/noir-repo` ([#5044](#5044)) ([add91ca](add91ca)) * Disable `hello_world_example` noir test in aztec-packages CI ([#5061](#5061)) ([1be9243](1be9243)) * Join-split example Part 1 ([#4965](#4965)) ([b9de0f5](b9de0f5)) * Moving RootRollupInputs impl ([#5087](#5087)) ([f3d9f9b](f3d9f9b)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](#5084)) ([77954ab](77954ab)) ### Documentation * Parity circuit naming fixes ([#5076](#5076)) ([c255255](c255255)) </details> <details><summary>barretenberg: 0.26.6</summary> ## [0.26.6](barretenberg-v0.26.5...barretenberg-v0.26.6) (2024-03-08) ### Features * IPA documentation ([#4924](#4924)) ([48bd22e](48bd22e)) * Updating an SMT solver class ([#4981](#4981)) ([4b94d58](4b94d58)) ### Bug Fixes * Storage v2 ([#5027](#5027)) ([fe3190e](fe3190e)) * Update protogalaxy cmake dependencies ([#5066](#5066)) ([507c374](507c374)) ### Miscellaneous * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](#5065)) ([58e1ff4](58e1ff4)) * Cleanup of prover and verifier instances ([#4959](#4959)) ([f2fdefd](f2fdefd)) * Join-split example Part 1 ([#4965](#4965)) ([b9de0f5](b9de0f5)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](#5084)) ([77954ab](77954ab)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-package-v0.26.5...aztec-package-v0.26.6) (2024-03-08) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@barretenberg.js-v0.26.5...barretenberg.js-v0.26.6) (2024-03-08) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-cli-v0.26.5...aztec-cli-v0.26.6) (2024-03-08) ### Features * Show bytecode size per function in CLI inspect-contract ([#5059](AztecProtocol/aztec-packages#5059)) ([cb9fdc6](AztecProtocol/aztec-packages@cb9fdc6)) </details> <details><summary>aztec-packages: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-packages-v0.26.5...aztec-packages-v0.26.6) (2024-03-08) ### Features * Basic public reverts ([#4870](AztecProtocol/aztec-packages#4870)) ([5cccc78](AztecProtocol/aztec-packages@5cccc78)) * Deploying new inbox ([#5036](AztecProtocol/aztec-packages#5036)) ([fed729d](AztecProtocol/aztec-packages@fed729d)) * Detect unknown note type ids in compute_note_hash ([#5086](AztecProtocol/aztec-packages#5086)) ([6206bec](AztecProtocol/aztec-packages@6206bec)) * Easy deployment of protocol contracts in e2e ([#4983](AztecProtocol/aztec-packages#4983)) ([480161f](AztecProtocol/aztec-packages@480161f)) * IPA documentation ([#4924](AztecProtocol/aztec-packages#4924)) ([48bd22e](AztecProtocol/aztec-packages@48bd22e)) * Nullifier read requests in public kernel ([#4910](AztecProtocol/aztec-packages#4910)) ([0e44247](AztecProtocol/aztec-packages@0e44247)) * Show bytecode size per function in CLI inspect-contract ([#5059](AztecProtocol/aztec-packages#5059)) ([cb9fdc6](AztecProtocol/aztec-packages@cb9fdc6)) * Updating an SMT solver class ([#4981](AztecProtocol/aztec-packages#4981)) ([4b94d58](AztecProtocol/aztec-packages@4b94d58)) ### Bug Fixes * Canonical contract address ([#5030](AztecProtocol/aztec-packages#5030)) ([b2af880](AztecProtocol/aztec-packages@b2af880)) * Flaky deployment test ([#5035](AztecProtocol/aztec-packages#5035)) ([039eafc](AztecProtocol/aztec-packages@039eafc)) * Pull the correct platform image for noir ([#5097](AztecProtocol/aztec-packages#5097)) ([3342371](AztecProtocol/aztec-packages@3342371)) * Sleep function memory leak ([#5023](AztecProtocol/aztec-packages#5023)) ([a72cfea](AztecProtocol/aztec-packages@a72cfea)), closes [#4817](AztecProtocol/aztec-packages#4817) * Storage v2 ([#5027](AztecProtocol/aztec-packages#5027)) ([fe3190e](AztecProtocol/aztec-packages@fe3190e)) * Update protogalaxy cmake dependencies ([#5066](AztecProtocol/aztec-packages#5066)) ([507c374](AztecProtocol/aztec-packages@507c374)) ### Miscellaneous * Address warnings in noir test suite ([#4966](AztecProtocol/aztec-packages#4966)) ([7ef4ef5](AztecProtocol/aztec-packages@7ef4ef5)) * Bootstrap noir natively if nargo is invalid ([#5034](AztecProtocol/aztec-packages#5034)) ([df089de](AztecProtocol/aztec-packages@df089de)) * Build avm transpiler if we are on mac ([#5039](AztecProtocol/aztec-packages#5039)) ([c2966b9](AztecProtocol/aztec-packages@c2966b9)) * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](AztecProtocol/aztec-packages#5065)) ([58e1ff4](AztecProtocol/aztec-packages@58e1ff4)) * Cleanup of prover and verifier instances ([#4959](AztecProtocol/aztec-packages#4959)) ([f2fdefd](AztecProtocol/aztec-packages@f2fdefd)) * Delete bootstrap scripts from `noir/noir-repo` ([#5044](AztecProtocol/aztec-packages#5044)) ([add91ca](AztecProtocol/aztec-packages@add91ca)) * Disable `hello_world_example` noir test in aztec-packages CI ([#5061](AztecProtocol/aztec-packages#5061)) ([1be9243](AztecProtocol/aztec-packages@1be9243)) * Join-split example Part 1 ([#4965](AztecProtocol/aztec-packages#4965)) ([b9de0f5](AztecProtocol/aztec-packages@b9de0f5)) * Moving RootRollupInputs impl ([#5087](AztecProtocol/aztec-packages#5087)) ([f3d9f9b](AztecProtocol/aztec-packages@f3d9f9b)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](AztecProtocol/aztec-packages#5084)) ([77954ab](AztecProtocol/aztec-packages@77954ab)) ### Documentation * Parity circuit naming fixes ([#5076](AztecProtocol/aztec-packages#5076)) ([c255255](AztecProtocol/aztec-packages@c255255)) </details> <details><summary>barretenberg: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@barretenberg-v0.26.5...barretenberg-v0.26.6) (2024-03-08) ### Features * IPA documentation ([#4924](AztecProtocol/aztec-packages#4924)) ([48bd22e](AztecProtocol/aztec-packages@48bd22e)) * Updating an SMT solver class ([#4981](AztecProtocol/aztec-packages#4981)) ([4b94d58](AztecProtocol/aztec-packages@4b94d58)) ### Bug Fixes * Storage v2 ([#5027](AztecProtocol/aztec-packages#5027)) ([fe3190e](AztecProtocol/aztec-packages@fe3190e)) * Update protogalaxy cmake dependencies ([#5066](AztecProtocol/aztec-packages#5066)) ([507c374](AztecProtocol/aztec-packages@507c374)) ### Miscellaneous * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](AztecProtocol/aztec-packages#5065)) ([58e1ff4](AztecProtocol/aztec-packages@58e1ff4)) * Cleanup of prover and verifier instances ([#4959](AztecProtocol/aztec-packages#4959)) ([f2fdefd](AztecProtocol/aztec-packages@f2fdefd)) * Join-split example Part 1 ([#4965](AztecProtocol/aztec-packages#4965)) ([b9de0f5](AztecProtocol/aztec-packages@b9de0f5)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](AztecProtocol/aztec-packages#5084)) ([77954ab](AztecProtocol/aztec-packages@77954ab)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
The main goal of this PR is to add documentation to the IPA that can be viewed in doxygen. However, it also coincides with a slight refactor of the class:
It also updates the doxygen file and adds a command "build_docs" to cmake for convenience