feat: Private refunds

[just-mitch]

This PR creates a new token contract and fee payment contract that support private refunds.

I.e. Alice pays Bob in private notes, and receives refunds in private notes within the same transaction.

This is a massive improvement over the existing PrivateFeePaymentMethod which uses an un/shield flow, which puts Alice in a never-ending loop of refunding refunds.

Note I suspect we will want to:

1. consolidate this token/fpc, and the other token/fpc
2. and/or create a more general pattern for this type of homomorphic operation

but the exact way forward there is not clear to me yet.

This PR also shows off some of the ugly things we need to do to get this working, like:

• storing notes as a set to avoid mixing in the owner's address when computing the note hash
• storing raw npk hashes in the contracts
• needing to use to_unconstrained to get private balances

This PR also fixes two bugs:

• supporting transactions that only have public teardown
• validation of complete addresses that did not have public keys associated with their deployment

This PR also has the TXE charge nominal TX fees, and basic support for a teardown function.

Side note, see https://hackmd.io/NUfIc2LJRlqL0-myhij3KQ for a cost analysis (in terms of TXEffects byte size) for different fee payment methods.

In conclusion

I vote to merge the PR roughly as is and start the discussion on how to clean the stuff up that we hate, but if someone has strong negative reactions, I'm definitely open to hearing which parts we want to tease out into individual PRs.

[AztecBot]

Benchmark results

Metrics with a significant change:

• proof_construction_time_sha256_30_ms (16): 1,516 (-18%)
• avm_simulation_time_ms (Token:mint_public): 586 (+710%)
• avm_simulation_time_ms (Token:assert_minter_and_mint): 80.9 (-63%)
• protocol_circuit_witness_generation_time_in_ms (private-kernel-tail-to-public): 8,600 (+24%)

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_30_ms	11,694 (-3%)	3,149 (-2%)	⚠️ 1,516 (-18%)	1,563 (-4%)	1,521 (-3%)
proof_construction_time_sha256_100_ms	43,788 (-2%)	11,756 (-2%)	5,422 (-2%)	5,400 (-2%)	5,342 (-2%)
proof_construction_time_poseidon_hash_ms	78.0 (-2%)	34.0 (-3%)	34.0 (-3%)	58.0 (-2%)	87.0 (-1%)
proof_construction_time_poseidon_hash_30_ms	1,556 (-1%)	414 (-2%)	200 (-1%)	224 (-3%)	267 (-1%)
proof_construction_time_poseidon_hash_100_ms	5,724 (-3%)	1,568 (-2%)	720 (-2%)	776 (-1%)	789 (-2%)

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	1,412	1,412	1,412
l1_rollup_calldata_gas	9,452	9,462	9,464
l1_rollup_execution_gas	611,191	611,352	611,505
l2_block_processing_time_in_ms	767 (-2%)	1,449 (+2%)	2,704 (-3%)
l2_block_building_time_in_ms	24,710 (-6%)	50,183 (+2%)	97,023 (-6%)
l2_block_rollup_simulation_time_in_ms	24,710 (-6%)	50,182 (+2%)	97,022 (-6%)
l2_block_public_tx_process_time_in_ms	21,217 (-7%)	46,406 (+2%)	93,374 (-6%)

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	7,186 (+2%)	10,141 (+2%)
node_database_size_in_bytes	12,251,216	16,166,992
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	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	116	526 (+15%)	12,768 (-4%)	20,634	67,190	92,352	2,819	524,288
private-kernel-inner	361 (-1%)	1,107 (+3%)	49,359 (+1%)	94,902	67,190	92,352	2,819	2,097,152
private-kernel-tail	303 (-1%)	1,698 (-4%)	48,267 (-1%)	99,121	71,733	14,912	399	2,097,152
base-parity	6.59 (+5%)	1,791 (-1%)	2,689 (-5%)	128	64.0	2,208	2.00	131,072
root-parity	49.1 (-2%)	71.4 (-5%)	42,171 (+2%)	27,100	64.0	2,720	18.0	2,097,152
base-rollup	7,793 (-1%)	4,898 (-2%)	85,328	170,330	728	3,648	47.0	4,194,304
root-rollup	110 (-2%)	82.4 (-10%)	22,850 (-3%)	25,253	620	3,456	41.0	1,048,576
public-kernel-setup	626 (-2%)	3,672 (-3%)	45,211	116,905	93,334	125,344	3,850	2,097,152
public-kernel-app-logic	590 (-2%)	4,703 (-2%)	46,901 (-4%)	116,905	93,334	125,344	3,850	2,097,152
public-kernel-tail	1,393 (-1%)	38,570 (-1%)	190,090 (+3%)	511,910	10,014	14,912	399	8,388,608
private-kernel-reset-small	554 (-1%)	2,040 (+4%)	47,988 (+1%)	123,313	67,190	92,352	2,819	2,097,152
public-kernel-teardown	567 (-1%)	4,712 (-3%)	47,232 (-2%)	116,905	93,334	125,344	3,850	2,097,152
merge-rollup	29.5 (-1%)	N/A	N/A	16,486	728	N/A	N/A	N/A
private-kernel-tail-to-public	N/A	⚠️ 8,600 (+24%)	95,595	N/A	N/A	125,344	3,850	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	408 (-2%)	N/A	N/A	N/A	N/A
ContractInstanceDeployer:deploy	1,408	9,944	39.5 (-1%)	N/A	N/A	N/A	N/A
MultiCallEntrypoint:entrypoint	1,920	9,944	1,302 (-1%)	N/A	N/A	N/A	N/A
GasToken:deploy	1,376	9,944	941 (-2%)	N/A	N/A	N/A	N/A
SchnorrAccount:constructor	1,312	9,944	487 (-2%)	N/A	N/A	N/A	N/A
SchnorrAccount:entrypoint	2,304	9,944	1,891	16,768	56,080 (+1%)	2,097,152	457
Token:privately_mint_private_note	1,280	9,944	636 (-1%)	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_public	1,344	9,944	298 (-3%)	16,768	11,171 (-3%)	524,288	457
Token:transfer	1,312	9,944	2,899	16,768	22,619	1,048,576	457
AuthRegistry:set_authorized (avm)	20,954	N/A	N/A	94,336	1,319 (-8%)	N/A	N/A
FPC:prepare_fee (avm)	28,396	N/A	N/A	94,400	2,991 (-6%)	N/A	N/A
Token:transfer_public (avm)	44,612	N/A	N/A	94,400	3,980 (-2%)	N/A	N/A
AuthRegistry:consume (avm)	34,832	N/A	N/A	94,336	2,963 (-1%)	N/A	N/A
FPC:pay_refund (avm)	38,561	N/A	N/A	94,368	24,576 (+3%)	N/A	N/A
Benchmarking:create_note	1,344	9,944	488	N/A	N/A	N/A	N/A
SchnorrAccount:verify_private_authwit	1,280	9,944	72.3	N/A	N/A	N/A	N/A
Token:unshield	1,376	9,944	2,700	N/A	N/A	N/A	N/A
FPC:fee_entrypoint_private	1,376	9,944	3,508 (+1%)	N/A	N/A	N/A	N/A

AVM Simulation

Time to simulate various public functions in the AVM.

Function	time_ms	bytecode_size_in_bytes
GasToken:_increase_public_balance	65.3 (-19%)	13,790
GasToken:set_portal	18.7 (-1%)	3,305
Token:constructor	96.7 (-2%)	23,658
FPC:constructor	62.2 (-2%)	13,592
GasToken:mint_public	51.4	10,158
Token:mint_public	⚠️ 586 (+710%)	19,000
Token:assert_minter_and_mint	⚠️ 80.9 (-63%)	12,891
AuthRegistry:set_authorized	31.5 (-5%)	7,812
FPC:prepare_fee	128 (-4%)	15,062
Token:transfer_public	55.7 (-23%)	31,184
FPC:pay_refund	146 (-1%)	25,260
Benchmarking:increment_balance	2,643 (-1%)	15,233
Token:_increase_public_balance	56.5 (+1%)	14,972
FPC:pay_refund_with_shielded_rebate	140 (-23%)	26,347

Public DB Access

Time to access various public DBs.

Function	time_ms
get-nullifier-index	0.160 (-1%)

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	10.4 (-1%)	16.8 (-1%)	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.603 (-1%)	0.516 (-1%)	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_32_depth_ms	N/A	N/A	48.8 (-1%)	77.0 (-1%)	137	251 (+2%)	472 (-4%)
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.497 (-1%)	0.473 (-1%)	0.469	0.454 (+2%)	0.441 (-4%)
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	60.0 (-1%)	113 (-1%)	183 (-4%)	362 (+2%)	695 (-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.506 (-1%)	0.508 (-1%)	0.484 (-4%)	0.489 (+2%)	0.476 (-4%)
batch_insert_into_indexed_tree_40_depth_ms	N/A	N/A	73.7	N/A	N/A	N/A	N/A
batch_insert_into_indexed_tree_40_depth_hash_count	N/A	N/A	133	N/A	N/A	N/A	N/A
batch_insert_into_indexed_tree_40_depth_hash_ms	N/A	N/A	0.524	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	85,707	670,983

Transaction size based on fee payment method

| Metric | |
| - | |

[spalladino]

Awesome PR @just-mitch!! Left a bunch of comments, but none that cannot be addressed in a subsequent PR.

Main thing I'd like to discuss about the overall design is whether we can decouple partial notes from fee refunds. In other words: having the token contract expose primitives "setup partial note" and "complete partial note" (the latter accepting a value as opposed to using the tx fee), and have the FPC orchestrate the whole thing. Seems like we'd need a new note type PartialTokenNoteAwaitingToBeCompleted to be set during "setup partial note" and to be consumed in "complete", so the caller can't just supply any values they wish, right?

[spalladino]

Why not use context.transaction_fee here?

[just-mitch]

Ah this is dead code. Not called. I will remove it.

[spalladino]

Not sure I understood: why does this convince the FPC we are not cheating?

[spalladino]

Also: should we mix the nonce with the msg.sender before emitting it as a nullifier? I'm worried a user could abuse it to invalidate another user's txs.

[just-mitch]

Suppose user chooses nonce of 42, and the amount due to the FPC is 100.

Then if user chooses a nonce of 42 again, then it will create a duplicate note if the amount due to the FPC is the same.

For that reason I think we ought not mix the nonce with the msg.sender, since we would want the other user's tx to be invalidated for the same reason.

[spalladino]

Got it. Still, it feels risky to have a nullifier purely controlled by the caller. What if we mixed msg_sender with the nonce both for this nullifier and for the nonce we pass onto the token contract?

[just-mitch]

I think we could do that, but from the protocol perspective, the nullifier will be siloed to this contract instance, so no risk there, and from the FPC's perspective, it wants to do absolutely as little as possible.

That said, I also have a somewhat icky feeling about this. Can you think of any attack or exploit that could come from it? It would be helpful to understand if this type of pattern is "safe" generally, even if it feels risky.

[benesjan]

Ah duh yes snooping the mempool would do it. Nice one! So I think then we don't need to mix in the sender, but we do need to encrypt the nonce when we emit it.

In this PR I just fixed the privacy leak which Lasse and Santiago spotted by introducing the second randomness value (computed by hashing the first one) and emitting as unencrypted log only the second value which should reliably prevent the front-running attack (basically the same solution as what Mitch described in the edit).

[benesjan]

Suppose user chooses nonce of 42, and the amount due to the FPC is 100.

Then if user chooses a nonce of 42 again, then it will create a duplicate note if the amount due to the FPC is the same.

For that reason I think we ought not mix the nonce with the msg.sender, since we would want the other user's tx to be invalidated for the same reason.

Do I understand this correctly that this is only a problem because we do not yet inject nonces to public note hashes (issue here)? If we had the nonces there I think there should be no issue with duplicate notes.

If this is the case I would prioritize finishing tackling that issue.

[just-mitch]

@benesjan I think that the original issue raised by Palla has already been fixed by emitting the hash of the user's randomness as the unencrypted log. But I think that if we injected nonces into public notes, we might be able to get away with not pushing the nullifier of the user's randomness (since that was originally done to convince the FPC that its note would have a unique preimage/image). Thought I'm not sure in that case how the FPC/user recover the nonce that gets injected?

[benesjan]

Cool cool.

I am just working on storage slot as point PR and with that we'll be able to fix the remaining vulnerabilities (#7323 and #7324). Once that is done the note hash will be siloed to user address by the contract. At that point the severity of the attack of re-using randomness (assuming we remove pushing the nullifier) drops from an attacker potentially preventing someone else from spending their notes (by emitting the same note and then spending it) to a user potentially losing money if they are stupid and use the same randomness twice.

We should protect users against stupidity so it makes sense to prioritize tackling public note hash nonce.

Once we have that implemented we can safely drop emitting the nullifier. This is super nice because we want to hyper-optimize these functions since it's a cost which I assume will be part of all the txs.

Will take a note to do that once all the pieces are in place.

[benesjan]

Just nuked the nullifier in this PR as it's no longer needed.

[spalladino]

Why pass the asset as an argument at all?

[just-mitch]

The thinking here is the same as the existing FPC- this FPC could be used with a whitelist of supported underlying tokens.

[spalladino]

I'd push for fixing this ASAP since it's a very unexpected behaviour for a dev testing via the txe.

[just-mitch]

Yeah agreed. I don't know enough about the TXE design to know how this ought to be done, since it needs to have an understanding of the TX lifecycle.

[spalladino]

FYI I think you can do sth like PrivateToken::storage().balances.slot to avoid hardcoding the slot

[spalladino]

Is this for getting the pubkeys into the registry?

[just-mitch]

Yep!

[spalladino]

Was any change from the original needed?

[just-mitch]

Yes. Original maintains
map: Map<AztecAddress, PrivateSet<T, Context>, Context>

this has
map: PrivateSet<T, Context>

The reason is that if we were to use Map<AztecAddress, ...> the address would get mixed in when computing the storage slots for notes. So back in PrivateToken::complete_refund we would need the aztec address of the note recipient in order to correctly emit the note hash.

So the functions below are slightly different to compensate for that, and we have to do funky things with to_unconstrained below. I owe @nventuro an issue documenting this.

[spalladino]

Good to know. Still, it seems that it should be fine for all practical purposes, right?

[just-mitch]

Yes I think so.

[spalladino]

FYI Lasse is removing this method from context given it's a privacy footgun.

@LHerskind seeing as it is needed in more than one place, should we instead keep it but rename it to something like "dangerously_privacy_leaking_emit_unencrypted_log" to be clear about its implications?

[just-mitch]

Lol at dangerously... We could also emit it as encrypted, but since the FPC is paying for this, and I don't think they care if nonce is revealed, they would prefer it to be unencrypted.

[spalladino]

Name's inspired by React's https://legacy.reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml

[spalladino]

Could this lead to a privacy leak? Let's assume we have an FPC that's used for a specific set of apps (eg swaps), that have a well-known gas cost, so the set of possible funded_amounts is known. The refund_nonce is known as well since it's emitted unencrypted by the FPC. So an attacker could brute-force user npk_ms fetched from the keys registry to try and recompute this sponsored_user_point (which is visible as part of the public call) and thus detect the tx sender.

Could we patch this by adding another value used for randomness that does not get publicly broadcasted?

[just-mitch]

Yes! @LHerskind had the same suggestion on the thread in show-and-tell. Alice supplies a nonce that gets emitted as a nullifier for the FPC and another that gets used for herself.

[benesjan]

I might be retarded but I feel like functions like expectMapping and then having complex types like export type BalancesFn = ReturnType<typeof getBalancesFn>; in tests just makes it all hard to read.

Also not following the convention of using first capital letter for classes and not for variables in InitialPrivateFPCGas doesn't help.

[benesjan]

Do we submit this tx just to get the tx fee? And if yes is it because we have a guarantee of the fee being constant between txs?

[spalladino]

I believe it is submitted so we can test the refund flow. If we didn't submit it, the public execution that creates the refund wouldn't run.

[benesjan]

Oh, got it. Will add a comment there. Thank you 🙏