feat: destroy_note_unsafe

[benesjan]

Fixes #7831

Gate diff: 82803 - 78619 = 4184 gates saved

[benesjan]

• feat: destroy_note_unsafe #7891 👈
• 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]

Benchmark results

Metrics with a significant change:

• proof_construction_time_sha256_100_ms (16): 6,630 (+22%)
• protocol_circuit_simulation_time_in_ms (private-kernel-tail-to-public): 846 (-43%)
• avm_simulation_time_ms (Token:mint_public): 81.3 (+68%)
• avm_simulation_time_ms (Token:assert_minter_and_mint): 39.1 (-48%)
• avm_simulation_time_ms (Token:transfer_public): 36.2 (+70%)

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,787	1,553 (-1%)	708 (+1%)	748 (-2%)	772 (+1%)
proof_construction_time_sha256_30_ms	11,843	3,178	1,413	1,457 (+2%)	1,471 (+1%)
proof_construction_time_sha256_100_ms	45,349 (-1%)	12,058	⚠️ 6,630 (+22%)	5,394	5,371
proof_construction_time_poseidon_hash_ms	78.0	35.0 (+3%)	35.0 (+3%)	59.0 (+2%)	88.0 (-1%)
proof_construction_time_poseidon_hash_30_ms	1,529	422	204 (+1%)	229 (-1%)	268 (-3%)
proof_construction_time_poseidon_hash_100_ms	5,646	1,519	676	732 (+2%)	749 (+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,672	92,492	177,608
l1_rollup_execution_gas	1,294,335	2,042,123	3,869,278
l2_block_processing_time_in_ms	259	450 (+2%)	802 (-2%)
l2_block_building_time_in_ms	9,293	18,240 (+1%)	36,166
l2_block_rollup_simulation_time_in_ms	9,292	18,240 (+1%)	36,166
l2_block_public_tx_process_time_in_ms	7,843	16,678 (+1%)	34,537

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,127 (+6%)	3,865 (+1%)
node_database_size_in_bytes	12,755,024	16,842,832
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	101 (+1%)	394 (-3%)	21,846	44,858	N/A
private-kernel-inner	169	710 (+2%)	72,545	45,005	N/A
private-kernel-reset-tiny	501 (-1%)	1,200 (+1%)	69,614	44,844	N/A
private-kernel-tail	270	195 (-1%)	50,760	52,256	N/A
base-parity	5.53	N/A	160	96.0	N/A
root-parity	33.5 (+2%)	N/A	69,084	96.0	N/A
base-rollup	2,870	N/A	187,817	664	N/A
root-rollup	38.5	N/A	54,525	716	N/A
public-kernel-setup	99.5 (-1%)	N/A	103,760	71,222	N/A
public-kernel-app-logic	106 (+1%)	N/A	103,599	71,222	N/A
public-kernel-tail	572 (+1%)	N/A	409,190	16,414	N/A
private-kernel-reset-small	482	N/A	66,533	45,629	N/A
private-kernel-tail-to-public	⚠️ 846 (-43%)	707	507,554	1,697	N/A
public-kernel-teardown	94.0	N/A	104,005	71,222	N/A
merge-rollup	19.5 (+2%)	N/A	35,742	664	N/A
undefined	N/A	N/A	N/A	N/A	68,683 (+2%)

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	345
ContractInstanceDeployer:deploy	1,408	11,731	18.1 (-1%)
MultiCallEntrypoint:entrypoint	1,920	11,731	458
FeeJuice:deploy	1,376	11,731	387
SchnorrAccount:constructor	1,312	11,731	170
SchnorrAccount:entrypoint	2,304	11,731	507
Token:privately_mint_private_note	1,280	11,731	215 (-1%)
FPC:fee_entrypoint_public	1,344	11,731	21.1 (-8%)
Token:transfer	1,312	11,731	401 (-2%)
Benchmarking:create_note	1,344	11,731	164
SchnorrAccount:verify_private_authwit	1,280	11,731	27.4 (-2%)
Token:unshield	1,376	11,731	636 (+1%)
FPC:fee_entrypoint_private	1,376	11,731	860 (+1%)

AVM Simulation

Time to simulate various public functions in the AVM.

Function	time_ms	bytecode_size_in_bytes
FeeJuice:_increase_public_balance	55.3 (+1%)	8,139
FeeJuice:set_portal	10.7 (+5%)	2,362
Token:constructor	85.6 (+3%)	31,107
FPC:constructor	53.5 (-1%)	22,380
FeeJuice:mint_public	37.7	6,150
Token:mint_public	⚠️ 81.3 (+68%)	11,720
Token:assert_minter_and_mint	⚠️ 39.1 (-48%)	8,028
AuthRegistry:set_authorized	45.7 (+1%)	4,537
FPC:prepare_fee	262 (+9%)	8,812
Token:transfer_public	⚠️ 36.2 (+70%)	47,374
FPC:pay_refund	60.1 (-4%)	12,114
Benchmarking:increment_balance	975 (+1%)	7,450
Token:_increase_public_balance	40.3 (+2%)	8,960
FPC:pay_refund_with_shielded_rebate	70.1 (+4%)	12,663

Public DB Access

Time to access various public DBs.

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

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.17	3.92 (+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.112	0.111 (+2%)	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_32_depth_ms	N/A	N/A	11.5	18.2 (+3%)	31.8	58.8	113 (-1%)
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	0.105 (+2%)	0.103	0.101	0.101 (-1%)
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	14.4	26.0 (+1%)	43.4 (-2%)	82.8 (+2%)	160 (-4%)
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.105 (+1%)	0.105 (-3%)	0.102	0.101 (-4%)
batch_insert_into_indexed_tree_40_depth_ms	N/A	N/A	16.4	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	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]

just cleaned up imports here

[benesjan]

The logic here would be significantly less twisted if we decided to drop this property - then we would not need to silo the note hash here (this would also save some gates).

I think this should be considered because I feel the usecase has too many limitations to really be practical.

[nventuro]

I went over the comments in that test a bunch of times but am afraid I still don't understand what property they refer to. I'm not even sure if they're talking about address siloing or nonce siloing. So it may be that what I write below is the same as what you're thinking, but I'll leave it here anyway in case it's useful to reference in the future.

I discussed this a little bit with @LeilaWang, and we arrived at a couple of useful conclusions. The real reason why we need multiple kinds of nullif hashes is because the kernel needs a 'hint' as to whether the new nullifier is going to result in the note + nullif being squashed or not. There are multiple reasons why squashing might not happen (e.g. non revertible notes), but there's a single scenario in which we can squash: a revertible transient note (i.e. zero nonce, non-zero note hash counter).

It seems additionally like there is also no need to do contract-siloing of the nullifier - if desired the kernel could always add this later. I'm not sure if this is the kind of siloing you were referring to in this comment?

Given the above, and if we can drop the address siloing, then all we need to do is identify the odd case in which the note is transient (with a non-zero hash counter), and so the read request does not include the nonce, but also non-revertible (the nonce is also non-zero!), and only in this case extend the note hash for read request with the nonce. I'm not sure if this also requires knowing if the nullifier is revertible or not.

[benesjan]

I'm not sure if this is the kind of siloing you were referring to in this comment?

Yes, we have a terminology issue here as some people use "siloing" to refer to hashing a hash with a contract address and other people use it to refer to hashing of a hash with whatever else (e.g. siloing with a nonce).

As @iAmMichaelConnor originally coined it it used to mean hashing of hash with contract address and that's what should be used if we don't want the naming to be 💩 (otherwise what is siloed_note_hash?)

additionally like there is also no need to do contract-siloing of the nullifier
I was referring to the siloing of the note_hash when computing note_hash_for_nullifiy here. The valuable property of this is that when we do the siloing there then a note nullifier can be derived just by having the nsk_app and observing note_hashes inserted into the note_hash_tree. This allows a 3rd party to detect that a note has been nullified just by having the nsk_app and watching for new. nullifiers being inserted. See this function for how exactly this works. The important property is that the note hash as it is inserted to a note hash tree is part of the preimage of the nullifier as it allows for brute-forcing the nullifiers from public info.

If we decided to drop this property we could simplify the compute_note_hash_for_nullify func. Which as I pointed out might be worthwhile because IDK if the "nullifier detection property" is valuable enough given that there are case which are hard to detect (like e.g. the DeFi withdrawing case) and it costs gates.

But anyway it feels like we should keep the property for now as we seem to be doing fine with the gate counts.

[nventuro]

Left some comments re. the note lifecycle, I don't feel like I understand this well enough to provide much useful feedback. Perhaps we should spend some time understanding and explaining this topic better so that we can improve NoteInterface, Header etc.

[nventuro]

I went over the comments in that test a bunch of times but am afraid I still don't understand what property they refer to. I'm not even sure if they're talking about address siloing or nonce siloing. So it may be that what I write below is the same as what you're thinking, but I'll leave it here anyway in case it's useful to reference in the future.

I discussed this a little bit with @LeilaWang, and we arrived at a couple of useful conclusions. The real reason why we need multiple kinds of nullif hashes is because the kernel needs a 'hint' as to whether the new nullifier is going to result in the note + nullif being squashed or not. There are multiple reasons why squashing might not happen (e.g. non revertible notes), but there's a single scenario in which we can squash: a revertible transient note (i.e. zero nonce, non-zero note hash counter).

It seems additionally like there is also no need to do contract-siloing of the nullifier - if desired the kernel could always add this later. I'm not sure if this is the kind of siloing you were referring to in this comment?

Given the above, and if we can drop the address siloing, then all we need to do is identify the odd case in which the note is transient (with a non-zero hash counter), and so the read request does not include the nonce, but also non-revertible (the nonce is also non-zero!), and only in this case extend the note hash for read request with the nonce. I'm not sure if this also requires knowing if the nullifier is revertible or not.

[AztecBot]

Docs Preview

Hey there! 👋 You can check your preview at https://66bc9ac894302c861f44fb48--aztec-docs-dev.netlify.app

[nventuro]

Let's go forward with this so that the new structure becomes more obvious and we get the expected gate counts, but we'll want to revisit all of note interface, hashing etc very soon, since it's getting out of control.