fix(aztec-nr): reject zero delays in DelayedPublicMutable

[vezenovm]

Fixes F-840

A zero delay leaves no window during which a private read is known to remain valid, and get_effective_minimum_delay_at underflowed instead of reporting one: private simulation aborted with an opaque "attempt to subtract with overflow" instead of failing where the delay was configured.

I decided as this does affect the soundness of the state variable we may as well validate on construction and restrict these invalid declarations. For setting a sufficiently high enough delay I think the current docs outline that clearly. I think we could potentially decide on a good "floor" delay policy, but that is an API decision that is unclear without more user stories.

Changes

• ScheduledDelayChange::schedule_change asserts a strictly positive delay.
• A zero initial delay is rejected at compile time when declaring a DelayedPublicMutable, in aztec-nr and the aztec_sublib fork. That is where a contract author supplies the generic, so it is where the error is most useful.
• unpack_delay_change carries the same compile-time guard. Every read of a stored delay funnels through it, so this also covers readers that do not go through a state variable, such as the private kernel's contract update horizon. static_assert emits no constraints, so neither guard costs anything at proving time.
• The restriction is documented alongside delay selection in the state variable docs, and on the docs site.

assert_effective_minimum_delay_invariants deliberately scheduled a zero delay, so it now reduces to 1, the smallest permitted value.

No test contract in this repo hits this case: declared delays are 360, 86400, 2 and DEFAULT_UPDATE_DELAY, and the instance registry already rejects anything below MINIMUM_UPDATE_DELAY.

[nventuro]

A tx in which the value is privately read sets its experation time to the anchor block time plus the minimum delay. This is because it is possible for there to be a second block at the same time in which the value is updated, and this update would happen (at the earliest) after said delay - hence the tx is only valid before then.

If the delay is zero, then there could be an immediate next block in which the value is instantly changed, and there is no possible expiration timestamp that can be set - if set at the anchor block then the value might still change and render the read invalid, and if set before then the tx would be invalid by construction.

Notably, this only affects private reads: in public the delay is a non-issue due to execution happening at the tip of the chain with perfect knowledge of current state.

I argue that preventing a zero delay is security theatre and provides a false sense of security. Minimum delays of e.g. one, five or ten seconds are equally useless, as it is impossible to create a block, broadcast it, use it as an anchor, craft a new transaction, prove it, broadcast it to the sequencer and have it be included in less than this time. We could opt for setting a minimum value of e.g. one or two minutes, though that would convey a sense of this being a safe value - and it's not clear to me that we can have a minimum that is safe for all usecases.

---

The 'Choosing Delays' section of the docs already recommends a minimum delay of 'at least a couple hours'. I'd expand that section slightly explaining why low delays are highly problematic, and would add a large minimum delay - I think one hour would be ok. The better reason to prevent delays so short that they might impede action is precisely to avoid letting admins impede user action via delay selection, which is a very subtle and implicit way to escalate privileges. If we don't add this, then they will need to add these safeguards to their setDelay functions, and we don't want that.

[vezenovm]

zero delay is security theatre and provides a false sense of security. Minimum delays of e.g. one, five or ten seconds are equally useless, as it is impossible to create a block, broadcast it, use it as an anchor, craft a new transaction, prove it, broadcast it to the sequencer and have it be included in less than this time.

Yes, I see how there is not a clear set safe floor value for all use cases. In fact in the PR description I say "I think we could potentially decide on a good "floor" delay policy, but that is an API decision that is unclear without more user stories." but deciding an appropriate "floor" felt out of scope for now and also it isn't clear what delay could be valid.

However, I think it is a bit strong to call it "theatre". The goal isn't to make a safety claim about over which delay the variable is safe. It is simply to add a correctness guard as a zero delay variable is broken by construction. When we try to read the value in private with get_current_value, ScheduledDelayChange::get_effective_minimum_delay_at is called and will error with "attempt to subtract with overflow". Independent of use case the protocol arithmetic breaks with an unclear error.

I felt like still restricting as zero still makes things clearer as we have a deterministic invalid state. 1s/5s/10s at least produce an expiration horizon, even if it is invalidly set. 0 does not produce any expiration horizon.

I agree the documentation can be improved a bit more though here so I mention explicitly how a zero delay does not make usage of the variable safe. I also added how delay selection can allow admins to prevent user actions. If you still think we should take out the zero guard I can remove it, but it feels like little harm and prevents an unclear error later on.

[nventuro]

I see your point re. disallowing zero, but I think this is the key part. We force users to do this or have a hidden escalation of privileges: I'd rather we had an e.g. 1 hour minimum so that this risk is massively diminished.