feat: meaningful outgoing by benesjan · Pull Request #6560 · AztecProtocol/aztec-packages

benesjan · 2024-05-21T12:28:13Z

Fixes #6410

benesjan · 2024-05-21T12:28:33Z

refactor: nuking broadcast param #6741
refactor: cleanup after outgoing #6736
feat: meaningful outgoing #6560 👈
master

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

Join @benesjan and the rest of your teammates on Graphite

AztecBot · 2024-05-23T14:21:11Z

Benchmark results

Metrics with a significant change:

app_circuit_witness_generation_time_in_ms (MultiCallEntrypoint:entrypoint): 1,806 (+25%)
app_circuit_witness_generation_time_in_ms (SchnorrAccount:constructor): 1,466 (+48%)
app_circuit_witness_generation_time_in_ms (SchnorrAccount:entrypoint): 2,680 (+28%)
app_circuit_witness_generation_time_in_ms (Token:privately_mint_private_note): 1,625 (+42%)
app_circuit_witness_generation_time_in_ms (Token:transfer): 5,008 (+23%)
app_circuit_witness_generation_time_in_ms (Benchmarking:create_note): 1,446 (+53%)

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 | |
| - | |

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	1,412	1,412	1,412
l1_rollup_calldata_gas	9,452	9,476	9,476
l1_rollup_execution_gas	616,093	616,117	616,117
l2_block_processing_time_in_ms	1,292	4,838 (+1%)	9,721 (+2%)
l2_block_building_time_in_ms	47,691 (+6%)	195,716 (+9%)	391,010 (+9%)
l2_block_rollup_simulation_time_in_ms	47,519 (+6%)	195,081 (+9%)	389,749 (+9%)
l2_block_public_tx_process_time_in_ms	25,971 (+7%)	112,490 (+11%)	229,101 (+11%)

L2 chain processing

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

Metric	3 blocks	5 blocks
node_history_sync_time_in_ms	9,514	14,471
node_database_size_in_bytes	14,483,536	21,385,296
pxe_database_size_in_bytes	18,071	29,868

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	proving_time_in_ms	input_size_in_bytes	output_size_in_bytes	proof_size_in_bytes	num_public_inputs	size_in_gates
private-kernel-init	174 (+9%)	3,716	22,293 (+1%)	20,630	64,614	89,536	2,731	1,048,576
private-kernel-inner	655 (+6%)	4,283 (-3%)	46,082 (+9%)	92,318	64,614	89,536	2,731	2,097,152
private-kernel-tail	586 (+4%)	2,923 (-3%)	35,454 (-2%)	96,541	77,732	11,648	297	2,097,152
base-parity	6.67 (+2%)	1,174 (-1%)	2,933 (+1%)	128	64.0	2,208	2.00	131,072
root-parity	50.9 (+2%)	51.5 (-2%)	44,946 (+2%)	27,084	64.0	2,720	18.0	2,097,152
base-rollup	799 (+2%)	2,321 (+1%)	74,710	119,734	756	3,648	47.0	4,194,304
root-rollup	112	67.3 (+7%)	19,446	25,297	620	3,456	41.0	1,048,576
public-kernel-app-logic	562 (+6%)	3,188	48,319	105,253	86,550	116,768	3,582	2,097,152
public-kernel-tail	1,155 (+4%)	22,464 (-7%)	155,375 (+1%)	401,002	7,646	11,648	297	8,388,608
private-kernel-reset-small	627 (+7%)	2,124 (+1%)	44,925 (-1%)	120,733	64,614	89,536	2,731	2,097,152
merge-rollup	28.8 (+1%)	N/A	N/A	16,534	756	N/A	N/A	N/A
public-kernel-setup	633	N/A	N/A	105,253	86,550	N/A	N/A	N/A
public-kernel-teardown	536	N/A	N/A	105,253	86,550	N/A	N/A	N/A
private-kernel-tail-to-public	N/A	8,826 (+2%)	92,704 (+2%)	N/A	N/A	116,768	3,582	4,194,304

Stats on running time collected for app circuits

Function	input_size_in_bytes	output_size_in_bytes	witness_generation_time_in_ms	proof_size_in_bytes	proving_time_in_ms	size_in_gates	num_public_inputs
ContractClassRegisterer:register	1,344	9,944	510 (+9%)	N/A	N/A	N/A	N/A
ContractInstanceDeployer:deploy	1,408	9,944	44.6 (+6%)	N/A	N/A	N/A	N/A
MultiCallEntrypoint:entrypoint	1,920	9,944	⚠️ 1,806 (+25%)	N/A	N/A	N/A	N/A
SchnorrAccount:constructor	1,312	9,944	⚠️ 1,466 (+48%)	N/A	N/A	N/A	N/A
SchnorrAccount:entrypoint	2,304	9,944	⚠️ 2,680 (+28%)	16,768	51,206	2,097,152	457
Token:privately_mint_private_note	1,280	9,944	⚠️ 1,625 (+42%)	N/A	N/A	N/A	N/A
Token:transfer	1,376	9,944	⚠️ 5,008 (+23%)	16,768	52,133 (+14%)	2,097,152	457
Benchmarking:create_note	1,344 (+2%)	9,944	⚠️ 1,446 (+53%)	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_public	1,344	9,944	225	N/A	N/A	N/A	N/A
SchnorrAccount:spend_private_authwit	1,280	9,944	76.7 (-1%)	N/A	N/A	N/A	N/A
Token:unshield	1,376	9,944	3,693 (+13%)	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_private	1,376	9,944	4,573 (+13%)	N/A	N/A	N/A	N/A

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	10.7 (+2%)	17.2 (+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.7	31.8	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.621 (+2%)	0.528 (+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	49.4 (+2%)	76.2	247	476 (-1%)	930	1,839	N/A
batch_insert_into_append_only_tree_32_depth_hash_count	N/A	N/A	95.9	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.505 (+2%)	0.469	0.448	0.444	0.440	0.439	N/A
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	59.1 (+1%)	113	357 (+1%)	703	1,388	2,762	N/A
batch_insert_into_indexed_tree_20_depth_hash_count	N/A	N/A	106	208	692	1,363	2,707	5,395	N/A
batch_insert_into_indexed_tree_20_depth_hash_ms	N/A	N/A	0.512 (+1%)	0.504	0.483 (+1%)	0.482	0.479	0.479	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	63.1 (+1%)
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	107
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.560 (+1%)

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	84,050	665,267

Transaction size based on fee payment method

| Metric | |
| - | |

benesjan · 2024-05-28T18:51:13Z

noir-projects/noir-contracts/contracts/ecdsa_account_contract/src/main.nr

+        // Not emitting outgoing for msg_sender here to not have to register keys for the contract through which we
+        // deploy this (typically MultiCallEntrypoint). I think it's ok here as I feel the outgoing here is not that
+        // important.
+        let this_ovpk_m = header.get_ovpk_m(&mut context, this);


@LHerskind in your original review you wanted me to use msg_sender here as outgoing viewer. After all I decided against it because of the comment above.

I feel like given that it's a constructor it's commonly deployed from non contract address (I think it was throwing address 0 in the tests when I tried to change it) so makes sense to not have them set properly here.

This contract is commonly deployed as the first contract with keys so there is a chicken and egg problem here if we wanted to have an outgoing_viewer arg here.

WDYT?

Likely fine otherwise would pass it as input 🤷. I'm still curious around the chicken and egg problem for fees as well 🐔 🥚

benesjan · 2024-05-28T18:52:19Z

noir-projects/noir-contracts/contracts/schnorr_account_contract/src/main.nr

+        // Not emitting outgoing for msg_sender here to not have to register keys for the contract through which we
+        // deploy this (typically MultiCallEntrypoint). I think it's ok here as I feel the outgoing here is not that
+        // important.
+        let this_ovpk_m = header.get_ovpk_m(&mut context, this);


Same as for the ecdsa account:

"I feel like given that it's a constructor it's commonly deployed from non contract address (I think it was throwing address 0 in the tests when I tried to change it) so makes sense to not have them set properly here."

Same answer.

benesjan · 2024-05-28T18:53:12Z

noir-projects/noir-contracts/contracts/static_parent_contract/src/main.nr


+    // Just like function above but with 3 args.
+    #[aztec(private)]
+    fn private_call_3_args(


Needed to add this because now the private_call func was used with nested functions with different arg lengths.

noir-projects/aztec-nr/easy-private-state/src/easy_private_uint.nr

LHerskind · 2024-05-29T14:15:15Z

noir-projects/noir-contracts/contracts/ecdsa_account_contract/src/main.nr

+        // Not emitting outgoing for msg_sender here to not have to register keys for the contract through which we
+        // deploy this (typically MultiCallEntrypoint). I think it's ok here as I feel the outgoing here is not that
+        // important.
+        let this_ovpk_m = header.get_ovpk_m(&mut context, this);


Likely fine otherwise would pass it as input 🤷. I'm still curious around the chicken and egg problem for fees as well 🐔 🥚

LHerskind · 2024-05-29T14:16:37Z

noir-projects/noir-contracts/contracts/key_registry_contract/src/main.nr

+        ovpk_m_x_registry.schedule_value_change(ovpk_m.x);
+        ovpk_m_y_registry.schedule_value_change(ovpk_m.y);
+        tpk_m_x_registry.schedule_value_change(tpk_m.x);
+        tpk_m_y_registry.schedule_value_change(tpk_m.y);


With this added down here, we could also expose the function at the getter higher up? The reason was that it would never be set but it can be now 🤔

Good idea, will do in a follow up cleanup PR.

LHerskind · 2024-05-29T14:18:56Z

noir-projects/noir-contracts/contracts/schnorr_account_contract/src/main.nr

+        // Not emitting outgoing for msg_sender here to not have to register keys for the contract through which we
+        // deploy this (typically MultiCallEntrypoint). I think it's ok here as I feel the outgoing here is not that
+        // important.
+        let this_ovpk_m = header.get_ovpk_m(&mut context, this);


Same answer.

LHerskind · 2024-05-29T14:25:58Z

noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/balances_map.nr

+impl<T> BalancesMap<T, &mut PrivateContext> {
    pub fn add<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
        self: Self,
+        outgoing_viewer: AztecAddress,


The ordering is going to fuck me up, I guarantee it 😆

LHerskind · 2024-05-29T14:33:13Z

yarn-project/end-to-end/src/e2e_deploy_contract/private_initialization.test.ts

      const owner = await t.registerRandomAccount();
-      const initArgs: StatefulContractCtorArgs = [owner, 42];
+      const outgoingViewer = owner;
+      const initArgs: StatefulContractCtorArgs = [owner, outgoingViewer, 42];


Damn on the test are just using the same values hehe.

LHerskind · 2024-05-29T14:37:03Z

yarn-project/simulator/src/client/private_execution.test.ts

-    oracle.getKeyValidationRequest.mockImplementation((npkMHash: Fr, contractAddress: AztecAddress) => {
-      if (npkMHash.equals(ownerCompleteAddress.publicKeys.masterNullifierPublicKey.hash())) {
+    oracle.getKeyValidationRequest.mockImplementation((pkMHash: Fr, contractAddress: AztecAddress) => {
+      if (pkMHash.equals(ownerCompleteAddress.publicKeys.masterNullifierPublicKey.hash())) {


The way all of this looks make me want to refactor so there is a small map or something to lookup 😆 but I think it can do for the test.

Good point, I will clean this up in my following PR to finally get this one in.

it's actually a pain to do because different functions are called for different branches so

benesjan force-pushed the 05-21-feat_meaningful_outgoing branch 6 times, most recently from 27d18ce to 213e586 Compare May 23, 2024 13:40

benesjan force-pushed the 05-21-feat_meaningful_outgoing branch 2 times, most recently from dfd0779 to d55bb94 Compare May 23, 2024 15:20

benesjan marked this pull request as ready for review May 23, 2024 15:21

benesjan force-pushed the 05-21-feat_meaningful_outgoing branch 6 times, most recently from b7f0e6d to f446654 Compare May 24, 2024 13:38

benesjan added 13 commits May 24, 2024 14:53

feat: meaningful outgoing

c276a90

comment

3210c81

WIP

7a96b22

WIP

e835064

fixed comment

28087fd

fixed test

202cdd9

fix

5e4ad48

WIP

b31532a

WIP

92a3106

correct ovpk in token

708157e

test fix

a2150f5

linking issue

ee43441

MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_CALL workaround

8299b7b

benesjan added 8 commits May 28, 2024 12:25

fixes

0410f9f

fix of fix

57862b7

Merge branch 'master' into 05-21-feat_meaningful_outgoing

acabe40

more fixes

916099d

Merge branch 'master' into 05-21-feat_meaningful_outgoing

ee41c92

Merge branch 'master' into 05-21-feat_meaningful_outgoing

2a35323

fix

ecf0b66

fixes

527d745

benesjan commented May 28, 2024

View reviewed changes

benesjan added 11 commits May 28, 2024 18:59

cleanup

d815c32

making it more ugly

b3d9b1d

test fixes

cac76a0

Merge branch 'master' into 05-21-feat_meaningful_outgoing

7090b4c

fix of fix of fix

7f00ec3

more escrowing

3a4e16d

Merge branch 'master' into 05-21-feat_meaningful_outgoing

7d24304

more changes

87ef714

more changes

6e2a1e6

Merge branch 'master' into 05-21-feat_meaningful_outgoing

601e21e

even more escrowing

7a16f73

benesjan requested a review from LHerskind May 29, 2024 11:35

benesjan mentioned this pull request May 29, 2024

refactor: cleanup after outgoing #6736

Merged

LHerskind approved these changes May 29, 2024

View reviewed changes

benesjan merged commit 3f93757 into master May 29, 2024

benesjan deleted the 05-21-feat_meaningful_outgoing branch May 29, 2024 14:47

AztecBot mentioned this pull request May 29, 2024

chore(master): Release 0.42.0 #6572

Merged

This was referenced May 29, 2024

feat: increasing MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_CALL, removing hack #6739

Merged

refactor: nuking broadcast param #6741

Merged

Conversation

benesjan commented May 21, 2024

Uh oh!

benesjan commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AztecBot commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark results

Proof generation

L2 block published to L1

L2 chain processing

Circuits stats

Tree insertion stats

Miscellaneous

Uh oh!

benesjan May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benesjan commented May 21, 2024 •

edited

Loading

AztecBot commented May 23, 2024 •

edited

Loading

benesjan May 28, 2024 •

edited

Loading