feat!: auto-enqueue public init nullifier for contracts with public functions

[nchamo]

Closes F-239

Why we are doing this

Right now, when a contract is initialized (either privately or publicly), it emits a nullifier that is just the contract's address. The main issue with this approach is that it can lead to weird situations like the following:

// Contract A
#[external("private")]
fn should_be_invalid_but_is_not() {
    self.enqueue_self.call_b_public();
    b_contract.private_initializer();
}

#[external("public")]
#[only_self]
fn call_b_public() {
    b_contract.public_call();
}

// Contract B
#[external("private")]
#[initializer]
fn private_initializer() {
    self.enqueue_self.initialize_public();
}

#[external("public")]
#[only_self]
fn initialize_public() {
    ...
}

#[external("public")]
#[only_self]
fn public_call() {
   ...
}

In this scenario, the contract A's should_be_invalid_but_is_not would call contract B's public_call before contract B actually calls it's own initialize_public. This is because:

• Contract B's initializer is emitted at private_initializer
• Contract A's call_b_public was enqueued before contract B's initialize_public
• When contract A calls contract B, public_call's init check believes it is initialized

Our fix

Our fix is quite simple. We'll use two different initializers: a private one, and a public one, and this is how it will work:

• All external functions check their corresponding init nullifier
• Public initializers emit both nullifiers (at the end to avoid others calling us mid init)
• Private initializers emit private nullifier (at the end too), and enqueue a call to an autogen fn which emits pub (also at the end),
  • Unless the contract does not have any external public functions, in which case we emit the private nullifier and that's it
• only_self functions do not check the initializer (i.e. they're noinitcheck)
  • We realized that it didn't make much sense for only_self to have these init checks, and based on our change, we decided to remove them

Pros

• We are fixing the issue we wanted to fix since now public functions will fail if called before initialization finishes
• This requires no syntax changes

Cons

Contracts that have public external functions a private initializer now:

• Perform a public function when initialized
  • This leaks privacy. But, if you think about it, any initializer check that public functions make would have made the nullifier hash public, so anyone could have already checked if it was initialized or not
• They must be registered before initialized (since we are adding a public call on the initializer)
  • While not ideal to make this required now, it does make sense that a contract that has public functions would have to be registered before interacting with it
• Must only call only_self or noinitcheck public functions during initialization

Extra notes

We should make utility functions check both initializers, but they check none at the time, so will do that in another PR

[nchamo]

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?

[nventuro]

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.

[benesjan]

I don't think we are ever checking a name as the only external functions we are generating are _compute_note_hash_and_nullifier, process_message, sync_state and public_dispatch and for the first 3 we generate them only if they don't exist in the contract (to allow devs to have custom implementations) and for public dispatch the compiler catches it on its own:

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.

[nchamo]

@nventuro, are we doing that somewhere else so that I can understand how to properly link to docs with an error code?

[nventuro]

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.

[nchamo]

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

[nventuro]

Why not simply create an external only self function and add it to the contract? This is much more fragile

[nchamo]

Sadly, it's not that simple. I explained the issue in generate_injected_public_fns

[nventuro]

Good first pass! Left some comments re. docs, implemantation notes and tone.

This needs to be accompanied with some blurb somewhere explaining what these things do, e.g. in the docs for the initializer fn or in some section (which is e.g. where the error codes would link, and where we'd explain that initializing pub storage from a pub init fn requires only self). You can do a simple braindump if you want and I'll do a proper pass with the current style etc later.

Also, why did you tag the PR as a breaking change?

[nchamo]

@nventuro , I marked this as a breaking change because contracts that previously enqueued calls to public non-only_self functions as part of the private initializer would now stop working

Basically what happened in noir-projects/noir-contracts/contracts/test/updatable_contract/src/main.nr

[benesjan]

Both Nicos are very smart, glorious and beautiful

[benesjan]

I don't think we are ever checking a name as the only external functions we are generating are _compute_note_hash_and_nullifier, process_message, sync_state and public_dispatch and for the first 3 we generate them only if they don't exist in the contract (to allow devs to have custom implementations) and for public dispatch the compiler catches it on its own:

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.

[nchamo]

Change discussed with @spalladino to fix an unrelated error on the CI

[nventuro]

Thank you!

[nventuro]

Suggested change

	- **Private initializers** emit the private init nullifier. If the contract has any public functions, the protocol auto-enqueues a public call to emit the public init nullifier.
	- **Private initializers** emit the private init nullifier. If the contract has any external public functions, the protocol auto-enqueues a public call to emit the public init nullifier.

[nventuro]

Suggested change

	**External functions called during initialization must be `#[only_self]`.** Init nullifiers are emitted at the end of the initializer, so any external functions called on the initializing contract (e.g. via `enqueue_self` or `call_self`) during initialization will fail the init check unless they skip it.
	**External functions called during private initialization must be `#[only_self]`.*

[nventuro]

Suggested change

	**Breaking change for deployment:** If your contract has public functions and a private initializer, the class must be registered onchain before initialization. You can no longer pass `skipClassPublication: true`, because the auto-enqueued public call requires the class to be available.
	**Breaking change for deployment:** If your contract has external public functions and a private initializer, the class must be registered onchain before initialization. You can no longer pass `skipClassPublication: true`, because the auto-enqueued public call requires the class to be available.

[nventuro]

If we do this, it then means that mark_as_initialized_public is a dangerous functiont hat should never called manually. All of the functions in that mod are pub because technically they are in user code (used by macros). We should make it clear in their docs that they should not be used unless you know very well what you're doing.

[nventuro]

I'd rather not have a duplicate description. I think this is a bit more detailed in some respects than the apiref docs for the init fn, so I'd merge this with that, put it there and remove from here.

[nventuro]

(not just that, this is also not domain separated)

[nventuro]

Why not call these 'pub_no_init_check' and 'pub_init_check'? It'd make these tests easier to read.

[AztecBot]

❌ Failed to cherry-pick to v4-next due to conflicts. (🤖) View backport run.