Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: shared mutable configurable delays #6104

Merged
merged 25 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
df451ff
Add scheduled delay change, adjust shared mutable
nventuro Apr 30, 2024
d0fe460
Add some more tests
nventuro Apr 30, 2024
80a860e
Adapt auth contract
nventuro Apr 30, 2024
b400ba0
Adapt to type param
nventuro May 2, 2024
503aa01
Add initial delay tests
nventuro May 2, 2024
f921f93
Update Auth contract
nventuro May 2, 2024
381a1f1
Add helper to make tests shorter
nventuro May 2, 2024
c6b0fa1
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 2, 2024
b47e5db
Add notes on Option usage
nventuro May 3, 2024
73ac5f5
Add migration notes
nventuro May 3, 2024
a9e7c75
Added comments re. max_block_number coordination
nventuro May 3, 2024
c095daa
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 3, 2024
27e64d6
update shared private getter
nventuro May 3, 2024
ddf6d4e
Apply suggestions from code review
nventuro May 7, 2024
d004ca5
Improvements from code review
nventuro May 7, 2024
88b6040
Clarfy diagram
nventuro May 7, 2024
ca2b228
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 7, 2024
f95f434
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 7, 2024
ad7fcd6
Fix diagrams
nventuro May 8, 2024
9512976
Fix migration notes
nventuro May 8, 2024
bbc4a67
Clarify docs
nventuro May 8, 2024
ce8ea8a
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 8, 2024
d8e9810
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 9, 2024
9053239
Fix nargo formatting
nventuro May 9, 2024
4fed5d3
Merge branch 'master' into nv/shared-mutable-delay
nventuro May 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
mod shared_mutable;
mod scheduled_delay_change;
mod scheduled_value_change;
mod shared_mutable_private_getter;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,387 @@
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 ScheuledValueChange object can
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// schedule a change.
// This delay is initally zero, but 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 {
pre: u32,
post: u32,
block_of_change: u32,
nventuro marked this conversation as resolved.
Show resolved Hide resolved
}

impl ScheduledDelayChange {
pub fn new(pre: u32, post: u32, block_of_change: u32) -> Self {
Self { pre, post, block_of_change }
}

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of / in here. Inconsistent number of / for comments throughout the code.

nventuro marked this conversation as resolved.
Show resolved Hide resolved
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
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// block of change will use the post value.

if current_block_number < self.block_of_change {
self.pre
} else {
self.post
}
}

/// 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, 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scheduling is always a little weird in my head. So for this case, if I have an initial delay of 3 days, and do any reduction, that change will take effect such that the value can be changed after 3 days.

Can you explain again the reason of doing that in here. Just that when I read this, my simply mind is like "why?", if you had the original 3 day delay as the block of change that seems easier to follow.

The benefit was that you avoid having this weird "almost change" period where if it is queued and I insert a change at block N, it might take 3 days before effective and N+1 might take 1 day (with your example). The comment is inside the function, but when I just read here I go "huh" and then write all this blabber before I see it 😆

Copy link
Contributor Author

@nventuro nventuro May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if you're asking 'why are we waiting the minimum amount of time instead of always simply waiting a longer time?', or if you're not sure why this is the correct amount of time to wait 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda both actually. When I read:

when reducing the delay, the change will take effect after a delay equal to the difference between old and new delay

It is not instantly clear to me why we use that and why that is the shortest time (i might be idiot) and I had to think about it 🤯.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From past experience it can be very annoying to be forced to wait extra time when you don't need to (e.g. if decreasing a delay from a month to two weeks, having to wait a total of 4 vs 6 weeks is a big difference). Hopefully most people will be relatively satisfied with the behavior and not be nerd-sniped by the API reference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I get why, was just not something that instantly clicked for me just from the comment. But think that might just be me reading comprehension 😆

/// 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 schedule_delay = if new > current {
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// 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=================>
// ^ ^ ^
// |-------------------|--------------------------------|
// | schedule delay new delay |
// ------------------------------------------------------
// current delay
current - new
};

self.pre = current;
self.post = new;
self.block_of_change = current_block_number + schedule_delay;
}

/// 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
nventuro marked this conversation as resolved.
Show resolved Hide resolved
/// `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
} 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 delay actual earliest value
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// block block of change block of change
// =======NS=============|=============================X===========Y=====>
benesjan marked this conversation as resolved.
Show resolved Hide resolved
// ^ ^ ^ ^
// earliest value | | |
// change schedule block | | |
// | | | |
// |-------------|------------------------------ |
// | blocks new delay |
// | until change |
// | |
// |-------------------------------------------------------|
// current delay at earliest value change schedule block
nventuro marked this conversation as resolved.
Show resolved Hide resolved
nventuro marked this conversation as resolved.
Show resolved Hide resolved

let blocks_until_change = self.block_of_change - (historical_block_number + 1);

min(self.pre, blocks_until_change + self.post)
}
}
}

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 | post ]
// High limb: [ empty | block_of_change ]

let lo = ((self.pre as u64) * (1 << 32))
+ (self.post as u64);
let hi = 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]);

// Pre and post are both stored in the lower limb, so we use division and modulo to clear the bits that correspond
// to the other value when reading these.

Self {
pre: ((packed.lo as u64) / (1 << 32)) as u32,
post: ((packed.lo as u64) % (1 << 32)) as u32,
block_of_change: packed.hi as u32,
}
}
}

mod test {
use crate::state_vars::shared_mutable::scheduled_delay_change::ScheduledDelayChange;

#[test]
fn test_serde() {
let pre = 1;
let post = 2;
let block_of_change = 50;

let original = ScheduledDelayChange::new(pre, post, block_of_change);
let converted = 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_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;

let original = ScheduledDelayChange::new(pre, post, block_of_change);
let converted = 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_get_current() {
let pre = 1;
let post = 2;
let block_of_change = 50;

let delay_change = ScheduledDelayChange::new(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_scheduled() {
let pre = 1;
let post = 2;
let block_of_change = 50;

let delay_change = ScheduledDelayChange::new(pre, post, block_of_change);

assert_eq(delay_change.get_scheduled(), (post, block_of_change));
}

#[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 = ScheduledDelayChange::new(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 different between the current value (pre) and new delay.
nventuro marked this conversation as resolved.
Show resolved Hide resolved
assert_eq(delay_change.pre, pre);
assert_eq(delay_change.post, 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 = ScheduledDelayChange::new(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, post);
assert_eq(delay_change.post, new);
assert_eq(delay_change.block_of_change, current_block_number + post - 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 = ScheduledDelayChange::new(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, pre);
assert_eq(delay_change.post, 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 = ScheduledDelayChange::new(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, post);
assert_eq(delay_change.post, 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 = ScheduledDelayChange::new(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 = ScheduledDelayChange::new(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 = ScheduledDelayChange::new(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 = ScheduledDelayChange::new(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);
}
}
Loading
Loading