Skip to content

chore!: remove Opcode::Brillig from ACIR#5995

Merged
jfecher merged 21 commits into
masterfrom
tf/remove-brillig
Apr 25, 2024
Merged

chore!: remove Opcode::Brillig from ACIR#5995
jfecher merged 21 commits into
masterfrom
tf/remove-brillig

Conversation

@TomAFrench

Copy link
Copy Markdown
Member

This PR removes Opcode::Brillig as it has been superceded by Opcode::BrilligCall

@TomAFrench TomAFrench requested a review from vezenovm April 24, 2024 11:04
@TomAFrench

Copy link
Copy Markdown
Member Author

I've cut some corners related to the ACVM's interface as this is handled in noir-lang/noir#4897.

@AztecBot

AztecBot commented Apr 24, 2024

Copy link
Copy Markdown
Collaborator

Benchmark results

Metrics with a significant change:

  • note_trial_decrypting_time_in_ms (8): 73.2 (+143%)
Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 64 txs
l1_rollup_calldata_size_in_bytes 772 772 772
l1_rollup_calldata_gas 6,832 6,832 6,844 (+1%)
l1_rollup_execution_gas 587,313 587,313 587,325
l2_block_processing_time_in_ms 1,466 (+1%) 5,348 (-1%) 10,393 (-1%)
note_successful_decrypting_time_in_ms 181 (-26%) 621 (+6%) 1,044 (-4%)
note_trial_decrypting_time_in_ms ⚠️ 73.2 (+143%) 23.9 (-71%) 122 (-15%)
l2_block_building_time_in_ms 18,730 72,842 (-1%) 145,137 (-1%)
l2_block_rollup_simulation_time_in_ms 18,515 72,079 (-1%) 143,638 (-1%)
l2_block_public_tx_process_time_in_ms 8,449 (-1%) 33,069 (-1%) 65,958 (-1%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 16,705 (+7%) 30,213
note_history_successful_decrypting_time_in_ms 1,453 (+7%) 2,750 (+1%)
note_history_trial_decrypting_time_in_ms 105 (+8%) 121 (-18%)
node_database_size_in_bytes 19,181,648 35,872,848
pxe_database_size_in_bytes 29,859 59,414

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 156 47,763 31,978
private-kernel-inner 177 (-1%) 80,794 31,978
private-kernel-ordering 137 (-1%) 57,732 50,043
base-parity 74.1 (-5%) 128 265
root-parity 123 (-5%) 1,244 265
base-rollup 12,192 110,926 957
root-rollup 49.9 4,551 821
public-kernel-app-logic 1,649 (-1%) 62,017 52,067
public-kernel-tail 4,201 (-1%) 168,566 7,491
merge-rollup 3,114 (-2%) 2,760 957
public-kernel-teardown 117 (-2%) 62,017 52,067
public-kernel-setup 118 62,017 52,067

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric 1 leaves 16 leaves 64 leaves 128 leaves 512 leaves 1024 leaves 2048 leaves 4096 leaves 32 leaves
batch_insert_into_append_only_tree_16_depth_ms 11.1 (+1%) 17.8 (+1%) N/A N/A N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.8 31.6 N/A N/A N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.642 (+1%) 0.550 (+1%) N/A N/A N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A 51.7 (-1%) 80.8 260 503 981 1,948 N/A
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A 96.0 159 543 1,055 2,079 4,127 N/A
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A 0.530 (-1%) 0.497 0.474 0.469 0.466 0.466 N/A
batch_insert_into_indexed_tree_20_depth_ms N/A N/A 60.6 120 (+1%) 377 (+1%) 746 (+1%) 1,462 2,908 (-1%) N/A
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A 105 207 691 1,363 2,707 5,395 N/A
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A 0.534 0.540 (+1%) 0.511 0.511 0.508 0.507 (-1%) N/A
batch_insert_into_indexed_tree_40_depth_ms N/A N/A N/A N/A N/A N/A N/A N/A 67.8
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A N/A N/A N/A N/A N/A N/A 109
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A N/A N/A N/A N/A N/A N/A 0.589

Miscellaneous

Transaction sizes based on how many contract classes are registered in the tx.

Metric 0 registered classes 1 registered classes
tx_size_in_bytes 51,411 543,524

Transaction size based on fee payment method

Metric native fee payment method fpc_public fee payment method fpc_private fee payment method
tx_with_fee_size_in_bytes 897 1,145 1,345

Transaction processing duration by data writes.

Metric 0 new note hashes 1 new note hashes 2 new note hashes
tx_pxe_processing_time_ms 1,261 (-1%) 873 (+1%) 3,514
Metric 1 public data writes 2 public data writes 3 public data writes 4 public data writes 5 public data writes 8 public data writes
tx_sequencer_processing_time_ms 833 (-1%) 761 1,353 903 2,075 1,087 (-1%)

@TomAFrench TomAFrench marked this pull request as ready for review April 24, 2024 13:20
@TomAFrench

Copy link
Copy Markdown
Member Author

This is now ready go in but we're going to do another sync between repos before merging.

@jfecher jfecher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you mark the remaining work a bit more explicitly with TODOs or similar? They look like they'd be hard to find after the fact at the moment.

Edit: Cross-referenced the debugger PR and resolved my comments related to unimplemented work from there. I originally thought we'd come back later to fix it so TODOs would be useful but if we already have the code then it should be fine.

Comment thread noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs Outdated
Comment thread noir/noir-repo/tooling/debugger/src/context.rs Outdated
Comment thread noir/noir-repo/tooling/debugger/src/context.rs Outdated
Comment thread noir/noir-repo/tooling/debugger/src/context.rs Outdated
Comment thread noir/noir-repo/tooling/debugger/src/context.rs Outdated
Comment thread noir/noir-repo/tooling/debugger/src/repl.rs Outdated
@vezenovm

Copy link
Copy Markdown
Contributor

This is now ready go in but we're going to do another sync between repos before merging.

Maybe we should wait for noir-lang/noir#4897 to be merged and syncd with this repo before merging this PR

ludamad and others added 17 commits April 24, 2024 19:36
We were seeing spot reaps, if costs are not terrible we should consider
on-demand
Cleans up Translator flavor: removes some redundant methods, replaces
list style getters with one built up from sub-components, and clarifies
the subtlety around getters that return everything except concatenated
polys.
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: fix curve parameters for bigints
(noir-lang/noir#4900)
chore: bump `rustls` to v0.21.11
(noir-lang/noir#4895)
fix: Update noir-gates-diff commit to use master reference report
(noir-lang/noir#4891)
fix: Reset the noir-gates-diff report on master
(noir-lang/noir#4878)
fix(experimental): Skip over comptime functions in scan pass
(noir-lang/noir#4893)
chore(experimental): Improve variable not defined error message in
comptime interpreter (noir-lang/noir#4889)
chore(experimental): Add scan pass and `into_expression` for comptime
interpreter (noir-lang/noir#4884)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <tom@tomfren.ch>
Similarly to noir-lang/noir#4878 the gates diff
report got corrupted following changes to `nargo info`. This PR simply
resets the gates diff so it is expected to see really large gates
differences. You can look into the Noir PR for more info. There will be
a followup which sets back to the correct noir-gates-diff commit.
Without this reset we will not be able to get accurate gate diffs based
off of master.
)

Follow-up to #6003.
This PR simply updates to using a noir gates diff that expects an
uncorrupted comparison report on master.
Avoids a bad situation where a sequencer node produces empty blocks that fail to validate in the rollup contract.
Serialize `PublicKernelCircuitPrivateInputs` to and from strings
Fixes #5947 and includes the notes as well. The notes was part of the
contract object, but not directly exposed on the artifact itself.

Needed to fix a few additional things to make it work:
- The `deploy_contract` function was broken (old call flow), it cannot
do the new flow because that would be a circular dependency.
- Using the `storageLayout` values to get the `storageSlot` that we are
using in multiple tests
- Using the `notes` values to get the `noteTypeId` that we are using in
multiple tests
- Removed `lodash.uniqby` as the dependency was not needed after this.
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: Nested array equality (noir-lang/noir#4903)
feat: Handle `BrilligCall` opcodes in the debugger
(noir-lang/noir#4897)
chore: Release Noir(0.28.0)
(noir-lang/noir#4776)
feat: Sync from aztec-packages
(noir-lang/noir#4902)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <tom@tomfren.ch>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
* master:
  feat: Sync from noir (#6007)
  feat: AES oracle (#5996)
  hotfix: better docker prune
  feat: add the storage layout to the contract artifact (#5952)
  feat: serialize public kernel private inputs (#5971)
  fix: refuse to start sequencer without a prover (#6000)
  chore(ci): hotfix runners not starting
  fix: Use correct gates diff commit now that master has been reset (#6004)
  chore: Reset noir-gates-diff report on master  (#6003)
  feat: Sync from noir (#5999)
  fix: make discv5 test deterministic (#5968)
  chore: Clean up and clarify some translator flavor logic (#5965)
  chore(ci): back to on-demand (#5998)
@TomAFrench TomAFrench requested a review from jfecher April 25, 2024 11:57

@jfecher jfecher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

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.

10 participants