From e61b91bb693b89374bd58b93341d66e4a20de960 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 13 Jan 2022 22:08:42 -0400 Subject: [PATCH 1/6] Gav wrote this code in pull #10195. Extracting to simplify that PR. --- frame/support/src/storage/bounded_vec.rs | 152 ++++++++++++++++++++++- 1 file changed, 150 insertions(+), 2 deletions(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 9f43d37a2d7f1..2ac02506ed6ee 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -149,6 +149,16 @@ impl BoundedVec { ) -> Option<&mut >::Output> { self.0.get_mut(index) } + + /// Exactly the same semantics as [`Vec::truncate`]. + pub fn truncate(&mut self, s: usize) { + self.0.truncate(s); + } + + /// Exactly the same semantics as [`Vec::pop`]. + pub fn pop(&mut self) -> Option { + self.0.pop() + } } impl> From> for Vec { @@ -176,6 +186,83 @@ impl> BoundedVec { S::get() as usize } + /// Forces the insertion of `s` into `self` retaining all items with index at least `index`. + /// + /// If `index == 0` and `self.len() == Self::bound()` then this is a no-op. + /// + /// Returns `true` if the item was inserted. + pub fn force_insert_keep_right(&mut self, index: usize, element: T) -> bool { + if self.len() < Self::bound() { + self.0.insert(index, element); + } else { + if index == 0 { + return false + } + self[0] = element; + self[0..index].rotate_left(1); + } + true + } + + /// Forces the insertion of `s` into `self` retaining all items with index at most `index`. + /// + /// If `index == Self::bound()` and `self.len() == Self::bound()` then this is a no-op. + /// + /// Returns `true` if the item was inserted. + pub fn force_insert_keep_left(&mut self, index: usize, element: T) -> bool { + if self.len() >= Self::bound() && index >= Self::bound() { + return false + } + self.0.insert(index, element); + self.0.truncate(Self::bound()); + true + } + + /// Move the position of an item from one location to another in the slice. + /// + /// Except for the item being moved, the order of the slice remains the same. + /// + /// - `index` is the location of the item to be moved. + /// - `insert_position` is the index of the item in the slice which should *immediately follow* + /// the item which is being moved. + pub fn slide(&mut self, index: usize, insert_position: usize) { + if insert_position < index { + // --- --- --- === === === === @@@ --- --- --- + // ^-- N ^O^ + // ... + // /-----<<<-----\ + // --- --- --- === === === === @@@ --- --- --- + // >>> >>> >>> >>> + // ... + // --- --- --- @@@ === === === === --- --- --- + // ^N^ + self[insert_position..=index].rotate_right(1); + } else if index + 1 < insert_position { + // Note that the apparent asymmetry of these two branches is due to the + // fact that the "new" position is the position to be inserted *before*. + // --- --- --- @@@ === === === === --- --- --- + // ^O^ ^-- N + // ... + // /----->>>-----\ + // --- --- --- @@@ === === === === --- --- --- + // <<< <<< <<< <<< + // ... + // --- --- --- === === === === @@@ --- --- --- + // ^N^ + self[index..=(insert_position - 1)].rotate_left(1); + } + } + + /// Forces the insertion of `s` into `self` truncating first if necessary. + /// + /// Infallible, but if the limit is zero, then it's a no-op. + pub fn force_push(&mut self, element: T) { + if Self::bound() > 0 { + self.0.truncate(Self::bound() as usize - 1); + self.0.push(element); + } + } + /// Same as `Vec::resize`, but if `size` is more than [`Self::bound`], then [`Self::bound`] is /// used. pub fn bounded_resize(&mut self, size: usize, value: T) @@ -397,8 +484,7 @@ where #[cfg(test)] pub mod test { use super::*; - use crate::Twox128; - use frame_support::traits::ConstU32; + use crate::{traits::ConstU32, Twox128}; use sp_io::TestExternalities; crate::generate_storage_alias! { Prefix, Foo => Value>> } @@ -408,6 +494,68 @@ pub mod test { FooDoubleMap => DoubleMap<(u32, Twox128), (u32, Twox128), BoundedVec>> } + #[test] + fn slide_works() { + let mut b: BoundedVec> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap(); + b.slide(1, 5); + assert_eq!(*b, vec![0, 2, 3, 4, 1, 5]); + b.slide(4, 0); + assert_eq!(*b, vec![1, 0, 2, 3, 4, 5]); + b.slide(0, 2); + assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); + b.slide(1, 6); + assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]); + b.slide(0, 6); + assert_eq!(*b, vec![2, 3, 4, 5, 1, 0]); + } + + #[test] + fn slide_noops_work() { + let mut b: BoundedVec> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap(); + b.slide(3, 3); + assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); + b.slide(3, 4); + assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); + } + + #[test] + fn force_insert_keep_left_works() { + let mut b: BoundedVec> = vec![].try_into().unwrap(); + assert!(b.force_insert_keep_left(0, 30)); + assert!(b.force_insert_keep_left(0, 10)); + assert!(b.force_insert_keep_left(1, 20)); + assert!(b.force_insert_keep_left(3, 40)); + assert_eq!(*b, vec![10, 20, 30, 40]); + // at capacity. + assert!(!b.force_insert_keep_left(4, 41)); + assert_eq!(*b, vec![10, 20, 30, 40]); + assert!(b.force_insert_keep_left(3, 31)); + assert_eq!(*b, vec![10, 20, 30, 31]); + assert!(b.force_insert_keep_left(1, 11)); + assert_eq!(*b, vec![10, 11, 20, 30]); + assert!(b.force_insert_keep_left(0, 1)); + assert_eq!(*b, vec![1, 10, 11, 20]); + } + + #[test] + fn force_insert_keep_right_works() { + let mut b: BoundedVec> = vec![].try_into().unwrap(); + assert!(b.force_insert_keep_right(0, 30)); + assert!(b.force_insert_keep_right(0, 10)); + assert!(b.force_insert_keep_right(1, 20)); + assert!(b.force_insert_keep_right(3, 40)); + assert_eq!(*b, vec![10, 20, 30, 40]); + // at capacity. + assert!(!b.force_insert_keep_right(0, 0)); + assert_eq!(*b, vec![10, 20, 30, 40]); + assert!(b.force_insert_keep_right(1, 11)); + assert_eq!(*b, vec![11, 20, 30, 40]); + assert!(b.force_insert_keep_right(3, 31)); + assert_eq!(*b, vec![20, 30, 31, 40]); + assert!(b.force_insert_keep_right(4, 41)); + assert_eq!(*b, vec![30, 31, 40, 41]); + } + #[test] fn try_append_is_correct() { assert_eq!(BoundedVec::>::bound(), 7); From a3e273a4918aaaf3bb99a61f1c2c4fa0251ae3e7 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 13 Jan 2022 23:04:36 -0400 Subject: [PATCH 2/6] fix potential panics --- frame/support/src/storage/bounded_vec.rs | 37 ++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 2ac02506ed6ee..0b88881f07862 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -188,17 +188,25 @@ impl> BoundedVec { /// Forces the insertion of `s` into `self` retaining all items with index at least `index`. /// - /// If `index == 0` and `self.len() == Self::bound()` then this is a no-op. + /// If `index == 0` and `self.len() == Self::bound()`, then this is a no-op. + /// + /// If `Self::bound() < index` or `self.len() < index`, then this is also a no-op. /// /// Returns `true` if the item was inserted. pub fn force_insert_keep_right(&mut self, index: usize, element: T) -> bool { + if Self::bound() < index || self.len() < index { + return false + } if self.len() < Self::bound() { + // Cannot panic since self.len() >= index; self.0.insert(index, element); } else { if index == 0 { return false } self[0] = element; + // `[0..index] cannot panic since self.len() >= index. + // `rotate_left(1)` cannot panic because there is at least 1 element. self[0..index].rotate_left(1); } true @@ -210,9 +218,15 @@ impl> BoundedVec { /// /// Returns `true` if the item was inserted. pub fn force_insert_keep_left(&mut self, index: usize, element: T) -> bool { - if self.len() >= Self::bound() && index >= Self::bound() { + // Check against panics. + if Self::bound() < index || self.len() < index { return false } + // Noop condition. + if Self::bound() == index && self.len() <= Self::bound() { + return false + } + // Cannot panic since self.len() >= index; self.0.insert(index, element); self.0.truncate(Self::bound()); true @@ -521,6 +535,9 @@ pub mod test { #[test] fn force_insert_keep_left_works() { let mut b: BoundedVec> = vec![].try_into().unwrap(); + assert!(!b.force_insert_keep_left(1, 10)); + assert!(b.is_empty()); + assert!(b.force_insert_keep_left(0, 30)); assert!(b.force_insert_keep_left(0, 10)); assert!(b.force_insert_keep_left(1, 20)); @@ -535,11 +552,19 @@ pub mod test { assert_eq!(*b, vec![10, 11, 20, 30]); assert!(b.force_insert_keep_left(0, 1)); assert_eq!(*b, vec![1, 10, 11, 20]); + + let mut z: BoundedVec> = vec![].try_into().unwrap(); + assert!(z.is_empty()); + assert!(!z.force_insert_keep_left(0, 10)); + assert!(z.is_empty()); } #[test] fn force_insert_keep_right_works() { let mut b: BoundedVec> = vec![].try_into().unwrap(); + assert!(!b.force_insert_keep_right(1, 10)); + assert!(b.is_empty()); + assert!(b.force_insert_keep_right(0, 30)); assert!(b.force_insert_keep_right(0, 10)); assert!(b.force_insert_keep_right(1, 20)); @@ -554,6 +579,14 @@ pub mod test { assert_eq!(*b, vec![20, 30, 31, 40]); assert!(b.force_insert_keep_right(4, 41)); assert_eq!(*b, vec![30, 31, 40, 41]); + + assert!(!b.force_insert_keep_right(5, 69)); + assert_eq!(*b, vec![30, 31, 40, 41]); + + let mut z: BoundedVec> = vec![].try_into().unwrap(); + assert!(z.is_empty()); + assert!(!z.force_insert_keep_right(0, 10)); + assert!(z.is_empty()); } #[test] From 0ed947c1e1223d19ee5de029898bbc19873bf224 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 13 Jan 2022 23:57:05 -0400 Subject: [PATCH 3/6] prevent panics in slide --- frame/support/src/storage/bounded_vec.rs | 55 ++++++++++++++++++------ 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 0b88881f07862..677ef467c8014 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -194,6 +194,7 @@ impl> BoundedVec { /// /// Returns `true` if the item was inserted. pub fn force_insert_keep_right(&mut self, index: usize, element: T) -> bool { + // Check against panics. if Self::bound() < index || self.len() < index { return false } @@ -239,8 +240,18 @@ impl> BoundedVec { /// - `index` is the location of the item to be moved. /// - `insert_position` is the index of the item in the slice which should *immediately follow* /// the item which is being moved. - pub fn slide(&mut self, index: usize, insert_position: usize) { - if insert_position < index { + /// + /// Returns `true` of the operation was successful, otherwise `false` if a noop. + pub fn slide(&mut self, index: usize, insert_position: usize) -> bool{ + // Check against panics. + if self.len() <= index || self.len() < insert_position || index == usize::MAX { + return false + } + // Noop conditions. + if index == insert_position || index + 1 == insert_position { + return false + } + if insert_position < index && index < self.len() { // --- --- --- === === === === @@@ --- --- --- // ^-- N ^O^ // ... @@ -250,8 +261,9 @@ impl> BoundedVec { // ... // --- --- --- @@@ === === === === --- --- --- // ^N^ - self[insert_position..=index].rotate_right(1); - } else if index + 1 < insert_position { + self[insert_position..index + 1].rotate_right(1); + return true + } else if insert_position > 0 && index + 1 < insert_position { // Note that the apparent asymmetry of these two branches is due to the // fact that the "new" position is the position to be inserted *before*. // --- --- --- @@@ === === === === --- --- --- @@ -263,8 +275,12 @@ impl> BoundedVec { // ... // --- --- --- === === === === @@@ --- --- --- // ^N^ - self[index..=(insert_position - 1)].rotate_left(1); + self[index..insert_position].rotate_left(1); + return true } + + debug_assert!(false, "all noop conditions should have been covered above"); + false } /// Forces the insertion of `s` into `self` truncating first if necessary. @@ -511,24 +527,39 @@ pub mod test { #[test] fn slide_works() { let mut b: BoundedVec> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap(); - b.slide(1, 5); + assert!(b.slide(1, 5)); assert_eq!(*b, vec![0, 2, 3, 4, 1, 5]); - b.slide(4, 0); + assert!(b.slide(4, 0)); assert_eq!(*b, vec![1, 0, 2, 3, 4, 5]); - b.slide(0, 2); + assert!(b.slide(0, 2)); assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); - b.slide(1, 6); + assert!(b.slide(1, 6)); assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]); - b.slide(0, 6); + assert!(b.slide(0, 6)); assert_eq!(*b, vec![2, 3, 4, 5, 1, 0]); + assert!(b.slide(5, 0)); + assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]); + assert!(!b.slide(6, 0)); + assert!(!b.slide(7, 0)); + assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]); + + let mut c: BoundedVec> = vec![0, 1, 2].try_into().unwrap(); + assert!(!c.slide(1, 5)); + assert_eq!(*c, vec![0, 1, 2]); + assert!(!c.slide(4, 0)); + assert_eq!(*c, vec![0, 1, 2]); + assert!(!c.slide(3, 0)); + assert_eq!(*c, vec![0, 1, 2]); + assert!(c.slide(2, 0)); + assert_eq!(*c, vec![2, 0, 1]); } #[test] fn slide_noops_work() { let mut b: BoundedVec> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap(); - b.slide(3, 3); + assert!(!b.slide(3, 3)); assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); - b.slide(3, 4); + assert!(!b.slide(3, 4)); assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]); } From ec062edf6f840a78922ee5af0750902ff44be08e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 13 Jan 2022 23:58:58 -0400 Subject: [PATCH 4/6] update doc --- frame/support/src/storage/bounded_vec.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 677ef467c8014..8c3811feac402 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -215,7 +215,9 @@ impl> BoundedVec { /// Forces the insertion of `s` into `self` retaining all items with index at most `index`. /// - /// If `index == Self::bound()` and `self.len() == Self::bound()` then this is a no-op. + /// If `index == Self::bound()` and `self.len() == Self::bound()`, then this is a no-op. + /// + /// If `Self::bound() < index` or `self.len() < index`, then this is also a no-op. /// /// Returns `true` if the item was inserted. pub fn force_insert_keep_left(&mut self, index: usize, element: T) -> bool { From 58981290ddecce5d4eaafe7204cd53886c0e7a31 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 14 Jan 2022 00:00:11 -0400 Subject: [PATCH 5/6] fmt --- frame/support/src/storage/bounded_vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 8c3811feac402..a68e15836f663 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -244,7 +244,7 @@ impl> BoundedVec { /// the item which is being moved. /// /// Returns `true` of the operation was successful, otherwise `false` if a noop. - pub fn slide(&mut self, index: usize, insert_position: usize) -> bool{ + pub fn slide(&mut self, index: usize, insert_position: usize) -> bool { // Check against panics. if self.len() <= index || self.len() < insert_position || index == usize::MAX { return false From 29305df2f6823a37926c5d3b35b675adc3eafabc Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 14 Jan 2022 15:26:28 -0400 Subject: [PATCH 6/6] Update frame/support/src/storage/bounded_vec.rs Co-authored-by: Keith Yeung --- frame/support/src/storage/bounded_vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index a68e15836f663..6d7206df6db10 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -287,7 +287,7 @@ impl> BoundedVec { /// Forces the insertion of `s` into `self` truncating first if necessary. /// - /// Infallible, but if the limit is zero, then it's a no-op. + /// Infallible, but if the bound is zero, then it's a no-op. pub fn force_push(&mut self, element: T) { if Self::bound() > 0 { self.0.truncate(Self::bound() as usize - 1);