From 42c73f7bde7aefff0fc8b1b10333c3cab6a96794 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Sep 2020 13:04:21 +0200 Subject: [PATCH 1/5] implementation and factorisation --- .../src/storage/generator/double_map.rs | 145 ++++++++------- frame/support/src/storage/generator/map.rs | 166 +++++++++++++++--- frame/support/src/storage/mod.rs | 139 ++++++++------- 3 files changed, 301 insertions(+), 149 deletions(-) diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index 3c82f4156a271..d35621ae0ae58 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -18,7 +18,7 @@ use sp_std::prelude::*; use sp_std::borrow::Borrow; use codec::{FullCodec, FullEncode, Decode, Encode, EncodeLike}; -use crate::{storage::{self, unhashed, StorageAppend}, Never}; +use crate::{storage::{self, unhashed, StorageAppend, PrefixIterator}, Never}; use crate::hash::{StorageHasher, Twox128, ReversibleStorageHasher}; /// Generator for `StorageDoubleMap` used by `decl_storage`. @@ -213,10 +213,11 @@ impl storage::StorageDoubleMap for G where KArg1: ?Sized + EncodeLike { let prefix = Self::storage_double_map_final_key1(k1); - storage::PrefixIterator:: { + storage::PrefixIterator { prefix: prefix.clone(), previous_key: prefix, - phantom_data: Default::default(), + drain: false, + closure: |_raw_key, mut raw_value| V::decode(&mut raw_value), } } @@ -322,54 +323,6 @@ impl storage::StorageDoubleMap for G where } } -/// Iterate over a prefix and decode raw_key and raw_value into `T`. -pub struct MapIterator { - prefix: Vec, - previous_key: Vec, - /// If true then value are removed while iterating - drain: bool, - /// Function that take `(raw_key_without_prefix, raw_value)` and decode `T`. - /// `raw_key_without_prefix` is the raw storage key without the prefix iterated on. - closure: fn(&[u8], &[u8]) -> Result, -} - -impl Iterator for MapIterator { - type Item = T; - - fn next(&mut self) -> Option { - loop { - let maybe_next = sp_io::storage::next_key(&self.previous_key) - .filter(|n| n.starts_with(&self.prefix)); - break match maybe_next { - Some(next) => { - self.previous_key = next; - let raw_value = match unhashed::get_raw(&self.previous_key) { - Some(raw_value) => raw_value, - None => { - frame_support::print("ERROR: next_key returned a key with no value in MapIterator"); - continue - } - }; - if self.drain { - unhashed::kill(&self.previous_key) - } - let raw_key_without_prefix = &self.previous_key[self.prefix.len()..]; - let item = match (self.closure)(raw_key_without_prefix, &raw_value[..]) { - Ok(item) => item, - Err(_e) => { - frame_support::print("ERROR: (key, value) failed to decode in MapIterator"); - continue - } - }; - - Some(item) - } - None => None, - } - } - } -} - impl< K1: FullCodec, K2: FullCodec, @@ -379,8 +332,8 @@ impl< G::Hasher1: ReversibleStorageHasher, G::Hasher2: ReversibleStorageHasher { - type PrefixIterator = MapIterator<(K2, V)>; - type Iterator = MapIterator<(K1, K2, V)>; + type PrefixIterator = PrefixIterator<(K2, V)>; + type Iterator = PrefixIterator<(K1, K2, V)>; fn iter_prefix(k1: impl EncodeLike) -> Self::PrefixIterator { let prefix = G::storage_double_map_final_key1(k1); @@ -423,20 +376,41 @@ impl< iterator } - fn translate Option>(f: F) { + fn translate Option>(f: F) { let prefix = G::prefix_hash(); let mut previous_key = prefix.clone(); loop { match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { Some(next) => { previous_key = next; - let maybe_value = unhashed::get::(&previous_key); - match maybe_value { - Some(value) => match f(value) { - Some(new) => unhashed::put::(&previous_key, &new), - None => unhashed::kill(&previous_key), + let value = match unhashed::get::(&previous_key) { + Some(value) => value, + None => { + crate::debug::error!("Invalid translate: fail to decode old value"); + continue + }, + }; + let mut key_material = G::Hasher1::reverse(&previous_key[prefix.len()..]); + let key1 = match K1::decode(&mut key_material) { + Ok(key1) => key1, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key1"); + continue + }, + }; + + let mut key2_material = G::Hasher1::reverse(&key_material); + let key2 = match K2::decode(&mut key2_material) { + Ok(key2) => key2, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key2"); + continue }, - None => continue, + }; + + match f(key1, key2, value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), } } None => return, @@ -450,7 +424,10 @@ impl< #[allow(dead_code)] mod test_iterators { use codec::{Encode, Decode}; - use crate::storage::{generator::StorageDoubleMap, IterableStorageDoubleMap, unhashed}; + use crate::{ + hash::StorageHasher, + storage::{generator::StorageDoubleMap, IterableStorageDoubleMap, unhashed}, + }; pub trait Trait { type Origin; @@ -484,11 +461,6 @@ mod test_iterators { prefix } - fn key_in_prefix(mut prefix: Vec) -> Vec { - prefix.push(0); - prefix - } - #[test] fn double_map_reversible_reversible_iteration() { sp_io::TestExternalities::default().execute_with(|| { @@ -550,6 +522,47 @@ mod test_iterators { assert_eq!(DoubleMap::iter_prefix(k1).collect::>(), vec![]); assert_eq!(unhashed::get(&key_before_prefix(prefix.clone())), Some(1u64)); assert_eq!(unhashed::get(&key_after_prefix(prefix.clone())), Some(1u64)); + + // Translate + let prefix = DoubleMap::prefix_hash(); + + unhashed::put(&key_before_prefix(prefix.clone()), &1u64); + unhashed::put(&key_after_prefix(prefix.clone()), &1u64); + for i in 0..4 { + DoubleMap::insert(i as u16, i as u32, i as u64); + } + + // Wrong key1 + unhashed::put( + &[prefix.clone(), vec![1, 2, 3]].concat(), + &3u64.encode() + ); + // Wrong key2 + unhashed::put( + &[prefix.clone(), crate::Blake2_128Concat::hash(&1u16.encode())].concat(), + &3u64.encode() + ); + + // Wrong value + unhashed::put( + &[ + prefix.clone(), + crate::Blake2_128Concat::hash(&1u16.encode()), + crate::Blake2_128Concat::hash(&2u32.encode()), + ].concat(), + &vec![1], + ); + + DoubleMap::translate(|_k1, _k2, v: u64| Some(v*2)); + assert_eq!( + DoubleMap::iter().collect::>(), + vec![ + (3, 3, 6), + (0, 0, 0), + (2, 2, 4), + (1, 1, 2), + ] + ); }) } } diff --git a/frame/support/src/storage/generator/map.rs b/frame/support/src/storage/generator/map.rs index fe932b797940b..f3d01a9875a39 100644 --- a/frame/support/src/storage/generator/map.rs +++ b/frame/support/src/storage/generator/map.rs @@ -20,7 +20,7 @@ use sp_std::prelude::*; use sp_std::borrow::Borrow; use codec::{FullCodec, FullEncode, Decode, Encode, EncodeLike}; use crate::{ - storage::{self, unhashed, StorageAppend}, + storage::{self, unhashed, StorageAppend, PrefixIterator}, Never, hash::{StorageHasher, Twox128, ReversibleStorageHasher}, }; @@ -139,28 +139,27 @@ impl< > storage::IterableStorageMap for G where G::Hasher: ReversibleStorageHasher { - type Iterator = StorageMapIterator; + type Iterator = PrefixIterator<(K, V)>; /// Enumerate all elements in the map. fn iter() -> Self::Iterator { let prefix = G::prefix_hash(); - Self::Iterator { + PrefixIterator { prefix: prefix.clone(), previous_key: prefix, drain: false, - _phantom: Default::default(), + closure: |raw_key_without_prefix, mut raw_value| { + let mut key_material = G::Hasher::reverse(raw_key_without_prefix); + Ok((K::decode(&mut key_material)?, V::decode(&mut raw_value)?)) + }, } } /// Enumerate all elements in the map. fn drain() -> Self::Iterator { - let prefix = G::prefix_hash(); - Self::Iterator { - prefix: prefix.clone(), - previous_key: prefix, - drain: true, - _phantom: Default::default(), - } + let mut iterator = Self::iter(); + iterator.drain = true; + iterator } fn translate Option>(f: F) { @@ -170,19 +169,26 @@ impl< match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { Some(next) => { previous_key = next; - let maybe_value = unhashed::get::(&previous_key); - match maybe_value { - Some(value) => { - let mut key_material = G::Hasher::reverse(&previous_key[prefix.len()..]); - match K::decode(&mut key_material) { - Ok(key) => match f(key, value) { - Some(new) => unhashed::put::(&previous_key, &new), - None => unhashed::kill(&previous_key), - }, - Err(_) => continue, - } - } - None => continue, + let value = match unhashed::get::(&previous_key) { + Some(value) => value, + None => { + crate::debug::error!("Invalid translate: fail to decode old value"); + continue + }, + }; + + let mut key_material = G::Hasher::reverse(&previous_key[prefix.len()..]); + let key = match K::decode(&mut key_material) { + Ok(key) => key, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key"); + continue + }, + }; + + match f(key, value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), } } None => return, @@ -312,3 +318,115 @@ impl> storage::StorageMap }) } } + +/// Test iterators for StorageDoubleMap +#[cfg(test)] +#[allow(dead_code)] +mod test_iterators { + use codec::{Encode, Decode}; + use crate::{ + hash::StorageHasher, + storage::{generator::StorageMap, IterableStorageMap, unhashed}, + }; + + pub trait Trait { + type Origin; + type BlockNumber; + } + + crate::decl_module! { + pub struct Module for enum Call where origin: T::Origin {} + } + + #[derive(PartialEq, Eq, Clone, Encode, Decode)] + struct NoDef(u32); + + crate::decl_storage! { + trait Store for Module as Test { + Map: map hasher(blake2_128_concat) u16 => u64; + } + } + + fn key_before_prefix(mut prefix: Vec) -> Vec { + let last = prefix.iter_mut().last().unwrap(); + assert!(*last != 0, "mock function not implemented for this prefix"); + *last -= 1; + prefix + } + + fn key_after_prefix(mut prefix: Vec) -> Vec { + let last = prefix.iter_mut().last().unwrap(); + assert!(*last != 255, "mock function not implemented for this prefix"); + *last += 1; + prefix + } + + #[test] + fn double_map_reversible_reversible_iteration() { + sp_io::TestExternalities::default().execute_with(|| { + // All map iterator + let prefix = Map::prefix_hash(); + + unhashed::put(&key_before_prefix(prefix.clone()), &1u64); + unhashed::put(&key_after_prefix(prefix.clone()), &1u64); + + for i in 0..4 { + Map::insert(i as u16, i as u64); + } + + assert_eq!( + Map::iter().collect::>(), + vec![(3, 3), (0, 0), (2, 2), (1, 1)], + ); + + assert_eq!( + Map::iter_values().collect::>(), + vec![3, 0, 2, 1], + ); + + assert_eq!( + Map::drain().collect::>(), + vec![(3, 3), (0, 0), (2, 2), (1, 1)], + ); + + assert_eq!(Map::iter().collect::>(), vec![]); + assert_eq!(unhashed::get(&key_before_prefix(prefix.clone())), Some(1u64)); + assert_eq!(unhashed::get(&key_after_prefix(prefix.clone())), Some(1u64)); + + // Translate + let prefix = Map::prefix_hash(); + + unhashed::put(&key_before_prefix(prefix.clone()), &1u64); + unhashed::put(&key_after_prefix(prefix.clone()), &1u64); + for i in 0..4 { + Map::insert(i as u16, i as u64); + } + + // Wrong key + unhashed::put( + &[prefix.clone(), vec![1, 2, 3]].concat(), + &3u64.encode() + ); + + // Wrong value + unhashed::put( + &[ + prefix.clone(), + crate::Blake2_128Concat::hash(&6u16.encode()), + ].concat(), + &vec![1], + ); + + Map::translate(|_k1, v: u64| Some(v*2)); + assert_eq!( + Map::iter().collect::>(), + vec![ + (3, 6), + (0, 0), + (2, 4), + (1, 2), + ] + ); + }) + } +} diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 347fd814136d7..9fe03cbaf0ed3 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -17,7 +17,7 @@ //! Stuff to do with the runtime's storage. -use sp_std::{prelude::*, marker::PhantomData}; +use sp_std::prelude::*; use codec::{FullCodec, FullEncode, Encode, EncodeLike, Decode}; use crate::hash::{Twox128, StorageHasher}; use sp_runtime::generic::{Digest, DigestItem}; @@ -251,6 +251,8 @@ pub trait IterableStorageMap: StorageMap { /// Translate the values of all elements by a function `f`, in the map in no particular order. /// By returning `None` from `f` for an element, you'll remove it from the map. + /// + /// NOTE: If a value fail to decode because storage is corrupted then it is skipped. fn translate Option>(f: F); } @@ -286,7 +288,7 @@ pub trait IterableStorageDoubleMap< /// Translate the values of all elements by a function `f`, in the map in no particular order. /// By returning `None` from `f` for an element, you'll remove it from the map. - fn translate Option>(f: F); + fn translate Option>(f: F); } /// An implementation of a map with a two keys. @@ -433,35 +435,56 @@ pub trait StorageDoubleMap { >(key1: KeyArg1, key2: KeyArg2) -> Option; } -/// Iterator for prefixed map. -pub struct PrefixIterator { +/// Iterate over a prefix and decode raw_key and raw_value into `T`. +pub struct PrefixIterator { prefix: Vec, previous_key: Vec, - phantom_data: PhantomData, + /// If true then value are removed while iterating + drain: bool, + /// Function that take `(raw_key_without_prefix, raw_value)` and decode `T`. + /// `raw_key_without_prefix` is the raw storage key without the prefix iterated on. + closure: fn(&[u8], &[u8]) -> Result, } -impl Iterator for PrefixIterator { - type Item = Value; +impl Iterator for PrefixIterator { + type Item = T; fn next(&mut self) -> Option { - match sp_io::storage::next_key(&self.previous_key) - .filter(|n| n.starts_with(&self.prefix[..])) - { - Some(next_key) => { - let value = unhashed::get(&next_key); - - if value.is_none() { - runtime_print!( - "ERROR: returned next_key has no value:\nkey is {:?}\nnext_key is {:?}", - &self.previous_key, &next_key, - ); + loop { + let maybe_next = sp_io::storage::next_key(&self.previous_key) + .filter(|n| n.starts_with(&self.prefix)); + break match maybe_next { + Some(next) => { + self.previous_key = next; + let raw_value = match unhashed::get_raw(&self.previous_key) { + Some(raw_value) => raw_value, + None => { + crate::debug::error!( + "next_key returned a key with no value at {:?}", + self.previous_key + ); + continue + } + }; + if self.drain { + unhashed::kill(&self.previous_key) + } + let raw_key_without_prefix = &self.previous_key[self.prefix.len()..]; + let item = match (self.closure)(raw_key_without_prefix, &raw_value[..]) { + Ok(item) => item, + Err(e) => { + crate::debug::error!( + "(key, value) failed to decode at {:?}: {:?}", + self.previous_key, e + ); + continue + } + }; + + Some(item) } - - self.previous_key = next_key; - - value - }, - _ => None, + None => None, + } } } } @@ -493,22 +516,22 @@ pub trait StoragePrefixedMap { } /// Iter over all value of the storage. + /// + /// NOTE: If a value failed to decode becaues storage is corrupted then it is skipped. fn iter_values() -> PrefixIterator { let prefix = Self::final_prefix(); PrefixIterator { prefix: prefix.to_vec(), previous_key: prefix.to_vec(), - phantom_data: Default::default(), + drain: false, + closure: |_raw_key, mut raw_value| Value::decode(&mut raw_value), } } - /// Translate the values from some previous `OldValue` to the current type. - /// - /// `TV` translates values. + /// Translate the values of all elements by a function `f`, in the map in no particular order. + /// By returning `None` from `f` for an element, you'll remove it from the map. /// - /// Returns `Err` if the map could not be interpreted as the old type, and Ok if it could. - /// The `Err` contains the number of value that couldn't be interpreted, those value are - /// removed from the map. + /// NOTE: If a value fail to decode because storage is corrupted then it is skipped. /// /// # Warning /// @@ -517,33 +540,31 @@ pub trait StoragePrefixedMap { /// /// # Usage /// - /// This would typically be called inside the module implementation of on_runtime_upgrade, while - /// ensuring **no usage of this storage are made before the call to `on_runtime_upgrade`**. (More - /// precisely prior initialized modules doesn't make use of this storage). - fn translate_values(translate_val: TV) -> Result<(), u32> - where OldValue: Decode, TV: Fn(OldValue) -> Value - { + /// This would typically be called inside the module implementation of on_runtime_upgrade. + fn translate_values Option>(f: F) { let prefix = Self::final_prefix(); - let mut previous_key = prefix.to_vec(); - let mut errors = 0; - while let Some(next_key) = sp_io::storage::next_key(&previous_key) - .filter(|n| n.starts_with(&prefix[..])) - { - if let Some(value) = unhashed::get(&next_key) { - unhashed::put(&next_key[..], &translate_val(value)); - } else { - // We failed to read the value. Remove the key and increment errors. - unhashed::kill(&next_key[..]); - errors += 1; + let mut previous_key = prefix.clone().to_vec(); + loop { + match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { + Some(next) => { + previous_key = next; + let maybe_value = unhashed::get::(&previous_key); + match maybe_value { + Some(value) => match f(value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), + }, + None => { + crate::debug::error!( + "old key failed to decode at {:?}", + previous_key + ); + continue + }, + } + } + None => return, } - - previous_key = next_key; - } - - if errors == 0 { - Ok(()) - } else { - Err(errors) } } } @@ -652,7 +673,7 @@ mod test { unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u32); assert_eq!(MyStorage::iter_values().collect::>(), vec![]); - MyStorage::translate_values(|v: u32| v as u64).unwrap(); + MyStorage::translate_values(|v: u32| Some(v as u64)); assert_eq!(MyStorage::iter_values().collect::>(), vec![1, 2]); MyStorage::remove_all(); @@ -664,8 +685,8 @@ mod test { // (contains some value that successfully decoded to u64) assert_eq!(MyStorage::iter_values().collect::>(), vec![1, 2, 3]); - assert_eq!(MyStorage::translate_values(|v: u128| v as u64), Err(2)); - assert_eq!(MyStorage::iter_values().collect::>(), vec![1, 3]); + MyStorage::translate_values(|v: u128| Some(v as u64)); + assert_eq!(MyStorage::iter_values().collect::>(), vec![1, 2, 3]); MyStorage::remove_all(); // test that other values are not modified. From fed1fda3f44b8754ab76f0fb66b4ca2db75b0f1a Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Sep 2020 13:07:43 +0200 Subject: [PATCH 2/5] factorize test --- .../src/storage/generator/double_map.rs | 8 ++--- frame/support/src/storage/generator/map.rs | 35 ++++--------------- 2 files changed, 8 insertions(+), 35 deletions(-) diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index d35621ae0ae58..f430d756219e8 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -537,6 +537,7 @@ mod test_iterators { &[prefix.clone(), vec![1, 2, 3]].concat(), &3u64.encode() ); + // Wrong key2 unhashed::put( &[prefix.clone(), crate::Blake2_128Concat::hash(&1u16.encode())].concat(), @@ -556,12 +557,7 @@ mod test_iterators { DoubleMap::translate(|_k1, _k2, v: u64| Some(v*2)); assert_eq!( DoubleMap::iter().collect::>(), - vec![ - (3, 3, 6), - (0, 0, 0), - (2, 2, 4), - (1, 1, 2), - ] + vec![(3, 3, 6), (0, 0, 0), (2, 2, 4), (1, 1, 2)], ); }) } diff --git a/frame/support/src/storage/generator/map.rs b/frame/support/src/storage/generator/map.rs index f3d01a9875a39..3a26fc2f59652 100644 --- a/frame/support/src/storage/generator/map.rs +++ b/frame/support/src/storage/generator/map.rs @@ -374,20 +374,11 @@ mod test_iterators { Map::insert(i as u16, i as u64); } - assert_eq!( - Map::iter().collect::>(), - vec![(3, 3), (0, 0), (2, 2), (1, 1)], - ); + assert_eq!(Map::iter().collect::>(), vec![(3, 3), (0, 0), (2, 2), (1, 1)]); - assert_eq!( - Map::iter_values().collect::>(), - vec![3, 0, 2, 1], - ); + assert_eq!(Map::iter_values().collect::>(), vec![3, 0, 2, 1]); - assert_eq!( - Map::drain().collect::>(), - vec![(3, 3), (0, 0), (2, 2), (1, 1)], - ); + assert_eq!(Map::drain().collect::>(), vec![(3, 3), (0, 0), (2, 2), (1, 1)]); assert_eq!(Map::iter().collect::>(), vec![]); assert_eq!(unhashed::get(&key_before_prefix(prefix.clone())), Some(1u64)); @@ -403,30 +394,16 @@ mod test_iterators { } // Wrong key - unhashed::put( - &[prefix.clone(), vec![1, 2, 3]].concat(), - &3u64.encode() - ); + unhashed::put(&[prefix.clone(), vec![1, 2, 3]].concat(), &3u64.encode()); // Wrong value unhashed::put( - &[ - prefix.clone(), - crate::Blake2_128Concat::hash(&6u16.encode()), - ].concat(), + &[prefix.clone(), crate::Blake2_128Concat::hash(&6u16.encode())].concat(), &vec![1], ); Map::translate(|_k1, v: u64| Some(v*2)); - assert_eq!( - Map::iter().collect::>(), - vec![ - (3, 6), - (0, 0), - (2, 4), - (1, 2), - ] - ); + assert_eq!(Map::iter().collect::>(), vec![(3, 6), (0, 0), (2, 4), (1, 2)]); }) } } From 3b5b4b0e740b1d6e97f6abef43332b4c4a35d500 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Sep 2020 13:12:26 +0200 Subject: [PATCH 3/5] doc --- frame/support/src/storage/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 9fe03cbaf0ed3..79625235fd680 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -288,6 +288,8 @@ pub trait IterableStorageDoubleMap< /// Translate the values of all elements by a function `f`, in the map in no particular order. /// By returning `None` from `f` for an element, you'll remove it from the map. + /// + /// NOTE: If a value fail to decode because storage is corrupted then it is skipped. fn translate Option>(f: F); } @@ -436,6 +438,8 @@ pub trait StorageDoubleMap { } /// Iterate over a prefix and decode raw_key and raw_value into `T`. +/// +/// If any decoding fails it skips it and continues to the next key. pub struct PrefixIterator { prefix: Vec, previous_key: Vec, From bf25dd0970c56549aeca5567ce83121baa12d37e Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Sep 2020 13:24:58 +0200 Subject: [PATCH 4/5] fix bug and improve test --- frame/support/src/storage/generator/double_map.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index f430d756219e8..e56a4a3d20123 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -399,7 +399,7 @@ impl< }, }; - let mut key2_material = G::Hasher1::reverse(&key_material); + let mut key2_material = G::Hasher2::reverse(&key_material); let key2 = match K2::decode(&mut key2_material) { Ok(key2) => key2, Err(_) => { @@ -443,7 +443,7 @@ mod test_iterators { crate::decl_storage! { trait Store for Module as Test { - DoubleMap: double_map hasher(blake2_128_concat) u16, hasher(blake2_128_concat) u32 => u64; + DoubleMap: double_map hasher(blake2_128_concat) u16, hasher(twox_64_concat) u32 => u64; } } @@ -506,17 +506,17 @@ mod test_iterators { assert_eq!( DoubleMap::iter_prefix(k1).collect::>(), - vec![(0, 0), (2, 2), (1, 1), (3, 3)], + vec![(1, 1), (2, 2), (0, 0), (3, 3)], ); assert_eq!( DoubleMap::iter_prefix_values(k1).collect::>(), - vec![0, 2, 1, 3], + vec![1, 2, 0, 3], ); assert_eq!( DoubleMap::drain_prefix(k1).collect::>(), - vec![(0, 0), (2, 2), (1, 1), (3, 3)], + vec![(1, 1), (2, 2), (0, 0), (3, 3)], ); assert_eq!(DoubleMap::iter_prefix(k1).collect::>(), vec![]); @@ -549,7 +549,7 @@ mod test_iterators { &[ prefix.clone(), crate::Blake2_128Concat::hash(&1u16.encode()), - crate::Blake2_128Concat::hash(&2u32.encode()), + crate::Twox64Concat::hash(&2u32.encode()), ].concat(), &vec![1], ); From 8e41aa802e900d3bd848ac77c932151e834941d7 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 10 Sep 2020 11:28:40 +0200 Subject: [PATCH 5/5] address suggestions --- frame/support/src/dispatch.rs | 2 + .../src/storage/generator/double_map.rs | 68 +++++++++---------- frame/support/src/storage/generator/map.rs | 56 +++++++-------- frame/support/src/storage/mod.rs | 37 +++++----- 4 files changed, 77 insertions(+), 86 deletions(-) diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index 442a99effadbc..b8a3e48c24294 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -2055,6 +2055,7 @@ macro_rules! __dispatch_impl_metadata { where $( $other_where_bounds )* { #[doc(hidden)] + #[allow(dead_code)] pub fn call_functions() -> &'static [$crate::dispatch::FunctionMetadata] { $crate::__call_to_functions!($($rest)*) } @@ -2128,6 +2129,7 @@ macro_rules! __impl_module_constants_metadata { $mod_type<$trait_instance $(, $instance)?> where $( $other_where_bounds )* { #[doc(hidden)] + #[allow(dead_code)] pub fn module_constants_metadata() -> &'static [$crate::dispatch::ModuleConstantMetadata] { // Create the `ByteGetter`s $( diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index e56a4a3d20123..9454ab401da28 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -379,41 +379,38 @@ impl< fn translate Option>(f: F) { let prefix = G::prefix_hash(); let mut previous_key = prefix.clone(); - loop { - match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { - Some(next) => { - previous_key = next; - let value = match unhashed::get::(&previous_key) { - Some(value) => value, - None => { - crate::debug::error!("Invalid translate: fail to decode old value"); - continue - }, - }; - let mut key_material = G::Hasher1::reverse(&previous_key[prefix.len()..]); - let key1 = match K1::decode(&mut key_material) { - Ok(key1) => key1, - Err(_) => { - crate::debug::error!("Invalid translate: fail to decode key1"); - continue - }, - }; - - let mut key2_material = G::Hasher2::reverse(&key_material); - let key2 = match K2::decode(&mut key2_material) { - Ok(key2) => key2, - Err(_) => { - crate::debug::error!("Invalid translate: fail to decode key2"); - continue - }, - }; - - match f(key1, key2, value) { - Some(new) => unhashed::put::(&previous_key, &new), - None => unhashed::kill(&previous_key), - } - } - None => return, + while let Some(next) = sp_io::storage::next_key(&previous_key) + .filter(|n| n.starts_with(&prefix)) + { + previous_key = next; + let value = match unhashed::get::(&previous_key) { + Some(value) => value, + None => { + crate::debug::error!("Invalid translate: fail to decode old value"); + continue + }, + }; + let mut key_material = G::Hasher1::reverse(&previous_key[prefix.len()..]); + let key1 = match K1::decode(&mut key_material) { + Ok(key1) => key1, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key1"); + continue + }, + }; + + let mut key2_material = G::Hasher2::reverse(&key_material); + let key2 = match K2::decode(&mut key2_material) { + Ok(key2) => key2, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key2"); + continue + }, + }; + + match f(key1, key2, value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), } } } @@ -421,7 +418,6 @@ impl< /// Test iterators for StorageDoubleMap #[cfg(test)] -#[allow(dead_code)] mod test_iterators { use codec::{Encode, Decode}; use crate::{ diff --git a/frame/support/src/storage/generator/map.rs b/frame/support/src/storage/generator/map.rs index 3a26fc2f59652..1c13de52e1640 100644 --- a/frame/support/src/storage/generator/map.rs +++ b/frame/support/src/storage/generator/map.rs @@ -165,33 +165,30 @@ impl< fn translate Option>(f: F) { let prefix = G::prefix_hash(); let mut previous_key = prefix.clone(); - loop { - match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { - Some(next) => { - previous_key = next; - let value = match unhashed::get::(&previous_key) { - Some(value) => value, - None => { - crate::debug::error!("Invalid translate: fail to decode old value"); - continue - }, - }; - - let mut key_material = G::Hasher::reverse(&previous_key[prefix.len()..]); - let key = match K::decode(&mut key_material) { - Ok(key) => key, - Err(_) => { - crate::debug::error!("Invalid translate: fail to decode key"); - continue - }, - }; - - match f(key, value) { - Some(new) => unhashed::put::(&previous_key, &new), - None => unhashed::kill(&previous_key), - } - } - None => return, + while let Some(next) = sp_io::storage::next_key(&previous_key) + .filter(|n| n.starts_with(&prefix)) + { + previous_key = next; + let value = match unhashed::get::(&previous_key) { + Some(value) => value, + None => { + crate::debug::error!("Invalid translate: fail to decode old value"); + continue + }, + }; + + let mut key_material = G::Hasher::reverse(&previous_key[prefix.len()..]); + let key = match K::decode(&mut key_material) { + Ok(key) => key, + Err(_) => { + crate::debug::error!("Invalid translate: fail to decode key"); + continue + }, + }; + + match f(key, value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), } } } @@ -319,9 +316,8 @@ impl> storage::StorageMap } } -/// Test iterators for StorageDoubleMap +/// Test iterators for StorageMap #[cfg(test)] -#[allow(dead_code)] mod test_iterators { use codec::{Encode, Decode}; use crate::{ @@ -362,7 +358,7 @@ mod test_iterators { } #[test] - fn double_map_reversible_reversible_iteration() { + fn map_reversible_reversible_iteration() { sp_io::TestExternalities::default().execute_with(|| { // All map iterator let prefix = Map::prefix_hash(); diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 79625235fd680..717a9a29ad5f5 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -548,26 +548,23 @@ pub trait StoragePrefixedMap { fn translate_values Option>(f: F) { let prefix = Self::final_prefix(); let mut previous_key = prefix.clone().to_vec(); - loop { - match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) { - Some(next) => { - previous_key = next; - let maybe_value = unhashed::get::(&previous_key); - match maybe_value { - Some(value) => match f(value) { - Some(new) => unhashed::put::(&previous_key, &new), - None => unhashed::kill(&previous_key), - }, - None => { - crate::debug::error!( - "old key failed to decode at {:?}", - previous_key - ); - continue - }, - } - } - None => return, + while let Some(next) = sp_io::storage::next_key(&previous_key) + .filter(|n| n.starts_with(&prefix)) + { + previous_key = next; + let maybe_value = unhashed::get::(&previous_key); + match maybe_value { + Some(value) => match f(value) { + Some(new) => unhashed::put::(&previous_key, &new), + None => unhashed::kill(&previous_key), + }, + None => { + crate::debug::error!( + "old key failed to decode at {:?}", + previous_key + ); + continue + }, } } }