Skip to content

chore(docs): common patterns and anti patterns in aztec.nr#3413

Merged
rahul-kothari merged 4 commits intomasterfrom
rk/common_patterns
Nov 24, 2023
Merged

chore(docs): common patterns and anti patterns in aztec.nr#3413
rahul-kothari merged 4 commits intomasterfrom
rk/common_patterns

Conversation

@rahul-kothari
Copy link
Contributor

@rahul-kothari rahul-kothari commented Nov 23, 2023

Add a list of common patterns and anti patterns when writing Aztec.nr

For a quick glance - just look at all the headings in common_patterns/main.md and lmk if there is any I have missed

@rahul-kothari rahul-kothari changed the title common patterns and anti patterns in aztec.nr chore(docs): common patterns and anti patterns in aztec.nr Nov 23, 2023

Comparison on Field is also not possible today. For such cases, we recommend using u120, which is wrapped under the SafeU120 class found in the SafeMath library.

### Approving another user/contract to call a function on your behalf
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Approving another user/contract to call a function on your behalf
### Approving another user/contract to execute an action on your behalf

Comment on lines +65 to +72
### When calling a public function from private, try to mark the public function as `internal`
This ensures your flow works as intended and that no one can call the public function without going through the private function first!
Copy link
Contributor

Choose a reason for hiding this comment

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

This one feels off, since private -> public flows can also happen across different contracts, and internal doesn't make sense in those cases. Maybe we need to narrow down the scenario? For instance, following the lead from the previous one, we could do: "Writing public storage from private: If you want to update public state from private, you can call a public function from private, just make sure you mark public as internal".

When emitting unencrypted log in private domain, think twice about the content you put there because the log in unencrypted and therefore anyone can see

### Passing along addresses when calling a public function from private
If you have a private function which calls a public function, remember that sequencer can see any parameters passed to the public function. So try to not pass the address
Copy link
Contributor

Choose a reason for hiding this comment

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

So try to not pass the address

What address?

@spalladino
Copy link
Contributor

I'd add the pattern of shared private note that came up from the work with Wonderland: if you have private state that needs to be handled by more than a single user (but no more than a handful), you can add the note commitment to the private data tree, and then encrypt the note once for each of the users that need to see it. And if any of those users should be able to consume the note, you can generate a random nullifier on creation and store it in the encrypted note, instead of relying on the user secret.

### Reading public storage in private
You can't read public storage in private domain. But nevertheless reading public storage is desirable. There are two ways:

1. You pass the data as a parameter to your private method and later assert in public that the data is correct. E.g.:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we warn that this leaks information about the private function that was executed (and the data which has been read)? This almost seems like an anti-pattern for privacy?

This ensures your flow works as intended and that no one can call the public function without going through the private function first!

### Passing data from public to private
You have public balance and want to move them into the private domain. You shouldn't pass your address that should receive the notes in private, because that leaks privacy (duh!). So what do you do?
Copy link
Contributor

Choose a reason for hiding this comment

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

These sentences aren't clear to me.

When you send someone a note, the note hash gets encrypted and added to the merkle tree, but the receiver needs to know they receive a note so they can attempt to decrypt leaves in the tree and add it to their PXE. There are two ways you can discover your notes:

1. When sending someone a note, use `emit_encrypted_log` (and encrypt the log with their public key). This way as the PXE processes all encrypted logs, it realizes there is a note for itself. [More info here](../../syntax/events.md)
2. Manually using `pxe.addNote()` - What if the contract doesn't emit such logs? Or you creating a note in the public domain and want to consume it in private domain (`emit_encrypted_log` shouldn't be called in the public domain because everything is public), like in the previous section where we created a TransparentNote in public.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the contract doesn't emit such logs?

This is a bit weird as it's up to the developer to set this.

Instead I would describe here that a dev might be inclined here to not emit the logs to save gas.

There are mistakes one can make to reduce their privacy set and therefore make it trivial to do analysis and link addresses. Some of them are:

### Emitting unencrypted log in private domain
When emitting unencrypted log in private domain, think twice about the content you put there because the log in unencrypted and therefore anyone can see
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not even mention this here since the function will be eventually removed and now it looks like this:

image


Here you approve someone to transfer funds publicly on your behalf

### Prevent the same user flow from happening twice or avoiding replay attacks using nullifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use the term replay attack here because this is not really a replay attack.

"A replay attack occurs when a cybercriminal eavesdrops on a secure network communication, intercepts it, and then fraudulently delays or resends it to misdirect the receiver into doing what the hacker wants. The added danger of replay attacks is that a hacker doesn't even need advanced skills to decrypt a message after capturing it from the network. The attack could be successful simply by resending the whole thing."

@AztecBot
Copy link
Collaborator

AztecBot commented Nov 23, 2023

Benchmark results

No metrics with a significant change found.

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.

Values are compared against data from master at commit d94d88bf and shown if the difference exceeds 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 128 txs
l1_rollup_calldata_size_in_bytes 45,444 179,588 716,132
l1_rollup_calldata_gas 222,900 868,064 3,449,612
l1_rollup_execution_gas 841,987 3,595,172 22,204,981
l2_block_processing_time_in_ms 2,052 (+1%) 7,782 (+1%) 30,701 (+1%)
note_successful_decrypting_time_in_ms 303 879 (-2%) 3,191 (-4%)
note_trial_decrypting_time_in_ms 7.49 (+22%) 89.6 (-17%) 142 (-1%)
l2_block_building_time_in_ms 20,671 (-1%) 81,850 327,335
l2_block_rollup_simulation_time_in_ms 11,998 (-1%) 47,476 189,458
l2_block_public_tx_process_time_in_ms 8,629 34,233 137,353

L2 chain processing

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

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 21,876 42,760 (-1%)
note_history_successful_decrypting_time_in_ms 2,044 3,987 (-1%)
note_history_trial_decrypting_time_in_ms 125 (-1%) 222 (+10%)
node_database_size_in_bytes 1,630,302 1,096,704
pxe_database_size_in_bytes 29,748 59,307

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 768 61,697 18,905
private-kernel-ordering 127 24,297 8,153
base-rollup 1,755 656,428 873
root-rollup 169 4,072 1,097
private-kernel-inner 791 81,568 18,905
public-kernel-private-input 569 41,519 18,905
public-kernel-non-first-iteration 408 41,561 18,905
merge-rollup 15.5 2,592 873

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts 1 deployed contracts
tx_size_in_bytes 8,787 27,547

@rahul-kothari rahul-kothari enabled auto-merge (squash) November 23, 2023 16:30
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

Once you'll update the link I'll approve it.

PS: when calling from private to public, `msg_sender` is the contract address which is calling the public function.

### Deterministic nullifiers
In the [avoid replay attacks section](#prevent-the-same-user-flow-from-happening-twice-or-avoiding-replay-attacks-using-nullifiers), we recommended using nullifiers. But what you put in the nullifier is also as important.
Copy link
Contributor

Choose a reason for hiding this comment

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

This link should be updated.

@rahul-kothari rahul-kothari merged commit 65bd855 into master Nov 24, 2023
@rahul-kothari rahul-kothari deleted the rk/common_patterns branch November 24, 2023 11:12
kevaundray pushed a commit that referenced this pull request Nov 27, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.16.0</summary>

##
[0.16.0](aztec-packages-v0.15.1...aztec-packages-v0.16.0)
(2023-11-27)


### ⚠ BREAKING CHANGES

* deprecate circuits/cpp
([#3421](#3421))
* call stack validation optimisation.
([#3387](#3387))

### Features

* Base rollup in noir
([#3257](#3257))
([4a1e9c3](4a1e9c3))
* Call stack validation optimisation.
([#3387](#3387))
([d06d5db](d06d5db))
* Goblin proof construction
([#3332](#3332))
([6a7ebb6](6a7ebb6))
* More logs relevant for debugging failures of 2 pixies test
([#3370](#3370))
([683a0f3](683a0f3))
* Noir subrepo.
([#3369](#3369))
([d94d88b](d94d88b))
* Noir_wasm compilation of noir programs
([#3272](#3272))
([f9981d5](f9981d5))
* Rollback public state changes on failure
([#3393](#3393))
([0e276fb](0e276fb))


### Bug Fixes

* **docs:** Doc explaining noir debug_log
([#3322](#3322))
([eed023d](eed023d))
* Naming inconsistency in private kernel
([#3384](#3384))
([4743486](4743486))
* Race condition in `PXE.getTxReceipt(...)`
([#3411](#3411))
([9557a66](9557a66))


### Miscellaneous

* Deprecate circuits/cpp
([#3421](#3421))
([4973cfb](4973cfb))
* Deterministically deduplicate
`cached_partial_non_native_field_multiplication` across wasm32 and
native compilations
([#3425](#3425))
([5524933](5524933))
* **docs:** Common patterns and anti patterns in aztec.nr
([#3413](#3413))
([65bd855](65bd855))
* Fix and reenable e2e quick start
([#3403](#3403))
([112740e](112740e)),
closes
[#3356](#3356)
* Fix intermittent failures for block-building e2e test
([#3404](#3404))
([e76e2d4](e76e2d4)),
closes
[#3358](#3358)
* Formatted `noir-contracts` and `aztec-nr`
([a73c4aa](a73c4aa))
* Initial clone of noir to subrepo
([#3409](#3409))
([8f1cb83](8f1cb83))
* **noir-contracts:** Remove redundant return value of 1
([#3415](#3415))
([2001d47](2001d47)),
closes
[#2615](#2615)
* Plumbs noir subrepo into yarn-project.
([#3420](#3420))
([63173c4](63173c4))
* Remove pxe / node /p2p-bootstrap docker images
([#3396](#3396))
([c236143](c236143))
* Skip artifacts for prettier
([#3399](#3399))
([98d9e04](98d9e04))
* Update path to acir artifacts
([#3426](#3426))
([f56f88d](f56f88d))
</details>

<details><summary>barretenberg.js: 0.16.0</summary>

##
[0.16.0](barretenberg.js-v0.15.1...barretenberg.js-v0.16.0)
(2023-11-27)


### Miscellaneous

* Plumbs noir subrepo into yarn-project.
([#3420](#3420))
([63173c4](63173c4))
</details>

<details><summary>barretenberg: 0.16.0</summary>

##
[0.16.0](barretenberg-v0.15.1...barretenberg-v0.16.0)
(2023-11-27)


### Features

* Goblin proof construction
([#3332](#3332))
([6a7ebb6](6a7ebb6))
* Noir subrepo.
([#3369](#3369))
([d94d88b](d94d88b))


### Miscellaneous

* Deterministically deduplicate
`cached_partial_non_native_field_multiplication` across wasm32 and
native compilations
([#3425](#3425))
([5524933](5524933))
* Plumbs noir subrepo into yarn-project.
([#3420](#3420))
([63173c4](63173c4))
* Update path to acir artifacts
([#3426](#3426))
([f56f88d](f56f88d))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Nov 28, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.16.0</summary>

##
[0.16.0](AztecProtocol/aztec-packages@aztec-packages-v0.15.1...aztec-packages-v0.16.0)
(2023-11-27)


### ⚠ BREAKING CHANGES

* deprecate circuits/cpp
([#3421](AztecProtocol/aztec-packages#3421))
* call stack validation optimisation.
([#3387](AztecProtocol/aztec-packages#3387))

### Features

* Base rollup in noir
([#3257](AztecProtocol/aztec-packages#3257))
([4a1e9c3](AztecProtocol/aztec-packages@4a1e9c3))
* Call stack validation optimisation.
([#3387](AztecProtocol/aztec-packages#3387))
([d06d5db](AztecProtocol/aztec-packages@d06d5db))
* Goblin proof construction
([#3332](AztecProtocol/aztec-packages#3332))
([6a7ebb6](AztecProtocol/aztec-packages@6a7ebb6))
* More logs relevant for debugging failures of 2 pixies test
([#3370](AztecProtocol/aztec-packages#3370))
([683a0f3](AztecProtocol/aztec-packages@683a0f3))
* Noir subrepo.
([#3369](AztecProtocol/aztec-packages#3369))
([d94d88b](AztecProtocol/aztec-packages@d94d88b))
* Noir_wasm compilation of noir programs
([#3272](AztecProtocol/aztec-packages#3272))
([f9981d5](AztecProtocol/aztec-packages@f9981d5))
* Rollback public state changes on failure
([#3393](AztecProtocol/aztec-packages#3393))
([0e276fb](AztecProtocol/aztec-packages@0e276fb))


### Bug Fixes

* **docs:** Doc explaining noir debug_log
([#3322](AztecProtocol/aztec-packages#3322))
([eed023d](AztecProtocol/aztec-packages@eed023d))
* Naming inconsistency in private kernel
([#3384](AztecProtocol/aztec-packages#3384))
([4743486](AztecProtocol/aztec-packages@4743486))
* Race condition in `PXE.getTxReceipt(...)`
([#3411](AztecProtocol/aztec-packages#3411))
([9557a66](AztecProtocol/aztec-packages@9557a66))


### Miscellaneous

* Deprecate circuits/cpp
([#3421](AztecProtocol/aztec-packages#3421))
([4973cfb](AztecProtocol/aztec-packages@4973cfb))
* Deterministically deduplicate
`cached_partial_non_native_field_multiplication` across wasm32 and
native compilations
([#3425](AztecProtocol/aztec-packages#3425))
([5524933](AztecProtocol/aztec-packages@5524933))
* **docs:** Common patterns and anti patterns in aztec.nr
([#3413](AztecProtocol/aztec-packages#3413))
([65bd855](AztecProtocol/aztec-packages@65bd855))
* Fix and reenable e2e quick start
([#3403](AztecProtocol/aztec-packages#3403))
([112740e](AztecProtocol/aztec-packages@112740e)),
closes
[#3356](AztecProtocol/aztec-packages#3356)
* Fix intermittent failures for block-building e2e test
([#3404](AztecProtocol/aztec-packages#3404))
([e76e2d4](AztecProtocol/aztec-packages@e76e2d4)),
closes
[#3358](AztecProtocol/aztec-packages#3358)
* Formatted `noir-contracts` and `aztec-nr`
([a73c4aa](AztecProtocol/aztec-packages@a73c4aa))
* Initial clone of noir to subrepo
([#3409](AztecProtocol/aztec-packages#3409))
([8f1cb83](AztecProtocol/aztec-packages@8f1cb83))
* **noir-contracts:** Remove redundant return value of 1
([#3415](AztecProtocol/aztec-packages#3415))
([2001d47](AztecProtocol/aztec-packages@2001d47)),
closes
[#2615](AztecProtocol/aztec-packages#2615)
* Plumbs noir subrepo into yarn-project.
([#3420](AztecProtocol/aztec-packages#3420))
([63173c4](AztecProtocol/aztec-packages@63173c4))
* Remove pxe / node /p2p-bootstrap docker images
([#3396](AztecProtocol/aztec-packages#3396))
([c236143](AztecProtocol/aztec-packages@c236143))
* Skip artifacts for prettier
([#3399](AztecProtocol/aztec-packages#3399))
([98d9e04](AztecProtocol/aztec-packages@98d9e04))
* Update path to acir artifacts
([#3426](AztecProtocol/aztec-packages#3426))
([f56f88d](AztecProtocol/aztec-packages@f56f88d))
</details>

<details><summary>barretenberg.js: 0.16.0</summary>

##
[0.16.0](AztecProtocol/aztec-packages@barretenberg.js-v0.15.1...barretenberg.js-v0.16.0)
(2023-11-27)


### Miscellaneous

* Plumbs noir subrepo into yarn-project.
([#3420](AztecProtocol/aztec-packages#3420))
([63173c4](AztecProtocol/aztec-packages@63173c4))
</details>

<details><summary>barretenberg: 0.16.0</summary>

##
[0.16.0](AztecProtocol/aztec-packages@barretenberg-v0.15.1...barretenberg-v0.16.0)
(2023-11-27)


### Features

* Goblin proof construction
([#3332](AztecProtocol/aztec-packages#3332))
([6a7ebb6](AztecProtocol/aztec-packages@6a7ebb6))
* Noir subrepo.
([#3369](AztecProtocol/aztec-packages#3369))
([d94d88b](AztecProtocol/aztec-packages@d94d88b))


### Miscellaneous

* Deterministically deduplicate
`cached_partial_non_native_field_multiplication` across wasm32 and
native compilations
([#3425](AztecProtocol/aztec-packages#3425))
([5524933](AztecProtocol/aztec-packages@5524933))
* Plumbs noir subrepo into yarn-project.
([#3420](AztecProtocol/aztec-packages#3420))
([63173c4](AztecProtocol/aztec-packages@63173c4))
* Update path to acir artifacts
([#3426](AztecProtocol/aztec-packages#3426))
([f56f88d](AztecProtocol/aztec-packages@f56f88d))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants