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.

Sign up for GitHub

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

Jump to bottom

refactor: replacing remaining use of pedersen with poseidon #7777

Closed
wants to merge 4 commits into from
Closed

refactor: replacing remaining use of pedersen with poseidon #7777

wants to merge 4 commits into from

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Aug 5, 2024 •
edited
Loading

Replaced the use of pedersen with poseidon in this PR + cleaned up imports (pedersen was imported and unused in a ton of places).

setup_refund before: 52168
setup_refund after: 52324

This was referenced Aug 5, 2024
Merged
Merged
Merged
@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Aug 5, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan mentioned this pull request Aug 5, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 5, 2024 •
edited
Loading

Changes to circuit sizes

Generated at commit: d0ef9f9a2c749db958ac7988fd2a9c6aac9590bc, compared to commit: d01833538027c20f1534c3ec2bbbb28cfbc6815a

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base +1 ❌ +0.00% +566 ❌ +0.01%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base 348,232 (+1) +0.00% 9,077,112 (+566) +0.01%

@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from 02e4602 to 36d305d Compare August 6, 2024 07:18
@benesjan benesjan changed the base branch from 08-05-feat_nuking_unnecessary_siloing to 08-05-refactor_slot_part_of_note_hiding_point August 6, 2024 07:18
@benesjan benesjan force-pushed the 08-05-refactor_slot_part_of_note_hiding_point branch from 806b864 to f5d733e Compare August 6, 2024 07:20
@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from 36d305d to 2ebac2b Compare August 6, 2024 07:20
@benesjan benesjan force-pushed the 08-05-refactor_slot_part_of_note_hiding_point branch from f5d733e to 67fb216 Compare August 6, 2024 07:32
@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from 2ebac2b to 4eab954 Compare August 6, 2024 07:32
benesjan
benesjan commented Aug 6, 2024
View reviewed changes
noir-projects/aztec-nr/aztec/src/note/utils.nr Outdated
// by watching for new note hashes and them hashing them all with nsk_app to derive future nullifiers.
// (See 'nsk_app and contract address are enough to detect note nullification' test case in e2e_keys.test.ts).
// Note: If we would drop the siloing here we would save ~1.2k gates per still a very unoptimized token
// transfer so the savings would not be that great here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated comment I sneaked in.

benesjan
benesjan commented Aug 6, 2024
View reviewed changes
noir-projects/aztec-nr/authwit/src/account.nr
@@ -1,5 +1,4 @@
use dep::aztec::context::{PrivateContext, PublicContext};
use dep::aztec::protocol_types::{address::AztecAddress, abis::function_selector::FunctionSelector, hash::pedersen_hash};
use dep::aztec::context::PrivateContext;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up plenty of imports in this PR.

@benesjan benesjan force-pushed the 08-05-refactor_slot_part_of_note_hiding_point branch from 67fb216 to 32b12df Compare August 6, 2024 09:46
@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from 28721b8 to d4f6262 Compare August 6, 2024 09:46
@benesjan benesjan force-pushed the 08-05-refactor_slot_part_of_note_hiding_point branch from 32b12df to 28299d5 Compare August 6, 2024 12:55
Base automatically changed from 08-05-refactor_slot_part_of_note_hiding_point to master August 6, 2024 14:09
@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from d4f6262 to ddb790e Compare August 13, 2024 08:29
@AztecBot
Copy link
Collaborator

AztecBot commented Aug 13, 2024 •
edited
Loading

Benchmark results

Metrics with a significant change:

  • protocol_circuit_simulation_time_in_ms (base-rollup): 2,338 (-18%)
  • avm_simulation_time_ms (FeeJuice:_increase_public_balance): 82.3 (+55%)
  • avm_simulation_time_ms (FeeJuice:set_portal): 14.7 (+59%)
  • avm_simulation_time_ms (FeeJuice:mint_public): 54.6 (+44%)
  • avm_simulation_time_ms (Token:mint_public): 291 (+576%)
  • avm_simulation_time_ms (Token:assert_minter_and_mint): 50.2 (-35%)
  • avm_simulation_time_ms (AuthRegistry:set_authorized): 66.0 (+47%)
  • avm_simulation_time_ms (FPC:prepare_fee): 326 (+34%)
  • avm_simulation_time_ms (Token:transfer_public): 64.4 (+65%)
  • avm_simulation_time_ms (FPC:pay_refund): 93.6 (+54%)
  • avm_simulation_time_ms (Token:_increase_public_balance): 56.3 (+39%)
  • avm_simulation_bytecode_size_in_bytes (FeeJuice:_increase_public_balance): 37,987 (+367%)
  • avm_simulation_bytecode_size_in_bytes (Token:constructor): 45,928 (+48%)
  • avm_simulation_bytecode_size_in_bytes (FeeJuice:mint_public): 35,928 (+484%)
  • avm_simulation_bytecode_size_in_bytes (Token:mint_public): 56,463 (+382%)
  • avm_simulation_bytecode_size_in_bytes (Token:assert_minter_and_mint): 22,942 (+186%)
  • avm_simulation_bytecode_size_in_bytes (AuthRegistry:set_authorized): 34,179 (+653%)
  • avm_simulation_bytecode_size_in_bytes (Token:transfer_public): 106,922 (+126%)
  • avm_simulation_bytecode_size_in_bytes (Benchmarking:increment_balance): 37,045 (+397%)
  • avm_simulation_bytecode_size_in_bytes (Token:_increase_public_balance): 38,774 (+333%)
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,756 1,555 (-1%) 715 (+1%) 740 (-3%) 766 (+1%)
proof_construction_time_sha256_30_ms 11,816 3,167 (-1%) 1,403 (-1%) 1,441 (-1%) 1,460 (-2%)
proof_construction_time_sha256_100_ms 45,247 (-1%) 12,118 5,416 (-1%) 5,382 (-1%) 5,337 (-1%)
proof_construction_time_poseidon_hash_ms 78.0 34.0 34.0 59.0 (+2%) 88.0 (+1%)
proof_construction_time_poseidon_hash_30_ms 1,530 421 203 218 (-4%) 268 (-3%)
proof_construction_time_poseidon_hash_100_ms 5,636 1,514 678 741 (+2%) 743 (-1%)

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,756 92,478 177,608
l1_rollup_execution_gas 1,294,419 2,042,109 3,869,278
l2_block_processing_time_in_ms 260 (+2%) 444 (-1%) 802 (+2%)
l2_block_building_time_in_ms 9,478 (+2%) 18,399 (+1%) 36,730 (+2%)
l2_block_rollup_simulation_time_in_ms 9,477 (+2%) 18,399 (+1%) 36,730 (+2%)
l2_block_public_tx_process_time_in_ms 8,070 (+3%) 16,972 (+2%) 35,271 (+2%)

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 3,010 (+1%) 3,849
node_database_size_in_bytes 12,750,928 (+1%) 16,838,736
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 100 (+4%) 394 (+1%) 21,846 44,858 N/A
private-kernel-inner 169 (-1%) 705 (+1%) 72,545 45,005 N/A
private-kernel-reset-tiny 505 1,200 (-1%) 69,614 44,844 N/A
private-kernel-tail 270 (-1%) 196 (+2%) 50,760 52,256 N/A
base-parity 5.54 (-3%) N/A 160 96.0 N/A
root-parity 33.0 N/A 69,084 96.0 N/A
base-rollup ⚠️ 2,338 (-18%) N/A 187,817 664 N/A
root-rollup 38.4 N/A 54,525 716 N/A
public-kernel-setup 99.6 (+1%) N/A 103,760 71,222 N/A
public-kernel-app-logic 107 (+1%) N/A 103,599 71,222 N/A
public-kernel-tail 571 N/A 409,190 16,414 N/A
private-kernel-reset-small 486 (+1%) N/A 66,533 45,629 N/A
private-kernel-tail-to-public 1,444 (+9%) 764 (+8%) 507,554 1,697 N/A
public-kernel-teardown 94.3 N/A 104,005 71,222 N/A
merge-rollup 18.9 N/A 35,742 664 N/A
undefined N/A N/A N/A N/A 67,165 (+1%)

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 (+1%)
ContractInstanceDeployer:deploy 1,408 11,731 18.1
MultiCallEntrypoint:entrypoint 1,920 11,731 427 (+2%)
FeeJuice:deploy 1,376 11,731 399 (+4%)
SchnorrAccount:constructor 1,312 11,731 104
SchnorrAccount:entrypoint 2,304 11,731 443 (+3%)
Token:privately_mint_private_note 1,280 11,731 143 (+3%)
FPC:fee_entrypoint_public 1,344 11,731 29.6 (+11%)
Token:transfer 1,312 11,731 286 (-1%)
Benchmarking:create_note 1,344 11,731 101 (-1%)
SchnorrAccount:verify_private_authwit 1,280 11,731 27.9
Token:unshield 1,376 11,731 573 (+2%)
FPC:fee_entrypoint_private 1,376 11,731 767 (+1%)

AVM Simulation

Time to simulate various public functions in the AVM.

Function time_ms bytecode_size_in_bytes
FeeJuice:_increase_public_balance ⚠️ 82.3 (+55%) ⚠️ 37,987 (+367%)
FeeJuice:set_portal ⚠️ 14.7 (+59%) 2,362
Token:constructor 89.3 (+9%) ⚠️ 45,928 (+48%)
FPC:constructor 51.6 (-5%) 22,380
FeeJuice:mint_public ⚠️ 54.6 (+44%) ⚠️ 35,928 (+484%)
Token:mint_public ⚠️ 291 (+576%) ⚠️ 56,463 (+382%)
Token:assert_minter_and_mint ⚠️ 50.2 (-35%) ⚠️ 22,942 (+186%)
AuthRegistry:set_authorized ⚠️ 66.0 (+47%) ⚠️ 34,179 (+653%)
FPC:prepare_fee ⚠️ 326 (+34%) 8,812
Token:transfer_public ⚠️ 64.4 (+65%) ⚠️ 106,922 (+126%)
FPC:pay_refund ⚠️ 93.6 (+54%) 12,114
Benchmarking:increment_balance 1,013 (+4%) ⚠️ 37,045 (+397%)
Token:_increase_public_balance ⚠️ 56.3 (+39%) ⚠️ 38,774 (+333%)
FPC:pay_refund_with_shielded_rebate 82.2 (+25%) 12,663

Public DB Access

Time to access various public DBs.

Function time_ms
get-nullifier-index 0.168 (+2%)

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.19 (+2%) 3.87 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.113 (+2%) 0.109 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A 11.7 (+3%) 17.5 (-1%) 30.8 (+2%) 59.4 112 (+3%)
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.112 (+3%) 0.101 (-1%) 0.100 (+2%) 0.102 0.101 (+2%)
batch_insert_into_indexed_tree_20_depth_ms N/A N/A 14.3 (+1%) 25.5 42.8 82.9 (+1%) 162 (+2%)
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 (+1%) 0.102 0.104 (-1%) 0.103 (+1%) 0.103 (+3%)
batch_insert_into_indexed_tree_40_depth_ms N/A N/A 16.4 (+1%) 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.105 (+1%) 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 | |
| - | |

@benesjan benesjan force-pushed the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch from ddb790e to 454a781 Compare August 15, 2024 08:12
@benesjan
Copy link
Contributor Author

Given that the change to poseidon2 didn't result in gate counts drop I decided to close this PR (I think the drop did not come due to setup costs but not really sure, asked here but got ghosted).

The minor improvements in this PR will be merged in this one.

@benesjan benesjan closed this Aug 15, 2024
@benesjan benesjan deleted the 08-05-refactor_replacing_remaining_use_of_pedersen_with_poseidon branch August 15, 2024 09:12
@benesjan benesjan mentioned this pull request Aug 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benesjan @AztecBot