-
Notifications
You must be signed in to change notification settings - Fork 615
feat!: auto-enqueue public init nullifier for contracts with public functions #20775
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
Changes from 2 commits
d588e8c
0e5db75
ad2b242
6a622ce
f748e4b
c2184a6
e2d3423
bcc1d63
7cb5443
1488a52
c3bcee1
e5894af
528e38f
772302b
7e8877d
5b84fbe
39f5d57
c389405
3535ec3
20249d5
8aec344
91cf5c7
dc54c88
a122c76
760a963
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 |
|---|---|---|
| @@ -1,8 +1,15 @@ | ||
| use crate::macros::internals_functions_generation::external_functions_registry::get_public_functions; | ||
| use crate::protocol::meta::utils::get_params_len_quote; | ||
| use crate::utils::cmap::CHashMap; | ||
| use super::utils::compute_fn_selector; | ||
| use std::panic; | ||
| use super::utils::{compute_fn_selector, module_has_initializer}; | ||
| use std::meta::unquote; | ||
|
|
||
| pub(crate) comptime fn compute_emit_public_init_nullifier_selector() -> Field { | ||
| let computation_quote = quote { | ||
| crate::protocol::traits::ToField::to_field(crate::protocol::abis::function_selector::FunctionSelector::from_signature("__emit_public_init_nullifier()")) | ||
| }; | ||
| unquote!(computation_quote) | ||
| } | ||
|
|
||
| /// Returns an `fn public_dispatch(...)` function for the given module that's assumed to be an Aztec contract. | ||
| pub comptime fn generate_public_dispatch(m: Module) -> Quoted { | ||
|
|
@@ -100,13 +107,35 @@ pub comptime fn generate_public_dispatch(m: Module) -> Quoted { | |
| // No dispatch function if there are no public functions | ||
| quote {} | ||
| } else { | ||
| // If the module has an initializer, add a dispatch case for the auto-generated | ||
|
Contributor
Author
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. This would the the auto generated function that set the public initializer nullifier. I tried different approaches and this one was the one that required the less changes. I don't love it if I'm being honest, so I'm more than open to suggestions
Contributor
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 not simply create an external only self function and add it to the contract? This is much more fragile
Contributor
Author
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. Sadly, it's not that simple. I explained the issue in |
||
| // __emit_public_init_nullifier function. This function is called by private initializers to emit | ||
| // the public init nullifier, ensuring public functions see the contract as initialized. | ||
| let emit_init_nullifier_case = if module_has_initializer(m) { | ||
| let emit_selector: Field = compute_emit_public_init_nullifier_selector(); | ||
| quote { | ||
| if selector == $emit_selector { | ||
| // This function takes no args, so the args_hash closure is unused (returns zero). | ||
| let context = aztec::context::PublicContext::new(|| { 0 }); | ||
| assert( | ||
| context.maybe_msg_sender().unwrap() == context.this_address(), | ||
| "Function __emit_public_init_nullifier can only be called by the same contract", | ||
| ); | ||
| aztec::macros::functions::initialization_utils::mark_as_initialized_public(context); | ||
| aztec::oracle::avm::avm_return([]); | ||
| } | ||
| } | ||
| } else { | ||
| quote {} | ||
| }; | ||
|
|
||
| let ifs = ifs.push_back(quote { panic(f"Unknown selector {selector}") }); | ||
| let dispatch = ifs.join(quote { }); | ||
|
|
||
| let body = quote { | ||
| // We mark this as public because our whole system depends on public functions having this attribute. | ||
| #[aztec::macros::internals_functions_generation::abi_attributes::abi_public] | ||
| pub unconstrained fn public_dispatch(selector: Field) { | ||
| $emit_init_nullifier_case | ||
| $dispatch | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| use crate::protocol::{ | ||
| abis::function_selector::FunctionSelector, address::AztecAddress, constants::DOM_SEP__INITIALIZER, | ||
| hash::poseidon2_hash_with_separator, traits::ToField, | ||
| abis::function_selector::FunctionSelector, | ||
| address::AztecAddress, | ||
| constants::{DOM_SEP__INITIALIZER, DOM_SEP__PUBLIC_INITIALIZER}, | ||
| hash::poseidon2_hash_with_separator, | ||
| traits::ToField, | ||
| }; | ||
|
|
||
| use crate::{ | ||
|
|
@@ -11,37 +14,67 @@ use crate::{ | |
| }, | ||
| }; | ||
|
|
||
| // Used by `create_mark_as_initialized` (you won't find it through searching) | ||
| // Two separate init nullifiers exist because private nullifiers are committed before public execution | ||
| // begins. If a private initializer emitted a single nullifier, public functions in the same tx would | ||
| // see it via the pending nullifier set and incorrectly consider the contract initialized before the | ||
| // public phase runs. | ||
| // | ||
| // - Private init nullifier: checked by `assert_is_initialized_private`, emitted by private initializers. | ||
| // - Public init nullifier: checked by `assert_is_initialized_public`, emitted during the public phase. | ||
| // | ||
| // Private initializers emit only the private nullifier, then enqueue `__emit_public_init_nullifier` | ||
| // (an auto-generated dispatch case) so the public nullifier is emitted during the public phase. | ||
| // Public initializers emit both nullifiers directly via `mark_as_initialized_from_public_initializer`. | ||
|
nchamo marked this conversation as resolved.
Outdated
|
||
|
|
||
| /// Emits the public init nullifier only. Called by the auto-generated `__emit_public_init_nullifier` | ||
| /// dispatch case, which is enqueued by private initializers. | ||
|
nchamo marked this conversation as resolved.
Outdated
|
||
| pub fn mark_as_initialized_public(context: PublicContext) { | ||
| let init_nullifier = compute_unsiloed_contract_initialization_nullifier((context).this_address()); | ||
| let init_nullifier = compute_public_init_nullifier(context.this_address()); | ||
| context.push_nullifier(init_nullifier); | ||
| } | ||
|
|
||
| // Used by `create_mark_as_initialized` (you won't find it through searching) | ||
| /// Enqueues a public call to `__emit_public_init_nullifier` so the public init nullifier is emitted | ||
| /// during the public phase. Called by private initializer macros for contracts that have public functions. | ||
| pub fn enqueue_emit_public_init_nullifier(context: &mut PrivateContext) { | ||
| let selector = FunctionSelector::from_signature("__emit_public_init_nullifier()"); | ||
|
nchamo marked this conversation as resolved.
Outdated
|
||
| context.call_public_function(context.this_address(), selector, [], false); | ||
| } | ||
|
|
||
| /// Emits the private init nullifier only. Called by private initializer macros. | ||
| pub fn mark_as_initialized_private(context: &mut PrivateContext) { | ||
| let init_nullifier = compute_unsiloed_contract_initialization_nullifier((*context).this_address()); | ||
| let init_nullifier = compute_private_init_nullifier((*context).this_address()); | ||
| context.push_nullifier(init_nullifier); | ||
| } | ||
|
|
||
| // Used by `create_init_check` (you won't find it through searching) | ||
| /// Emits both init nullifiers. Called by public initializer macros, since public initializers must | ||
| /// set both so that both private and public functions see the contract as initialized. | ||
| pub fn mark_as_initialized_from_public_initializer(context: PublicContext) { | ||
| let private_nullifier = compute_private_init_nullifier(context.this_address()); | ||
| context.push_nullifier(private_nullifier); | ||
| mark_as_initialized_public(context); | ||
| } | ||
|
|
||
| /// Checks that the public init nullifier exists (i.e. the contract has been initialized from public's perspective). | ||
| pub fn assert_is_initialized_public(context: PublicContext) { | ||
| let init_nullifier = compute_unsiloed_contract_initialization_nullifier(context.this_address()); | ||
| // Safety: TODO(F-239) - this is currently unsafe, we cannot rely on the nullifier existing to determine that any | ||
| // public component of contract initialization has been complete. | ||
| let init_nullifier = compute_public_init_nullifier(context.this_address()); | ||
|
nchamo marked this conversation as resolved.
|
||
| assert(context.nullifier_exists_unsafe(init_nullifier, context.this_address()), "Not initialized"); | ||
| } | ||
|
|
||
| // Used by `create_init_check` (you won't find it through searching) | ||
| /// Checks that the private init nullifier exists (i.e. the contract has been initialized from private's perspective). | ||
| pub fn assert_is_initialized_private(context: &mut PrivateContext) { | ||
| let init_nullifier = compute_unsiloed_contract_initialization_nullifier(context.this_address()); | ||
| let init_nullifier = compute_private_init_nullifier(context.this_address()); | ||
| let nullifier_existence_request = compute_nullifier_existence_request(init_nullifier, context.this_address()); | ||
| context.assert_nullifier_exists(nullifier_existence_request); | ||
| } | ||
|
|
||
| fn compute_unsiloed_contract_initialization_nullifier(address: AztecAddress) -> Field { | ||
| fn compute_private_init_nullifier(address: AztecAddress) -> Field { | ||
| address.to_field() | ||
|
Contributor
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. (not just that, this is also not domain separated) |
||
| } | ||
|
|
||
| fn compute_public_init_nullifier(address: AztecAddress) -> Field { | ||
| poseidon2_hash_with_separator([address.to_field()], DOM_SEP__PUBLIC_INITIALIZER) | ||
| } | ||
|
nchamo marked this conversation as resolved.
|
||
|
|
||
| // Used by `create_assert_correct_initializer_args` (you won't find it through searching) | ||
| pub fn assert_initialization_matches_address_preimage_public(context: PublicContext) { | ||
| let address = context.this_address(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -379,6 +379,12 @@ comptime fn assert_valid_public(f: FunctionDefinition) { | |
| f"The #[allow_phase_change] attribute cannot be applied to #[external(\"public\")] functions - {name}", | ||
| ); | ||
| } | ||
|
|
||
| if f.name() == quote { __emit_public_init_nullifier } { | ||
|
Contributor
Author
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. We are now adding this check and making sure that end users can't write a function with this name. Are we ok with this?
Contributor
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. Yes, this check is a good idea. Are we following this pattern elsewhere? This is the kind of error that should have a link to the docsite with an error code.
Contributor
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 don't think we are ever checking a name as the only external functions we are generating are
Sounds like a good idea to also add here a check for the public dispatch name collision to get the same kind of error message.
Contributor
Author
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. @nventuro, are we doing that somewhere else so that I can understand how to properly link to docs with an error code?
Contributor
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 in #20893, I don't like the mechanism of not injecting if the fn name already exists - it's too implicit. I'm in favor of erroring if the user defines, and providing an explicit mechanism for an alterantive impl. |
||
| panic( | ||
| f"Function name '__emit_public_init_nullifier' is reserved for internal use", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| comptime fn assert_valid_utility(f: FunctionDefinition) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [package] | ||
| name = "reserved_emit_public_init_nullifier" | ||
| type = "contract" | ||
| authors = [""] | ||
|
|
||
| [dependencies] | ||
| aztec = { path = "../../../aztec" } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /// Public functions cannot use the reserved name __emit_public_init_nullifier. | ||
| /// | ||
| use aztec::macros::aztec; | ||
|
|
||
| #[aztec] | ||
| contract ReservedEmitPublicInitNullifier { | ||
| use aztec::macros::functions::external; | ||
|
|
||
| #[external("public")] | ||
| fn __emit_public_init_nullifier() {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ pub contract StatefulTest { | |
| use aztec::macros::{ | ||
| functions::{ | ||
| external, initialization_utils::assert_is_initialized_private, initializer, noinitcheck, | ||
| view, | ||
| only_self, view, | ||
| }, | ||
| storage::storage, | ||
| }; | ||
|
|
@@ -43,6 +43,39 @@ pub contract StatefulTest { | |
| self.call_self.increment_public_value_no_init_check(owner, value); | ||
| } | ||
|
|
||
| #[external("private")] | ||
| #[initializer] | ||
| fn constructor_self_calling_init_checked(owner: AztecAddress, value: Field) { | ||
| self.call_self.create_note(owner, value); | ||
| } | ||
|
Contributor
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 add these here? Create a new contract with the specific behavior you want to test (e.g. with and without public fns) |
||
|
|
||
| #[external("private")] | ||
| #[initializer] | ||
| fn constructor_enqueuing_public_self_call(owner: AztecAddress, value: Field) { | ||
| self.enqueue_self.increment_public_value(owner, value); | ||
| } | ||
|
|
||
| #[external("public")] | ||
| #[initializer] | ||
| fn public_constructor_self_calling_init_checked(owner: AztecAddress, value: Field) { | ||
| self.call_self.increment_public_value(owner, value); | ||
| } | ||
|
|
||
| #[external("private")] | ||
| #[only_self] | ||
| fn only_self_create_note(owner: AztecAddress, value: Field) { | ||
| if (value != 0) { | ||
| let note = FieldNote { value }; | ||
| self.storage.notes.at(owner).insert(note).deliver(MessageDelivery.ONCHAIN_CONSTRAINED); | ||
| } | ||
| } | ||
|
|
||
| #[external("private")] | ||
| #[initializer] | ||
| fn constructor_calling_only_self(owner: AztecAddress, value: Field) { | ||
| self.call_self.only_self_create_note(owner, value); | ||
| } | ||
|
|
||
| #[external("private")] | ||
| fn create_note(owner: AztecAddress, value: Field) { | ||
| if (value != 0) { | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.