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

chore: barretenberg in the monorepo #1081

Merged
merged 1,104 commits into from
Jul 24, 2023
Merged

chore: barretenberg in the monorepo #1081

merged 1,104 commits into from
Jul 24, 2023

Conversation

ludamad
Copy link
Collaborator

@ludamad ludamad commented Jul 14, 2023

Description

This implements "Integrate barretenberg code and CI situated at circuits/cpp/barretenberg" in #1126. The motion to include barretenberg in the monorepo has not had major opposition, though for sure there was hesitation around the disruption and effort. Minor opposition was noted around the repo barrier and CI times, but overall we believe we can solve these issues.

Note about "Ensure barretenberg circuits/cpp/barretenberg is either a viable path or move it" in #1126:
One non-ideal feature of this PR is that barretenberg is nested in circuits/cpp/barrretenberg. This might make more sense for the circuits team's current setup than someone trying to find barretenberg to use standalone. However,

  • There's an interest to get onto a monorepo quickly to have a small cost in disruption
  • Moving barretenberg from the location in this PR will not cause issues with the git blame function in e.g. vscode

Future improvements are welcome, although there isn't necessarily a 'correct place' for barretenberg.

Process:

set -xue
# Clone both repositories
git clone https://github.com/AztecProtocol/barretenberg.git
git clone https://github.com/AztecProtocol/aztec-packages.git

# Enter barretenberg repository and filter it
cd barretenberg
git filter-repo --to-subdirectory-filter barretenberg
git filter-repo --message-callback """
  import re

  def replace_hash_with_url(text):
       url_format = 'https://github.com/AztecProtocol/barretenberg/issues/{}'
       # the regular expression pattern r'#(\d+)' matches a '#' followed by one or more digits (\d+)
       return re.sub(r'#(\d+)', lambda match: url_format.format(match.group(1)), text)
  message = message.decode('utf-8') # Decode from bytes to string
  message = message.replace('(#', '(https://github.com/AztecProtocol/barretenberg/pull/')
  message = message.replace('Merge pull request #', 'Merge pull request https://github.com/AztecProtocol/barretenberg/pull/')
  return replace_hash_with_url(message).encode('utf-8') # Encode back into bytes
"""

# Enter aztec-packages repository
cd ../aztec-packages

# Create a new branch for the combined history
git checkout -b combined_history

# Add barretenberg as a remote and fetch it
git remote add barretenberg ../barretenberg
git fetch barretenberg

# Merge the histories, preserving the directory structure
git merge --no-commit --allow-unrelated-histories barretenberg/master

# Commit the changes
git commit -m "chore: barretenberg in the monorepo" -m "See #1126 for motivation."

Checklist:

  • [Sorry, no] I have reviewed my diff in github, line by line.
  • [*] Every change is related to the PR description.
  • [*] I have linked this pull request to the issue(s) that it resolves.
  • [*] There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • [*] The branch has been merged or rebased against the head of its merge target.
  • [*] I'm happy for the PR to be merged at the reviewer's next convenience.

suyash67 and others added 30 commits January 28, 2023 19:23
* 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>
Added Ultraplonk and StandardHonk names to  GET_COMPOSER_NAME
* 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>
@ludamad ludamad changed the title chore: barretenberg in the monorepo (WIP, do not merge) chore: barretenberg in the monorepo Jul 23, 2023
@ludamad ludamad force-pushed the ad/barretenberg-monorepo branch from 2db8ad0 to 9a80972 Compare July 23, 2023 13:40
@ludamad
Copy link
Collaborator Author

ludamad commented Jul 23, 2023

@dbanks12 @spalladino @charlielye @codygunton This is now a viable base that I think can be merged. I have some followup stuff (github action, subrepo file, team-based job filters) but given that I want every commit to go into master here, I don't want to add too much more to this one

@ludamad ludamad force-pushed the ad/barretenberg-monorepo branch 2 times, most recently from 5c8e0d6 to a46419e Compare July 24, 2023 01:34
@codygunton
Copy link
Contributor

Can you explain how you made this PR? If you just delete the submodule definition, I guess all of Bb shows up as new files. but then there's no history for those files, right? But we see the full history. What was the workflow?

@ludamad
Copy link
Collaborator Author

ludamad commented Jul 24, 2023

We will have full history now, this I checked. Github is chugging with this PR so you might not see good details. This is the process

set -xue
# Clone both repositories
git clone https://github.com/AztecProtocol/barretenberg.git
git clone https://github.com/AztecProtocol/aztec-packages.git

# Enter barretenberg repository and filter it
cd barretenberg
git filter-repo --to-subdirectory-filter barretenberg
git filter-repo --message-callback """
  import re

  def replace_hash_with_url(text):
       url_format = 'https://github.com/AztecProtocol/barretenberg/issues/{}'
       # the regular expression pattern r'#(\d+)' matches a '#' followed by one or more digits (\d+)
       return re.sub(r'#(\d+)', lambda match: url_format.format(match.group(1)), text)
  message = message.decode('utf-8') # Decode from bytes to string
  message = message.replace('(#', '(https://github.com/AztecProtocol/barretenberg/pull/')
  message = message.replace('Merge pull request #', 'Merge pull request https://github.com/AztecProtocol/barretenberg/pull/')
  return replace_hash_with_url(message).encode('utf-8') # Encode back into bytes
"""

# Enter aztec-packages repository
cd ../aztec-packages

# Create a new branch for the combined history
git checkout -b combined_history

# Add barretenberg as a remote and fetch it
git remote add barretenberg ../barretenberg
git fetch barretenberg

# Merge the histories, preserving the directory structure
git merge --no-commit --allow-unrelated-histories barretenberg/master

# Commit the changes
git commit -m "chore: barretenberg in the monorepo" -m "See #1126 for motivation."

@codygunton
Copy link
Contributor

I tried out continuing work on my branch cg/simulate-spike in the new monorepo. Workflow: clone fresh, open barretenberg.code-workspace in the embedded copy of Bb, check out my branch. Result: VS code restarts; no more files the embedded barretenberg folder. What's the intended workflow to continue existing work?

@ludamad
Copy link
Collaborator Author

ludamad commented Jul 24, 2023

As I mentioned privately,

  1. you need to try ad/bb-monorepo/github-action to try this out as that PR adds .gitrepo info
  2. open workspace as you did
  3. ./scripts/git_subrepo.sh pull circuits/cpp/barretenberg --branch=cg/simulate-spike
  4. there's a merge conflict with instructions how to fix (avoidable if you first pull master in your bb branch?)

See the result #1167

@codygunton
Copy link
Contributor

OK, looks like a reasonable workflow after your followup. I checked out cg/example and see that we lose the PR branch history, but I think that's acceptable, especially if you accept the temporary role of git tech support for a week or so 😅

Copy link
Contributor

@codygunton codygunton 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, excited for the saner workflow.

@ludamad
Copy link
Collaborator Author

ludamad commented Jul 24, 2023

OK, looks like a reasonable workflow after your followup. I checked out cg/example and see that we lose the PR branch history, but I think that's acceptable, especially if you accept the temporary role of git tech support for a week or so 😅

Yeah I think given that we squash PRs anyway we should settle for a squashed pull request if we're porting. Let's just link to the PR in bb if it has moved and call it a day (it won't appear in git blame either way, so we don't lose much, and when I do investigate as far as the PR I don't mind clicking one more link). Now I do want to replay any history that makes it to master - you can leave that to me.

Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

LGTM. Some small comments.

@@ -853,16 +1076,13 @@ workflows:
- integration-l1-publisher: *e2e_test
- integration-archiver-l1-to-l2: *e2e_test
- e2e-p2p: *e2e_test
- e2e-uniswap-sandbox: *e2e_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this and other e2e tests removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, shouldn't have been.

Comment on lines 35 to 31
static random(): Coordinate {
static random(_a: number): Coordinate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this change from? Just a small fix you included?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surprised by this, I'll double check any changes outside of barretenberg

Comment on lines 2 to 3
'TSMethodDefinition[accessibility=public]',
'MethodDefinition[accessibility=public]',
'TSParameterProperty[accessibility=public]',
'TSPropertySignature',
'PropertySignature',
'TSInterfaceDeclaration',
'InterfaceDeclaration',
'TSPropertyDefinition[accessibility=public]',
'PropertyDefinition[accessibility=public]',
'TSTypeAliasDeclaration',
'TypeAliasDeclaration',
'TSTypeDeclaration',
'TypeDeclaration',
'TSEnumDeclaration',
'EnumDeclaration',
'TSClassDeclaration',
'ClassDeclaration',
'TSClassExpression',
'ClassExpression',
'TSFunctionExpression',
'FunctionExpression',
'TSInterfaceExpression',
'InterfaceExpression',
'TSEnumExpression',
'EnumExpression',
'TSMethodDefinition[accessibility!=private] > TSParameterProperty',
'MethodDefinition[accessibility!=private] > ParameterProperty',
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this diff doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing good!

@netlify
Copy link

netlify bot commented Jul 24, 2023

Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name Link
🔨 Latest commit c7fb04e
🔍 Latest deploy log https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64bef34542d39f00081e5f38

@ludamad ludamad force-pushed the ad/barretenberg-monorepo branch from e794bf6 to 299f9f6 Compare July 24, 2023 21:50
@ludamad ludamad force-pushed the ad/barretenberg-monorepo branch from 299f9f6 to c7fb04e Compare July 24, 2023 21:55
@ludamad ludamad merged commit a44f22d into master Jul 24, 2023
@ludamad ludamad deleted the ad/barretenberg-monorepo branch July 24, 2023 22:12
@ludamad ludamad restored the ad/barretenberg-monorepo branch July 24, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.