feat: unconstraining keys in unconstrained encryption (#7912)

noir-projects/aztec-nr/aztec/src/encrypted_logs/encrypted_note_emission.nr

@@ -1,71 +1,29 @@
 use crate::{
     context::PrivateContext, note::{note_emission::NoteEmission, note_interface::NoteInterface},
-    keys::{getters::get_current_public_keys, public_keys::{OvpkM, IvpkM}},
+    keys::{getters::{get_current_public_keys, get_ovsk_app}, public_keys::{OvpkM, IvpkM}},
     encrypted_logs::payload::compute_encrypted_note_log, oracle::logs_traits::LensForEncryptedLog
 };
 use dep::protocol_types::{hash::sha256_to_field, address::AztecAddress, abis::note_hash::NoteHash};
 
-unconstrained fn compute_unconstrained<Note, let N: u32, let NB: u32, let M: u32>(
-    contract_address: AztecAddress,
-    storage_slot: Field,
-    ovsk_app: Field,
-    ovpk: OvpkM,
-    ivpk: IvpkM,
-    recipient: AztecAddress,
-    note: Note
-) -> ([u8; M], Field) where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
-    compute(
-        contract_address,
-        storage_slot,
-        ovsk_app,
-        ovpk,
-        ivpk,
-        recipient,
-        note
-    )
-}
-
-fn compute<Note, let N: u32, let NB: u32, let M: u32>(
-    contract_address: AztecAddress,
-    storage_slot: Field,
-    ovsk_app: Field,
-    ovpk: OvpkM,
-    ivpk: IvpkM,
-    recipient: AztecAddress,
-    note: Note
-) -> ([u8; M], Field) where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
-    let encrypted_log: [u8; M] = compute_encrypted_note_log(
-        contract_address,
-        storage_slot,
-        ovsk_app,
-        ovpk,
-        ivpk,
-        recipient,
-        note
-    );
-    let log_hash = sha256_to_field(encrypted_log);
-    (encrypted_log, log_hash)
-}
-
-fn emit_with_keys<Note, let N: u32, let NB: u32, let M: u32>(
-    context: &mut PrivateContext,
+fn compute_raw_note_log<Note, let N: u32, let NB: u32, let M: u32>(
+    context: PrivateContext,
     note: Note,
+    ovsk_app: Field,
     ovpk: OvpkM,
     ivpk: IvpkM,
-    recipient: AztecAddress,
-    inner_compute: fn(AztecAddress, Field, Field, OvpkM, IvpkM, AztecAddress, Note) -> ([u8; M], Field)
-) where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
+    recipient: AztecAddress
+) -> (u32, [u8; M], Field) where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
     let note_header = note.get_header();
     let note_hash_counter = note_header.note_hash_counter;
     let storage_slot = note_header.storage_slot;
 
+    // TODO(#8589): use typesystem to skip this check when not needed
     let note_exists = context.note_hashes.storage.any(|n: NoteHash| n.counter == note_hash_counter);
     assert(note_exists, "Can only emit a note log for an existing note.");
 
     let contract_address: AztecAddress = context.this_address();
-    let ovsk_app: Field  = context.request_ovsk_app(ovpk.hash());
 
-    let (encrypted_log, log_hash) = inner_compute(
+    let encrypted_log: [u8; M] = compute_encrypted_note_log(
         contract_address,
         storage_slot,
         ovsk_app,
@@ -74,8 +32,20 @@ fn emit_with_keys<Note, let N: u32, let NB: u32, let M: u32>(
         recipient,
         note
     );
+    let log_hash = sha256_to_field(encrypted_log);
+
+    (note_hash_counter, encrypted_log, log_hash)
+}
 
-    context.emit_raw_note_log(note_hash_counter, encrypted_log, log_hash);
+unconstrained fn compute_raw_note_log_unconstrained<Note, let N: u32, let NB: u32, let M: u32>(
+    context: PrivateContext,
+    note: Note,
+    ovpk: OvpkM,
+    ivpk: IvpkM,
+    recipient: AztecAddress
+) -> (u32, [u8; M], Field) where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
+    let ovsk_app = get_ovsk_app(ovpk.hash());
+    compute_raw_note_log(context, note, ovsk_app, ovpk, ivpk, recipient)
 }
 
 pub fn encode_and_encrypt_note<Note, let N: u32, let NB: u32, let M: u32>(
@@ -86,7 +56,10 @@ pub fn encode_and_encrypt_note<Note, let N: u32, let NB: u32, let M: u32>(
     | e: NoteEmission<Note> | {
         let ovpk = get_current_public_keys(context, ov).ovpk_m;
         let ivpk = get_current_public_keys(context, iv).ivpk_m;
-        emit_with_keys(context, e.note, ovpk, ivpk, iv, compute);
+        let ovsk_app: Field  = context.request_ovsk_app(ovpk.hash());
+
+        let (note_hash_counter, encrypted_log, log_hash) = compute_raw_note_log(*context, e.note, ovsk_app, ovpk, ivpk, iv);
+        context.emit_raw_note_log(note_hash_counter, encrypted_log, log_hash);
     }
 }
 
@@ -96,9 +69,17 @@ pub fn encode_and_encrypt_note_unconstrained<Note, let N: u32, let NB: u32, let
     iv: AztecAddress
 ) -> fn[(&mut PrivateContext, AztecAddress, AztecAddress)](NoteEmission<Note>) -> () where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
     | e: NoteEmission<Note> | {
+        // Note: We could save a lot of gates by obtaining the following keys in an unconstrained context but this
+        // function is currently not used anywhere so we are not optimizing it.
         let ovpk = get_current_public_keys(context, ov).ovpk_m;
         let ivpk = get_current_public_keys(context, iv).ivpk_m;
-        emit_with_keys(context, e.note, ovpk, ivpk, iv, compute_unconstrained);
+
+        // See the comment in `encode_and_encrypt_note_with_keys_unconstrained` for why having note hash counter
+        // and log hash unconstrained here is fine.
+        let (note_hash_counter, encrypted_log, log_hash) = unsafe {
+            compute_raw_note_log_unconstrained(*context, e.note, ovpk, ivpk, iv)
+        };
+        context.emit_raw_note_log(note_hash_counter, encrypted_log, log_hash);
     }
 }
 
@@ -109,7 +90,10 @@ pub fn encode_and_encrypt_note_with_keys<Note, let N: u32, let NB: u32, let M: u
     recipient: AztecAddress
 ) -> fn[(&mut PrivateContext, OvpkM, IvpkM, AztecAddress)](NoteEmission<Note>) -> () where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
     | e: NoteEmission<Note> | {
-        emit_with_keys(context, e.note, ovpk, ivpk, recipient, compute);
+        let ovsk_app: Field  = context.request_ovsk_app(ovpk.hash());
+
+        let (note_hash_counter, encrypted_log, log_hash) = compute_raw_note_log(*context, e.note, ovsk_app, ovpk, ivpk, recipient);
+        context.emit_raw_note_log(note_hash_counter, encrypted_log, log_hash);
     }
 }
 
@@ -120,6 +104,28 @@ pub fn encode_and_encrypt_note_with_keys_unconstrained<Note, let N: u32, let NB:
     recipient: AztecAddress
 ) -> fn[(&mut PrivateContext, OvpkM, IvpkM, AztecAddress)](NoteEmission<Note>) -> () where Note: NoteInterface<N, NB>, [Field; N]: LensForEncryptedLog<N, M> {
     | e: NoteEmission<Note> | {
-        emit_with_keys(context, e.note, ovpk, ivpk, recipient, compute_unconstrained);
+        //   Having the log hash be unconstrained here is fine because the way this works is we send the log hash
+        // to the kernel, and it gets included as part of its public inputs. Then we send the tx to the sequencer,
+        // which includes the kernel proof and the log preimages. The sequencer computes the hashes of the logs
+        // and checks that they are the ones in the public inputs of the kernel, and drops the tx otherwise (proposing
+        // the block on L1 would later fail if it didn't because of txs effects hash mismatch).
+        //   So if we don't constrain the log hash, then a malicious sender can compute the correct log, submit a bad
+        // log hash to the kernel, and then submit the bad log preimage to the sequencer. All checks will pass, but
+        // the submitted log will not be the one that was computed by the app.
+        //   In the unconstrained case, we don't care about the log at all because we don't do anything with it,
+        // and because it's unconstrained: it could be anything. So if a sender chooses to broadcast the tx with a log
+        // that is different from the one that was used in the circuit, then they'll be able to, but they were already
+        // able to change the log before anyway, so the end result is the same. It's important here that we do not
+        // return the log from this function to the app, otherwise it could try to do stuff with it and then that might
+        // be wrong.
+        //   Regarding the note hash counter, this is used for squashing. The kernel assumes that a given note can have
+        // more than one log and removes all of the matching ones, so all a malicious sender could do is either: cause
+        // for the log to be deleted when it shouldn't have (which is fine - they can already make the content be
+        // whatever), or cause for the log to not be deleted when it should have (which is also fine - it'll be a log
+        // for a note that doesn't exist).
+        let (note_hash_counter, encrypted_log, log_hash) = unsafe {
+            compute_raw_note_log_unconstrained(*context, e.note, ovpk, ivpk, recipient)
+        };
+        context.emit_raw_note_log(note_hash_counter, encrypted_log, log_hash);
     }
 }

noir-projects/aztec-nr/aztec/src/keys/getters/mod.nr

@@ -5,7 +5,10 @@ use dep::protocol_types::{
 use crate::{
     context::{PrivateContext, UnconstrainedContext},
     oracle::{keys::get_public_keys_and_partial_address, key_validation_request::get_key_validation_request},
-    keys::{public_keys::{PublicKeys, PUBLIC_KEYS_LENGTH}, stored_keys::StoredKeys, constants::NULLIFIER_INDEX},
+    keys::{
+    public_keys::{PublicKeys, PUBLIC_KEYS_LENGTH}, stored_keys::StoredKeys,
+    constants::{NULLIFIER_INDEX, OUTGOING_INDEX}
+},
     state_vars::{public_mutable::PublicMutable, map::Map}
 };
 
@@ -17,10 +20,21 @@ global KEY_REGISTRY_UPDATE_BLOCKS = 5;
 
 global KEY_REGISTRY_STORAGE_SLOT = 1;
 
+// A helper function that gets app-siloed nullifier secret key for a given `npk_m_hash`. This function is used
+// in unconstrained contexts only - in Note::compute_nullifier_without_context which in turn is called by
+// `compute_note_hash_and_optionally_a_nullifier` function that is used by the NoteProcessor. The safe alternative
+// is `request_nsk_app` function define on `PrivateContext`.
 unconstrained pub fn get_nsk_app(npk_m_hash: Field) -> Field {
     get_key_validation_request(npk_m_hash, NULLIFIER_INDEX).sk_app
 }
 
+// A helper function that gets app-siloed outgoing viewing key for a given `ovpk_m_hash`. This function is used
+// in unconstrained contexts only - when computing unconstrained note logs. The safe alternative is `request_ovsk_app`
+// function defined on `PrivateContext`.
+unconstrained pub fn get_ovsk_app(ovpk_m_hash: Field) -> Field {
+    get_key_validation_request(ovpk_m_hash, OUTGOING_INDEX).sk_app
+}
+
 // Returns all current public keys for a given account, applying proper constraints to the context. We read all
 // keys at once since the constraints for reading them all are actually fewer than if we read them one at a time - any
 // read keys that are not required by the caller can simply be discarded.

noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr

@@ -13,7 +13,8 @@ contract TokenBlacklist {
     use dep::aztec::{
         hash::compute_secret_hash,
         prelude::{AztecAddress, Map, NoteGetterOptions, PrivateSet, PublicMutable, SharedMutable},
-        encrypted_logs::encrypted_note_emission::encode_and_encrypt_note, utils::comparison::Comparator
+        encrypted_logs::encrypted_note_emission::{encode_and_encrypt_note, encode_and_encrypt_note_unconstrained},
+        utils::comparison::Comparator
     };
 
     use dep::authwit::{auth::{assert_current_call_valid_authwit, assert_current_call_valid_authwit_public}};
@@ -214,8 +215,8 @@ contract TokenBlacklist {
         }
 
         let amount = U128::from_integer(amount);
-        storage.balances.sub(from, amount).emit(encode_and_encrypt_note(&mut context, from, from));
-        storage.balances.add(to, amount).emit(encode_and_encrypt_note(&mut context, from, to));
+        storage.balances.sub(from, amount).emit(encode_and_encrypt_note_unconstrained(&mut context, from, from));
+        storage.balances.add(to, amount).emit(encode_and_encrypt_note_unconstrained(&mut context, from, to));
     }
 
     #[aztec(private)]

yarn-project/end-to-end/src/e2e_keys.test.ts

@@ -63,9 +63,8 @@ describe('Key Registry', () => {
     //    There are some examples where the action is fully hidden though. One of those examples is shielding where you
     // instantly consume the note after creating it. In this case, the nullifier is never emitted and hence the action
     // is impossible to detect with this scheme.
-    //    Another example is a withdraw is withdrawing from DeFi and then immediately spending the funds. In this case,
-    // we would need nsk_app and the contract address of the DeFi contract to detect the nullification of the initial
-    // note.
+    //    Another example is withdrawing from DeFi and then immediately spending the funds. In this case, we would
+    // need nsk_app and the contract address of the DeFi contract to detect the nullification of the initial note.
     it('nsk_app and contract address are enough to detect note nullification', async () => {
       const masterNullifierSecretKey = deriveMasterNullifierSecretKey(secret);
       const nskApp = computeAppNullifierSecretKey(masterNullifierSecretKey, testContract.address);

yarn-project/sequencer-client/src/tx_validator/metadata_validator.ts

@@ -51,7 +51,11 @@ export class MetadataTxValidator<T extends AnyTx> implements TxValidator<T> {
     const maxBlockNumber = target.maxBlockNumber;
 
     if (maxBlockNumber.isSome && maxBlockNumber.value < this.#globalVariables.blockNumber) {
-      this.#log.warn(`Rejecting tx ${Tx.getHash(tx)} for low max block number`);
+      this.#log.warn(
+        `Rejecting tx ${Tx.getHash(tx)} for low max block number. Tx max block number: ${
+          maxBlockNumber.value
+        }, current block number: ${this.#globalVariables.blockNumber}.`,
+      );
       return false;
     } else {
       return true;