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

refactor: generate public tail hints in noir #8113

Merged
merged 5 commits into from
Aug 23, 2024

Conversation

LeilaWang
Copy link
Contributor

@LeilaWang LeilaWang commented Aug 21, 2024

  • Generating hints and output for public_kernel_tail in unconstrained functions.
  • Fixing sort_by_counter and assert_deduped_array.
  • Adding new util assert_combined_sorted_transformed_value_array to check the output arrays from public tail that they are merged and sorted correctly.

Copy link
Contributor

github-actions bot commented Aug 21, 2024

Changes to circuit sizes

Generated at commit: bfde246cfb0a2bb5333898c1deb50fb4db4f6f28, compared to commit: bf38d61364a0fb55ae79ef09b05df2533f3a6f17

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_tail -480,378 ✅ -50.71% -1,090,990 ✅ -26.78%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_tail 467,012 (-480,378) -50.71% 2,982,928 (-1,090,990) -26.78%

@AztecBot
Copy link
Collaborator

AztecBot commented Aug 21, 2024

Benchmark results

Metrics with a significant change:

  • protocol_circuit_simulation_time_in_ms (private-kernel-reset-tiny): 307 (-34%)
  • protocol_circuit_simulation_time_in_ms (base-rollup): 3,543 (+28%)
  • protocol_circuit_simulation_time_in_ms (public-kernel-tail): 891 (+60%)
  • protocol_circuit_simulation_time_in_ms (private-kernel-reset-small): 308 (-39%)
  • avm_simulation_time_ms (Token:mint_public): 77.3 (+69%)
  • avm_simulation_time_ms (Token:assert_minter_and_mint): 42.5 (-36%)
  • avm_simulation_time_ms (Benchmarking:increment_balance): 1,202 (+28%)
  • protocol_circuit_witness_generation_time_in_ms (private-kernel-reset-tiny): 726 (-18%)
  • l2_block_building_time_in_ms (4): 11,457 (+26%)
  • l2_block_building_time_in_ms (8): 23,072 (+31%)
  • l2_block_building_time_in_ms (16): 44,725 (+27%)
  • l2_block_rollup_simulation_time_in_ms (4): 11,457 (+26%)
  • l2_block_rollup_simulation_time_in_ms (8): 23,072 (+31%)
  • l2_block_rollup_simulation_time_in_ms (16): 44,725 (+27%)
  • l2_block_public_tx_process_time_in_ms (4): 9,714 (+27%)
  • l2_block_public_tx_process_time_in_ms (8): 21,252 (+32%)
  • l2_block_public_tx_process_time_in_ms (16): 42,873 (+27%)
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.

Proof generation

Each column represents the number of threads used in proof generation.

Metric 1 threads 4 threads 16 threads 32 threads 64 threads
proof_construction_time_sha256_ms 5,772 1,561 (-1%) 714 762 (+1%) 779 (+1%)
proof_construction_time_sha256_30_ms 11,508 3,101 1,390 (+1%) 1,433 (+1%) 1,471 (+1%)
proof_construction_time_sha256_100_ms 44,285 11,868 5,417 5,395 (-4%) 5,845 (+3%)
proof_construction_time_poseidon_hash_ms 79.0 34.0 34.0 58.0 (-2%) 88.0 (-1%)
proof_construction_time_poseidon_hash_30_ms 1,535 423 204 228 273 (-1%)
proof_construction_time_poseidon_hash_100_ms 5,686 1,521 674 724 (-3%) 746

L2 block published to L1

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

Metric 4 txs 8 txs 16 txs
l1_rollup_calldata_size_in_bytes 4,324 7,844 14,852
l1_rollup_calldata_gas 49,744 92,444 177,380
l1_rollup_execution_gas 1,373,772 2,107,368 3,892,360
l2_block_processing_time_in_ms 248 (-1%) 452 (+1%) 791 (-2%)
l2_block_building_time_in_ms ⚠️ 11,457 (+26%) ⚠️ 23,072 (+31%) ⚠️ 44,725 (+27%)
l2_block_rollup_simulation_time_in_ms ⚠️ 11,457 (+26%) ⚠️ 23,072 (+31%) ⚠️ 44,725 (+27%)
l2_block_public_tx_process_time_in_ms ⚠️ 9,714 (+27%) ⚠️ 21,252 (+32%) ⚠️ 42,873 (+27%)

L2 chain processing

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

Metric 3 blocks 5 blocks
node_history_sync_time_in_ms 2,928 (+2%) 3,939
node_database_size_in_bytes 12,648,528 16,728,144
pxe_database_size_in_bytes 16,254 26,813

Circuits stats

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

Circuit simulation_time_in_ms witness_generation_time_in_ms input_size_in_bytes output_size_in_bytes proving_time_in_ms
private-kernel-init 89.4 399 21,755 44,860 N/A
private-kernel-inner 167 (-1%) 708 (-1%) 72,566 45,007 N/A
private-kernel-reset-tiny ⚠️ 307 (-34%) ⚠️ 726 (-18%) 65,590 44,846 N/A
private-kernel-tail 163 (-17%) 136 (-15%) 50,643 52,257 N/A
base-parity 5.62 N/A 160 96.0 N/A
root-parity 35.4 (-1%) N/A 73,948 96.0 N/A
base-rollup ⚠️ 3,543 (+28%) N/A 189,136 664 N/A
root-rollup 40.7 N/A 58,173 716 N/A
public-kernel-setup 83.9 (-1%) N/A 105,085 71,222 N/A
public-kernel-app-logic 99.0 (+3%) N/A 104,911 71,222 N/A
public-kernel-tail ⚠️ 891 (+60%) N/A 390,582 (-5%) 16,414 N/A
private-kernel-reset-small ⚠️ 308 (-39%) N/A 66,341 45,629 N/A
private-kernel-tail-to-public 1,386 618 (-6%) 455,020 (-1%) 1,825 N/A
public-kernel-teardown 83.5 N/A 105,349 71,222 N/A
merge-rollup 20.4 (-3%) N/A 38,174 664 N/A
undefined N/A N/A N/A N/A 81,272 (+3%)

Stats on running time collected for app circuits

Function input_size_in_bytes output_size_in_bytes witness_generation_time_in_ms
ContractClassRegisterer:register 1,344 11,731 346
ContractInstanceDeployer:deploy 1,408 11,731 18.4
MultiCallEntrypoint:entrypoint 1,920 11,731 408
FeeJuice:deploy 1,376 11,731 389 (-2%)
SchnorrAccount:constructor 1,312 11,731 75.8 (+1%)
SchnorrAccount:entrypoint 2,304 11,731 414
Token:privately_mint_private_note 1,280 11,731 108 (+6%)
FPC:fee_entrypoint_public 1,344 11,731 28.4 (-9%)
Token:transfer 1,312 11,731 243 (+4%)
Benchmarking:create_note 1,344 11,731 85.7 (-3%)
SchnorrAccount:verify_private_authwit 1,280 11,731 27.6 (-11%)
Token:unshield 1,376 11,731 527
FPC:fee_entrypoint_private 1,376 11,731 703 (-2%)

AVM Simulation

Time to simulate various public functions in the AVM.

Function time_ms bytecode_size_in_bytes
FeeJuice:_increase_public_balance 54.8 (-1%) 7,739
FeeJuice:set_portal 12.0 (-3%) 2,354
Token:constructor 81.9 26,051
FPC:constructor 53.5 18,001
FeeJuice:mint_public 39.4 (-1%) 5,877
Token:mint_public ⚠️ 77.3 (+69%) 10,917
Token:assert_minter_and_mint ⚠️ 42.5 (-36%) 7,512
AuthRegistry:set_authorized 47.0 4,391
FPC:prepare_fee 234 (+1%) 7,043
Token:transfer_public 25.5 (-19%) 39,426
FPC:pay_refund 55.1 (-17%) 10,234
Benchmarking:increment_balance ⚠️ 1,202 (+28%) 6,563
Token:_increase_public_balance 40.4 (-1%) 8,433
FPC:pay_refund_with_shielded_rebate 63.3 (+1%) 10,783

Public DB Access

Time to access various public DBs.

Function time_ms
get-nullifier-index 0.164

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 256 leaves 512 leaves 1024 leaves
batch_insert_into_append_only_tree_16_depth_ms 2.23 (+1%) 3.96 (+2%) N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.8 31.7 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.116 (+1%) 0.111 (+1%) N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A 11.4 (-2%) 17.9 31.2 (+1%) 60.2 (+1%) 113
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A 95.9 159 287 543 1,055
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A 0.110 (-2%) 0.104 0.101 (+1%) 0.103 (+1%) 0.101
batch_insert_into_indexed_tree_20_depth_ms N/A N/A 14.4 (-2%) 26.1 (+1%) 44.1 86.3 (+3%) 161
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A 109 207 355 691 1,363
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A 0.109 (-2%) 0.106 (+1%) 0.107 0.106 (+3%) 0.102 (+1%)
batch_insert_into_indexed_tree_40_depth_ms N/A N/A 17.0 (+3%) N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A 132 N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A 0.108 (+2%) N/A N/A N/A N/A

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 64,779 668,997

Transaction size based on fee payment method

| Metric | |
| - | |

// we're in a run, so this container must have a lower counter.
// note we don't check for overflow here, as the run_lengths array must be correct.
assert(
current_container.counter() <= original_array[i + 1].counter(), "Containers in a run must be sorted by counter"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old code only checked that items in each run meet the requirement - same position with incrementing counters. But we can split items of the same position into two runs and it would pass.

TestItem { value: 33, counter: 2 },
TestItem::empty(),
TestItem::empty()
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous code would reverse the ordering of items with the same counter. The result would be:

[
    TestItem { value: 22, counter: 0 },
    TestItem { value: 44, counter: 0 },
    TestItem { value: 11, counter: 0 },
    TestItem { value: 55, counter: 1 },
    TestItem { value: 33, counter: 2 },
    TestItem::empty(),
    TestItem::empty()
]

Which would be wrong when sorting an array that has values emitted from private.

@LeilaWang LeilaWang marked this pull request as ready for review August 22, 2024 10:03
Copy link
Contributor

@MirandaWood MirandaWood left a comment

Choose a reason for hiding this comment

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

Great! 🚀

Comment on lines +17 to +20
// Ensure the array is sorted
// for i in 0..N - 1 {
// assert(ordering(result[i], result[i + 1]));
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this because of the assert and get_sorting_index above, IIUC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! And this additional check prevents features that require keeping ordering unchanged. For example, in public land, the final array can contain items with 0 counter (emitted from private), and items with non-zero counter (emitted from public). We don't want to change the ordering of the items with 0 counter because they were already sorted in private.

So we need ordering(b, a) (run in get_sorting_index) to be false so the two items won't be swapped. But then it will fail the check if we apply the ordering again after they are "sorted".

And this is only used by unconstrained functions. We always use other constrained functions to check if the results are sorted correctly.

@LeilaWang LeilaWang merged commit 576e217 into master Aug 23, 2024
97 checks passed
@LeilaWang LeilaWang deleted the lw/public_kernel_refactoring branch August 23, 2024 17:13
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