fix: shared mutable private getter delay change fix - failing test by sklppy88 · Pull Request #6654 · AztecProtocol/aztec-packages

sklppy88 · 2024-05-23T23:28:56Z

Adding another state vars test that fails

sklppy88 · 2024-05-23T23:29:05Z

fix: shared mutable private getter delay change fix - failing test #6654 : 2 dependent PRs (#6655 , #6656 ) 👈
fix: shared mutable private getter fixes #6652 : 1 other dependent PR (#6653 )
master

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

Join @sklppy88 and the rest of your teammates on Graphite

LHerskind

Minor changes suggested, comments and a view, lets merge #6655 into this to have each member of the stack pass 👍

LHerskind · 2024-05-24T12:30:17Z

    }

+    #[aztec(public)]
+    fn get_authorized_delay() -> u32 {


Being a bit of a pain in the ass, but this one can be a view as well 👀

#[aztec(view)]

Good point, have addressed for both you mentioned and any potential others.

LHerskind · 2024-05-24T12:34:24Z

      expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(new Fr(expectedMaxBlockNumber));
    });
+
+    it('checks max block number propagated by reading with our SharedMutablePrivateGetter in TestContract accessing AuthContract; before and after a decreasing then increasing delay change', async () => {


Would prefer a shorter test name here. Not sure we need to be told exactly how, but more what it is testing, should be ok with

it('check propagation of max block number in private', async () => {

LHerskind · 2024-05-24T12:36:45Z

+      // We change the SharedMutable authorized delay here to 100, this means that a change to the "authorized" value can
+      // only be applied 100 blocks after it is initiated, and thus read requests on a historical state without an initiated change is
+      // valid for at least 100 blocks
+      await authContract.methods.set_authorized_delay(2).send().wait();


The comment is saying 100, but the code is saying 2 👀

You are first shortening the delay, and then extending it? Is there a special case when we do both of these, or would it be the same if two tests? One that is extending and one reducing?

Ah yep, fixed. Also added a note for why we delay 4 below.

Would be the same if it is in two tests, and its nicer to go short -> long, as long -> short has a dependency on the previous delay, and also just a bit easier to show peculiarities within the whole flow as one test.

LHerskind · 2024-05-24T12:38:00Z

+      // We now call our AuthContract to see if the change in max block number has reflected our delay change
+      const tx2 = await authContract.methods.get_authorized_in_private().prove();
+
+      // The validity of our SharedMutable read request should now be limited to 100 blocks, instead of 5


Stale comment here

LHerskind · 2024-05-24T12:39:24Z

+        new Fr(expectedModifiedMaxBlockNumber),
+      );
+
+      // We access the same SharedMutable from our SharedMutablePrivateGetter, expecting the validity assumptions to remain the same.


Checking parity between our fancy getter and the direct call? 👀

LHerskind · 2024-05-24T12:54:06Z

+      // We now call our AuthContract to see if the change in max block number has reflected our delay change
+      const tx4 = await authContract.methods.get_authorized_in_private().prove();
+
+      // The validity of our SharedMutable read request should now be limited to 100 blocks, instead of 5


Stale comment.

This one is always a little funky to me.
So you reduced it to 2, and now you are increasing it 100.

The reduction needs to wait for the full execution before it is "happy", but for the extension it can take effect more quickly where there is not a pending value change 🤔

Yeah to me as well. Have added a comment there making this difference explicit

AztecBot · 2024-05-24T15:41:39Z

Benchmark results

Metrics with a significant change:

protocol_circuit_proving_time_in_ms (private-kernel-reset-small): 48,868 (+17%)
protocol_circuit_proving_time_in_ms (private-kernel-tail-to-public): 96,077 (+18%)
protocol_circuit_proving_time_in_ms (private-kernel-tail): 45,060 (+24%)
protocol_circuit_proving_time_in_ms (base-rollup): 87,167 (+18%)
protocol_circuit_proving_time_in_ms (root-parity): 51,349 (+17%)
protocol_circuit_proving_time_in_ms (public-kernel-tail): 191,597 (+24%)
protocol_circuit_proving_time_in_ms (root-rollup): 22,970 (+20%)

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	5,682 (-2%)	1,554 (-1%)	711	754 (-1%)	783 (+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	64 txs
l1_rollup_calldata_size_in_bytes	1,412	1,412	1,412
l1_rollup_calldata_gas	9,464	9,464	9,464
l1_rollup_execution_gas	616,105	616,105	616,105
l2_block_processing_time_in_ms	1,359 (-2%)	5,081 (-3%)	10,056 (-2%)
l2_block_building_time_in_ms	46,830 (-2%)	184,901 (-3%)	370,440 (-3%)
l2_block_rollup_simulation_time_in_ms	46,656 (-2%)	184,268 (-3%)	369,180 (-3%)
l2_block_public_tx_process_time_in_ms	25,006 (-5%)	103,840 (-5%)	212,724 (-3%)

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,963	15,216
node_database_size_in_bytes	14,487,632	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	163 (-1%)	3,546 (-5%)	25,304 (+13%)	20,630	64,614	89,536	2,731	1,048,576
private-kernel-inner	624 (-1%)	4,056 (-6%)	52,410 (+8%)	92,318	64,614	89,536	2,731	2,097,152
private-kernel-tail	546 (-2%)	2,768 (-6%)	⚠️ 45,060 (+24%)	90,237	77,498	10,656	266	2,097,152
base-parity	6.44 (-1%)	1,288 (-3%)	2,892 (-1%)	128	64.0	2,208	2.00	131,072
root-parity	48.9 (-3%)	64.8 (+1%)	⚠️ 51,349 (+17%)	27,084	64.0	2,720	18.0	2,097,152
base-rollup	778 (-3%)	2,461 (+5%)	⚠️ 87,167 (+18%)	119,610	756	3,648	47.0	4,194,304
root-rollup	112 (-3%)	79.3 (+21%)	⚠️ 22,970 (+20%)	25,297	620	3,456	41.0	1,048,576
public-kernel-app-logic	522 (-2%)	2,850 (-1%)	51,714 (+9%)	104,941	86,302	114,784	3,520	2,097,152
public-kernel-tail	1,185 (-1%)	24,887 (+5%)	⚠️ 191,597 (+24%)	395,386	7,522	10,656	266	8,388,608
private-kernel-reset-small	591 (-1%)	2,030 (-8%)	⚠️ 48,868 (+17%)	120,733	64,614	89,536	2,731	2,097,152
merge-rollup	28.4 (-1%)	N/A	N/A	16,534	756	N/A	N/A	N/A
public-kernel-setup	634 (+2%)	N/A	N/A	104,941	86,302	N/A	N/A	N/A
public-kernel-teardown	536 (+2%)	N/A	N/A	104,941	86,302	N/A	N/A	N/A
private-kernel-tail-to-public	N/A	8,154 (-6%)	⚠️ 96,077 (+18%)	N/A	N/A	114,784	3,520	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	468 (-2%)	N/A	N/A	N/A	N/A
ContractInstanceDeployer:deploy	1,408	9,944	41.3 (-3%)	N/A	N/A	N/A	N/A
MultiCallEntrypoint:entrypoint	1,920	9,944	1,435 (-2%)	N/A	N/A	N/A	N/A
SchnorrAccount:constructor	1,312	9,944	1,007 (-2%)	N/A	N/A	N/A	N/A
SchnorrAccount:entrypoint	2,304	9,944	2,129	16,768	50,774 (+10%)	2,097,152	457
Token:privately_mint_private_note	1,280	9,944	1,115 (-5%)	N/A	N/A	N/A	N/A
Token:transfer	1,376	9,944	4,049 (-2%)	16,768	59,284 (+14%)	2,097,152	457
Benchmarking:create_note	1,312	9,944	972 (-1%)	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_public	1,344	9,944	230 (+1%)	N/A	N/A	N/A	N/A
SchnorrAccount:spend_private_authwit	1,280	9,944	77.2	N/A	N/A	N/A	N/A
Token:unshield	1,376	9,944	3,291	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_private	1,376	9,944	4,060	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	11.1 (-3%)	19.2 (+3%)	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.644 (-3%)	0.587 (+3%)	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	51.9 (-1%)	81.3 (-1%)	262 (-2%)	508 (+1%)	990 (-2%)	1,959 (-2%)	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.530 (-1%)	0.501 (-1%)	0.476 (-2%)	0.474	0.469 (-2%)	0.468 (-1%)	N/A
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	61.9 (-2%)	119 (-2%)	379 (-2%)	742	1,474 (-2%)	2,927 (-2%)	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.537 (-2%)	0.535 (-1%)	0.515 (-2%)	0.511	0.510 (-2%)	0.509 (-2%)	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	66.2 (-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.588 (-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	83,794	665,117

Transaction size based on fee payment method

| Metric | |
| - | |

sklppy88 changed the title ~~Initial~~ fix: shared mutable private getter delay change fix - failing test May 23, 2024

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch from 95f13c1 to 50c4550 Compare May 23, 2024 23:38

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-context-fix-failing-test-mitigation branch from ac77790 to 145deec Compare May 24, 2024 01:08

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch from 50c4550 to 90d9a67 Compare May 24, 2024 01:08

sklppy88 mentioned this pull request May 24, 2024

fix: adding encrypted logs test #6662

Closed

Base automatically changed from ek/fix/shared-mutable-private-getter-context-fix-failing-test-mitigation to ek/fix/shared-mutable-private-getter-context-fix-failing-test May 24, 2024 12:54

LHerskind approved these changes May 24, 2024

View reviewed changes

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-context-fix-failing-test branch from 7ce1f9f to 9e276ec Compare May 24, 2024 13:15

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch from 20fb637 to 4443965 Compare May 24, 2024 13:31

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-context-fix-failing-test branch from 9e276ec to 4ce1f0e Compare May 24, 2024 13:38

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch 3 times, most recently from bf7019a to f094691 Compare May 24, 2024 14:49

Initial

511478a

sklppy88 force-pushed the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch from f094691 to 511478a Compare May 24, 2024 15:07

sklppy88 marked this pull request as ready for review May 24, 2024 15:15

sklppy88 merged commit f6cbf00 into ek/fix/shared-mutable-private-getter-context-fix-failing-test May 24, 2024

sklppy88 deleted the ek/fix/shared-mutable-private-getter-delay-change-fix-failing-test branch May 24, 2024 15:23

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

LHerskind left a comment

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!

Choose a reason for hiding this comment

Uh oh!

AztecBot commented May 24, 2024

Benchmark results

Proof generation

L2 block published to L1

L2 chain processing

Circuits stats

Tree insertion stats

Miscellaneous

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sklppy88 commented May 23, 2024 •

edited

Loading

sklppy88 commented May 23, 2024 •

edited

Loading