refactor: encrypted logs - address minor pr review comments by iAmMichaelConnor · Pull Request #11554 · AztecProtocol/aztec-packages

iAmMichaelConnor · 2025-01-28T11:51:06Z

Addresses the comments from here #11503

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

nventuro · 2025-01-28T14:28:01Z

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

+    // Note: this tidier approach did not work: it broke the compiler with some kind of
+    // stack too deep error, maybe as a result of the huge arithmetic generic statements
+    // it needed to evaluate to get the fields_padding array size.
+    // let final_log_random_fields_padding = get_arr_of_size__fields_padding__from_plaintext(final_plaintext_bytes);
+    // for i in 0..final_log_random_fields_padding.len() {
+    //     /// Safety: randomness cannot be constrained.
+    //     final_log[i] = unsafe { random() };
+    // }
+    // let final_log = array_concat_3([tag, eph_pk.x], log_bytes_as_fields, final_log_random_fields_padding);


It'd be nice to report this, you can likely create a small reproducible example to show them. I don't think we're doing anything too unreasonable here, and I don't like being at the very boundary of what's allowed.

I tried before pushing, but couldn't trigger the bug in a smaller example. Will find time to try further.

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

nventuro · 2025-01-28T14:34:24Z

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr

+/// Within an unconstrained function, this is more efficient than using `array_concat`
+/// twice, since this doesn't have redundant `for` loops. It is uglier, though.


Is this not true for constrained fns?

Apparently not; Jake and Tom were debating it here noir-lang/noir#7200 (comment)

I'd expect brilling to be fast enough that this is not an issue. If there were extra constraints then sure we'd have the more efficient ones, but if it's just some more brilling I'd not really care.

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

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

…trategies/default_aes128/event.nr

iAmMichaelConnor · 2025-01-30T16:27:02Z

...cts/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/partial_note.nr

-    let mut log_bytes_padding_to_mult_31 = get_arr_of_size__log_bytes_padding__from_PT::<2 + N>();
+    let mut log_bytes_padding_to_mult_31 =
+        get_arr_of_size__log_bytes_padding__from_plaintext(final_plaintext);
+    /// Safety: this randomness won't be constrained to be random. It's in the
+    /// interest of the executor of this fn to encrypt with random bytes.
    log_bytes_padding_to_mult_31 = unsafe { get_random_bytes() };

-    let mut log_bytes = get_arr_of_size__log_bytes__from_PT::<2 + N>();


@nventuro there was a bug in the code I merged the other day. This should be N; not 2 + N. The 2 + N is from the previous log encoding. I guess we got lucky (or unlucky) with the size of N in all of our tests not triggering a case where 2 + N resulted in a different amount of padding.
Anyway, this is (accidentally, but luckily) fixed in this PR. People might run into issues of partial notes not syncing until this fix is merged.

benesjan

Looks good. Just pls address the minor comments I did before merging

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

noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/note.nr

...cts/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/partial_note.nr

…ztec-packages into mc/address-nico-comments

iAmMichaelConnor · 2025-01-31T19:28:00Z

Bootstrapping now hangs forever when compiling the Noir contracts. It worked yesterday.
I can't get anything from the Noir compiler about why it's hanging. Even a nargo test in aztec_nr is hanging. What's happening?!

iAmMichaelConnor · 2025-12-20T19:54:02Z

Closing as stale and outdated.

address nico's pr comments

edf9f8d

iAmMichaelConnor requested a review from nventuro January 28, 2025 11:51

iAmMichaelConnor added 3 commits January 28, 2025 11:56

more changes

0530777

a todo

11db2d4

array tests

322f67f

nventuro reviewed Jan 28, 2025

View reviewed changes

noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/event.nr Show resolved Hide resolved

nventuro reviewed Jan 28, 2025

View reviewed changes

iAmMichaelConnor and others added 2 commits January 28, 2025 15:34

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

4bd0c0f

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

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

2894b14

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

iAmMichaelConnor commented Jan 28, 2025

View reviewed changes

noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/event.nr Outdated Show resolved Hide resolved

iAmMichaelConnor added 3 commits January 28, 2025 15:58

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

d458346

…trategies/default_aes128/event.nr

Merge branch 'master' into mc/address-nico-comments

7a432b0

merge master

5d000b4

iAmMichaelConnor commented Jan 30, 2025

View reviewed changes

iAmMichaelConnor mentioned this pull request Jan 31, 2025

Add tests to manually test note/event/partial note payloads of various sizes #11646

Open

benesjan approved these changes Jan 31, 2025

View reviewed changes

iAmMichaelConnor added 5 commits January 31, 2025 16:54

jan comments

2bf2d1f

Merge branch 'master' into mc/address-nico-comments

15c6130

rm unnecessary assert

6542319

Merge branch 'mc/address-nico-comments' of github.com:AztecProtocol/a…

5bfbb90

…ztec-packages into mc/address-nico-comments

Merge branch 'master' into mc/address-nico-comments

9405571

iAmMichaelConnor mentioned this pull request Jan 31, 2025

Example of compile hanging forever with no output noir-lang/noir#7256

Closed

nventuro approved these changes Feb 17, 2025

View reviewed changes

iAmMichaelConnor changed the base branch from master to next June 5, 2025 15:14

iAmMichaelConnor requested a review from LeilaWang as a code owner June 5, 2025 15:14

ludamad force-pushed the next branch from 366f4d8 to bf5d93a Compare June 27, 2025 00:12

iAmMichaelConnor closed this Dec 20, 2025

		/// Within an unconstrained function, this is more efficient than using `array_concat`
		/// twice, since this doesn't have redundant `for` loops. It is uglier, though.

Conversation

iAmMichaelConnor commented Jan 28, 2025

Uh oh!

Uh oh!

nventuro Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nventuro Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nventuro Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iAmMichaelConnor Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

benesjan left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iAmMichaelConnor commented Jan 31, 2025

Uh oh!

iAmMichaelConnor commented Dec 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benesjan left a comment •

edited

Loading