feat(StateVariables): Ensure that immutable state vars can only be initialized at deployment

[benesjan]

Currently it's not enforced that initialize methods of immutable state vars are called only during deployment from functions of the deployed contract.

We essentially want to add is_deployment() function to private and public contexts that returns whether the contract whose address can be obtained via context.this_address() is currently being deployed or is already deployed.

This method would then be used from all the initialize methods of immutable vars to assert that we are deploying.

As far as I(@LHerskind) see it, we have two directions to implement this

• Add #[aztec(in_constructor)] which checks whether we are in the constructor, and make a new is_constructor() on the context which returns this value.
• Have the is_constructor() itself perform the check.

The benefit of the macro would be that it can be done more efficiently as we can do just one computation and reuse it for all the other calls to is_constructor.

I think the macro is the best solution.
As I see it you essentially have the following function:

pub fn populate_is_constructor(self) {
  let init_nullifier = compute_contract_initialization_nullifier(self);  
  // Say we have a version that don't assert, but return a bool
  let not_in = prove_nullifier_not_included(init_nullifier, self); 
  let is_in = prove_nullifier_included(init_nullifier, self);

  // We want to ensure that the user could not just make the nullifier inclusion 
  // fail with a bad path, and then do things as if he was in the constructor
  // therefore it must either be in or not
  
  let xor = ((not_in | is_in) - (not_in & is_in);
  assert(xor == 1, "Both or neither was true");

  self.is_constructor_val = not_in; // is_constructor_val is set to false by default
}

To do things with immutables using public storage, you simple need to run populate_is_constructor() before you try any initialization.

Idea without macro:

#[aztec(public)]
#[aztec(internal)]
fn _initialize(new_admin: AztecAddress, slow_updates_contract: AztecAddress) {
    context.populate_is_constructor();  
 
    assert(!new_admin.is_zero(), "invalid admin");
    storage.admin.write(new_admin);
    storage.slow_update.initialize(slow_updates_contract);
    SlowMap::at(slow_updates_contract).initialize(context);
}

And with macro:

#[aztec(public)]
#[aztec(internal)]
#[aztec(in_constructor)]
fn _initialize(new_admin: AztecAddress, slow_updates_contract: AztecAddress) {
    assert(!new_admin.is_zero(), "invalid admin");
    storage.admin.write(new_admin);
    storage.slow_update.initialize(slow_updates_contract);
    SlowMap::at(slow_updates_contract).initialize(context);
}

This will also allow us to get rid of the "scratchmark" that we leave on storage to ensure it is not overwritten.

[benesjan]

@LHerskind in both of the approaches I don't like that a user would have to manually trigger the populate_is_constructor() function or define the macro. It feels like it's something which should be handled automatically.

Is there some reason why we would not internally store in context is_deployment_populated variable which would get used in the context.is_deployment() function to determine whether to perform the check or not? (then we would internally store the is_deployment value as well and set is_deployment_populated to true so that in the next call we don't perform the check).

[LHerskind]

We could trigger it whenever, was just that it feels wasteful to do a lot of extra checks if it is just the same.

For the case of "are we in constructor" we need to check both membership and non-membership, if we assert one or the other we can get away with just one of them. Right now we are always asserting that it is initialized and the noinitcheck just mean that we skip that, where we would likely want to have the "opposite" check for these.

If we want to pass it on, we need to alter the kernels to only pass it along if it is the same contract you are making a call to, e.g., would be a pain if I can deploy, and while in that deployment make calls to other contracts as if I was inside their constructors.

[LHerskind]

Will benefit from #5336

[LHerskind]

Closing this one. @nventuro argued that we should allow it to be set whenever, as long as it is not reset. I don't care enough about it, so I'm fine with that, the only real case that made pain between our approaches is maps of immutables which don't seem like something you see often so I'm closing this for now.