Unverified Commit 3803a266 authored by Michael Müller's avatar Michael Müller Committed by GitHub
Browse files

Make nested Lazy's clear storage properly (#583)



* Fix typo: invariances ➜ invariants

* Fix comment

* Reduce code duplication in HashMap tests

* Add test api getters for used storage

* Add regression tests for complete storage clearance of nested Lazy's

* Always clear inner cell

* Bring costs of bug fix down

* Apply suggestions from code review
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>

* Rename function

* Throw out convenience method, clarify variable names

* Clarify comment
Co-authored-by: default avatarHero Bird <robin.freyler@gmail.com>
parent bb39fe15
Pipeline #114270 passed with stages
in 22 minutes and 53 seconds
......@@ -267,6 +267,12 @@ impl Account {
pub fn get_storage_rw(&self) -> Result<(usize, usize)> {
self.contract_or_err().map(|contract| contract.get_rw())
}
/// Returns the amount of used storage entries.
pub fn count_used_storage_cells(&self) -> Result<usize> {
self.contract_or_err()
.map(|contract| contract.count_used_storage_cells())
}
}
/// The kind of the account.
......@@ -301,6 +307,11 @@ impl ContractAccount {
pub fn get_rw(&self) -> (usize, usize) {
self.storage.get_rw()
}
/// Returns the number of used storage entries.
pub fn count_used_storage_cells(&self) -> usize {
self.storage.count_used_storage_cells()
}
}
/// The storage of a contract instance.
......@@ -355,4 +366,9 @@ impl ContractStorage {
self.count_writes += 1;
self.entries.remove(&at);
}
/// Returns the number of used storage entries.
pub fn count_used_storage_cells(&self) -> usize {
self.entries.len()
}
}
......@@ -352,6 +352,23 @@ where
})
}
/// Returns the amount of storage cells used by the account `account_id`.
///
/// Returns `None` if the `account_id` is non-existent.
pub fn count_used_storage_cells<T>(account_id: &T::AccountId) -> Result<usize>
where
T: Environment,
{
<EnvInstance as OnInstance>::on_instance(|instance| {
instance
.accounts
.get_account::<T>(account_id)
.ok_or_else(|| AccountError::no_account_for_id::<T>(account_id))
.map_err(Into::into)
.and_then(|account| account.count_used_storage_cells().map_err(Into::into))
})
}
/// Returns the account id of the currently executing contract.
pub fn get_current_contract_account_id<T>() -> Result<T::AccountId>
where
......
......@@ -13,9 +13,12 @@
// limitations under the License.
use super::HashMap as StorageHashMap;
use crate::traits::{
KeyPtr,
SpreadLayout,
use crate::{
traits::{
KeyPtr,
SpreadLayout,
},
Lazy,
};
use ink_primitives::Key;
......@@ -35,6 +38,13 @@ fn pull_hmap() -> StorageHashMap<u8, i32> {
<StorageHashMap<u8, i32> as SpreadLayout>::pull_spread(&mut key_ptr())
}
fn filled_hmap() -> StorageHashMap<u8, i32> {
[(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>()
}
#[test]
fn new_works() {
// `StorageHashMap::new`
......@@ -102,10 +112,7 @@ fn get_works() {
assert_eq!(hmap.get(&b'A'), None);
assert_eq!(hmap.get(&b'E'), None);
// Filled hash map: `get`
let hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap = filled_hmap();
assert_eq!(hmap.get(&b'A'), Some(&1));
assert_eq!(hmap.get(&b'B'), Some(&2));
assert_eq!(hmap.get(&b'C'), Some(&3));
......@@ -150,10 +157,7 @@ fn take_works() {
assert_eq!(hmap.take(&b'A'), None);
assert_eq!(hmap.take(&b'E'), None);
// Filled hash map: `get`
let mut hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let mut hmap = filled_hmap();
assert_eq!(hmap.len(), 4);
assert_eq!(hmap.take(&b'A'), Some(1));
assert_eq!(hmap.len(), 3);
......@@ -171,10 +175,7 @@ fn take_works() {
#[test]
fn iter_next_works() {
let hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap = filled_hmap();
// Test iterator over shared references:
let mut iter = hmap.iter();
assert_eq!(iter.count(), 4);
......@@ -208,10 +209,7 @@ fn iter_next_works() {
#[test]
fn values_next_works() {
let hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap = filled_hmap();
// Test iterator over shared references:
let mut iter = hmap.values();
assert_eq!(iter.count(), 4);
......@@ -245,10 +243,7 @@ fn values_next_works() {
#[test]
fn keys_next_works() {
let hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap = filled_hmap();
let mut iter = hmap.keys();
assert_eq!(iter.count(), 4);
assert_eq!(iter.size_hint(), (4, Some(4)));
......@@ -272,10 +267,7 @@ fn defrag_works() {
.copied()
.collect::<StorageHashMap<u8, i32>>();
// Defrag without limits:
let mut hmap = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let mut hmap = filled_hmap();
assert_eq!(hmap.defrag(None), 0);
assert_eq!(hmap.take(&b'B'), Some(2));
assert_eq!(hmap.take(&b'C'), Some(3));
......@@ -299,12 +291,9 @@ fn defrag_works() {
#[test]
fn spread_layout_push_pull_works() -> ink_env::Result<()> {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let hmap1 = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap1 = filled_hmap();
push_hmap(&hmap1);
// Load the pushed storage vector into another instance and check that
// Load the pushed storage hmap into another instance and check that
// both instances are equal:
let hmap2 = pull_hmap();
assert_eq!(hmap1, hmap2);
......@@ -316,10 +305,7 @@ fn spread_layout_push_pull_works() -> ink_env::Result<()> {
#[should_panic(expected = "storage entry was empty")]
fn spread_layout_clear_works() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let hmap1 = [(b'A', 1), (b'B', 2), (b'C', 3), (b'D', 4)]
.iter()
.copied()
.collect::<StorageHashMap<u8, i32>>();
let hmap1 = filled_hmap();
let root_key = Key::from([0x42; 32]);
SpreadLayout::push_spread(&hmap1, &mut KeyPtr::from(root_key));
// It has already been asserted that a valid instance can be pulled
......@@ -327,7 +313,7 @@ fn spread_layout_clear_works() {
//
// Now clear the associated storage from `hmap1` and check whether
// loading another instance from this storage will panic since the
// vector's length property cannot read a value:
// hmap's length property cannot read a value:
SpreadLayout::clear_spread(&hmap1, &mut KeyPtr::from(root_key));
let _ = <StorageHashMap<u8, i32> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
......@@ -336,3 +322,33 @@ fn spread_layout_clear_works() {
})
.unwrap()
}
#[test]
fn storage_is_cleared_completely_after_pull_lazy() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
// given
let root_key = Key::from([0x42; 32]);
let lazy_hmap = Lazy::new(filled_hmap());
SpreadLayout::push_spread(&lazy_hmap, &mut KeyPtr::from(root_key));
let pulled_hmap = <Lazy<StorageHashMap<u8, i32>> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
);
// when
SpreadLayout::clear_spread(&pulled_hmap, &mut KeyPtr::from(root_key));
// then
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot get contract id");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
Ok(())
})
.unwrap()
}
......@@ -85,7 +85,7 @@ where
///
/// # Note
///
/// This completely invalidates the storage vector's invariances about
/// This completely invalidates the storage vector's invariants about
/// the contents of its associated storage region.
///
/// This API is used for the `Drop` implementation of [`Vec`] as well as
......
......@@ -13,9 +13,12 @@
// limitations under the License.
use super::SmallVec;
use crate::traits::{
KeyPtr,
SpreadLayout,
use crate::{
traits::{
KeyPtr,
SpreadLayout,
},
Lazy,
};
use generic_array::typenum::*;
use ink_primitives::Key;
......@@ -394,3 +397,67 @@ fn spread_layout_clear_works() {
})
.unwrap()
}
#[test]
fn storage_is_cleared_completely_after_pull_lazy() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
// given
let root_key = Key::from([0x42; 32]);
let lazy_vec = Lazy::new(vec_from_slice(&[b'a', b'b', b'c', b'd']));
SpreadLayout::push_spread(&lazy_vec, &mut KeyPtr::from(root_key));
let pulled_vec = <Lazy<SmallVec<u8, U4>> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
);
// when
SpreadLayout::clear_spread(&pulled_vec, &mut KeyPtr::from(root_key));
// then
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("contract id must exist");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
Ok(())
})
.unwrap()
}
#[test]
#[should_panic(expected = "encountered empty storage cell")]
fn drop_works() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let root_key = Key::from([0x42; 32]);
// if the setup panics it should not cause the test to pass
let setup_result = std::panic::catch_unwind(|| {
let vec = vec_from_slice(&[b'a', b'b', b'c', b'd']);
SpreadLayout::push_spread(&vec, &mut KeyPtr::from(root_key));
let _ = <SmallVec<u8, U4> as SpreadLayout>::pull_spread(&mut KeyPtr::from(
root_key,
));
// vec is dropped which should clear the cells
});
assert!(setup_result.is_ok(), "setup should not panic");
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot get contract id");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
let _ =
<SmallVec<u8, U4> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key));
Ok(())
})
.unwrap()
}
......@@ -262,7 +262,7 @@ where
///
/// # Note
///
/// This completely invalidates the storage vector's invariances about
/// This completely invalidates the storage vector's invariants about
/// the contents of its associated storage region.
///
/// This API is used for the `Drop` implementation of [`Vec`] as well as
......
......@@ -13,9 +13,12 @@
// limitations under the License.
use super::Stash as StorageStash;
use crate::traits::{
KeyPtr,
SpreadLayout,
use crate::{
traits::{
KeyPtr,
SpreadLayout,
},
Lazy,
};
use ink_primitives::Key;
......@@ -748,3 +751,33 @@ fn spread_layout_clear_works() {
})
.unwrap()
}
#[test]
fn storage_is_cleared_completely_after_pull_lazy() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
// given
let root_key = Key::from([0x42; 32]);
let lazy_stash = Lazy::new(create_holey_stash());
SpreadLayout::push_spread(&lazy_stash, &mut KeyPtr::from(root_key));
let pulled_stash = <Lazy<StorageStash<u8>> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
);
// when
SpreadLayout::clear_spread(&pulled_stash, &mut KeyPtr::from(root_key));
// then
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot yet contract id");
let storage_used = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(storage_used, 0);
Ok(())
})
.unwrap()
}
......@@ -105,7 +105,7 @@ where
///
/// # Note
///
/// This completely invalidates the storage vector's invariances about
/// This completely invalidates the storage vector's invariants about
/// the contents of its associated storage region.
///
/// This API is used for the `Drop` implementation of [`Vec`] as well as
......
......@@ -19,6 +19,7 @@ use crate::{
KeyPtr,
SpreadLayout,
},
Lazy,
};
use ink_primitives::Key;
......@@ -425,3 +426,72 @@ fn clear_works_on_empty_vec() {
vec.clear();
assert!(vec.is_empty());
}
#[test]
#[should_panic(expected = "encountered empty storage cell")]
fn storage_is_cleared_completely_after_pull_lazy() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
// given
let root_key = Key::from([0x42; 32]);
let mut lazy_vec: Lazy<StorageVec<u32>> = Lazy::new(StorageVec::new());
lazy_vec.push(13u32);
lazy_vec.push(13u32);
SpreadLayout::push_spread(&lazy_vec, &mut KeyPtr::from(root_key));
let pulled_vec = <Lazy<StorageVec<u32>> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
);
// when
SpreadLayout::clear_spread(&pulled_vec, &mut KeyPtr::from(root_key));
// then
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot get contract id");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
let _ =
*<Lazy<Lazy<u32>> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key));
Ok(())
})
.unwrap()
}
#[test]
#[should_panic(expected = "encountered empty storage cell")]
fn drop_works() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let root_key = Key::from([0x42; 32]);
// if the setup panics it should not cause the test to pass
let setup_result = std::panic::catch_unwind(|| {
let vec = vec_from_slice(&[b'a', b'b', b'c', b'd']);
SpreadLayout::push_spread(&vec, &mut KeyPtr::from(root_key));
let _ = <StorageVec<u8> as SpreadLayout>::pull_spread(&mut KeyPtr::from(
root_key,
));
// vec is dropped which should clear the cells
});
assert!(setup_result.is_ok(), "setup should not panic");
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot yet contract id");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
let _ =
<StorageVec<u8> as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key));
Ok(())
})
.unwrap()
}
......@@ -151,8 +151,22 @@ where
fn clear_spread(&self, ptr: &mut KeyPtr) {
let root_key = ExtKeyPtr::next_for::<Self>(ptr);
if let Some(entry) = self.entry() {
entry.clear_spread_root(root_key)
match <T as SpreadLayout>::REQUIRES_DEEP_CLEAN_UP {
true => {
// The inner cell needs to be cleared, no matter if it has
// been loaded or not. Otherwise there might be leftovers.
// Load from storage and then clear:
clear_spread_root_opt::<T, _>(root_key, || self.get())
}
false => {
// Clear without loading from storage:
let footprint = <T as SpreadLayout>::FOOTPRINT;
assert!(footprint <= 16); // magic number
let mut key_ptr = KeyPtr::from(*root_key);
for _ in 0..footprint {
ink_env::clear_contract_storage(key_ptr.advance_by(1));
}
}
}
}
}
......@@ -653,4 +667,37 @@ mod tests {
Ok(())
})
}
#[test]
#[should_panic(expected = "encountered empty storage cell")]
fn nested_lazies_are_cleared_completely_after_pull() {
ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
// given
let root_key = Key::from([0x42; 32]);
let nested_lazy: Lazy<Lazy<u32>> = Lazy::new(Lazy::new(13u32));
SpreadLayout::push_spread(&nested_lazy, &mut KeyPtr::from(root_key));
let pulled_lazy = <Lazy<Lazy<u32>> as SpreadLayout>::pull_spread(
&mut KeyPtr::from(root_key),
);
// when
SpreadLayout::clear_spread(&pulled_lazy, &mut KeyPtr::from(root_key));
// then
let contract_id = ink_env::test::get_current_contract_account_id::<
ink_env::DefaultEnvironment,
>()
.expect("Cannot yet contract id");
let used_cells = ink_env::test::count_used_storage_cells::<
ink_env::DefaultEnvironment,
>(&contract_id)
.expect("used cells must be returned");
assert_eq!(used_cells, 0);
let _ = *<Lazy<Lazy<u32>> as SpreadLayout>::pull_spread(&mut KeyPtr::from(
root_key,
));
Ok(())
})
.unwrap()
}
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment