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: fix array access issue - nuclear edition #4814

Closed
wants to merge 40 commits into from

Conversation

Maddiaa0
Copy link
Member

@Maddiaa0 Maddiaa0 commented Feb 27, 2024

changes the default word size in noir to be u32, as this is the default word size of the avm.

@Maddiaa0 Maddiaa0 force-pushed the md/02-27-nuclear_u32_swap_to_solve_issue branch from ba50564 to ef9a36c Compare February 27, 2024 23:27
Copy link
Contributor

github-actions bot commented Feb 27, 2024

Changes to circuit sizes

Generated at commit: fa73842793045c5d00a562a546ae5a3a10ba25fd, compared to commit: a932153b52169d89fb8abf02bcd7bf813ea3c625

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_root -8 ✅ -0.41% 0 ➖ 0.00%
rollup_base -8 ✅ -0.00% -493 ✅ -0.03%
private_kernel_init 0 ➖ 0.00% -832 ✅ -0.23%
private_kernel_inner 0 ➖ 0.00% -1,265 ✅ -0.24%
private_kernel_tail 0 ➖ 0.00% -2,239 ✅ -0.28%
public_kernel_setup 0 ➖ 0.00% -710 ✅ -0.40%
public_kernel_teardown 0 ➖ 0.00% -710 ✅ -0.40%
public_kernel_app_logic 0 ➖ 0.00% -1,199 ✅ -0.50%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_root 1,950 (-8) -0.41% 99,124 (0) 0.00%
rollup_base 254,146 (-8) -0.00% 1,941,523 (-493) -0.03%
private_kernel_init 47,154 (0) 0.00% 355,951 (-832) -0.23%
private_kernel_inner 94,146 (0) 0.00% 525,384 (-1,265) -0.24%
private_kernel_tail 370,516 (0) 0.00% 791,510 (-2,239) -0.28%
public_kernel_setup 26,084 (0) 0.00% 175,062 (-710) -0.40%
public_kernel_teardown 26,076 (0) 0.00% 175,054 (-710) -0.40%
public_kernel_app_logic 43,453 (0) 0.00% 238,935 (-1,199) -0.50%

@Maddiaa0 Maddiaa0 force-pushed the md/02-27-nuclear_u32_swap_to_solve_issue branch from bd0d25e to ba7c7df Compare February 27, 2024 23:39
@Maddiaa0 Maddiaa0 force-pushed the md/02-27-nuclear_u32_swap_to_solve_issue branch from ba7c7df to 800b9e3 Compare February 28, 2024 00:30
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 28, 2024

Benchmark results

Metrics with a significant change:

  • tx_size_in_bytes (0): 18,431 (-77%)
  • node_history_sync_time_in_ms (5): 15,708 (+82%)
  • node_database_size_in_bytes (5): 19,095,632 (+50%)
  • pxe_database_size_in_bytes (5): 29,923 (+98%)
  • l2_block_building_time_in_ms (8): 15,984 (-65%)
  • l2_block_rollup_simulation_time_in_ms (8): 12,251 (-73%)
  • l2_block_public_tx_process_time_in_ms (8): 3,703 (-91%)
  • l1_rollup_calldata_gas (8): 259,892 (+2653%)
  • l1_rollup_calldata_size_in_bytes (8): 54,212 (+3739%)
  • l1_rollup_execution_gas (8): 354,078 (-42%)
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 ce7f0e24 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 ⚠️ 54,212 (+3739%) 214,436 855,332
l1_rollup_calldata_gas ⚠️ 259,892 (+2653%) 1,019,648 4,058,612
l1_rollup_execution_gas ⚠️ 354,078 (-42%) 1,139,096 4,280,013
l2_block_processing_time_in_ms 1,155 (-11%) 4,739 17,458
note_successful_decrypting_time_in_ms 199 555 1,918
note_trial_decrypting_time_in_ms 111 97.3 166
l2_block_building_time_in_ms ⚠️ 15,984 (-65%) 61,698 253,854
l2_block_rollup_simulation_time_in_ms ⚠️ 12,251 (-73%) 47,355 196,201
l2_block_public_tx_process_time_in_ms ⚠️ 3,703 (-91%) 14,250 57,359

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 ⚠️ 15,708 (+82%) 28,089
note_history_successful_decrypting_time_in_ms 1,324 2,498
note_history_trial_decrypting_time_in_ms 174 271
node_database_size_in_bytes ⚠️ 19,095,632 (+50%) 36,347,984
pxe_database_size_in_bytes ⚠️ 29,923 (+98%) 59,478

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 235 44,664 27,457
private-kernel-ordering 152 45,777 14,627
base-rollup 1,292 175,344 933
root-rollup 69.6 4,192 825
private-kernel-inner 299 73,099 27,457
public-kernel-app-logic 184 32,254 25,379
merge-rollup 6.17 2,712 933

Tree insertion stats

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

Metric 1 leaves 2 leaves 8 leaves 16 leaves 32 leaves 128 leaves 64 leaves 512 leaves 1024 leaves 2048 leaves 8192 leaves
batch_insert_into_append_only_tree_16_depth_ms 9.80 (-7%) 10.6 17.0 16.8 (-1%) 21.2 60.4 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.9 (+1%) 17.5 23.0 31.6 47.0 143 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.568 (-7%) 0.586 0.722 0.519 (-1%) 0.445 0.416 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A N/A N/A N/A 72.1 (-6%) 45.5 (-7%) 225 (-9%) 449 (-5%) 833 3,339
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A N/A N/A N/A 159 96.0 543 1,055 2,079 8,223
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A N/A N/A N/A 0.445 (-5%) 0.466 (-7%) 0.411 (-9%) 0.420 (-5%) 0.397 0.401
batch_insert_into_indexed_tree_20_depth_ms N/A N/A N/A N/A N/A 98.6 (-12%) 54.2 (-8%) 325 (-8%) 666 (+6%) 1,262 5,109
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A N/A N/A N/A 197 (-5%) 104 (-3%) 691 (+1%) 1,363 (+8%) 2,707 10,771
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A N/A N/A N/A 0.466 (-7%) 0.477 (-6%) 0.441 (-8%) 0.458 (-2%) 0.438 0.442
batch_insert_into_indexed_tree_40_depth_ms N/A N/A N/A N/A 61.2 (+4%) N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A N/A N/A 110 (+9%) N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A N/A N/A 0.530 (-4%) N/A N/A N/A N/A N/A N/A

Miscellaneous

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

Metric 0 deployed contracts
tx_size_in_bytes ⚠️ 18,431 (-77%)

Transaction processing duration by data writes.

Metric 0 new note hashes 1 new note hashes
tx_pxe_processing_time_ms 2,411 1,273
Metric 0 public data writes 1 public data writes
tx_sequencer_processing_time_ms 0.0270 452

Base automatically changed from md/02-16-feat_avm_storage to master March 6, 2024 18:43
Maddiaa0 added a commit that referenced this pull request Mar 6, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot added a commit to noir-lang/noir that referenced this pull request Mar 6, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
@Maddiaa0 Maddiaa0 mentioned this pull request Mar 7, 2024
Maddiaa0 added a commit that referenced this pull request Mar 7, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot added a commit to noir-lang/noir that referenced this pull request Mar 7, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot added a commit to noir-lang/noir that referenced this pull request Mar 7, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Mar 8, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request Mar 19, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request Mar 19, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
@Maddiaa0
Copy link
Member Author

Maddiaa0 commented Apr 8, 2024

closing as stale, can be reopened when this discussion resurfaces

@Maddiaa0 Maddiaa0 closed this Apr 8, 2024
@Maddiaa0 Maddiaa0 reopened this Jun 4, 2024
@Maddiaa0 Maddiaa0 closed this Jun 4, 2024
superstar0402 added a commit to superstar0402/aztec-nr that referenced this pull request Aug 16, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
superstar0402 added a commit to superstar0402/aztec-nr that referenced this pull request Aug 16, 2024
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <[email protected]>
@ludamad ludamad deleted the md/02-27-nuclear_u32_swap_to_solve_issue branch August 22, 2024 14: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.

3 participants