feat(monorepo): enable shorter CI team branches #1151
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Hack poly mfst to get multivariates from pk. * d is not a template param * Prover executes sumcheck. * Address some comments from Luke.
* Added verification key computation * Fixed test * Replaced constants * Added a comment that Luke wanted
* update multivariates constructor to handle shifts and add evals to transcript using poly manifest * computing claims in prover; still using mock commitments for non witness polynomials * calling gemini reduce prove in prover; all tests passing; still using raw pointer for commitment key * Updating PCS to use shared pointer instead of raw pointer for commitment key * Codys sumcheck round size fix * fixing multivariates constructor * gemini, shplonk, and kzg all running in prover
…tocol/barretenberg#71) * adding three additional manifest elements related to Gemini; formalizing transcript debug prints
* Lots of work in the now-extinct AztecProtocol/barretenberg#65. * Composer tests shows memory issue * NOT working; TwoGates too trivial. * Zero out univariate accumulator (hack). * Init relation works. * Add failure tests. * Added Sage notebook. *Cleanup.
## Clang-tidy related
There are many changes that silence warnings from `clang-tidy`.
- Initializing class members when possible
- Explicitly deleting copy constructors/assignment operators for non-copyable classes
- Using `= default` for trivial destructors
- Adding `noexcept` to move constructors/assignment
- Using `auto` after `static_cast` to avoid duplication.
- Using `std::move` when copying an argument like a `string` or `vector` to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
- Removing unused headers, and adding `#pragma once` to prevent import clashes.
- Reorder some imports, and use the module-relative paths
- Replace some `typedef` with `using`
- Use `const auto& element : container` to prevent unnecessary copies.
## Circuit Constructor
- Replace `n` by `num_gates` to prevent clashes with `n` we use to refer to the `subgroup_size`
- Added comments about things that seemed fishy, (all marked with `TODO(Adrian)`
- Removed `NUM_RESERVED_GATES` and `WireType` since they are not used/duplicated
- Simplified `set_public_input`, though not sure if the removal of the `ASSERT` is correct.
## Composer
- Refactor `compute_proving_key_base`
- Take the `CircuitConstructor` by `const` reference and modify the behavior so we only modify the `proving_key`. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials.
- Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0.
- Explicitly state where the public inputs are stored.
- Refactor `compute_witness_base`
- Use explicit types when referencing the `circuit_constructor` and ensure we only get objects that we don't modify.
- Create `array` of wires to handle the `program_width` more generally.
- No longer modify the `circuit_constructor` wires to add padding, and instead add 0-padding to the wire polynomials.
- Remove `fr::__copy`
- Ensure all calls to `circuit_constructor` are `const`
- PermutationHelper: Big refactor to clarify handling of public inputs.
- Simplify `cycle_node` behavior, and more generically handle different number of columns, and remove confusing `WireType` enum.
- Make `compute_wire_copy_cycles` return a vector of `CyclicPermutation` for clarity.
- Remove `resize` by noting that the number of cycles is equal to the number of `variables`
- Reverse the order in which we were applying each `CyclicPermutation` to the `sigma` polynomials. Now, each `cycle_node` will map to the next one in the list.
- Add comments to explain what parts of the function are necessary for our public input handling.
- Changed the `composer_test` to also test for public inputs.
- Add many more tests to ensure the permutation polynomials that we create have the expected form.
- Compare results with the `public_input_delta`
## Prover & Verifier
- Comment-out the `work_queue` related members.
- Restore `gamma` challenge that was removed due to a misunderstanding
- Remove default argument `beta = 1` for the `grand_product` computation.
## Sumcheck
- Add `#pragma once` to headers
- Modify the `GrandProductComputationRelation` to work with `Z_perm` that has the first coefficient equal to 0.
- Handle `public_input_delta` in `GrandProductComputationRelation`
- Modify `GrandProductInitializationRelation` to instead check that the last "real" value of `Z_perm_shift` is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later.
- Renamed `LAGRANGE_1` to `LAGRANGE_FIRST` to be consistent with other parts of the code.
…tecProtocol/barretenberg#30) * moving verification key from plonk/proof_system to shared proof_system * moving polynomial_manifest to shared dir while I'm at it
* proving_system is also a bad name for this, OK? * Stdlib field tests pass (partial proof system). * Lazily convert bool tests; IOU typed_test. * Byte array tests; IOU typed versions. * uint tests; don't copy when extending edges. * Convert bit_array tests. * packed_byte_array tests; IOU other composers. * safe_uint; should use typed tests... * bigfield tests * stdlib group; IOU other composers. * Hide some bigfield tests to save time. * biggroup tests (hide some tests to speed testing. * Change selection of biggroup tests. * A bit of cleanup. * More cleanup. * Reinstate missing constraint. * More cleanup.
* reinstating PCS in manifest and prover; all tests pass except composer BaseCase possibly due to zero commitment * successfully calling gemini reduce_verify from verifier; all tests pass aside from composer BaseCase * removing junk elements from manifest and adding expository test that mimicks PCS fucntionality in practice * simplifying prover claim construction and improving some comments * fleshing out expository full PCS test
Fixes various bugs that existed in sumcheck relations and creates a test to test the basic formulas
* debugging; added test in pcs-tests for sumcheck evals * weird backwards u eval point makes sumcheck and mle eval consistent * Commiting to 0 poly is the issue (TwoGates passes, BaseCase fails) * Ensure no poly is ever zero. Co-authored-by: codygunton <codygunton@gmail.com>
Fixes sumcheck relations so the full relation is now correct --------- Co-authored-by: Rumata888 <isennovskiy@gmail.com> Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>
* Pow zeta in arithmetic relation * Pow in init relation. * Prover uses real powers of zeta. * pow is temprarily a witness. * Split out biggroup tests to save time. * Hide more biggroup tests to save time. * Include pow_zeta in grand product. * Respond to Luke's review. --------- Co-authored-by: codygunton <codygunton@gmail.com>
…tocol/barretenberg#98) Co-authored-by: Rumata888 <isennovskiy@gmail.com>
Added Ultraplonk and StandardHonk names to GET_COMPOSER_NAME
…enberg#99) Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>
* basic framework for honk bench * basic honk benchmarks in place * moving timing in plonk bench and deleting unused reset function
…llup/ removal, kesha's bigfield fix) (AztecProtocol/barretenberg#123) * Target skylake but without avx. * Fix indent. * Fixing fuzzer build (AztecProtocol/barretenberg#14) * Add cmake logic to support building of dependent C++/WASM projects (AztecProtocol/barretenberg#54) * cmake updates to support dependent projects (native + wasm) * nothing should depend on constants from `rollup/` * use `cmake --build <dir> --parallel` everywhere instead of -j with nprocs * forward all bootstrap args to cmake with --target prefix for each. Use cleaner subshell instead of cd'ing back up after ignition in bootstrap. * allow dependent project to set WASI_SDK_PREFIX and have it be used in wasm-linux-clang.cmake * update readme to use cmake * Removed all files in rollup/ not necessary for join split tests * dockerfile, script, and ci changes now that rollup contents are gone except join split * remove more unused code from rollup/js dir * removed standard example * rename rollup directory to join_split_example * bigfield fix * filter out longer join split tests (leave only one full proof test in) for CI. fix bb-tests * fix dockerfile now that rollup executables were removed * rename rollup namespace to join_split_example * add blake 3 to executables/libraries in aztec cmakelists * Removing non-join-split constants as per recommendation here https://github.com/AztecProtocol/barretenberg/pull/124\#discussion_r1098698470 * remove vk constants from join-split --------- Co-authored-by: Charlie Lye <karl.lye@gmail.com> Co-authored-by: Innokentii Sennovskii <isennovskiy@gmail.com> Co-authored-by: Adam Domurad <adam.domurad@gmail.com> Co-authored-by: ludamad <adam@aztecprotocol.com>
* attempting to remove any call to bump_memory * add back in mistakenly removed fft * fixing some bump memory in PCS and poly mem store * ensuring no bump memory with assert and removing initial_size * removing allocated_pages and page_size * fix bad polynomial sizing in prover test * getting rid of the notion of max_size * removing some erroneous zero_memory calls in poly operators * fix bad sizing in poly arith test * doing away with the poly(size_t, size_t) constructor entirely * progress towards coherent size-capacity concept * fix after rebase * improving move operations with exchange and adding a coresponding test * addressing several of Adrians PR comments * restoring single instantiation of tmp poly in shplonk * remove size check from factor_roots test since size is no longer changed * default constructor, bad move operation, etc * allowing assignment after default construction * standardizing assertions in polynomial * fix return type on mapped
* updating honk bench and adding comparison script * make requirements install a part of the script
…barretenberg#127) * fix: Ensure TBB is optional using OPTIONAL_COMPONENTS * chore: Avoid noisy message when tbb not found
* Remove conditional logic on [1]_1 in reading monomial transcript. * Enable read/write of Lagrange transcripts in multiple files for sizes > 2^24. * Enable >2^24 Lagrange transcript downloading. Use new monomial transcript00.dat form new folder in s3 bucket. * Fix download lagrange igniton script. * attempt to fix gcc build * Luke's suggestion. * fix --------- Co-authored-by: Suyash Bagad <suyashnbagad1997@gmail.com>
* Proof system works by using lagrange srs. * Prover c_bind changes. * Circuit c binds. * fixes. * Download more lagrange transcripts. * fix. * Add comment. * Add `commit_lagrange` in commitment key. --------- Co-authored-by: Suyash Bagad <suyashnbagad1997@gmail.com>
Co-authored-by: codygunton <codygunton@gmail.com>
Co-authored-by: ludamad <adam@aztecprotocol.com>
…arretenberg#559) Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>
ludamad
changed the title
See #1126 for motivation.
ludamad
force-pushed
the
ad/barretenberg-monorepo
branch
from
9a80972 to
a46419e
Compare
ludamad
force-pushed
the
barretenberg/test-team-branches
branch
from
eda1b8a to
9f4731b
Compare
|
Personally I'd be less aggressive on the ignore-lists. I think a change in bb should still trigger a rebuild of circuits, to make sure there are no breaking changes to the external interface, but we can possibly skip most (if not all) of the tests. Same with e2e tests: maybe we can just keep a single "happy path" e2e test that makes sure that nothing has been broken horribly upstream, so we don't risk accidentally breaking master with merging one of these PRs? And yeah, I'm aware I'm being super annoying. Anyway, I know it's a matter of preference. I'm fine merging as it is now if there's consensus. |
|
So we shouldn't merge one of these into master but instead a feature branch that runs CI. There's value in what you said, but it's also more complex, I'm not sure anyone should really use this - our CI I think takes 15 minutes, just an option for very high frequency churning dev. Not sure here! |
Then we should have the CI check that: if we are merging into master, then we don't skip anything (unless it's a |
ludamad
force-pushed
the
ad/barretenberg-monorepo
branch
from
299f9f6 to
c7fb04e
Compare
|
Interestingly, re the notion that changes to docs/ are isolated, my latest work (#1172) introduces a sort-of macro to the docs' markdown files, allowing code from the codebase to be injected into the docs. This requires comments to be added to files in the codebase, to delineate where a code snippet starts and ends. |
|
I'll hold this one a bit to think on it, for reasons Mike and Santiago mention |
spalladino
added
the
S-needs-discussion
|
There's some argument here for smaller per team CI, but I think everyone is good to merge into master overall so likely shouldn't do it based on target branch. Maybe we do with one end to end test per team etc, but yeah unclear what a 'safe' subset of CI is |
|
Closing for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Filtered (shorter) CI for feature branches. After this, branches can be prefixed with
barretenberg/orcircuits/. The logic for implementing these two in particular is that the rest of the stack generally skips a lot of CI already by virtue of not needing to rebuild, but working on these base components causes lots of churn in the stack. There can be a branch such asbarretenberg-masterandcircuits-master(which do not match the filtering here) and have PRs go into them asbarretenberg/ad/featureorcircuits/ad/featurefor example, and those branches will not run CI, but thebarretenberg-masterorcircuits-masterbranches will. This is meant to allow for a fast iterative process while still being able to merge into mainline frequently. Those branches can then have a merge policy that makes sense for the team to move quickly.If using this, it is still a good idea to run end to end tests locally periodically.