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

Make nested Lazy's clear storage properly #583

Merged
merged 11 commits into from
Nov 17, 2020

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Nov 17, 2020

Closes #580.

The bug is that a Lazy doesn't clear up it's inner cell for clear_spread, if the inner cell has not been loaded. This can easily happen with nested Lazy's ‒ like Lazy<Lazy<…>>.

But also we often use Lazy in storage structs like StorageHashMap<…> (with values: LazyHashMap<…> ‒ if these structs are then wrapped in a Lazy there is already a nested Lazy. SmallVec, Vec, Stash or other storage structs which have this bug if they are wrapped in a Lazy.

The way I fixed this for now will probably lead to some discussion and I'm open for other approaches.

@cmichi cmichi changed the title Make nested `Lazys Make nested Lazy's clear storage properly Nov 17, 2020
@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #583 (2fbe824) into master (3a9301f) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
- Coverage   67.86%   67.78%   -0.08%     
==========================================
  Files         155      155              
  Lines        6895     6914      +19     
==========================================
+ Hits         4679     4687       +8     
- Misses       2216     2227      +11     
Impacted Files Coverage Δ
crates/storage/src/collections/smallvec/mod.rs 96.47% <ø> (ø)
crates/storage/src/collections/smallvec/tests.rs 100.00% <ø> (ø)
crates/storage/src/collections/stash/mod.rs 96.81% <ø> (ø)
crates/storage/src/collections/stash/tests.rs 100.00% <ø> (ø)
crates/storage/src/collections/vec/mod.rs 96.77% <ø> (ø)
crates/storage/src/collections/vec/tests.rs 100.00% <ø> (ø)
crates/env/src/engine/off_chain/db/accounts.rs 86.90% <100.00%> (+1.19%) ⬆️
crates/env/src/engine/off_chain/test_api.rs 74.46% <100.00%> (+3.73%) ⬆️
crates/storage/src/collections/hashmap/tests.rs 100.00% <100.00%> (ø)
crates/storage/src/lazy/lazy_cell.rs 98.46% <100.00%> (+0.15%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9301f...463510b. Read the comment docs.

@Robbepop
Copy link
Collaborator

Good catch!

The fixed SpreadLayout::clear_spread impl for LazyCell should look like this:

fn clear_spread(&self, ptr: &mut KeyPtr) {
    let root_key = ExtKeyPtr::next_for::<Self>(ptr);
    match <T as SpreadLayout>::REQUIRES_DEEP_CLEAN_UP {
        true => {
            // 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));
            }
        }
    }
}

crates/env/src/engine/off_chain/db/accounts.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/db/accounts.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/db/accounts.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/test_api.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/test_api.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/test_api.rs Outdated Show resolved Hide resolved
crates/storage/src/lazy/lazy_cell.rs Show resolved Hide resolved
@Robbepop Robbepop merged commit 3803a26 into master Nov 17, 2020
@Robbepop Robbepop deleted the cmichi-fix-hashmap-clear-storage branch November 17, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leftovers when clearing storage of nested Lazy-like structures
3 participants