feat: log decryption in Noir by benesjan · Pull Request #12596 · AztecProtocol/aztec-packages

benesjan · 2025-03-08T18:13:25Z

benesjan · 2025-03-08T18:19:45Z

    plaintext_bytes
 }

+/// Computes an encrypted log for an event using AES-128 encryption in CBC mode.


Sneaked this unrelated change in. I needed it to orient myself in the structure of the log and AI is perfect for generating these code docs.

benesjan · 2025-03-08T18:19:54Z

    plaintext_bytes
 }

+/// Computes an encrypted log for a note using AES-128 encryption in CBC mode.


Sneaked this unrelated change in. I needed it to orient myself in the structure of the log and AI is perfect for generating these code docs.

benesjan · 2025-03-08T18:20:24Z

+        // (all the "real" log fields contain at most 31 bytes because of the way we convert the bytes to fields).
        // Safety: randomness cannot be constrained.
-        final_log[i] = unsafe { random() };
+        final_log[i] = unsafe { random_with_max_byte_size::<31>() };


Same as in the event.nr above - this fixes privacy leak.

benesjan · 2025-03-11T16:49:01Z

+    [address]: ACVMField[],
+    [ephPKField0]: ACVMField[],
+    [ephPKField1]: ACVMField[],
+    [ephPKField2]: ACVMField[],


When I had the ephPk input defined only as "ephPk: ACVMField[]" then ephPk was only 1 Field instead of 3. Does the reviewer if it's really necessary to list the components directly here? It's ugly.

I'm not sure I follow. It's a struct with 3 values so it gets converted to three acvm fields no?

If I put there ephPk: ACVField[] instead of the 3 components I got just one field.

benesjan · 2025-03-11T16:50:33Z

+pub unconstrained fn get_shared_secret_oracle(
+    address: AztecAddress,
+    ephPk: Point,
+) -> [Field; POINT_LENGTH] {}


When I directly returned Point from the oracle, I got an error with num destination slots. I see that there is not other case that returns a multi-field struct from oracle and e.g. ingetIndexedTaggingSecretAsSender we also return an array of fields and then deserialize. Is it really just unsupported? I would expect Noir to manage to map the struct fields. Is this just a tech debt.

BoundedVec is an example of a multi-field struct that is being returned by the decryption oracle.

pub struct BoundedVec<T, let MaxLen: u32> { storage: [T; MaxLen], len: u32, }

Yes, this is the same as the decrypt oracle

Good point. Will investigate what did I do wrong here. Thanks

benesjan · 2025-03-11T17:03:03Z

    }

-    plaintext_bytes
+    fields_to_be_bytes_32(fields)


Did this change here and in the note.nr because when decrypting we use be_bytes_32_to_fields. And those 2 functions are guaranteed to match.

benesjan · 2025-03-11T17:06:36Z

+    // (This process is then reversed when processing the log in `do_process_log`)
+    let final_plaintext_bytes = fields_to_be_bytes_32(final_plaintext);
+
+    encrypt_log(


The encrypt_log function originally didn't exist but I separated it from compute_log because that is against what I wanted to unit test my decryption functionality.

benesjan · 2025-03-11T17:07:32Z

+        fields[i + 2] = packed_note[i];
+    }
+
+    fields


I return fields here instead of bytes. This is because that is the natural representation and I think it makes sense to convert it to bytes before calling the aes function.

I left it as bytes, because we should be packing the note_type_id into much less than a field. Plus, this function is compute_note_plaintext_for_this_strategy and this strategy is specifically for AES, so bytes is the natural output.

We eventually need to settle on what standard format aztec-nr uses for logs once it considers non-AES flows. In my mind the only sane choice is a field array (since that is what noir deals with, that is what the logs are, and that is the only thing that has no overhead), so this is more beginning to nudge things in that direction. Ultimately the note type id will share the field with some other values, likely in what is today the 'combined type id'.

benesjan · 2025-03-11T17:08:03Z

+
+    // AES 128 operates on bytes, not fields, so we need to convert the fields to bytes.
+    // (This process is then reversed when processing the log in `do_process_log`)
+    let final_plaintext_bytes = fields_to_be_bytes_32(final_plaintext);


This is new here. As for the event I now use this method to have a guraanteed match between encryption and decryption.

benesjan · 2025-03-11T17:10:39Z

    }

-    #[private]
-    fn emit_encrypted_logs_nested(value: Field, owner: AztecAddress, sender: AztecAddress) {


Nuked this function because it was used in a test that was no longer viable to have because of no longer having decryption in Typescript and it was also unnecessary. Will describe details in the test.

benesjan · 2025-03-11T17:12:16Z

            block.number,
            /* isFromPublic */ false,
-            log.toBuffer(),
+            log.toFields(),


TxScopedL2Log now represents the log as fields and not as a buffer. I did this change because Fields are the natural representation and it results in less conversions in the code.

nventuro

Was not able to go through the entire thing yet, leaving some preliminary comments

nventuro · 2025-03-11T19:11:22Z

@@ -1,2 +1,2 @@
 export * from './encrypted_log_payload.js';
-export * from './l1_note_payload.js';
+export * from './shared_secret_derivation.js';


Can't we get rid of this entire directory?

We cannot because there is still l1_event_payload that uses this stuff.

I was thinking about this and I think we should prioritize moving the event decryption to Noir as well once this PR is merged because I currently have it all loaded in my brain and it would allows us to finally purge a lot of this code.

It would require modifying the process_log function such that it stores the logs in capsules and then exposing some event getter on the relevant contracts so it would be quite a bit of work though.

nventuro · 2025-03-11T19:18:31Z

+    const symKeyBuffer = fromUintArray(symKey, 8);
+
+    const plaintext = await this.typedOracle.aes128Decrypt(ciphertext, ivBuffer, symKeyBuffer);
+    return bufferToBoundedVec(plaintext, ciphertextBVecStorage.length);


Is this the first oracle that returns a boundedvec?

nventuro · 2025-03-11T19:20:11Z

+    [address]: ACVMField[],
+    [ephPKField0]: ACVMField[],
+    [ephPKField1]: ACVMField[],
+    [ephPKField2]: ACVMField[],


I'm not sure I follow. It's a struct with 3 values so it gets converted to three acvm fields no?

nventuro · 2025-03-11T19:31:43Z

-  // TODO: I forgot to add a corresponding function here, when I introduced an oracle method to txe_oracle.ts. The compiler didn't throw an error, so it took me a while to learn of the existence of this file, and that I need to implement this function here. Isn't there a way to programmatically identify that this is missing, given the existence of a txe_oracle method?
-  async aes128Decrypt(ciphertext: ForeignCallArray, iv: ForeignCallArray, symKey: ForeignCallArray) {
-    const ciphertextBuffer = fromUintArray(ciphertext, 8);
+  // TODO: I forgot to add a corresponding function here, when I introduced an oracle method to txe_oracle.ts.


cc @Thunkar

benesjan · 2025-03-12T19:35:08Z

feat: log decryption in Noir #12596 👈 (View in Graphite)
master

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

iAmMichaelConnor

I've left some comments :)

iAmMichaelConnor · 2025-03-13T11:15:25Z

@@ -0,0 +1,352 @@
+use crate::{


I'm surprised git hasn't figured out that this file is a git mv of a file. It's showing as a deleted file, then a new file. How did you move around the files? VScode usually correctly does a git mv.
It's very hard to compare the diff as a result.

I pointed out in the code where I did changes. Sorry the diff is 💩 but feel like I have an excuse here since I could not really separate this into a different PR because of the branch situation being tricky when I started the PR.

iAmMichaelConnor · 2025-03-13T11:21:32Z

 }

+/// Just like `random`, but returns a field that fits in `N` bytes.
+pub unconstrained fn random_with_max_byte_size<let N: u32>() -> Field {


This probably shouldn't live here; oracle files should focus on the oracle funtionality. A util which makes use of an oracle should live elsewhere.

Re the function name: it's not returning a max byte size, it'll return a value which is this byte size. random_bytes might be clearer?

Good point. Moved it in 0298842

Nuked the function in 65377a1 and I just use get_random_bytes there instead.

iAmMichaelConnor · 2025-03-13T11:24:26Z

+pub unconstrained fn get_shared_secret_oracle(
+    address: AztecAddress,
+    ephPk: Point,
+) -> [Field; POINT_LENGTH] {}


BoundedVec is an example of a multi-field struct that is being returned by the decryption oracle.

pub struct BoundedVec<T, let MaxLen: u32> { storage: [T; MaxLen], len: u32, }

iAmMichaelConnor · 2025-03-13T11:29:46Z

+/// equation for y.
+/// @param x - The x coordinate of the point
+/// @param sign - The "sign" of the y coordinate - determines whether y <= (Fr.MODULUS - 1) / 2
+pub fn point_from_x_coord_and_sign(x: Field, sign: bool) -> Point {


We should probably rename the point_from_x_coord to point_from_x_coord_unsafe. It's only being used in tests.

Why is it unsafe?

It's not unsafe per se but the the name is bad because you cannot recover a point just from x_coordinate. You can however recover a point corresponding to an address just from x_coordinate.

So I guess it would be better to instead just rename it to address_point_from_x_coord. Will take a note to do this in a separate PR.

iAmMichaelConnor · 2025-03-13T11:55:47Z

+        fields[i + 2] = packed_note[i];
+    }
+
+    fields


I left it as bytes, because we should be packing the note_type_id into much less than a field. Plus, this function is compute_note_plaintext_for_this_strategy and this strategy is specifically for AES, so bytes is the natural output.

iAmMichaelConnor · 2025-03-13T12:06:15Z

+{
+    let final_plaintext = compute_note_plaintext_for_this_strategy(note, storage_slot);
+
+    encrypt_log(context.this_address(), final_plaintext, recipient, sender)


Why was this function extracted into its own file?
One of my goals when I refactored encryption was for everything to be in one long, flattened file, so that it was easier to see the whole unravelled log assembly process, whilst we did all our other refactoring. Please can we revert this particular change?
The encrypt_log function that's been extracted has a name which isn't descriptive of what the function does: it computes a log (so should live in this compute_log function) (Or actually, it assembles a log, so maybe compute_log should be renamed to assemble_log). A "log" can't be encrypted; a plaintext can be encrypted. And the extracted encrypt_log function also computes a header and encrypts that and then concatenates data according to this log assembly strategy.

I needed to unit test it against the decryption functionality and during decryption we don't spit out a note but a log plaintext (see process_log function). This is not decided yet but the process_log contract function will most likely process event logs as well and there we will need 1 decryption functionality shared between events and notes. For this reason changing the process_log function to not destructure the log plaintext there seemed like a bad direction to go down.

Agree that the name could be improved but I think it can probably wait for when we handle the events.

iAmMichaelConnor · 2025-03-13T12:09:30Z

+    final_log
+}
+
+pub unconstrained fn decrypt_log(


I mentioned in another comment that the encrypted_log fn shouldn't be extracted from the compute_log function.
This function does the process in reverse, so it doesn't just "decrypt" it also decodes the various components of the log. process_log might be a better name (because it's literally processing the log according to this log assembly/disassembly strategy).

I'm not sure we'd be able to keep that model, given that we'll need to agree on some common encoding that all schemes must follow (or else abstract both encryption and encoding). Given that, the primitive being 'encrypt/decrypt' a field array makes sense (i.e. potato encoding).

benesjan · 2025-03-14T17:16:05Z

 }

-/// Returns as many random bytes as specified through N.
-pub unconstrained fn get_random_bytes<let N: u32>() -> [u8; N] {


As Mike pointed out, we should keep oracle folder for oracles. For this reason I moved this function to utils/random.nr (didn't do any changes to it).

nventuro · 2025-03-14T18:08:22Z

+/// equation for y.
+/// @param x - The x coordinate of the point
+/// @param sign - The "sign" of the y coordinate - determines whether y <= (Fr.MODULUS - 1) / 2
+pub fn point_from_x_coord_and_sign(x: Field, sign: bool) -> Point {


Why is it unsafe?

nventuro · 2025-03-14T18:11:08Z

+    // IMPORTANT: This test has a misleading name and behavior. When TxScopedL2Log was updated to work with
+    // PrivateLog and PublicLog types directly, the `isFromPublic` getter was initially left unimplemented.
+    // At that time, this test appeared to pass - not because it was correctly filtering logs by contract address,
+    // but because `isFromPublic` was returning undefined (falsey). As a result, the test is actually filtering
+    // based on the log type (public vs private) rather than the intended contract address check.


Is it working correctly now?

It does not. But felt like it's not a scope of this PR to rework that so I just put there this warning that it's fucked up.

Please create a tracking issue or address it then

nventuro · 2025-03-14T18:26:31Z

+    final_log
+}
+
+pub unconstrained fn decrypt_log(


I'm not sure we'd be able to keep that model, given that we'll need to agree on some common encoding that all schemes must follow (or else abstract both encryption and encoding). Given that, the primitive being 'encrypt/decrypt' a field array makes sense (i.e. potato encoding).

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

…trategies/default_aes128/note/note.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

benesjan marked this pull request as draft March 8, 2025 18:13

benesjan commented Mar 8, 2025

View reviewed changes

Comment thread noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/event.nr Outdated

benesjan commented Mar 8, 2025

View reviewed changes

benesjan commented Mar 11, 2025

View reviewed changes

Comment thread noir-projects/aztec-nr/aztec/src/discovery/mod.nr

benesjan commented Mar 11, 2025

View reviewed changes

Comment thread ...aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/arithmetic_generics_utils.nr

benesjan commented Mar 11, 2025

View reviewed changes

Comment thread ...ojects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/note/note.nr

benesjan commented Mar 11, 2025

View reviewed changes

nventuro mentioned this pull request Mar 11, 2025

app-silo shared secrets #12656

Closed

nventuro reviewed Mar 11, 2025

View reviewed changes

Base automatically changed from nv/redo-partial to master March 12, 2025 18:25

benesjan force-pushed the janb/log_decryption_in_noir branch 2 times, most recently from 8011dc0 to 054534e Compare March 12, 2025 19:34

benesjan marked this pull request as ready for review March 12, 2025 19:34

iAmMichaelConnor reviewed Mar 13, 2025

View reviewed changes

benesjan marked this pull request as draft March 13, 2025 18:55

benesjan force-pushed the janb/log_decryption_in_noir branch from 593052c to bebc037 Compare March 13, 2025 19:55

benesjan marked this pull request as ready for review March 13, 2025 19:56

benesjan marked this pull request as draft March 13, 2025 20:32

benesjan marked this pull request as ready for review March 13, 2025 21:40

benesjan requested a review from nventuro March 13, 2025 22:03

benesjan marked this pull request as draft March 14, 2025 15:14

benesjan marked this pull request as ready for review March 14, 2025 15:35

nventuro mentioned this pull request Mar 14, 2025

Support alternative encryption schemes in aztec-nr private message processing #12750

Open

benesjan commented Mar 14, 2025

View reviewed changes

nventuro approved these changes Mar 14, 2025

View reviewed changes

benesjan and others added 19 commits March 14, 2025 19:26

WIP

2721ba1

cute imports

ecba8e7

test fix

80b4bce

fixes

597e5e6

e2e_offchain_note_delivery test fix

8837569

fix

5ea3014

sf

2124fb9

moving non-oracle stuff to utils

0fc4a70

linking a todo

1184bbb

more TODOs

43b7c79

nuking random_with_max_byte_size

6a55c08

more TODOs

18c3f8e

Apply suggestions from code review

b0dfb2a

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_s…

fc7f0a8

…trategies/default_aes128/note/note.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/aztec-nr/aztec/src/oracle/shared_secret.nr

69ea4b2

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/aztec-nr/aztec/src/oracle/shared_secret.nr

538e90e

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr

8ce8276

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

typo fix

e1de1aa

fixes after rebase

f09095b

benesjan force-pushed the janb/log_decryption_in_noir branch from c419b8c to f09095b Compare March 14, 2025 19:29

benesjan enabled auto-merge (squash) March 14, 2025 19:29

process_private_note_log --> attempt_note_discovery

89abba9

benesjan merged commit 238d57c into master Mar 14, 2025

benesjan deleted the janb/log_decryption_in_noir branch March 14, 2025 20:00

AztecBot mentioned this pull request Mar 14, 2025

chore(master): release 0.81.0 #12689

Merged

Conversation

benesjan commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

benesjan Mar 11, 2025 • 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!

iAmMichaelConnor Mar 13, 2025 • 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!

Uh oh!

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!

nventuro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan commented Mar 12, 2025

Uh oh!

iAmMichaelConnor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan Mar 13, 2025 • 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!

benesjan commented Mar 8, 2025 •

edited

Loading

benesjan Mar 11, 2025 •

edited

Loading

iAmMichaelConnor Mar 13, 2025 •

edited

Loading

benesjan Mar 13, 2025 •

edited

Loading

iAmMichaelConnor Mar 13, 2025 •

edited

Loading

benesjan Mar 13, 2025 •

edited

Loading