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

feat: Barretenberg C++ binary overhaul #11459

Merged
merged 159 commits into from
Feb 20, 2025
Merged

feat: Barretenberg C++ binary overhaul #11459

merged 159 commits into from
Feb 20, 2025

Conversation

codygunton
Copy link
Contributor

@codygunton codygunton commented Jan 23, 2025

Overhaul the Barretenberg binary and its API.

  • Breaks up bb main into different files organized by proving system / IVC scheme.
  • Make UltraHonk conform to the new API interface introduced earlier for Client IVC.
  • Refines the API a bit.
  • Introduces CLI11 to: provide help / documentation; validate opts (options can be required, exlusive of each other, validated against predicates like "path exists" or "string is in list"); also allows for easy environment variable aliasing.

This could definitely use some more a help.

  • Lots of documentation needed
  • Defaults are set in a weird and inconsistent way and that information isn't included in the documentation.
  • The help menus are perhaps too verbose. Subcommands can't inherit options or flags so we end up repeating.
  • Empty string cannot be passed and parsed to a "nothing argument" which can lead to frustrating debugging...
  • Little option validation is actually implemented.
  • Deprecated options aren't noted but they could be.

It was requested that the default change from UltraPlonk to UltraHonk, but we get rid of a default set of commands altogether. As a workaround, we can have users set BB_SCHEME=ultra_honk.

Newly created issues: AztecProtocol/barretenberg#1252, AztecProtocol/barretenberg#1253, AztecProtocol/barretenberg#1254, AztecProtocol/barretenberg#1255, AztecProtocol/barretenberg#1256, AztecProtocol/barretenberg#1257, AztecProtocol/barretenberg#1258, AztecProtocol/barretenberg#1259

Resolves AztecProtocol/barretenberg#1260

NB the line count is large because 1) CLI11 is a single 11k-line header; 2) I moved a lot of functions and some git mvs didn't show up as such. Main new code is api_ultra_honk.hpp.

@ludamad
Copy link
Collaborator

ludamad commented Jan 24, 2025

Merging master as boxes to get CI passing here

@@ -40,7 +40,7 @@ for artifact in $artifacts_to_process; do

echo "Generating verification key for function $fn_name"
# BB outputs the verification key to stdout as raw bytes, however, we need to base64 encode it before storing it in the artifact
verification_key=$($BB write_vk_for_ivc -b ${fn_artifact_path} -o - | base64)
verification_key=$($BB write_vk --scheme client_ivc -b ${fn_artifact_path} -o - | base64)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is immediately going to become confusing when we add a flow to write a CIVC VK. I wonder if this should just become the write_vk for mega honk..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought about this but then it's the only mega honk command which seems weird. It think it makes sense to put it behind the --input_type flag in CIVC?

@codygunton codygunton added e2e-all CI: Deprecated, use ci-full. Enable all master checks. e2e labels Feb 18, 2025
if (recursive) {
args.push('--init_kzg_accumulator');
}
const result = await executeBB(pathToBB, `write_vk`, args, logFunction);
Copy link
Member

Choose a reason for hiding this comment

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

Is --scheme arg not needed for write_vk here?
earlier it used to be one of write_vk_mega_honk, write_vk_ultra_honk or write_vk_ultra_keccak_honk

Copy link
Member

Choose a reason for hiding this comment

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

Oh looks like this computeVerificationKey method is not used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing--removed the function.

@ludamad ludamad requested a review from charlielye February 20, 2025 19:11
@codygunton codygunton merged commit db2a503 into master Feb 20, 2025
20 checks passed
@codygunton codygunton deleted the cg/uh-api branch February 20, 2025 19:12
debug_logging = flags.debug;
verbose_logging = debug_logging || flags.verbose;

// prob this construction is too much
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be possible to avoid this chain of if-elses by using callbacks on each subcommand. At least that's what I would've expected and seems supported: https://github.com/CLIUtils/CLI11?tab=readme-ov-file#callbacks

(but of course I only skimmed through it)

codygunton added a commit that referenced this pull request Feb 20, 2025
I did e2e and e2e_all in
#11459 and the
darwin builds didn't fail there, but they fail in master. This is
basically a typo fix.
TomAFrench added a commit that referenced this pull request Feb 20, 2025
* master: (300 commits)
  fix(ci): don't have checks go green immediately (#12168)
  fix: ASSERTS that should throw (#12167)
  fix: retry rm operation in cleanup (#12162)
  chore: Fix linter errors (#12164)
  feat: Barretenberg C++ binary overhaul (#11459)
  fix: call install_hooks in bootstrap (#12159)
  chore: @aztec/stdlib pt. 3: aztec-address out of foundation (#12140)
  test: verify proving is resumed after broker crash (#11122)
  chore(ci3): update ci.md with swc notes (#12147)
  fix: don't try to get bench artifacts on external PR (#12157)
  feat: partial note handling in aztec-nr (#12122)
  fix: external fixes pt 2 (#12153)
  chore: fix message path (#12150)
  chore(ci3): refactor ci3.yml, fix external PR flow (#12037)
  fix: Do not try flushing txs in bot setup if not set (#12144)
  chore: Silence warns on invalid bootnode enr (#12135)
  fix: don't early-out on test fails (#12143)
  feat(avm): deduplicating event emitters (#12137)
  chore: @aztec/stdlib pt.2 -> remove @aztec/types (#12133)
  test: kill prover node and see it recover (#11118)
  ...
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Feb 21, 2025
I did e2e and e2e_all in
AztecProtocol/aztec-packages#11459 and the
darwin builds didn't fail there, but they fail in master. This is
basically a typo fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Deprecated, use ci-full. Enable all master checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only construct circuit once when writing two vks.
6 participants