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: removing superfluous call to MSM #7708

Merged
merged 3 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions noir-projects/aztec-nr/aztec/src/generators.nr
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,6 @@ global Ga3 = Point { x: 0x0edb1e293c3ce91bfc04e3ceaa50d2c541fa9d091c72eb403efb1c
// If you change this update `G_SLOT` in `yarn-project/simulator/src/client/test_utils.ts` as well
global G_slot = Point { x: 0x041223147b680850dc82e8a55a952d4df20256fe0593d949a9541ca00f0abf15, y: 0x0a8c72e60d0e60f5d804549d48f3044d06140b98ed717a9b532af630c1530791, is_infinite: false };

// TODO(#7551): nuke this func - is only temporarily used in note_interface.rs before we get some AVM compatible
// hash func hashing to a point
pub fn field_to_point(slot_preimage: Field) -> Point {
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let slot_preimage_scalar = dep::std::hash::from_field_unsafe(slot_preimage);
dep::std::embedded_curve_ops::multi_scalar_mul([G_slot], [slot_preimage_scalar])
}

mod test {
use crate::generators::{Ga1, Ga2, Ga3, G_slot};
use dep::protocol_types::point::Point;
Expand Down
3 changes: 2 additions & 1 deletion noir-projects/aztec-nr/aztec/src/hash.nr
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ use dep::protocol_types::{
GENERATOR_INDEX__SECRET_HASH, GENERATOR_INDEX__MESSAGE_NULLIFIER, ARGS_HASH_CHUNK_COUNT,
GENERATOR_INDEX__FUNCTION_ARGS, ARGS_HASH_CHUNK_LENGTH, MAX_ARGS_LENGTH
},
traits::Hash, hash::{pedersen_hash, compute_siloed_nullifier, sha256_to_field}
traits::Hash, hash::{pedersen_hash, compute_siloed_nullifier, sha256_to_field, pedersen_commitment}
};
// Note: pedersen_commitment is used only as a re-export here
use crate::oracle::logs_traits::{LensForEncryptedLog, ToBytesForUnencryptedLog};

pub fn compute_secret_hash(secret: Field) -> Field {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ use dep::aztec::{
keys::getters::get_nsk_app
};

// TODO(#7738): Nuke the following imports
use dep::aztec::{
generators::{Ga1 as G_amt, Ga2 as G_npk, Ga3 as G_rnd},
protocol_types::{point::Point, scalar::Scalar}
};
use dep::std::{embedded_curve_ops::multi_scalar_mul, hash::from_field_unsafe};

trait OwnedNote {
fn new(amount: U128, owner_npk_m_hash: Field) -> Self;
fn get_amount(self) -> U128;
Expand Down Expand Up @@ -48,6 +55,21 @@ impl NoteInterface<TOKEN_NOTE_LEN, TOKEN_NOTE_BYTES_LEN> for TokenNote {
]);
(note_hash_for_nullify, nullifier)
}

Copy link
Contributor Author

@benesjan benesjan Aug 2, 2024

Choose a reason for hiding this comment

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

I've manually implemented this function here because of #7738

This is a good tradeoff because it makes the token be more closer to final version when it comes to gate counts (because in the future we will most likely make the macros generate this function such that it does exactly what this impl does). On master now this function does 2 MSM calls which is very expensive and can skew our measurements significantly (@nventuro this is possibly why you so the gate counts rise yesterday I merged the PR with 2 MSM calls yesterday).

// TODO(#7738): Nuke this function and have it auto-generated by macros
fn compute_note_hiding_point(self) -> Point {
let npk_m_hash_scalar = from_field_unsafe(self.npk_m_hash);
let randomness_scalar = from_field_unsafe(self.randomness);
multi_scalar_mul(
[G_amt, G_npk, G_rnd],
[Scalar {
lo: self.amount.to_integer(),
hi: 0
},
npk_m_hash_scalar,
randomness_scalar]
)
}
}

impl OwnedNote for TokenNote {
Expand Down
11 changes: 7 additions & 4 deletions noir-projects/noir-protocol-circuits/crates/types/src/hash.nr
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ use crate::{
},
merkle_tree::root::root_from_sibling_path, messaging::l2_to_l1_message::ScopedL2ToL1Message,
recursion::verification_key::VerificationKey, traits::is_empty,
utils::field::field_from_bytes_32_trunc
utils::field::field_from_bytes_32_trunc, point::Point
};
use std::hash::{pedersen_hash_with_separator, sha256};

pub fn sha256_to_field<let N: u32>(bytes_to_hash: [u8; N]) -> Field {
let sha256_hashed = sha256(bytes_to_hash);
let sha256_hashed = std::hash::sha256(bytes_to_hash);
let hash_in_a_field = field_from_bytes_32_trunc(sha256_hashed);

hash_in_a_field
Expand Down Expand Up @@ -256,6 +255,10 @@ pub fn poseidon2_hash<let N: u32>(inputs: [Field; N]) -> Field {
std::hash::poseidon2::Poseidon2::hash(inputs, N)
}

pub fn pedersen_commitment<let N: u32>(inputs: [Field; N], hash_index: u32) -> Point {
std::hash::pedersen_commitment_with_separator(inputs, hash_index)
}

#[test]
fn smoke_sha256_to_field() {
let full_buffer = [
Expand All @@ -273,7 +276,7 @@ fn smoke_sha256_to_field() {
assert(result == 0x448ebbc9e1a31220a2f3830c18eef61b9bd070e5084b7fa2a359fe729184c7);

// to show correctness of the current ver (truncate one byte) vs old ver (mod full bytes):
let result_bytes = sha256(full_buffer);
let result_bytes = std::hash::sha256(full_buffer);
let truncated_field = crate::utils::field::field_from_bytes_32_trunc(result_bytes);
assert(truncated_field == result);
let mod_res = result + (result_bytes[31] as Field);
Expand Down
6 changes: 1 addition & 5 deletions noir/noir-repo/aztec_macros/src/transforms/note_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,14 +503,10 @@ fn generate_compute_note_hiding_point(
note_type: &String,
impl_span: Option<Span>,
) -> Result<NoirFunction, AztecMacroError> {
// TODO(#7551): Replace the pedersen hash with something that returns a point directly to avoid
// the superfluous call to field_to_point(...). I was trying to use pedersen_commitment for that
// but that is currently not supported by the AVM (but might be soon).
let function_source = format!(
"
fn compute_note_hiding_point(self: {}) -> aztec::protocol_types::point::Point {{
let h = aztec::hash::pedersen_hash(self.serialize_content(), aztec::protocol_types::constants::GENERATOR_INDEX__NOTE_HIDING_POINT);
aztec::generators::field_to_point(h)
aztec::hash::pedersen_commitment(self.serialize_content(), aztec::protocol_types::constants::GENERATOR_INDEX__NOTE_HIDING_POINT)
}}
",
note_type
Expand Down
11 changes: 7 additions & 4 deletions yarn-project/simulator/src/client/simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { type FunctionArtifact, getFunctionArtifact } from '@aztec/foundation/ab
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { poseidon2Hash } from '@aztec/foundation/crypto';
import { Fr, type Point } from '@aztec/foundation/fields';
import { TokenContractArtifact } from '@aztec/noir-contracts.js/Token';
import { TokenBlacklistContractArtifact } from '@aztec/noir-contracts.js';
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 this change here because TokenContract can no longer be used with the standard computeNoteHidingPoint func.


import { type MockProxy, mock } from 'jest-mock-extended';

Expand Down Expand Up @@ -47,10 +47,13 @@ describe('Simulator', () => {
});

describe('computeNoteHashAndOptionallyANullifier', () => {
const artifact = getFunctionArtifact(TokenContractArtifact, 'compute_note_hash_and_optionally_a_nullifier');
const artifact = getFunctionArtifact(
TokenBlacklistContractArtifact,
'compute_note_hash_and_optionally_a_nullifier',
);
const nonce = Fr.random();
const storageSlot = TokenContractArtifact.storageLayout['balances'].slot;
const noteTypeId = TokenContractArtifact.notes['TokenNote'].id;
const storageSlot = TokenBlacklistContractArtifact.storageLayout['balances'].slot;
const noteTypeId = TokenBlacklistContractArtifact.notes['TokenNote'].id;

const createNote = (amount = 123n) => new Note([new Fr(amount), ownerMasterNullifierPublicKey.hash(), Fr.random()]);

Expand Down
11 changes: 6 additions & 5 deletions yarn-project/simulator/src/client/test_utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Fq, Fr, GeneratorIndex, Point } from '@aztec/circuits.js';
import { Grumpkin } from '@aztec/circuits.js/barretenberg';
import { pedersenHash } from '@aztec/foundation/crypto';
import { pedersenCommit } from '@aztec/foundation/crypto';

// Copied over from `noir-projects/aztec-nr/aztec/src/generators.nr`
const G_SLOT = new Point(
Expand All @@ -15,10 +15,11 @@ const G_SLOT = new Point(
* @returns A note hiding point.
*/
export function computeNoteHidingPoint(noteContent: Fr[]): Point {
// TODO(#7551): how this is computed will need to be updated
const h = pedersenHash(noteContent, GeneratorIndex.NOTE_HIDING_POINT);
const grumpkin = new Grumpkin();
return grumpkin.mul(G_SLOT, new Fq(h.toBigInt()));
const c = pedersenCommit(
noteContent.map(f => f.toBuffer()),
GeneratorIndex.NOTE_HIDING_POINT,
);
return new Point(new Fr(c[0]), new Fr(c[1]), false);
}

/**
Expand Down
Loading