Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

reset cache when storage possibly change (fix init of tests). #9665

Merged
4 commits merged into from
Sep 6, 2021

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Aug 31, 2021

Very small pr to reset root cache when building state (in some other branch my tests did fail due to the root caching when using test externalities).

@cheme cheme added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Aug 31, 2021
@bkchr
Copy link
Member

bkchr commented Aug 31, 2021

Where is this function being used?

@cheme
Copy link
Contributor Author

cheme commented Aug 31, 2021

On 'assimilate' (did hit that when adding content to test_externalities in another branch), via 'apply_transaction'.

@bkchr
Copy link
Member

bkchr commented Sep 4, 2021

So we should reset the cache there?

@cheme
Copy link
Contributor Author

cheme commented Sep 4, 2021

Yes, modifying the storage with the mutable access means you are changing the backend (in normal condition this never happen, storage of trie backend stay the same during the whole processing, but in tests this function can be use).
We could clear cache after changing the backend in the calling function, but it looks safer to do it here (mutable access to trie backend so potentially something change and get invalidated).

(it is just the local cache of child trie roots)

@bkchr
Copy link
Member

bkchr commented Sep 4, 2021

Hmm all of this feels very dirty.

@cheme
Copy link
Contributor Author

cheme commented Sep 5, 2021

I can move the the clear_cache after calling backend_storage_mut but it will not prevent future issue.
I would prefer renaming the function to backend_storage_test_modify or something that let the user know storage from a trie backend should not be change this way.

@cheme
Copy link
Contributor Author

cheme commented Sep 5, 2021

I could also just remove backend_storage_mut, and replace it by trie backend rebuilding (thus using a new empty cache).

@cheme
Copy link
Contributor Author

cheme commented Sep 5, 2021

Hmm all of this feels very dirty.

I feel that it is more backend_storage_mut that is a bit out of place, and if you audit usage it is mostly for building test or in memory backend as a convenience function (trie backend purpose is to implement backend that is a read api (with some inner mutable access for caching)).

@cheme
Copy link
Contributor Author

cheme commented Sep 5, 2021

I use this code in #8931 , but with recent refactoring it may not be needed anymore, but in any case it is still a footgun.

@bkchr
Copy link
Member

bkchr commented Sep 5, 2021

but in any case it is still a footgun.

If we can remove it, perfect.

@@ -94,6 +94,9 @@ where

/// Get backend storage reference.
pub fn backend_storage_mut(&mut self) -> &mut S {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I did forget it , thanks.

@bkchr
Copy link
Member

bkchr commented Sep 6, 2021

bot merge

@ghost
Copy link

ghost commented Sep 6, 2021

Trying merge.

@ghost
Copy link

ghost commented Sep 6, 2021

Bot will approve on the behalf of @bkchr, since they are a team lead, in an attempt to reach the minimum approval count

@ghost ghost merged commit 60078b3 into paritytech:master Sep 6, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants