From c191a40bebf5910d4001f3fac61bb7235f805104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 9 May 2024 14:06:49 -0300 Subject: [PATCH] feat!: shared mutable configurable delays (#6104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` instead of plain integers and using `unwrap_or(DEFAULT)`. We could then make this a type parameter for SharedMutable, e.g. `registry: Map>`. 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š Co-authored-by: Lasse Herskind <16536249+LHerskind@users.noreply.github.com> --- .../references/storage/shared_state.md | 6 +- docs/docs/misc/migration_notes.md | 6 + .../aztec/src/state_vars/shared_mutable.nr | 1 + .../shared_mutable/scheduled_delay_change.nr | 512 ++++++++++++++++++ .../shared_mutable/scheduled_value_change.nr | 201 ++++--- .../shared_mutable/shared_mutable.nr | 468 ++++++++++++---- .../shared_mutable_private_getter.nr | 61 ++- .../contracts/auth_contract/src/main.nr | 2 - 8 files changed, 1050 insertions(+), 207 deletions(-) create mode 100644 noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr diff --git a/docs/docs/developers/contracts/references/storage/shared_state.md b/docs/docs/developers/contracts/references/storage/shared_state.md index 2039eaa56b7..7490c503175 100644 --- a/docs/docs/developers/contracts/references/storage/shared_state.md +++ b/docs/docs/developers/contracts/references/storage/shared_state.md @@ -28,11 +28,9 @@ While shared state variables are much less leaky than the assertion in public ap The `max_block_number` transaction property will be set to a value close to the current block number plus the duration of the delay in blocks. The exact value depends on the historical block over which the private proof is constructed. For example, if the current block number is 100 and a shared state variable has a delay of 20 blocks, then transactions that read this value privately will set `max_block_number` to a value close to 120 (clients building proofs on older state will select a lower `max_block_number`). This implicitly leaks the duration of the delay. -Applications using similar delays will therefore be part of the same privacy set. It is expected for social coordination to result in small set of predetermined delays that developers choose from depending on their needs, as an example a viable set might be: 12 hours (for time-sensitive operations, such as emergency mechanisms), 5 days (for middle-of-the-road operations) and 2 weeks (for operations that require lengthy public scrutiny). +Applications using similar delays will therefore be part of the same privacy set. It is expected for social coordination to result in small set of predetermined delays that developers choose from depending on their needs, as an example a viable set might be: 12 hours (for time-sensitive operations, such as emergency mechanisms), 5 days (for middle-of-the-road operations) and 2 weeks (for operations that require lengthy public scrutiny). These delays can be changed during the contract lifetime as the application's needs evolve. -:::note -Shared state delays are currently hardcoded at compilation time and cannot be changed, but there are plans to make this a mutable value. -:::note +Additionally, users might choose to coordinate and constrain their transactions to set `max_block_number` to a value lower than would be strictly needed by the applications they interact with (if any!) using some common delay, and by doing so prevent privacy leakage. ### Choosing Epochs diff --git a/docs/docs/misc/migration_notes.md b/docs/docs/misc/migration_notes.md index c792470b0b5..902eae67a4d 100644 --- a/docs/docs/misc/migration_notes.md +++ b/docs/docs/misc/migration_notes.md @@ -6,6 +6,12 @@ keywords: [sandbox, cli, aztec, notes, migration, updating, upgrading] Aztec is in full-speed development. Literally every version breaks compatibility with the previous ones. This page attempts to target errors and difficulties you might encounter when upgrading, and how to resolve them. +## 0.39.0 + +### [Aztec.nr] Mutable delays in `SharedMutable` + +The type signature for `SharedMutable` changed from `SharedMutable` to `SharedMutable`. The behavior is the same as before, except the delay can now be changed after deployment by calling `schedule_delay_change`. + ## 0.38.0 ### [Aztec.nr] Emmiting encrypted logs diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr index 533639390d8..13b726cc2af 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr @@ -1,4 +1,5 @@ mod shared_mutable; +mod scheduled_delay_change; mod scheduled_value_change; mod shared_mutable_private_getter; diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr new file mode 100644 index 00000000000..55634984f33 --- /dev/null +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr @@ -0,0 +1,512 @@ +use dep::protocol_types::traits::{Serialize, Deserialize, FromField, ToField}; +use dep::std::cmp::min; + +// This data structure is used by SharedMutable to store the minimum delay with which a ScheduledValueChange object can +// schedule a change. +// This delay is initally equal to INITIAL_DELAY, and can be safely mutated to any other value over time. This mutation +// is performed via `schedule_change` in order to satisfy ScheduleValueChange constraints: if e.g. we allowed for the +// delay to be decreased immediately then it'd be possible for the state variable to schedule a value change with a +// reduced delay, invalidating prior private reads. +struct ScheduledDelayChange { + // Both pre and post are stored in public storage, so by default they are zeroed. By wrapping them in an Option, + // they default to Option::none(), which we detect and replace with INITIAL_DELAY. The end result is that a + // ScheduledDelayChange that has not been initialized has a delay equal to INITIAL_DELAY, which is the desired + // effect. Once initialized, the Option will never be none again. + pre: Option, + post: Option, + // Block at which `post` value is used instead of `pre` + block_of_change: u32, + // The _dummy variable forces INITIAL_DELAY to be interpreted as a numeric value. This is a workaround to + // https://github.com/noir-lang/noir/issues/4633. Remove once resolved. + _dummy: [Field; INITIAL_DELAY], +} + +impl ScheduledDelayChange { + pub fn new(pre: Option, post: Option, block_of_change: u32) -> Self { + Self { pre, post, block_of_change, _dummy: [0; INITIAL_DELAY] } + } + + /// Returns the current value of the delay stored in the data structure. + /// This function only returns a meaningful value when called in public with the current block number - for + /// historical private reads use `get_effective_minimum_delay_at` instead. + pub fn get_current(self, current_block_number: u32) -> u32 { + // The post value becomes the current one at the block of change, so any transaction that is included in the + // block of change will use the post value. + + if current_block_number < self.block_of_change { + self.pre.unwrap_or(INITIAL_DELAY) + } else { + self.post.unwrap_or(INITIAL_DELAY) + } + } + + /// Returns the scheduled change, i.e. the post-change delay and the block at which it will become the current + /// delay. Note that this block may be in the past if the change has already taken place. + /// Additionally, further changes might be later scheduled, potentially canceling the one returned by this function. + pub fn get_scheduled(self) -> (u32, u32) { + (self.post.unwrap_or(INITIAL_DELAY), self.block_of_change) + } + + /// Mutates the delay change by scheduling a change at the current block number. This function is only meaningful + /// when called in public with the current block number. + /// The block at which the new delay will become effective is determined automatically: + /// - when increasing the delay, the change is effective immediately + /// - when reducing the delay, the change will take effect after a delay equal to the difference between old and + /// new delay. For example, if reducing from 3 days to 1 day, the reduction will be scheduled to happen after 2 + /// days. + pub fn schedule_change(&mut self, new: u32, current_block_number: u32) { + let current = self.get_current(current_block_number); + + // When changing the delay value we must ensure that it is not possible to produce a value change with a delay + // shorter than the current one. + let blocks_until_change = if new > current { + // Increasing the delay value can therefore be done immediately: this does not invalidate prior contraints + // about how quickly a value might be changed (indeed it strengthens them). + 0 + } else { + // Decreasing the delay requires waiting for the difference between current and new delay in order to ensure + // that overall the current delay is respected. + // + // current delay earliest value block of change + // block block of change if delay remained unchanged + // =======N=========================|================================X=================> + // ^ ^ ^ + // |-------------------------|--------------------------------| + // | blocks until change new delay | + // ------------------------------------------------------------ + // current delay + current - new + }; + + self.pre = Option::some(current); + self.post = Option::some(new); + self.block_of_change = current_block_number + blocks_until_change; + } + + /// Returns the minimum delay before a value might mutate due to a scheduled change, from the perspective of some + /// historical block number. It only returns a meaningful value when called in private with historical blocks. This + /// function can be used alongside `ScheduledValueChange.get_block_horizon` to properly constrain the + /// `max_block_number` transaction property when reading mutable shared state. + /// This value typically equals the current delay at the block following the historical one (the earliest one in + /// which a value change could be scheduled), but it also considers scenarios in which a delay reduction is + /// scheduled to happen in the near future, resulting in a way to schedule a change with an overall delay lower than + /// the current one. + pub fn get_effective_minimum_delay_at(self, historical_block_number: u32) -> u32 { + if self.block_of_change <= historical_block_number { + // If no delay changes were scheduled, then the delay value at the historical block (post) is guaranteed to + // hold due to how further delay changes would be scheduled by `schedule_change`. + self.post.unwrap_or(INITIAL_DELAY) + } else { + // If a change is scheduled, then the effective delay might be lower than the current one (pre). At the + // block of change the current delay will be the scheduled one, with an overall delay from the historical + // block number equal to the number of blocks until the change plus the new delay. If this value is lower + // than the current delay, then that is the effective minimum delay. + // + // historical + // block delay actual earliest value + // v block of change block of change + // =========NS=====================|=============================X===========Y=====> + // ^ ^ ^ ^ + // earliest block in | | | + // which to schedule change | | | + // | | | | + // |----------------------|------------------------------ | + // | blocks new delay | + // | until change | + // | | + // |----------------------------------------------------------------| + // current delay at the earliest block in + // which to scheduled value change + + let blocks_until_change = self.block_of_change - (historical_block_number + 1); + + min( + self.pre.unwrap_or(INITIAL_DELAY), + blocks_until_change + self.post.unwrap_or(INITIAL_DELAY) + ) + } + } +} + +impl Serialize<1> for ScheduledDelayChange { + fn serialize(self) -> [Field; 1] { + // We pack all three u32 values into a single U128, which is made up of two u64 limbs. + // Low limb: [ pre_inner: u32 | post_inner: u32 ] + // High limb: [ empty | pre_is_some: u8 | post_is_some: u8 | block_of_change: u32 ] + + let lo = ((self.pre.unwrap_unchecked() as u64) * (1 << 32)) + + (self.post.unwrap_unchecked() as u64); + + let hi = (self.pre.is_some() as u64) * (1 << 33) + + (self.post.is_some() as u64 * (1 << 32)) + + self.block_of_change as u64; + + let packed = U128::from_u64s_le(lo, hi); + + [packed.to_integer()] + } +} + +impl Deserialize<1> for ScheduledDelayChange { + fn deserialize(input: [Field; 1]) -> Self { + let packed = U128::from_integer(input[0]); + + // We use division and modulo to clear the bits that correspond to other values when unpacking. + + let pre_is_some = ((packed.hi as u64) / (1 << 33)) as bool; + let pre_inner = ((packed.lo as u64) / (1 << 32)) as u32; + + let post_is_some = (((packed.hi as u64) / (1 << 32)) % (1 << 1)) as bool; + let post_inner = ((packed.lo as u64) % (1 << 32)) as u32; + + let block_of_change = ((packed.hi as u64) % (1 << 32)) as u32; + + Self { + pre: if pre_is_some { Option::some(pre_inner) } else { Option::none() }, + post: if post_is_some { Option::some(post_inner) } else { Option::none() }, + block_of_change, + _dummy: [0; INITIAL_DELAY], + } + } +} + +mod test { + use crate::state_vars::shared_mutable::scheduled_delay_change::ScheduledDelayChange; + + global TEST_INITIAL_DELAY = 13; + + fn assert_equal_after_conversion(original: ScheduledDelayChange) { + // We have to do explicit type annotations because Noir lacks turbofish support. + // TODO: improve syntax once https://github.com/noir-lang/noir/issues/4710 is implemented. + let converted: ScheduledDelayChange = ScheduledDelayChange::deserialize((original).serialize()); + + assert_eq(original.pre, converted.pre); + assert_eq(original.post, converted.post); + assert_eq(original.block_of_change, converted.block_of_change); + } + + #[test] + fn test_serde() { + let pre = 1; + let post = 2; + let block_of_change = 50; + + assert_equal_after_conversion(ScheduledDelayChange::new(Option::some(pre), Option::some(post), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::some(pre), Option::none(), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::none(), Option::some(post), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::none(), Option::none(), block_of_change)); + } + + #[test] + fn test_serde_large_values() { + let max_u32 = (1 << 32) - 1; + + let pre = max_u32 as u32; + let post = (max_u32 - 1) as u32; + let block_of_change = (max_u32 - 2) as u32; + + assert_equal_after_conversion(ScheduledDelayChange::new(Option::some(pre), Option::some(post), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::some(pre), Option::none(), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::none(), Option::some(post), block_of_change)); + assert_equal_after_conversion(ScheduledDelayChange::new(Option::none(), Option::none(), block_of_change)); + } + + fn get_non_initial_delay_change( + pre: u32, + post: u32, + block_of_change: u32 + ) -> ScheduledDelayChange { + ScheduledDelayChange::new(Option::some(pre), Option::some(post), block_of_change) + } + + fn get_initial_delay_change() -> ScheduledDelayChange { + ScheduledDelayChange::deserialize([0]) + } + + #[test] + fn test_get_current() { + let pre = 1; + let post = 2; + let block_of_change = 50; + + let delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + assert_eq(delay_change.get_current(0), pre); + assert_eq(delay_change.get_current(block_of_change - 1), pre); + assert_eq(delay_change.get_current(block_of_change), post); + assert_eq(delay_change.get_current(block_of_change + 1), post); + } + + #[test] + fn test_get_current_initial() { + let delay_change = get_initial_delay_change(); + + assert_eq(delay_change.get_current(0), TEST_INITIAL_DELAY); + assert_eq(delay_change.get_current(1), TEST_INITIAL_DELAY); + } + + #[test] + fn test_get_scheduled() { + let pre = 1; + let post = 2; + let block_of_change = 50; + + let delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + assert_eq(delay_change.get_scheduled(), (post, block_of_change)); + } + + #[test] + fn test_get_scheduled_initial() { + let delay_change = get_initial_delay_change(); + + assert_eq(delay_change.get_scheduled(), (TEST_INITIAL_DELAY, 0)); + } + + #[test] + fn test_schedule_change_to_shorter_delay_before_change() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let new = 10; + let current_block_number = block_of_change - 50; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + delay_change.schedule_change(new, current_block_number); + + // Because we re-schedule before the last scheduled change takes effect, the old `post` value is lost. The + // schedule time is determined by the difference between the current value (pre) and new delay. + assert_eq(delay_change.pre.unwrap(), pre); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number + pre - new); + } + + #[test] + fn test_schedule_change_to_shorter_delay_after_change() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let new = 10; + let current_block_number = block_of_change + 50; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + delay_change.schedule_change(new, current_block_number); + + // The schedule time is determined by the different between the current value (ex post, now pre) and new delay. + assert_eq(delay_change.pre.unwrap(), post); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number + post - new); + } + + #[test] + fn test_schedule_change_to_shorter_delay_from_initial() { + let new = TEST_INITIAL_DELAY - 1; + let current_block_number = 50; + + let mut delay_change = get_initial_delay_change(); + delay_change.schedule_change(new, current_block_number); + + // Like in the after change scenario, the schedule time is determined by the difference between the current value + // (initial) and new delay. + assert_eq(delay_change.pre.unwrap(), TEST_INITIAL_DELAY); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number + TEST_INITIAL_DELAY - new); + } + + #[test] + fn test_schedule_change_to_longer_delay_before_change() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let new = 40; + let current_block_number = block_of_change - 50; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + delay_change.schedule_change(new, current_block_number); + + // Because we re-schedule before the last scheduled change takes effect, the old `post` value is lost. The + // change is effective immediately because the new delay is longer than the current one. + assert_eq(delay_change.pre.unwrap(), pre); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number); + assert_eq(delay_change.get_current(current_block_number), new); + } + + #[test] + fn test_schedule_change_to_longer_delay_after_change() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let new = 40; + let current_block_number = block_of_change + 50; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + delay_change.schedule_change(new, current_block_number); + + // Change is effective immediately because the new delay is longer than the current one. + assert_eq(delay_change.pre.unwrap(), post); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number); + assert_eq(delay_change.get_current(current_block_number), new); + } + + #[test] + fn test_schedule_change_to_longer_delay_from_initial() { + let new = TEST_INITIAL_DELAY + 1; + let current_block_number = 50; + + let mut delay_change = get_initial_delay_change(); + delay_change.schedule_change(new, current_block_number); + + // Like in the after change scenario, change is effective immediately because the new delay is longer than the + // current one. + assert_eq(delay_change.pre.unwrap(), TEST_INITIAL_DELAY); + assert_eq(delay_change.post.unwrap(), new); + assert_eq(delay_change.block_of_change, current_block_number); + assert_eq(delay_change.get_current(current_block_number), new); + } + + fn assert_effective_minimum_delay_invariants( + delay_change: &mut ScheduledDelayChange, + historical_block_number: u32, + effective_minimum_delay: u32 + ) { + // The effective minimum delays guarantees the earliest block in which a scheduled value change could be made + // effective. No action, even if executed immediately after the historical block, should result in a scheduled + // value change having a block of change lower than this. + let expected_earliest_value_change_block = historical_block_number + 1 + effective_minimum_delay; + + if delay_change.block_of_change > historical_block_number { + // If a delay change is already scheduled to happen in the future, we then must consider the scenario in + // which a value change is scheduled to occur right as the delay changes and becomes the current one. + let delay_change_block = delay_change.block_of_change; + + let value_change_block = delay_change_block + delay_change.get_current(delay_change_block); + assert(expected_earliest_value_change_block <= value_change_block); + } + + // Another possibility would be to schedule a value change immediately after the historical block. + let change_schedule_block = historical_block_number + 1; + let value_change_block = change_schedule_block + delay_change.get_current(change_schedule_block); + assert(expected_earliest_value_change_block <= value_change_block); + + // Finally, a delay reduction could be scheduled immediately after the historical block. We reduce the delay to + // zero, which means that at the delay block of change there'll be no delay and a value change could be + // performed immediately then. + delay_change.schedule_change(0, historical_block_number + 1); + assert(expected_earliest_value_change_block <= delay_change.block_of_change); + } + + #[test] + fn test_get_effective_delay_at_before_change_in_far_future() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let historical_block_number = 200; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + // The scheduled delay change is far into the future (further than the current delay is), so it doesn't affect + // the effective delay, which is simply the current one (pre). + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + assert_eq(effective_minimum_delay, pre); + + assert_effective_minimum_delay_invariants( + &mut delay_change, + historical_block_number, + effective_minimum_delay + ); + } + + #[test] + fn test_get_effective_delay_at_before_change_to_long_delay() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let historical_block_number = 495; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + // The scheduled delay change will be effective soon (it's fewer blocks away than the current delay), but due to + // it being larger than the current one it doesn't affect the effective delay, which is simply the current one + // (pre). + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + assert_eq(effective_minimum_delay, pre); + + assert_effective_minimum_delay_invariants( + &mut delay_change, + historical_block_number, + effective_minimum_delay + ); + } + + #[test] + fn test_get_effective_delay_at_before_near_change_to_short_delay() { + let pre = 15; + let post = 3; + let block_of_change = 500; + + let historical_block_number = 495; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + // The scheduled delay change will be effective soon (it's fewer blocks away than the current delay), and it's + // changing to a value smaller than the current one. This means that at the block of change the delay will be + // reduced, and a delay change would be scheduled there with an overall delay lower than the current one. + // The effective delay therefore is the new delay plus the number of blocks that need to elapse until it becomes + // effective (i.e. until the block of change). + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + assert_eq(effective_minimum_delay, post + block_of_change - (historical_block_number + 1)); + + assert_effective_minimum_delay_invariants( + &mut delay_change, + historical_block_number, + effective_minimum_delay + ); + } + + #[test] + fn test_get_effective_delay_at_after_change() { + let pre = 15; + let post = 25; + let block_of_change = 500; + + let historical_block_number = block_of_change + 50; + + let mut delay_change = get_non_initial_delay_change(pre, post, block_of_change); + + // No delay change is scheduled, so the effective delay is simply the current one (post). + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + assert_eq(effective_minimum_delay, post); + + assert_effective_minimum_delay_invariants( + &mut delay_change, + historical_block_number, + effective_minimum_delay + ); + } + + #[test] + fn test_get_effective_delay_at_initial() { + let mut delay_change = get_initial_delay_change(); + + let historical_block_number = 200; + + // Like in the after change scenario, no delay change is scheduled, so the effective delay is simply the current + // one (initial). + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + assert_eq(effective_minimum_delay, TEST_INITIAL_DELAY); + + assert_effective_minimum_delay_invariants( + &mut delay_change, + historical_block_number, + effective_minimum_delay + ); + } +} diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_value_change.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_value_change.nr index 52aba6277ea..bfdbe356506 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_value_change.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_value_change.nr @@ -1,13 +1,15 @@ use dep::protocol_types::traits::{Serialize, Deserialize, FromField, ToField}; +use dep::std::cmp::min; -// This data structure is used by SharedMutable to represent a value that changes from `pre` to `post` at some block +// This data structure is used by SharedMutable to represent a value that changes from `pre` to `post` at some block // called the `block_of_change`. The value can only be made to change by scheduling a change event at some future block -// of change after some minimum delay measured in blocks has elapsed. This means that at any given block number we know -// both the current value and the smallest block number at which the value might change - this is called the +// of change after some minimum delay measured in blocks has elapsed. This means that at any given block number we know +// both the current value and the smallest block number at which the value might change - this is called the // 'block horizon'. struct ScheduledValueChange { pre: T, post: T, + // Block at which `post` value is used instead of `pre` block_of_change: u32, } @@ -16,11 +18,11 @@ impl ScheduledValueChange { Self { pre, post, block_of_change } } - /// Returns the value stored in the data structure at a given block. This function can be called both in public - /// (where `block_number` is simply the current block number, i.e. the number of the block in which the current - /// transaction will be included) and in private (where `block_number` is the historical block number that is used + /// Returns the value stored in the data structure at a given block. This function can be called both in public + /// (where `block_number` is simply the current block number, i.e. the number of the block in which the current + /// transaction will be included) and in private (where `block_number` is the historical block number that is used /// to construct the proof). - /// Reading in private is only safe if the transaction's `max_block_number` property is set to a value lower or + /// Reading in private is only safe if the transaction's `max_block_number` property is set to a value lower or /// equal to the block horizon (see `get_block_horizon()`). pub fn get_current_at(self, block_number: u32) -> T { // The post value becomes the current one at the block of change. This means different things in each realm: @@ -35,7 +37,7 @@ impl ScheduledValueChange { } } - /// Returns the scheduled change, i.e. the post-change value and the block at which it will become the current + /// Returns the scheduled change, i.e. the post-change value and the block at which it will become the current /// value. Note that this block may be in the past if the change has already taken place. /// Additionally, further changes might be later scheduled, potentially canceling the one returned by this function. pub fn get_scheduled(self) -> (T, u32) { @@ -43,15 +45,18 @@ impl ScheduledValueChange { } /// Returns the largest block number at which the value returned by `get_current_at` is known to remain the current - /// value. This value is only meaningful in private when constructing a proof at some `historical_block_number`, + /// value. This value is only meaningful in private when constructing a proof at some `historical_block_number`, /// since due to its asynchronous nature private execution cannot know about any later scheduled changes. - /// The caller of this function must know how quickly the value can change due to a scheduled change in the form of - /// `minimum_delay`. If the delay itself is immutable, then this is just its duration. + /// The caller of this function must know how quickly the value can change due to a scheduled change in the form of + /// `minimum_delay`. If the delay itself is immutable, then this is just its duration. If the delay is mutable + /// however, then this value is the 'effective minimum delay' (obtained by calling + /// `ScheduledDelayChange.get_effective_minimum_delay_at`), which equals the minimum number of blocks that need to + /// elapse from the next block until the value changes, regardless of further delay changes. /// The value returned by `get_current_at` in private when called with a historical block number is only safe to use /// if the transaction's `max_block_number` property is set to a value lower or equal to the block horizon computed /// using the same historical block number. pub fn get_block_horizon(self, historical_block_number: u32, minimum_delay: u32) -> u32 { - // The block horizon is the very last block in which the current value is known. Any block past the horizon + // The block horizon is the very last block in which the current value is known. Any block past the horizon // (i.e. with a block number larger than the block horizon) may have a different current value. Reading the // current value in private typically requires constraining the maximum valid block number to be equal to the // block horizon. @@ -61,10 +66,10 @@ impl ScheduledValueChange { // change is scheduled. This did not happen at the historical block number (or else it would not be // greater or equal to the block of change), and therefore could only happen after the historical block // number. The earliest would be the immediate next block, and so the smallest possible next block of change - // equals `historical_block_number + 1 + minimum_delay`. Our block horizon is simply the previous block to + // equals `historical_block_number + 1 + minimum_delay`. Our block horizon is simply the previous block to // that one. // - // block of historical + // block of historical // change block block horizon // =======|=============N===================H===========> // ^ ^ @@ -74,34 +79,34 @@ impl ScheduledValueChange { historical_block_number + minimum_delay } else { // If the block of change has not yet been mined however, then there are two possible scenarios. - // a) It could be so far into the future that the block horizon is actually determined by the minimum - // delay, because a new change could be scheduled and take place _before_ the currently scheduled one. - // This is similar to the scenario where the block of change is in the past: the time horizon is the + // a) It could be so far into the future that the block horizon is actually determined by the minimum + // delay, because a new change could be scheduled and take place _before_ the currently scheduled one. + // This is similar to the scenario where the block of change is in the past: the time horizon is the // block prior to the earliest one in which a new block of change might land. - // - // historical + // + // historical // block block horizon block of change // =====N=================================H=================|=========> // ^ ^ - // | | + // | | // ----------------------------------- // minimum delay // - // b) It could be fewer than `minimum_delay` blocks away from the historical block number, in which case - // the block of change would become the limiting factor for the time horizon, which would equal the - // block right before the block of change (since by definition the value changes at the block of + // b) It could be fewer than `minimum_delay` blocks away from the historical block number, in which case + // the block of change would become the limiting factor for the time horizon, which would equal the + // block right before the block of change (since by definition the value changes at the block of // change). // // historical block horizon // block block of change if not scheduled // =======N=============|===================H=================> // ^ ^ ^ - // | actual horizon | + // | actual horizon | // ----------------------------------- - // minimum delay - // + // minimum delay + // // Note that the current implementation does not allow the caller to set the block of change to an arbitrary - // value, and therefore scenario a) is not currently possible. However implementing #5501 would allow for + // value, and therefore scenario a) is not currently possible. However implementing #5501 would allow for // this to happen. // Because historical_block_number < self.block_of_change, then block_of_change > 0 and we can safely @@ -113,8 +118,8 @@ impl ScheduledValueChange { } } - /// Mutates a scheduled value change by scheduling a change at the current block number. This function is only - /// meaningful when called in public with the current block number. + /// Mutates the value by scheduling a change at the current block number. This function is only meaningful when + /// called in public with the current block number. pub fn schedule_change( &mut self, new_value: T, @@ -138,42 +143,45 @@ impl Serialize<3> for ScheduledValueChange { impl Deserialize<3> for ScheduledValueChange { fn deserialize(input: [Field; 3]) -> Self where T: FromField { - Self { - pre: FromField::from_field(input[0]), - post: FromField::from_field(input[1]), + Self { + pre: FromField::from_field(input[0]), + post: FromField::from_field(input[1]), block_of_change: FromField::from_field(input[2]), } } } -fn min(lhs: u32, rhs: u32) -> u32 { - if lhs < rhs { lhs } else { rhs } -} - -#[test] -fn test_min() { - assert(min(3, 5) == 3); - assert(min(5, 3) == 3); - assert(min(3, 3) == 3); -} - mod test { use crate::state_vars::shared_mutable::scheduled_value_change::ScheduledValueChange; global TEST_DELAY: u32 = 200; + #[test] + fn test_serde() { + let pre = 1; + let post = 2; + let block_of_change = 50; + + let original = ScheduledValueChange::new(pre, post, block_of_change); + let converted = ScheduledValueChange::deserialize((original).serialize()); + + assert_eq(original.pre, converted.pre); + assert_eq(original.post, converted.post); + assert_eq(original.block_of_change, converted.block_of_change); + } + #[test] fn test_get_current_at() { let pre = 1; let post = 2; let block_of_change = 50; - let value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let value_change: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); - assert_eq(value.get_current_at(0), pre); - assert_eq(value.get_current_at(block_of_change - 1), pre); - assert_eq(value.get_current_at(block_of_change), post); - assert_eq(value.get_current_at(block_of_change + 1), post); + assert_eq(value_change.get_current_at(0), pre); + assert_eq(value_change.get_current_at(block_of_change - 1), pre); + assert_eq(value_change.get_current_at(block_of_change), post); + assert_eq(value_change.get_current_at(block_of_change + 1), post); } #[test] @@ -182,34 +190,34 @@ mod test { let post = 2; let block_of_change = 50; - let value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let value_change: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); - assert_eq(value.get_scheduled(), (post, block_of_change)); + assert_eq(value_change.get_scheduled(), (post, block_of_change)); } fn assert_block_horizon_invariants( - value: &mut ScheduledValueChange, + value_change: &mut ScheduledValueChange, historical_block_number: u32, block_horizon: u32 ) { // The current value should not change at the block horizon (but it might later). - let current_at_historical = value.get_current_at(historical_block_number); - assert_eq(current_at_historical, value.get_current_at(block_horizon)); + let current_at_historical = value_change.get_current_at(historical_block_number); + assert_eq(current_at_historical, value_change.get_current_at(block_horizon)); // The earliest a new change could be scheduled in would be the immediate next block to the historical one. This // should result in the new block of change landing *after* the block horizon, and the current value still not // changing at the previously determined block_horizon. - let new = value.pre + value.post; // Make sure it's different to both pre and post - value.schedule_change( + let new = value_change.pre + value_change.post; // Make sure it's different to both pre and post + value_change.schedule_change( new, historical_block_number + 1, TEST_DELAY, historical_block_number + 1 + TEST_DELAY ); - assert(value.block_of_change > block_horizon); - assert_eq(current_at_historical, value.get_current_at(block_horizon)); + assert(value_change.block_of_change > block_horizon); + assert_eq(current_at_historical, value_change.get_current_at(block_horizon)); } #[test] @@ -217,12 +225,12 @@ mod test { let historical_block_number = 100; let block_of_change = 50; - let mut value: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number, TEST_DELAY); + let block_horizon = value_change.get_block_horizon(historical_block_number, TEST_DELAY); assert_eq(block_horizon, historical_block_number + TEST_DELAY); - assert_block_horizon_invariants(&mut value, historical_block_number, block_horizon); + assert_block_horizon_invariants(&mut value_change, historical_block_number, block_horizon); } #[test] @@ -230,12 +238,12 @@ mod test { let historical_block_number = 100; let block_of_change = 100; - let mut value: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number, TEST_DELAY); + let block_horizon = value_change.get_block_horizon(historical_block_number, TEST_DELAY); assert_eq(block_horizon, historical_block_number + TEST_DELAY); - assert_block_horizon_invariants(&mut value, historical_block_number, block_horizon); + assert_block_horizon_invariants(&mut value_change, historical_block_number, block_horizon); } #[test] @@ -243,15 +251,15 @@ mod test { let historical_block_number = 100; let block_of_change = 120; - let mut value: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); // Note that this is the only scenario in which the block of change informs the block horizon. // This may result in privacy leaks when interacting with applications that have a scheduled change // in the near future. - let block_horizon = value.get_block_horizon(historical_block_number, TEST_DELAY); + let block_horizon = value_change.get_block_horizon(historical_block_number, TEST_DELAY); assert_eq(block_horizon, block_of_change - 1); - assert_block_horizon_invariants(&mut value, historical_block_number, block_horizon); + assert_block_horizon_invariants(&mut value_change, historical_block_number, block_horizon); } #[test] @@ -259,25 +267,38 @@ mod test { let historical_block_number = 100; let block_of_change = 500; - let mut value: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number, TEST_DELAY); + let block_horizon = value_change.get_block_horizon(historical_block_number, TEST_DELAY); assert_eq(block_horizon, historical_block_number + TEST_DELAY); - assert_block_horizon_invariants(&mut value, historical_block_number, block_horizon); + assert_block_horizon_invariants(&mut value_change, historical_block_number, block_horizon); } #[test] - fn test_schedule_change_before_prior_change() { + fn test_get_block_horizon_n0_delay() { + let historical_block_number = 100; + let block_of_change = 50; + + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); + + let block_horizon = value_change.get_block_horizon(historical_block_number, 0); + // Since the block horizon equals the historical block number, it is not possible to read the current value in + // private since the transaction `max_block_number` property would equal an already mined block. + assert_eq(block_horizon, historical_block_number); + } + + #[test] + fn test_schedule_change_before_change() { let pre = 1; let post = 2; let block_of_change = 500; - let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); let new = 42; let current_block_number = block_of_change - 50; - value.schedule_change( + value_change.schedule_change( new, current_block_number, TEST_DELAY, @@ -285,30 +306,48 @@ mod test { ); // Because we re-schedule before the last scheduled change takes effect, the old `post` value is lost. - assert_eq(value.pre, pre); - assert_eq(value.post, new); - assert_eq(value.block_of_change, current_block_number + TEST_DELAY); + assert_eq(value_change.pre, pre); + assert_eq(value_change.post, new); + assert_eq(value_change.block_of_change, current_block_number + TEST_DELAY); } #[test] - fn test_schedule_change_after_prior_change() { + fn test_schedule_change_after_change() { let pre = 1; let post = 2; let block_of_change = 500; - let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); let new = 42; let current_block_number = block_of_change + 50; - value.schedule_change( + value_change.schedule_change( new, current_block_number, TEST_DELAY, current_block_number + TEST_DELAY ); - assert_eq(value.pre, post); - assert_eq(value.post, new); - assert_eq(value.block_of_change, current_block_number + TEST_DELAY); + assert_eq(value_change.pre, post); + assert_eq(value_change.post, new); + assert_eq(value_change.block_of_change, current_block_number + TEST_DELAY); + } + + #[test] + fn test_schedule_change_no_delay() { + let pre = 1; + let post = 2; + let block_of_change = 500; + + let mut value_change: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + + let new = 42; + let current_block_number = block_of_change + 50; + value_change.schedule_change(new, current_block_number, 0, current_block_number); + + assert_eq(value_change.pre, post); + assert_eq(value_change.post, new); + assert_eq(value_change.block_of_change, current_block_number); + assert_eq(value_change.get_current_at(current_block_number), new); } } diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr index 8ab974c7389..a36bda7e6cb 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr @@ -3,17 +3,24 @@ use dep::protocol_types::{hash::pedersen_hash, traits::FromField}; use crate::context::{PrivateContext, PublicContext, Context}; use crate::history::public_storage::public_storage_historical_read; use crate::public_storage; -use crate::state_vars::{storage::Storage, shared_mutable::scheduled_value_change::ScheduledValueChange}; +use crate::state_vars::{ + storage::Storage, + shared_mutable::{scheduled_value_change::ScheduledValueChange, scheduled_delay_change::ScheduledDelayChange} +}; -struct SharedMutable { +struct SharedMutable { context: Context, storage_slot: Field, - // The _dummy variable forces DELAY to be interpreted as a numberic value. This is a workaround to - // https://github.com/noir-lang/noir/issues/4633. Remove once resolved. - _dummy: [Field; DELAY], } -impl Storage for SharedMutable {} +// This will make the Aztec macros require that T implements the Serialize trait, and allocate N storage slots to +// this state variable. This is incorrect, since what we actually store is: +// - a ScheduledValueChange, which requires 1 + 2 * M storage slots, where M is the serialization length of T +// - a ScheduledDelayChange, which requires another storage slot +// +// TODO https://github.com/AztecProtocol/aztec-packages/issues/5736: change the storage allocation scheme so that we +// can actually use it here +impl Storage for SharedMutable {} // SharedMutable stores a value of type T that is: // - publicly known (i.e. unencrypted) @@ -24,79 +31,139 @@ impl Storage for SharedMutable {} // the value is not changed immediately but rather a value change is scheduled to happen in the future after some delay // measured in blocks. Reads in private are only valid as long as they are included in a block not too far into the // future, so that they can guarantee the value will not have possibly changed by then (because of the delay). -impl SharedMutable { +// The delay for changing a value is initially equal to INITIAL_DELAY, but can be changed by calling +// `schedule_delay_change`. +impl SharedMutable { pub fn new(context: Context, storage_slot: Field) -> Self { assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1."); - Self { context, storage_slot, _dummy: [0; DELAY] } + Self { context, storage_slot } } pub fn schedule_value_change(self, new_value: T) { let context = self.context.public.unwrap(); - let mut scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); + let mut value_change = self.read_value_change(); + let delay_change = self.read_delay_change(); let block_number = context.block_number() as u32; + let current_delay = delay_change.get_current(block_number); + // TODO: make this configurable // https://github.com/AztecProtocol/aztec-packages/issues/5501 - let block_of_change = block_number + DELAY; + let block_of_change = block_number + current_delay; + value_change.schedule_change(new_value, block_number, current_delay, block_of_change); + + self.write_value_change(value_change); + } + + pub fn schedule_delay_change(self, new_delay: u32) { + let context = self.context.public.unwrap(); + let mut delay_change = self.read_delay_change(); + + let block_number = context.block_number() as u32; - scheduled_value_change.schedule_change(new_value, block_number, DELAY, block_of_change); + delay_change.schedule_change(new_delay, block_number); - public_storage::write(self.get_derived_storage_slot(), scheduled_value_change); + self.write_delay_change(delay_change); } pub fn get_current_value_in_public(self) -> T { - let scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); + let block_number = self.context.public.unwrap().block_number() as u32; + self.read_value_change().get_current_at(block_number) + } + pub fn get_current_delay_in_public(self) -> u32 { let block_number = self.context.public.unwrap().block_number() as u32; - scheduled_value_change.get_current_at(block_number) + self.read_delay_change().get_current(block_number) } pub fn get_scheduled_value_in_public(self) -> (T, u32) { - let scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); - scheduled_value_change.get_scheduled() + self.read_value_change().get_scheduled() + } + + pub fn get_scheduled_delay_in_public(self) -> (u32, u32) { + self.read_delay_change().get_scheduled() } pub fn get_current_value_in_private(self) -> T where T: FromField { let mut context = self.context.private.unwrap(); - let (scheduled_value_change, historical_block_number) = self.historical_read_from_public_storage(*context); - let block_horizon = scheduled_value_change.get_block_horizon(historical_block_number, DELAY); + // When reading the current value in private we construct a historical state proof for the public value. + // However, since this value might change, we must constrain the maximum transaction block number as this proof + // will only be valid for however many blocks we can ensure the value will not change, which will depend on the + // current delay and any scheduled delay changes. + + let (value_change, delay_change, historical_block_number) = self.historical_read_from_public_storage(*context); + + // We use the effective minimum delay as opposed to the current delay at the historical block as this one also + // takes into consideration any scheduled delay changes. + // For example, consider a scenario in which at block 200 the current delay was 50. We may naively think that + // the earliest we could change the value would be at block 251 by scheduling immediately after the historical + // block, i.e. at block 201. But if there was a delay change scheduled for block 210 to reduce the delay to 20 + // blocks, then if a value change was scheduled at block 210 it would go into effect at block 230, which is + // earlier than what we'd expect if we only considered the current delay. + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + let block_horizon = value_change.get_block_horizon(historical_block_number, effective_minimum_delay); // We prevent this transaction from being included in any block after the block horizon, ensuring that the // historical public value matches the current one, since it can only change after the horizon. context.set_tx_max_block_number(block_horizon); - scheduled_value_change.get_current_at(historical_block_number) + value_change.get_current_at(historical_block_number) } fn historical_read_from_public_storage( self, context: PrivateContext - ) -> (ScheduledValueChange, u32) where T: FromField { - let derived_slot = self.get_derived_storage_slot(); - + ) -> (ScheduledValueChange, ScheduledDelayChange, u32) where T: FromField { // Ideally the following would be simply public_storage::read_historical, but we can't implement that yet. - let mut raw_fields = [0; 3]; + let value_change_slot = self.get_value_change_storage_slot(); + let mut raw_value_change_fields = [0; 3]; for i in 0..3 { - raw_fields[i] = public_storage_historical_read( + raw_value_change_fields[i] = public_storage_historical_read( context, - derived_slot + i as Field, + value_change_slot + i as Field, context.this_address() ); } - let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); + // Ideally the following would be simply public_storage::read_historical, but we can't implement that yet. + let delay_change_slot = self.get_delay_change_storage_slot(); + let raw_delay_change_fields = [public_storage_historical_read(context, delay_change_slot, context.this_address())]; + + let value_change = ScheduledValueChange::deserialize(raw_value_change_fields); + let delay_change = ScheduledDelayChange::deserialize(raw_delay_change_fields); + let historical_block_number = context.historical_header.global_variables.block_number as u32; - (scheduled_value, historical_block_number) + (value_change, delay_change, historical_block_number) + } + + fn read_value_change(self) -> ScheduledValueChange { + public_storage::read(self.get_value_change_storage_slot()) + } + + fn read_delay_change(self) -> ScheduledDelayChange { + public_storage::read(self.get_delay_change_storage_slot()) + } + + fn write_value_change(self, value_change: ScheduledValueChange) { + public_storage::write(self.get_value_change_storage_slot(), value_change); + } + + fn write_delay_change(self, delay_change: ScheduledDelayChange) { + public_storage::write(self.get_delay_change_storage_slot(), delay_change); } - fn get_derived_storage_slot(self) -> Field { - // Since we're actually storing three values (a ScheduledValueChange struct), we hash the storage slot to get a - // unique location in which we can safely store as much data as we need. This could be removed if we informed - // the slot allocator of how much space we need so that proper padding could be added. - // See https://github.com/AztecProtocol/aztec-packages/issues/5492 + // Since we can't rely on the native storage allocation scheme, we hash the storage slot to get a unique location in + // which we can safely store as much data as we need. + // See https://github.com/AztecProtocol/aztec-packages/issues/5492 and + // https://github.com/AztecProtocol/aztec-packages/issues/5736 + fn get_value_change_storage_slot(self) -> Field { pedersen_hash([self.storage_slot, 0], 0) } + + fn get_delay_change_storage_slot(self) -> Field { + pedersen_hash([self.storage_slot, 1], 0) + } } mod test { @@ -104,7 +171,10 @@ mod test { use crate::{ context::{PublicContext, PrivateContext, Context}, - state_vars::shared_mutable::shared_mutable::SharedMutable, + state_vars::shared_mutable::{ + shared_mutable::SharedMutable, scheduled_value_change::ScheduledValueChange, + scheduled_delay_change::ScheduledDelayChange + }, oracle::get_public_data_witness::PublicDataWitness }; @@ -113,12 +183,22 @@ mod test { address::AztecAddress, public_data_tree_leaf_preimage::PublicDataTreeLeafPreimage }; - fn setup(private: bool) -> (SharedMutable, Field) { + global pre_value = 13; + global post_value = 42; + + global new_value = 57; + + global pre_delay = 20; + global post_delay = 15; + + global TEST_INITIAL_DELAY = 3; + + fn setup(private: bool) -> (SharedMutable, Field) { let block_number = 40; let context = create_context(block_number, private); let storage_slot = 57; - let state_var: SharedMutable = SharedMutable::new(context, storage_slot); + let state_var: SharedMutable = SharedMutable::new(context, storage_slot); (state_var, block_number) } @@ -135,129 +215,333 @@ mod test { } } - global TEST_DELAY = 20; + fn mock_value_change_read( + state_var: SharedMutable, + pre: Field, + post: Field, + block_of_change: Field + ) { + let value_change_slot = state_var.get_value_change_storage_slot(); + let fields = ScheduledValueChange::new(pre, post, block_of_change as u32).serialize(); + + let _ = OracleMock::mock("storageRead").with_params((value_change_slot, 3)).returns(fields).times(1); + } + + fn mock_delay_change_read( + state_var: SharedMutable, + pre: Field, + post: Field, + block_of_change: Field + ) { + let delay_change_slot = state_var.get_delay_change_storage_slot(); + let delay_change: ScheduledDelayChange = ScheduledDelayChange::new( + Option::some(pre as u32), + Option::some(post as u32), + block_of_change as u32 + ); + let fields = delay_change.serialize(); + + let _ = OracleMock::mock("storageRead").with_params((delay_change_slot, 1)).returns(fields).times(1); + } + + fn mock_delay_change_read_uninitialized(state_var: SharedMutable) { + let delay_change_slot = state_var.get_delay_change_storage_slot(); + let _ = OracleMock::mock("storageRead").with_params((delay_change_slot, 1)).returns([0]).times(1); + } + + // Useful since change and delay values are always the global pre/post ones, so we typically only care about their + // block of change. + fn mock_value_and_delay_read( + state_var: SharedMutable, + value_block_of_change: Field, + delay_block_of_change: Field + ) { + mock_value_change_read(state_var, pre_value, post_value, value_block_of_change); + mock_delay_change_read(state_var, pre_delay, post_delay, delay_block_of_change); + } + + fn mock_value_change_write() -> OracleMock { + OracleMock::mock("storageWrite").returns([0; 3]) + } + + fn mock_delay_change_write() -> OracleMock { + OracleMock::mock("storageWrite").returns([0; 1]) + } + + fn assert_value_change_write( + state_var: SharedMutable, + mock: OracleMock, + pre: Field, + post: Field, + block_of_change: Field + ) { + let fields = ScheduledValueChange::new(pre, post, block_of_change as u32).serialize(); + assert_eq(mock.get_last_params(), (state_var.get_value_change_storage_slot(), fields)); + } - global pre = 13; - global post = 42; + fn assert_delay_change_write( + state_var: SharedMutable, + mock: OracleMock, + pre: Field, + post: Field, + block_of_change: Field + ) { + let delay_change: ScheduledDelayChange = ScheduledDelayChange::new( + Option::some(pre as u32), + Option::some(post as u32), + block_of_change as u32 + ); + + let fields = delay_change.serialize(); + assert_eq(mock.get_last_params(), (state_var.get_delay_change_storage_slot(), fields)); + } #[test] - fn test_get_current_value_in_public_before_change() { + fn test_get_current_value_in_public() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); - // Change in the future, current value is pre - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number + 1]); - assert_eq(state_var.get_current_value_in_public(), pre); + mock_value_change_read(state_var, pre_value, post_value, block_number + 1); + assert_eq(state_var.get_current_value_in_public(), pre_value); + + // Change in the current block, current value is post + mock_value_change_read(state_var, pre_value, post_value, block_number); + assert_eq(state_var.get_current_value_in_public(), post_value); + + // Change in the past, current value is post + mock_value_change_read(state_var, pre_value, post_value, block_number - 1); + assert_eq(state_var.get_current_value_in_public(), post_value); } #[test] - fn test_get_current_value_in_public_at_change() { + fn test_get_scheduled_value_in_public() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Change in the future, scheduled is post (always is) + mock_value_change_read(state_var, pre_value, post_value, block_number + 1); + assert_eq(state_var.get_scheduled_value_in_public(), (post_value, (block_number + 1) as u32)); - // Change in the current block, current value is post - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number]); - assert_eq(state_var.get_current_value_in_public(), post); + // Change in the current block, scheduled is post (always is) + mock_value_change_read(state_var, pre_value, post_value, block_number); + assert_eq(state_var.get_scheduled_value_in_public(), (post_value, block_number as u32)); + + // Change in the past, scheduled is post (always is) + mock_value_change_read(state_var, pre_value, post_value, block_number - 1); + assert_eq(state_var.get_scheduled_value_in_public(), (post_value, (block_number - 1) as u32)); } #[test] - fn test_get_current_value_in_public_after_change() { + fn test_get_current_delay_in_public() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Uninitialized + mock_delay_change_read_uninitialized(state_var); + assert_eq(state_var.get_current_delay_in_public(), TEST_INITIAL_DELAY as u32); + + // Change in the future, current value is pre + mock_delay_change_read(state_var, pre_delay, post_delay, block_number + 1); + assert_eq(state_var.get_current_delay_in_public(), pre_delay as u32); + + // Change in the current block, current value is post + mock_delay_change_read(state_var, pre_delay, post_delay, block_number); + assert_eq(state_var.get_current_delay_in_public(), post_delay as u32); // Change in the past, current value is post - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number - 1]); - assert_eq(state_var.get_current_value_in_public(), post); + mock_delay_change_read(state_var, pre_delay, post_delay, block_number - 1); + assert_eq(state_var.get_current_delay_in_public(), post_delay as u32); } #[test] - fn test_get_scheduled_value_in_public_before_change() { + fn test_get_scheduled_delay_in_public_before_change() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Uninitialized + mock_delay_change_read_uninitialized(state_var); + assert_eq(state_var.get_scheduled_delay_in_public(), (TEST_INITIAL_DELAY as u32, 0)); // Change in the future, scheduled is post (always is) - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number + 1]); - assert_eq(state_var.get_scheduled_value_in_public(), (post, (block_number + 1) as u32)); + mock_delay_change_read(state_var, pre_delay, post_delay, block_number + 1); + assert_eq(state_var.get_scheduled_delay_in_public(), (post_delay as u32, (block_number + 1) as u32)); + + // Change in the current block, scheduled is post (always is) + mock_delay_change_read(state_var, pre_delay, post_delay, block_number); + assert_eq(state_var.get_scheduled_delay_in_public(), (post_delay as u32, block_number as u32)); + + // Change in the past, scheduled is post (always is) + mock_delay_change_read(state_var, pre_delay, post_delay, block_number - 1); + assert_eq(state_var.get_scheduled_delay_in_public(), (post_delay as u32, (block_number - 1) as u32)); } #[test] - fn test_get_scheduled_value_in_public_at_change() { + fn test_schedule_value_change_no_delay() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Last value change was in the past + mock_value_change_read(state_var, pre_value, post_value, 0); - // Change in the current block, scheduled is post (always is) - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number]); - assert_eq(state_var.get_scheduled_value_in_public(), (post, block_number as u32)); + // Current delay is 0 + mock_delay_change_read(state_var, 0, 0, block_number); + + let write_mock = mock_value_change_write(); + + state_var.schedule_value_change(new_value); + + // The new value has a block of change equal to the current block, i.e. it is the current value + assert_value_change_write(state_var, write_mock, post_value, new_value, block_number); } #[test] - fn test_get_scheduled_value_in_public_after_change() { + fn test_schedule_value_change_before_change_no_scheduled_delay() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Value change in the future, delay change in the past + mock_value_and_delay_read(state_var, block_number + 1, block_number - 1); + let write_mock = mock_value_change_write(); - // Change in the past, scheduled is post (always is) - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number - 1]); - assert_eq(state_var.get_scheduled_value_in_public(), (post, (block_number - 1) as u32)); + state_var.schedule_value_change(new_value); + + // The new scheduled value change replaces the old one, post delay (current) is used + assert_value_change_write( + state_var, + write_mock, + pre_value, + new_value, + block_number + post_delay + ); } #[test] - fn test_schedule_value_change_before_change() { + fn test_schedule_value_change_before_change_scheduled_delay() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Value change in the future, delay change in the future + mock_value_and_delay_read(state_var, block_number + 1, block_number + 1); - // Change in the future - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number + 1]); + let write_mock = mock_value_change_write(); - let write_mock = OracleMock::mock("storageWrite").returns([0; 3]); // The oracle return value is actually unused - - let new_value = 42; state_var.schedule_value_change(new_value); - // The new scheduled change replaces the old one - assert_eq(write_mock.get_last_params(), (slot, [pre, new_value, block_number + TEST_DELAY])); + // The new scheduled value change replaces the old one, pre delay (current, not scheduled) is used + assert_value_change_write( + state_var, + write_mock, + pre_value, + new_value, + block_number + pre_delay + ); } #[test] - fn test_schedule_value_change_at_change() { + fn test_schedule_value_change_after_change_no_scheduled_delay() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Value change in the past, delay change in the past + mock_value_and_delay_read(state_var, block_number - 1, block_number - 1); + let write_mock = mock_value_change_write(); + + state_var.schedule_value_change(new_value); + + // The previous post value becomes the pre value, post delay (current) is used + assert_value_change_write( + state_var, + write_mock, + post_value, + new_value, + block_number + post_delay + ); + } + + #[test] + fn test_schedule_value_change_after_change_scheduled_delay() { + let (state_var, block_number) = setup(false); - // Change in the current block - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number]); + // Value change in the past, delay change in the future + mock_value_and_delay_read(state_var, block_number - 1, block_number + 1); - let write_mock = OracleMock::mock("storageWrite").returns([0; 3]); // The oracle return value is actually unused + let write_mock = mock_value_change_write(); - let new_value = 42; state_var.schedule_value_change(new_value); - // The previous 'post' value is the current one and becomes the 'pre' value - assert_eq(write_mock.get_last_params(), (slot, [post, new_value, block_number + TEST_DELAY])); + // The previous post value becomes the pre value, pre delay (current, not scheduled) is used + assert_value_change_write( + state_var, + write_mock, + post_value, + new_value, + block_number + pre_delay + ); } #[test] - fn test_schedule_value_change_after_change() { + fn test_schedule_delay_increase_before_change() { let (state_var, block_number) = setup(false); - let slot = state_var.get_derived_storage_slot(); + // Delay change in future, current delay is pre + mock_delay_change_read(state_var, pre_delay, post_delay, block_number + 1); + let write_mock = mock_delay_change_write(); - // Change in the past - OracleMock::mock("storageRead").with_params((slot, 3)).returns([pre, post, block_number - 1]); + let new_delay = pre_delay + 1; + state_var.schedule_delay_change(new_delay as u32); - let write_mock = OracleMock::mock("storageWrite").returns([0; 3]); // The oracle return value is actually unused + // The previous scheduled change is lost, change is immediate (due to increase) + assert_delay_change_write(state_var, write_mock, pre_delay, new_delay, block_number); + } - let new_value = 42; - state_var.schedule_value_change(new_value); + #[test] + fn test_schedule_delay_reduction_before_change() { + let (state_var, block_number) = setup(false); + + // Delay change in future, current delay is pre + mock_delay_change_read(state_var, pre_delay, post_delay, block_number + 1); + let write_mock = mock_delay_change_write(); + + let new_delay = pre_delay - 1; + state_var.schedule_delay_change(new_delay as u32); + + // The previous scheduled change is lost, change delay equals difference (due to reduction) + assert_delay_change_write( + state_var, + write_mock, + pre_delay, + new_delay, + block_number + pre_delay - new_delay + ); + } + + #[test] + fn test_schedule_delay_increase_after_change() { + let (state_var, block_number) = setup(false); + + // Delay change in the past, current delay is post + mock_delay_change_read(state_var, pre_delay, post_delay, block_number - 1); + let write_mock = mock_delay_change_write(); + + let new_delay = post_delay + 1; + state_var.schedule_delay_change(new_delay as u32); + + // The current value becomes pre, change is immediate (due to increase) + assert_delay_change_write(state_var, write_mock, post_delay, new_delay, block_number); + } + + #[test] + fn test_schedule_delay_reduction_after_change() { + let (state_var, block_number) = setup(false); - // The previous 'post' value is the current one and becomes the 'pre' value - assert_eq(write_mock.get_last_params(), (slot, [post, new_value, block_number + TEST_DELAY])); + // Delay change in the past, current delay is post + mock_delay_change_read(state_var, pre_delay, post_delay, block_number - 1); + let write_mock = mock_delay_change_write(); + + let new_delay = post_delay - 1; + state_var.schedule_delay_change(new_delay as u32); + + // The current value becomes pre, change delay equals difference (due to reduction) + assert_delay_change_write( + state_var, + write_mock, + post_delay, + new_delay, + block_number + post_delay - new_delay + ); } #[test] diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr index d4e9ddb6bd2..7da8f1524fc 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr @@ -3,22 +3,25 @@ use dep::protocol_types::{hash::pedersen_hash, traits::FromField, address::Aztec use crate::context::{PrivateContext, Context}; use crate::history::public_storage::public_storage_historical_read; use crate::public_storage; -use crate::state_vars::{storage::Storage, shared_mutable::scheduled_value_change::ScheduledValueChange}; +use crate::state_vars::{ + storage::Storage, + shared_mutable::{scheduled_delay_change::ScheduledDelayChange, scheduled_value_change::ScheduledValueChange} +}; -struct SharedMutablePrivateGetter { +struct SharedMutablePrivateGetter { context: PrivateContext, // The contract address of the contract we want to read from other_contract_address: AztecAddress, // The storage slot where the SharedMutable is stored on the other contract storage_slot: Field, - // The _dummy variable forces DELAY to be interpreted as a numberic value. This is a workaround to + // The _dummy variable forces INITIAL_DELAY to be interpreted as a numberic value. This is a workaround to // https://github.com/noir-lang/noir/issues/4633. Remove once resolved. - _dummy: [Field; DELAY], + _dummy: [Field; INITIAL_DELAY], } // We have this as a view-only interface to reading Shared Mutables in other contracts. // Currently the Shared Mutable does not support this. We can adapt SharedMutable at a later date -impl SharedMutablePrivateGetter { +impl SharedMutablePrivateGetter { pub fn new( context: PrivateContext, other_contract_address: AztecAddress, @@ -26,48 +29,50 @@ impl SharedMutablePrivateGetter { ) -> Self { assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1."); assert(other_contract_address.to_field() != 0, "Other contract address cannot be 0"); - Self { context, other_contract_address, storage_slot, _dummy: [0; DELAY] } + Self { context, other_contract_address, storage_slot, _dummy: [0; INITIAL_DELAY] } } pub fn get_current_value_in_private(self) -> T where T: FromField { let mut context = self.context; - let (scheduled_value_change, historical_block_number) = self.historical_read_from_public_storage(context); - let block_horizon = scheduled_value_change.get_block_horizon(historical_block_number, DELAY); + let (value_change, delay_change, historical_block_number) = self.historical_read_from_public_storage(context); + let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); + let block_horizon = value_change.get_block_horizon(historical_block_number, effective_minimum_delay); - // We prevent this transaction from being included in any block after the block horizon, ensuring that the - // historical public value matches the current one, since it can only change after the horizon. context.set_tx_max_block_number(block_horizon); - scheduled_value_change.get_current_at(historical_block_number) + value_change.get_current_at(historical_block_number) } fn historical_read_from_public_storage( self, context: PrivateContext - ) -> (ScheduledValueChange, u32) where T: FromField { - let derived_slot = self.get_derived_storage_slot(); - - // Ideally the following would be simply public_storage::read_historical, but we can't implement that yet. - let mut raw_fields = [0; 3]; + ) -> (ScheduledValueChange, ScheduledDelayChange, u32) where T: FromField { + let value_change_slot = self.get_value_change_storage_slot(); + let mut raw_value_change_fields = [0; 3]; for i in 0..3 { - raw_fields[i] = public_storage_historical_read( - context, - derived_slot + i as Field, - self.other_contract_address - ); + raw_value_change_fields[i] = public_storage_historical_read( + context, + value_change_slot + i as Field, + self.other_contract_address + ); } - let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); + let delay_change_slot = self.get_delay_change_storage_slot(); + let raw_delay_change_fields = [public_storage_historical_read(context, delay_change_slot, context.this_address())]; + + let value_change = ScheduledValueChange::deserialize(raw_value_change_fields); + let delay_change = ScheduledDelayChange::deserialize(raw_delay_change_fields); + let historical_block_number = context.historical_header.global_variables.block_number as u32; - (scheduled_value, historical_block_number) + (value_change, delay_change, historical_block_number) } - fn get_derived_storage_slot(self) -> Field { - // Since we're actually storing three values (a ScheduledValueChange struct), we hash the storage slot to get a - // unique location in which we can safely store as much data as we need. This could be removed if we informed - // the slot allocator of how much space we need so that proper padding could be added. - // See https://github.com/AztecProtocol/aztec-packages/issues/5492 + fn get_value_change_storage_slot(self) -> Field { pedersen_hash([self.storage_slot, 0], 0) } + + fn get_delay_change_storage_slot(self) -> Field { + pedersen_hash([self.storage_slot, 1], 0) + } } diff --git a/noir-projects/noir-contracts/contracts/auth_contract/src/main.nr b/noir-projects/noir-contracts/contracts/auth_contract/src/main.nr index 0de4f7c2093..deb5e34315e 100644 --- a/noir-projects/noir-contracts/contracts/auth_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/auth_contract/src/main.nr @@ -21,8 +21,6 @@ contract Auth { fn constructor(admin: AztecAddress) { assert(!admin.is_zero(), "invalid admin"); storage.admin.initialize(admin); - // Note that we don't initialize authorized with any value: because storage defaults to 0 it'll have a 'post' - // value of 0 and block of change 0, meaning it is effectively autoinitialized at the zero address. } // docs:start:shared_mutable_schedule