From cfa850bbbad9ff54a7efd9fd7a045a6d3f158ebf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 30 Apr 2024 13:32:41 -0300 Subject: [PATCH] chore: prepare ScheduledValueChange for mutable delays. (#6085) 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. --- .../shared_mutable/scheduled_value_change.nr | 126 ++++++++++-------- .../shared_mutable/shared_mutable.nr | 24 ++-- .../shared_mutable_private_getter.nr | 91 +++++++------ 3 files changed, 135 insertions(+), 106 deletions(-) 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 482bbf0d3f7..52aba6277ea 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 @@ -2,27 +2,18 @@ use dep::protocol_types::traits::{Serialize, Deserialize, FromField, ToField}; // 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 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'. -// -// The delay being a type parameter instead of a struct field is an implementation detail, and is due to a number of -// reasons: -// - we want to serialize and deserialize this object in order to store it in public storage, but we don't want to -// include the delay there because it is immutable -// - because of how aztec-nr state variables are declared, having a type with some immutable property is better -// expressed via types, since they are always constructed with the same `::new(context, storage_slot)` function. -struct ScheduledValueChange { +// 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_of_change: u32, - // 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 ScheduledValueChange { +impl ScheduledValueChange { pub fn new(pre: T, post: T, block_of_change: u32) -> Self { - Self { pre, post, block_of_change, _dummy: [0; DELAY] } + 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 @@ -54,10 +45,12 @@ 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`, /// 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 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) -> u32 { + 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 // (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 @@ -68,22 +61,23 @@ 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 + DELAY`. Our block horizon is simply the previous block to that one. + // equals `historical_block_number + 1 + minimum_delay`. Our block horizon is simply the previous block to + // that one. // // block of historical // change block block horizon // =======|=============N===================H===========> // ^ ^ // --------------------- - // delay + // minimum delay - historical_block_number + DELAY + 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 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. + // 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 // block block horizon block of change @@ -91,11 +85,12 @@ impl ScheduledValueChange { // ^ ^ // | | // ----------------------------------- - // delay + // minimum delay // - // b) It could be fewer than `delay` blocks away from the historical block number, in which case it would - // become the limiting factor for the time horizon, which would be the block right before the block of - // change (since by definition the value changes at the block of change). + // 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 @@ -103,7 +98,7 @@ impl ScheduledValueChange { // ^ ^ ^ // | actual horizon | // ----------------------------------- - // 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 @@ -111,34 +106,42 @@ impl ScheduledValueChange { // Because historical_block_number < self.block_of_change, then block_of_change > 0 and we can safely // subtract 1. - min(self.block_of_change - 1, historical_block_number + DELAY) + min( + self.block_of_change - 1, + historical_block_number + minimum_delay + ) } } /// 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. - pub fn schedule_change(&mut self, new_value: T, current_block_number: u32) { + pub fn schedule_change( + &mut self, + new_value: T, + current_block_number: u32, + minimum_delay: u32, + block_of_change: u32 + ) { + assert(block_of_change >= current_block_number + minimum_delay); + self.pre = self.get_current_at(current_block_number); self.post = new_value; - // TODO: make this configurable - // https://github.com/AztecProtocol/aztec-packages/issues/5501 - self.block_of_change = current_block_number + DELAY; + self.block_of_change = block_of_change; } } -impl Serialize<3> for ScheduledValueChange { +impl Serialize<3> for ScheduledValueChange { fn serialize(self) -> [Field; 3] where T: ToField { [self.pre.to_field(), self.post.to_field(), self.block_of_change.to_field()] } } -impl Deserialize<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]), - block_of_change: FromField::from_field(input[2]), - _dummy: [0; DELAY] + block_of_change: FromField::from_field(input[2]), } } } @@ -157,7 +160,7 @@ fn test_min() { mod test { use crate::state_vars::shared_mutable::scheduled_value_change::ScheduledValueChange; - global TEST_DELAY = 200; + global TEST_DELAY: u32 = 200; #[test] fn test_get_current_at() { @@ -165,7 +168,7 @@ mod test { let post = 2; let block_of_change = 50; - let value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let value: 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); @@ -179,13 +182,13 @@ mod test { let post = 2; let block_of_change = 50; - let value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); assert_eq(value.get_scheduled(), (post, block_of_change)); } fn assert_block_horizon_invariants( - value: &mut ScheduledValueChange, + value: &mut ScheduledValueChange, historical_block_number: u32, block_horizon: u32 ) { @@ -198,7 +201,12 @@ mod test { // 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(new, historical_block_number + 1); + value.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)); @@ -209,9 +217,9 @@ 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: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number); + let block_horizon = value.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); @@ -222,9 +230,9 @@ 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: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number); + let block_horizon = value.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); @@ -235,12 +243,12 @@ 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: 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); + let block_horizon = value.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); @@ -251,9 +259,9 @@ 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: ScheduledValueChange = ScheduledValueChange::new(1, 2, block_of_change); - let block_horizon = value.get_block_horizon(historical_block_number); + let block_horizon = value.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); @@ -265,11 +273,16 @@ mod test { let post = 2; let block_of_change = 500; - let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); let new = 42; let current_block_number = block_of_change - 50; - value.schedule_change(new, current_block_number); + value.schedule_change( + new, + current_block_number, + TEST_DELAY, + current_block_number + TEST_DELAY + ); // Because we re-schedule before the last scheduled change takes effect, the old `post` value is lost. assert_eq(value.pre, pre); @@ -283,11 +296,16 @@ mod test { let post = 2; let block_of_change = 500; - let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); + let mut value: ScheduledValueChange = ScheduledValueChange::new(pre, post, block_of_change); let new = 42; let current_block_number = block_of_change + 50; - value.schedule_change(new, current_block_number); + value.schedule_change( + new, + current_block_number, + TEST_DELAY, + current_block_number + TEST_DELAY + ); assert_eq(value.pre, post); assert_eq(value.post, 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 71cf0a85956..8ab974c7389 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 @@ -8,6 +8,9 @@ use crate::state_vars::{storage::Storage, shared_mutable::scheduled_value_change 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 {} @@ -24,27 +27,32 @@ impl Storage for SharedMutable {} 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 } + Self { context, storage_slot, _dummy: [0; DELAY] } } 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 scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); - scheduled_value_change.schedule_change(new_value, context.block_number() as u32); + let block_number = context.block_number() as u32; + // TODO: make this configurable + // https://github.com/AztecProtocol/aztec-packages/issues/5501 + let block_of_change = block_number + DELAY; + + scheduled_value_change.schedule_change(new_value, block_number, DELAY, block_of_change); public_storage::write(self.get_derived_storage_slot(), scheduled_value_change); } pub fn get_current_value_in_public(self) -> T { - let scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); + let scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); let block_number = self.context.public.unwrap().block_number() as u32; scheduled_value_change.get_current_at(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()); + let scheduled_value_change: ScheduledValueChange = public_storage::read(self.get_derived_storage_slot()); scheduled_value_change.get_scheduled() } @@ -52,7 +60,7 @@ impl SharedMutable { 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); + let block_horizon = scheduled_value_change.get_block_horizon(historical_block_number, 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. @@ -63,7 +71,7 @@ impl SharedMutable { fn historical_read_from_public_storage( self, context: PrivateContext - ) -> (ScheduledValueChange, u32) where T: FromField { + ) -> (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. @@ -76,7 +84,7 @@ impl SharedMutable { ); } - let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); + let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); let historical_block_number = context.historical_header.global_variables.block_number as u32; (scheduled_value, historical_block_number) 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 2f4f4339481..d4e9ddb6bd2 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 @@ -6,65 +6,68 @@ use crate::public_storage; use crate::state_vars::{storage::Storage, shared_mutable::scheduled_value_change::ScheduledValueChange}; 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, + 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 + // https://github.com/noir-lang/noir/issues/4633. Remove once resolved. + _dummy: [Field; 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 { - pub fn new( - context: PrivateContext, - other_contract_address: AztecAddress, - storage_slot: Field, - ) -> 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 } - } + pub fn new( + context: PrivateContext, + other_contract_address: AztecAddress, + storage_slot: Field + ) -> 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] } + } - pub fn get_current_value_in_private(self) -> T where T: FromField { - let mut context = self.context; + 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); + 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); - // 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) - } + // 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) + } - fn historical_read_from_public_storage( - self, - context: PrivateContext - ) -> (ScheduledValueChange, u32) where T: FromField { - let derived_slot = self.get_derived_storage_slot(); + 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]; - for i in 0..3 { - raw_fields[i] = public_storage_historical_read( + // Ideally the following would be simply public_storage::read_historical, but we can't implement that yet. + let mut raw_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 ); - } + } - let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); - let historical_block_number = context.historical_header.global_variables.block_number as u32; + let scheduled_value: ScheduledValueChange = ScheduledValueChange::deserialize(raw_fields); + let historical_block_number = context.historical_header.global_variables.block_number as u32; - (scheduled_value, historical_block_number) - } + (scheduled_value, 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 - pedersen_hash([self.storage_slot, 0], 0) - } + 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 + pedersen_hash([self.storage_slot, 0], 0) + } }