feat(StateVariables): Make SharedMutable's delay mutable #5493
Labels
S-needs-discussion
Status: Still needs more discussion before work can start.
Comments
nventuro
added
the
S-needs-discussion
|
Note that, for reasons similar to the ones described in #5501, most applications will likely want to share delays, so we may want to make them pick from a small predetermined set (e.g. 6 hours, 2 days, one week). Otherwise users would leak a lot of privacy when setting |
LHerskind
changed the title
nventuro
added a commit
that referenced
this issue
Apr 10, 2024
(Large) part of #4761. This is an initial implementation of `SharedMutableStorage`, with some limitations. I think those are best worked on in follow-up PRs, once we have the bones working. The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a pure Noir struct that has all of the block number related logic. `SharedMutable` then makes a state variable out of that struct, adding public storage access both in public and private (via historical reads - see #5379), and using the new `request_max_block_number` function (from #5251). I made an effort to test as much as I could of these in Noir, with partial success in the case of `SharedMutable` due to lack of certain features, notably noir-lang/noir#4652. There is also an end-to-end test that goes through two scheuled value changes, showing that scheduled values do not affect the current one. I added some inline docs but didn't include proper docsite pages yet so that we can discuss the implementation, API, etc., and make e.g. renamings less troublesome. ### Notable implementation details I chose to make the delay a type parameter instead of a value mostly because of two reasons: - it lets us nicely serialize and deserialize `ScheduledValueChange` without including this field (which we are not currently interested in storing) - it lets us declare a state variable of type `SharedMutable<T, DELAY>` without having to change the signature of the `new` function, which is automatically injected by the macro. Overall I think this is fine, especially since we may later make the delay mutable (see below), but still worth noting. Additionally, I created a simple `public_storage` module to get slightly nicer API and encapsulation. This highlighted a Noir issue (noir-lang/noir#4633), which currently only affects public historical reads but will also affect current reads once we migrate to using the AVM opcodes. ### Future work - #5491 - #5492 (this takes care of padding during storage slot allocation) - #5501 - #5493 --------- Co-authored-by: Jan Beneš <[email protected]>
AztecBot
pushed a commit
to AztecProtocol/aztec-nr
that referenced
this issue
Apr 11, 2024
(Large) part of AztecProtocol/aztec-packages#4761. This is an initial implementation of `SharedMutableStorage`, with some limitations. I think those are best worked on in follow-up PRs, once we have the bones working. The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a pure Noir struct that has all of the block number related logic. `SharedMutable` then makes a state variable out of that struct, adding public storage access both in public and private (via historical reads - see #5379), and using the new `request_max_block_number` function (from #5251). I made an effort to test as much as I could of these in Noir, with partial success in the case of `SharedMutable` due to lack of certain features, notably noir-lang/noir#4652. There is also an end-to-end test that goes through two scheuled value changes, showing that scheduled values do not affect the current one. I added some inline docs but didn't include proper docsite pages yet so that we can discuss the implementation, API, etc., and make e.g. renamings less troublesome. ### Notable implementation details I chose to make the delay a type parameter instead of a value mostly because of two reasons: - it lets us nicely serialize and deserialize `ScheduledValueChange` without including this field (which we are not currently interested in storing) - it lets us declare a state variable of type `SharedMutable<T, DELAY>` without having to change the signature of the `new` function, which is automatically injected by the macro. Overall I think this is fine, especially since we may later make the delay mutable (see below), but still worth noting. Additionally, I created a simple `public_storage` module to get slightly nicer API and encapsulation. This highlighted a Noir issue (noir-lang/noir#4633), which currently only affects public historical reads but will also affect current reads once we migrate to using the AVM opcodes. ### Future work - AztecProtocol/aztec-packages#5491 - AztecProtocol/aztec-packages#5492 (this takes care of padding during storage slot allocation) - AztecProtocol/aztec-packages#5501 - AztecProtocol/aztec-packages#5493 --------- Co-authored-by: Jan Beneš <[email protected]>
nventuro
added a commit
that referenced
this issue
Apr 30, 2024
Part of #5493. This lays some of the groundwork by making `ScheduledValueChange` not have an immutable delay. The only user of this struct is `SharedMutable` however, which does (so far) have immutable delays, so there's no functionality changes.
nventuro
added a commit
that referenced
this issue
May 9, 2024
Closes #5493, follow up of #6085. This makes the delay in SharedMutable not be fixed and instead configurable by users throughout the lifetime of the contract. This is however more complicated than it sounds at first: because private proofs are being created relying on the public values being stable until a future point in time, it must not be possible to cause for a shared value to change before some delay. Two scenarios are particularly tricky: - if the delay is reduced, then it is possible to schedule a value change with a shorter delay, violating the original delay's constraints. The solution to this is to make delay changes be scheduled actions themselves, so that the total delay (wait time for the new delay to come into effect plus the new reduced delay) equals the original delay. Note that increasing a delay cna be done instantly. - if we schedule delay changes as per the above, then we must consider a scenario in which a delay reduction is scheduled in the near future. It may happen that waiting for the reduction to come into effect and then scheduling results in a shorter delay than if the scheduling were to happen immediately - this lower 'effective delay' is the value that must be used in private proofs. ## How I had originally considered creating a sort of wrapper state variable that held two SharedMutables, one for the value and one for the delay, or alternatively two ScheduledValueChanges, but ultimately I realized that a scheduled value change is significantly different from a scheduled delay change. Namely: - the notion of the 'current' delay is meaningless in private - we only care about the 'effective' delay - there's no use for the block horizon of a delay change - scheduling a delay change requires setting a delay depending on the current and new values, not an externally defined one Due to these differences, I introduced ScheduledDelayChange, which is essentially a variant of the value change, but with these considerations baked in. I think this is a reasonable way to do things, even if at first this may seem to introduce too many concepts. It also helps with the fact that there's so many values involved (pre, post and block of change for value and delays, as well as current, effective, historical values, etc.), and with language becoming weird - we need to describe the delay for scheduling a delay change, which will later affect the delays of scheduling value changes. With ScheduledDelayChange, extending the functionality of SharedMutable was relatively straightforward. The unit tests became a bit more complicated due to there bieng more scenarios, so I also used this as an opportunity to try to create slightly more complex Noir tests. I didn't go too crazy here, but they seem to be right at the point where we'd want to introduce something like a `Test` struct with custom impls for setup, common assertions, etc. ## Problems An uninitialized `SharedMutable` has both delay and value of 0. A zero delay transforms `SharedMutable` into `PublicMutable`: scheduled value changes become effective immediately, and it is not possible to read from private since `tx.max_block_number` would equal a historical block (i.e. an already mined one). Delay initialization is therefore required, and this is typically fine: since the initial delay is 0 any change will be an increase, and therefore instant. The problem arises when we cannot have explicit initialization and instead wish to rely on defaults. This happens e.g. when we put a SharedMutable inside a `Map`: we can't initialize all entries for all keys, and we run into trouble. This is a pattern followed by `KeyRegistry` and `TokenBlacklist`: we have per-user configuration, and cant really ask users to initialize their state before interacting with the system. ## Solution? A possible solution would be to have a default value for the delay, and to store e.g. `Option<u32>` instead of plain integers and using `unwrap_or(DEFAULT)`. We could then make this a type parameter for SharedMutable, e.g. `registry: Map<Address, SharedMutable<Key, DEFAULT_DELAY>>`. This would make certain things more complicated, particularly the effective delay and delay change block of change computations, but it should all be containable within `ScheduledDelayChange`, which sounds just about right. ---- I'm keeping this is a draft so we can discuss the current approach and wether we think the above or an alternative solution would be reasonable to attempt. Note that this PR won't pass CI as some of the contracts won't build. --------- Co-authored-by: Jan Beneš <[email protected]> Co-authored-by: Lasse Herskind <[email protected]>
superstar0402
added a commit
to superstar0402/aztec-nr
that referenced
this issue
Aug 16, 2024
(Large) part of AztecProtocol/aztec-packages#4761. This is an initial implementation of `SharedMutableStorage`, with some limitations. I think those are best worked on in follow-up PRs, once we have the bones working. The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a pure Noir struct that has all of the block number related logic. `SharedMutable` then makes a state variable out of that struct, adding public storage access both in public and private (via historical reads - see #5379), and using the new `request_max_block_number` function (from #5251). I made an effort to test as much as I could of these in Noir, with partial success in the case of `SharedMutable` due to lack of certain features, notably noir-lang/noir#4652. There is also an end-to-end test that goes through two scheuled value changes, showing that scheduled values do not affect the current one. I added some inline docs but didn't include proper docsite pages yet so that we can discuss the implementation, API, etc., and make e.g. renamings less troublesome. ### Notable implementation details I chose to make the delay a type parameter instead of a value mostly because of two reasons: - it lets us nicely serialize and deserialize `ScheduledValueChange` without including this field (which we are not currently interested in storing) - it lets us declare a state variable of type `SharedMutable<T, DELAY>` without having to change the signature of the `new` function, which is automatically injected by the macro. Overall I think this is fine, especially since we may later make the delay mutable (see below), but still worth noting. Additionally, I created a simple `public_storage` module to get slightly nicer API and encapsulation. This highlighted a Noir issue (noir-lang/noir#4633), which currently only affects public historical reads but will also affect current reads once we migrate to using the AVM opcodes. ### Future work - AztecProtocol/aztec-packages#5491 - AztecProtocol/aztec-packages#5492 (this takes care of padding during storage slot allocation) - AztecProtocol/aztec-packages#5501 - AztecProtocol/aztec-packages#5493 --------- Co-authored-by: Jan Beneš <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #5490, each
SharedMutablevariable has a fixed delay. It is unlikely that users will be able to come up with a delay that works for all future scenarios prior to deployment, so we will eventually need to figure out a way to change this.In order to safely change a delay, we must ensure certain things:
current delay - new delayIf the above is true, then any constraints created prior to the delay change will remain valid (i.e. it will not be possible to change a value before any block horizon prior to the delay change).
This is of course tricky because the delay would likely become itself a
SharedMutablevalue. I recommend holding off this feature untilSharedMutableis more polished and has more usage.The text was updated successfully, but these errors were encountered: