-
Notifications
You must be signed in to change notification settings - Fork 270
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: aztec nr lib constraining nullifier key is fresh #5939
Changes from 26 commits
3acc686
1f53bd5
177445f
fdbb60a
2d3a95f
58f9650
42a9c35
f82d1c8
ea93350
781bbe3
3201fd4
1b13804
211a060
29bfb80
7d1620d
f0ece69
0461006
e4f6d15
f9a844b
0e0b63e
7572be2
9051c8e
6e3509a
6fbd1d9
711afac
7f5dbfc
0ca8578
c852740
3b9e7ba
b76ab1a
7eb2366
e2a479a
59a4c18
194df19
938761a
0baf281
a85f513
94db9f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
mod assert_public_key_freshness; | ||
|
||
use crate::keys::assert_public_key_freshness::assert_nullifier_public_key_is_fresh; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
use dep::protocol_types::{ | ||
address::{ | ||
AztecAddress, | ||
PartialAddress | ||
}, | ||
constants::{ | ||
GENERATOR_INDEX__PUBLIC_KEYS_HASH, | ||
GENERATOR_INDEX__CONTRACT_ADDRESS_V1, | ||
CANONICAL_KEY_REGISTRY_ADDRESS | ||
}, | ||
grumpkin_point::GrumpkinPoint, | ||
}; | ||
|
||
use crate::context::PrivateContext; | ||
use crate::hash::{ | ||
pedersen_hash, | ||
poseidon2_hash, | ||
}; | ||
use crate::oracle; | ||
use crate::state_vars::shared_mutable::shared_mutable_private_getter::SharedMutablePrivateGetter; | ||
|
||
struct PublicKeyTypeEnum { | ||
NULLIFIER: u8, | ||
} | ||
|
||
global PublicKeyType = PublicKeyTypeEnum { | ||
NULLIFIER: 0, | ||
}; | ||
|
||
pub fn assert_nullifier_public_key_is_fresh( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed yesterday, think it would be better if we just have the getter here that is doing the assertions. In my mental model, you want to retrieve it for a user when you are populating the note, so having just one function do it instead of doing a retrieval and then a check that is doing the same internally seems nicer in my mind. A similar change was made with the public storage some time back as an application can then easily add an explicit check if it want to see if it is a specific value etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, addressed as part of a stack, but popped into this pr for ease of review. Addressed in |
||
context: &mut PrivateContext, | ||
address: AztecAddress, | ||
nullifier_public_key_to_test: GrumpkinPoint, | ||
) { | ||
// This is the storage slot of the nullifier_public_key inside the key registry contract | ||
let storage_slot_of_nullifier_public_key = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to canonical registry that we have the slot hardcoded here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On this, could we not expose "global" values on the struct directly? Seems like it could be super sleek it we could do something like
@Thunkar do you have any thoughts here? I have actually not checked if this is already possible with the code you made 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed the original comment in |
||
// We have to derive this slot to get the location of the shared mutable inside the Map | ||
// This should mimic how maps derive their slots | ||
let derived_slot = pedersen_hash( | ||
[storage_slot_of_nullifier_public_key, address.to_field()], | ||
0 | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having hardcoded the inner logic of a Map here would result in hard to debug bug if something got changed there. I would introduce a derive_map_storage_slot function and use it from both Map and here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍. Addressed in |
||
|
||
// It's a bit wonky because we need to know the delay for get_current_value_in_private to work correctly | ||
// We read from the canonical Key Registry | ||
let registry_private_getter: SharedMutablePrivateGetter<Field, 5> = SharedMutablePrivateGetter::new(*context, AztecAddress::from_field(CANONICAL_KEY_REGISTRY_ADDRESS), derived_slot); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would also add comment to registry that we use the value here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
let hashed_nullifier_public_key_in_registry = registry_private_getter.get_current_value_in_private(); | ||
|
||
// In the case that the value is not found in the registry we need to manually pass in the address preimage | ||
if (hashed_nullifier_public_key_in_registry == 0) { | ||
check_public_key_validity(address, PublicKeyType.NULLIFIER, nullifier_public_key_to_test); | ||
} else { | ||
assert(hashed_nullifier_public_key_in_registry == poseidon2_hash(nullifier_public_key_to_test.serialize())); | ||
} | ||
} | ||
|
||
fn check_public_key_validity(address: AztecAddress, key_type: u8, key: GrumpkinPoint) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the comment below, think this could be done inline, seems a bit unnecessary to have these two separate functions when it is as small as they are. Seems like it would just be two lines instead of two functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
let keys = get_public_keys_internal(address); | ||
|
||
assert(keys[key_type].eq(key)); | ||
} | ||
|
||
fn get_public_keys_internal(address: AztecAddress) -> [GrumpkinPoint; 4] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like having this function just makes it harder to read then if you just had line 64 on line 58 but maybe it's just a matter of taste There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think it is mostly taste, but I agree, if not using more than once would just insert it directly in there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also gree with your matter of taste, I thought it was convention to do it this way but I guess I was wrong. Addressed in |
||
let (_, public_keys) = oracle::keys::get_public_keys_and_partial_address(address); | ||
|
||
public_keys | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ mod deploy; | |
mod hash; | ||
mod history; | ||
mod initializer; | ||
mod keys; | ||
mod log; | ||
mod messaging; | ||
mod note; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
use dep::protocol_types::{ | ||
address::{ | ||
AztecAddress, | ||
PartialAddress, | ||
PublicKeysHash, | ||
}, | ||
constants::{ | ||
GENERATOR_INDEX__PUBLIC_KEYS_HASH, | ||
GENERATOR_INDEX__CONTRACT_ADDRESS_V1, | ||
}, | ||
grumpkin_point::GrumpkinPoint, | ||
}; | ||
|
||
use crate::hash::poseidon2_hash; | ||
|
||
#[oracle(getPublicKeysAndPartialAddress)] | ||
fn get_public_keys_and_partial_address_oracle(_address: AztecAddress) -> [Field; 9] {} | ||
|
||
unconstrained fn get_public_keys_and_partial_address_oracle_wrapper(address: AztecAddress) -> [Field; 9] { | ||
get_public_keys_and_partial_address_oracle(address) | ||
} | ||
|
||
pub fn get_public_keys_and_partial_address(address: AztecAddress) -> (PartialAddress, [GrumpkinPoint; 4]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you aware of us needing the partial address somewhere? As far as I know the whole purpose of it is to be able to check the preimage of aztec address which we only do internally in the function so I don't think it makes sense to return here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
let result = get_public_keys_and_partial_address_oracle_wrapper(address); | ||
|
||
let partial_address = PartialAddress::from_field(result[0]); | ||
let nullifier_pub_key = GrumpkinPoint::new(result[1], result[2]); | ||
let incoming_pub_key = GrumpkinPoint::new(result[3], result[4]); | ||
let outgoing_pub_key = GrumpkinPoint::new(result[5], result[6]); | ||
let tagging_pub_key = GrumpkinPoint::new(result[7], result[8]); | ||
|
||
_check_public_key_validity_constrain_oracle( | ||
address, | ||
partial_address, | ||
nullifier_pub_key, | ||
incoming_pub_key, | ||
outgoing_pub_key, | ||
tagging_pub_key, | ||
); | ||
|
||
(partial_address, [nullifier_pub_key, incoming_pub_key, outgoing_pub_key, tagging_pub_key]) | ||
} | ||
|
||
fn _check_public_key_validity_constrain_oracle( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would likely be easier to follow, if you have that the function is returning the address instead, and then constrain it in the caller. Would then have function that is essentially take the preimage and compue, which should be similar to what we have other places (as jan mentioned) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, addresssed in |
||
address: AztecAddress, | ||
partial_address: PartialAddress, | ||
nullifier_public_key: GrumpkinPoint, | ||
incoming_public_key: GrumpkinPoint, | ||
outgoing_public_key: GrumpkinPoint, | ||
tagging_public_key: GrumpkinPoint | ||
) { | ||
let public_keys_hash = poseidon2_hash([ | ||
nullifier_public_key.serialize()[0], | ||
nullifier_public_key.serialize()[1], | ||
incoming_public_key.serialize()[0], | ||
incoming_public_key.serialize()[1], | ||
outgoing_public_key.serialize()[0], | ||
outgoing_public_key.serialize()[1], | ||
tagging_public_key.serialize()[0], | ||
tagging_public_key.serialize()[1], | ||
GENERATOR_INDEX__PUBLIC_KEYS_HASH, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just introduce a compute_public_keys_hash function and use it in both the registry and here to avoid pain during changes. I would also expect that calling serialize() twice on each key would result in a lot of unnecessary gates and I think it would be better to just directly create the array of preimage as @vezenovm can you confirm that I am not proposing non-sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed both points in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I imagine |
||
]); | ||
|
||
// TODO: #5830: we can compute this like below once refactored | ||
// let calculated_address = AztecAddress::compute(PublicKeysHash::compute(pub_key), partial_address); | ||
|
||
let computed_address = AztecAddress::from_field( | ||
poseidon2_hash([ | ||
partial_address.to_field(), | ||
public_keys_hash.to_field(), | ||
GENERATOR_INDEX__CONTRACT_ADDRESS_V1, | ||
]) | ||
); | ||
|
||
assert(computed_address.eq(address)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
"moduleNameMapper": { | ||
"^(\\.{1,2}/.*)\\.js$": "$1" | ||
}, | ||
"reporters": [["default", {"summaryThreshold": 9999}]], | ||
"reporters": [["default", { "summaryThreshold": 9999 }]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like just a good ol formatting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lasse was right, via yarn format. Not sure why though its a diff though. Reverted it in |
||
"testRegex": "./src/.*\\.test\\.ts$", | ||
"rootDir": "./src" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there different number of spaces per tab between users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will stick to four from now on 🙃