Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: encrypted logs refactor #11400

Closed
wants to merge 34 commits into from
Closed

Conversation

iAmMichaelConnor
Copy link
Contributor

@iAmMichaelConnor iAmMichaelConnor commented Jan 21, 2025

Description:

  • Seeking to enable users to specify custom "log assembly strategies".
    • Moving some files around, to make it clear that our current AES approach is one "default strategy".
    • Separating the code paths of log assembly for: Events, Notes, Partial Notes, because we want to:
      • Completely overhaul partial notes
      • Implement the randomness optimisation for notes.
    • I didn't go as far as abstracting the partial note logs - they are still hard-coded as using AES - because they're about to be refactored.
    • Note: there are libs like ValueNote which also hard-code the use of the "default strategy".
    • Decryption is still happening in typescript, because I know Nico's doing stuff on this front.

Optimisations:

  • Some byte conversion function constraint optimisations.
  • The AES log construction was very inefficient, constraint-wise, because it was AES-encrypting all 17 usable fields of a log, even if the note only needed to use 3 or 4 of those fields. I changed the log layout so that we only AES-encrypt those 3 or 4 fields, and the rest is padding. How? I convey the length of the actual ciphertext through the header ciphertext. But isn't the header about to be deleted when we don't need to encrypt the contract address for the PXE to discover which contract a log is for? Yes, but I'm saying it's much more efficient to keep a thing called a "header" around, even if we only convey the ciphertext length in it, because then we avoid AES-encrypting the entire log. Huge saving. This was my main battleground in this PR.

New features:

  • I threw my Poseidon2 encryption functions in here too, but they're not-yet used by anything.
  • I realised there's a privacy leakage in the way we convert bytes31's to fields, because the "big-end" 6 bits are always 0. I wrote a function to pad those bits with randomness, but didn't go as far as plugging it into the log strategy, because it felt like too much at once.

@iAmMichaelConnor iAmMichaelConnor changed the title WIP: encrypted logs refactor refactor: encrypted logs refactor Jan 21, 2025
@AztecBot
Copy link
Collaborator

AztecBot commented Jan 22, 2025

Docs Preview

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

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
pub fn bytes_to_fields<let N: u32>(input: [u8; N]) -> [Field; (N + 30) / 31] {
let mut dst = [0; (N + 30) / 31];
// Note: ceil(N / 31) = (N + 30) / 31
pub fn be_bytes_31_to_fields<let N: u32>(bytes: [u8; N]) -> [Field; (N + 30) / 31] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optimised vs the previous implementation that did a load of < checks on indices at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you measure this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also no need to throw in optimizations on top of further changes 🙃 this could just be its own thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you measure this?

No, but each comparison is something like 15 constraints, so the if byte_index < N for every byte of the old version will have blown up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is an example of me seeing something and going "I should fix this" instead of "I should open an issue for this and come back to it".

@@ -0,0 +1,124 @@
global KNOWN_NON_RESIDUE: Field = 5; // This is a non-residue in Noir's native Field.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately, this should all go in the stdlib Field dir. It's all to get a sqrt function.

@iAmMichaelConnor iAmMichaelConnor changed the title refactor: encrypted logs refactor feat!: encrypted logs refactor Jan 22, 2025
Copy link
Contributor

github-actions bot commented Jan 22, 2025

Changes to public function bytecode sizes

Generated at commit: 5b4b6107e9a6ce8157c39a585fff7a0438e51d7a, compared to commit: ab2c860c747d3051a1cb85ad6ce5fac2a68867f7

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
Token::finalize_mint_to_private +168 ❌ +3.43%
Token::_finalize_mint_to_private_unsafe +168 ❌ +3.40%
Token::finalize_transfer_to_private +168 ❌ +3.20%
Token::_finalize_transfer_to_private_unsafe +168 ❌ +3.17%
Token::complete_refund +168 ❌ +2.91%
NFT::finalize_transfer_to_private +118 ❌ +2.65%
NFT::_finalize_transfer_to_private_unsafe +118 ❌ +2.62%
Token::public_dispatch +168 ❌ +0.54%
NFT::public_dispatch +118 ❌ +0.52%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
Token::finalize_mint_to_private 5,063 (+168) +3.43%
Token::_finalize_mint_to_private_unsafe 5,110 (+168) +3.40%
Token::finalize_transfer_to_private 5,415 (+168) +3.20%
Token::_finalize_transfer_to_private_unsafe 5,462 (+168) +3.17%
Token::complete_refund 5,939 (+168) +2.91%
NFT::finalize_transfer_to_private 4,569 (+118) +2.65%
NFT::_finalize_transfer_to_private_unsafe 4,616 (+118) +2.62%
Token::public_dispatch 31,313 (+168) +0.54%
NFT::public_dispatch 22,850 (+118) +0.52%

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

I am so sorry about what you had to go through

@@ -334,6 +330,7 @@ comptime fn generate_multi_scalar_mul(
aztec::protocol_types::point::Point { x: $generator_x, y: $generator_y, is_infinite: false }
},
);
// TODO: explain why it's ok to use the unsafe `from_field_unsafe`, here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok? Can we not use the fn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I couldn't determine whether it was safe or not, so I planted that todo as a flag.

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
@iAmMichaelConnor
Copy link
Contributor Author

iAmMichaelConnor commented Jan 25, 2025

I don't know how to diagnose these failed tests. Will need to find help from someone on Monday.
They seem to be an unrelated part of the stack. Maybe timing / polling issues?

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
@iAmMichaelConnor
Copy link
Contributor Author

Converting this to draft. Superseded by #11503 (which is a superset of this PR), which is ready to me merged.

@iAmMichaelConnor iAmMichaelConnor marked this pull request as draft January 25, 2025 22:31
@nventuro nventuro closed this Jan 27, 2025
@nventuro nventuro deleted the mc/potato-encode branch January 27, 2025 17:48
iAmMichaelConnor added a commit that referenced this pull request Feb 20, 2025

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
Addresses the comments from this old PR:
#11400

Introduces Poseidon2 encryption, but doesn't plug it into any example
contracts.

---------

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request Feb 21, 2025
Addresses the comments from this old PR:
AztecProtocol/aztec-packages#11400

Introduces Poseidon2 encryption, but doesn't plug it into any example
contracts.

---------

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
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.

None yet

3 participants