fix: extract whole archive instead of subset #3604

alexghr · 2023-12-06T16:14:02Z

This PR changes the behaviour of the dependency resolver in noir-compiler to extract a whole zip archiver from Github instead of cherry-picking just the given path. This makes it so that relative dependencies inside the archive are resolved correctly with the tradeoff that now it extract everything (e.g. aztec-packages extracts 80mb but aztec-nr is only ~1MB of code)

alexghr · 2023-12-06T16:15:03Z

yarn-project/noir-compiler/src/compile/noir/dependencies/local-dependency-resolver.ts

    if ('path' in config) {
+      const parentPath = parent.getPackagePath();
+      const dependencyPath = isAbsolute(config.path) ? config.path : join(parentPath, config.path);


Kinda worried about someone putting /etc/passwd in their dependency list. Maybe we make it only accept relative deps?

resolveDependency is only used for loading stuff, right? We're not writing to the resolved directory at any time? If so, I think it's not that bad, since at most it'd exfiltrate /etc/passwd to the noir compiler, and won't overwrite anything.

Yep, that's how it works. The Github resolver does write to the disk (to unzip the archive), but writing to the disk is strictly checked (can only write to a given subfolder).

AztecBot · 2023-12-06T16:37:13Z

Benchmark results

Metrics with a significant change:

node_database_size_in_bytes (10): 6,700,579 (+71%)
note_history_trial_decrypting_time_in_ms (10): 228 (-38%)
note_trial_decrypting_time_in_ms (32): 76.0 (+87%)

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.

Values are compared against data from master at commit ceb2ed26 and shown if the difference exceeds 1%.

L2 block published to L1

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

Metric	8 txs	32 txs	128 txs
l1_rollup_calldata_size_in_bytes	45,444	179,588	716,132
l1_rollup_calldata_gas	222,864	868,172	3,449,684
l1_rollup_execution_gas	841,951	3,595,280	22,205,053
l2_block_processing_time_in_ms	2,226 (-6%)	8,556 (+1%)	35,421 (+1%)
note_successful_decrypting_time_in_ms	314 (-2%)	946 (+4%)	3,395 (+2%)
note_trial_decrypting_time_in_ms	26.6 (-1%)	⚠️ 76.0 (+87%)	201
l2_block_building_time_in_ms	20,387 (+1%)	80,635 (+1%)	323,463 (+1%)
l2_block_rollup_simulation_time_in_ms	16,847 (+1%)	66,692 (+1%)	267,447 (+1%)
l2_block_public_tx_process_time_in_ms	3,509	13,873	55,764 (+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	24,645 (-1%)	47,360
note_history_successful_decrypting_time_in_ms	2,139 (-1%)	4,145 (-2%)
note_history_trial_decrypting_time_in_ms	162 (+1%)	⚠️ 228 (-38%)
node_database_size_in_bytes	3,414,068 (-6%)	⚠️ 6,700,579 (+71%)
pxe_database_size_in_bytes	29,748	59,307

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	199	43,109	20,441
private-kernel-ordering	115 (+1%)	25,833	9,689
base-rollup	2,955	667,692	873
root-rollup	86.5 (+3%)	4,072	881
private-kernel-inner	260	64,516	20,441
public-kernel-private-input	171	25,203	20,441
public-kernel-non-first-iteration	168	25,245	20,441
merge-rollup	10.9 (+1%)	2,592	873

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric	0 deployed contracts	1 deployed contracts
tx_size_in_bytes	10,323	25,938

spalladino

I understand that this would fix things for aztec-nr just because its dependencies happen to be on the same repository, right? If so, let's make sure we roll back this change when we implement a more thorough solution. But it looks good to get us out from this issue!

spalladino · 2023-12-06T17:23:51Z

yarn-project/noir-compiler/src/compile/noir/dependencies/local-dependency-resolver.ts

    if ('path' in config) {
+      const parentPath = parent.getPackagePath();
+      const dependencyPath = isAbsolute(config.path) ? config.path : join(parentPath, config.path);


resolveDependency is only used for loading stuff, right? We're not writing to the resolved directory at any time? If so, I think it's not that bad, since at most it'd exfiltrate /etc/passwd to the noir compiler, and won't overwrite anything.

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.16.7</summary> ## [0.16.7](aztec-packages-v0.16.6...aztec-packages-v0.16.7) (2023-12-06) ### Features * Encapsulated Goblin ([#3524](#3524)) ([2f08423](2f08423)) ### Bug Fixes * Extract whole archive instead of subset ([#3604](#3604)) ([cb000d8](cb000d8)) ### Documentation * **yellow-paper:** Note hash, nullifier, and public data trees ([#3518](#3518)) ([0e2db8b](0e2db8b)) </details> <details><summary>barretenberg.js: 0.16.7</summary> ## [0.16.7](barretenberg.js-v0.16.6...barretenberg.js-v0.16.7) (2023-12-06) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.16.7</summary> ## [0.16.7](barretenberg-v0.16.6...barretenberg-v0.16.7) (2023-12-06) ### Features * Encapsulated Goblin ([#3524](#3524)) ([2f08423](2f08423)) </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-packages: 0.16.7</summary> ## [0.16.7](AztecProtocol/aztec-packages@aztec-packages-v0.16.6...aztec-packages-v0.16.7) (2023-12-06) ### Features * Encapsulated Goblin ([#3524](AztecProtocol/aztec-packages#3524)) ([2f08423](AztecProtocol/aztec-packages@2f08423)) ### Bug Fixes * Extract whole archive instead of subset ([#3604](AztecProtocol/aztec-packages#3604)) ([cb000d8](AztecProtocol/aztec-packages@cb000d8)) ### Documentation * **yellow-paper:** Note hash, nullifier, and public data trees ([#3518](AztecProtocol/aztec-packages#3518)) ([0e2db8b](AztecProtocol/aztec-packages@0e2db8b)) </details> <details><summary>barretenberg.js: 0.16.7</summary> ## [0.16.7](AztecProtocol/aztec-packages@barretenberg.js-v0.16.6...barretenberg.js-v0.16.7) (2023-12-06) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.16.7</summary> ## [0.16.7](AztecProtocol/aztec-packages@barretenberg-v0.16.6...barretenberg-v0.16.7) (2023-12-06) ### Features * Encapsulated Goblin ([#3524](AztecProtocol/aztec-packages#3524)) ([2f08423](AztecProtocol/aztec-packages@2f08423)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This PR fixes an issue with how we mirror code in the monorepo to aztecprotocol/aztec-nr. A while back the root aztec library added a dependency on noir-protocol-circuits. In the monorepo this is achieved by use of a relative path in Nargo.toml. This works fine for contracts that pull in Aztec.nr through the monorepo (see also #3604) but it breaks projects that use [AztecProtocol/aztec-nr](https://github.com/AztecProtocol/aztec-nr) because the relative path won't exist in the mirrored repo. What this PR does is map any relative dependencies to protocol circuits to a git dependency before pushing that commit to the mirrored repo. It then undoes this change before pushing to the monorepo (we want to keep using relative paths in the monorepo). I would've preferred using `yq` since it claims it suports TOML (and is included in the default Github actions package list) but its support is limited to only basic types (so no objects like `{path="..."}`) and it can only read toml but not write it. mikefarah/yq#1364 (comment)

This PR fixes an issue with how we mirror code in the monorepo to aztecprotocol/aztec-nr. A while back the root aztec library added a dependency on noir-protocol-circuits. In the monorepo this is achieved by use of a relative path in Nargo.toml. This works fine for contracts that pull in Aztec.nr through the monorepo (see also AztecProtocol#3604) but it breaks projects that use [AztecProtocol/aztec-nr](https://github.com/AztecProtocol/aztec-nr) because the relative path won't exist in the mirrored repo. What this PR does is map any relative dependencies to protocol circuits to a git dependency before pushing that commit to the mirrored repo. It then undoes this change before pushing to the monorepo (we want to keep using relative paths in the monorepo). I would've preferred using `yq` since it claims it suports TOML (and is included in the default Github actions package list) but its support is limited to only basic types (so no objects like `{path="..."}`) and it can only read toml but not write it. mikefarah/yq#1364 (comment)

fix: extract whole archive instead of subset

24ca1f8

alexghr commented Dec 6, 2023

View reviewed changes

spalladino approved these changes Dec 6, 2023

View reviewed changes

alexghr merged commit cb000d8 into master Dec 6, 2023
80 checks passed

alexghr deleted the alexg/fix/noir-compiler-deps branch December 6, 2023 17:34

AztecBot mentioned this pull request Dec 6, 2023

chore(master): Release 0.16.7 #3603

Merged

This was referenced Dec 11, 2023

noir-compiler should have better support for mixed remote and relative dependencies #3646

Closed

fix: map relative path to protocol circuits #3694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: extract whole archive instead of subset #3604

fix: extract whole archive instead of subset #3604

alexghr commented Dec 6, 2023

alexghr Dec 6, 2023

spalladino Dec 6, 2023

alexghr Dec 6, 2023

AztecBot commented Dec 6, 2023

L2 block published to L1

L2 chain processing

Circuits stats

Miscellaneous

spalladino left a comment

spalladino Dec 6, 2023

fix: extract whole archive instead of subset #3604

fix: extract whole archive instead of subset #3604

Conversation

alexghr commented Dec 6, 2023

alexghr Dec 6, 2023

Choose a reason for hiding this comment

spalladino Dec 6, 2023

Choose a reason for hiding this comment

alexghr Dec 6, 2023

Choose a reason for hiding this comment

AztecBot commented Dec 6, 2023

Benchmark results

L2 block published to L1

L2 chain processing

Circuits stats

Miscellaneous

spalladino left a comment

Choose a reason for hiding this comment

spalladino Dec 6, 2023

Choose a reason for hiding this comment