Optimize key directory reloads #4583
Conversation
NikVolf
added
A0-pleasereview 🤓
NikVolf
changed the title
util/src/sha3.rs
Outdated
| @@ -68,6 +68,24 @@ impl<T> Hashable for T where T: AsRef<[u8]> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T> Hashable for [T] where T: Hashable { | |||
| fn sha3(&self) -> H256 { | |||
There was a problem hiding this comment.
This implementation seems strange: intuitively I'm not sure what it would mean to call
vec!["a, "b", "c"].sha3(), but I don't think I would expect it to be
sha3("a") ^ sha3("b") ^ sha3("c") ^ sha3("").
Seems like any invocation of this could be replaced with:
things_to_hash.into_iter().map(|x| x.sha3()).fold(H256::default(), BitXor::bitxor)To me, it has a much more apparent meaning.
There was a problem hiding this comment.
Hmm.. actually, I think i won't use sha3 at all
There was a problem hiding this comment.
and btw, just fyi, xoring is the preferred way to combine multiple hashes (that is proven to be as good as the individual hashes are themselves), so that's quite natural to expect this implementation when hashing list of hashes
ethstore/src/dir/mod.rs
Outdated
| @@ -62,6 +63,8 @@ pub trait KeyDirectory: Send + Sync { | |||
| fn path(&self) -> Option<&PathBuf> { None } | |||
| /// Return vault provider, if available | |||
| fn as_vault_provider(&self) -> Option<&VaultKeyDirectoryProvider> { None } | |||
| /// Returns hash of the directory content, if supported | |||
| fn hash(&self) -> Result<H256, Error> { Ok(H256::zero()) } | |||
There was a problem hiding this comment.
this default implementation would lead to accounts never being reloaded?
There was a problem hiding this comment.
key directory does not operate the concept of "reloading accounts" at all
it just returns hash of all account headers, if it is able to
There was a problem hiding this comment.
Even so, there should not be a valid implementation of a key directory which causes reload_if_changed to think the data has never changed.
If we broaden the description of the function to something like "Returns a unique 32-byte representation of the directory's contents.", then I don't imagine any implementation which couldn't do it.
(in that case, a more descriptive name like unique_repr(&self) -> Result<H256, Error> might make sense)
| return Ok(()) | ||
| } | ||
| self.reload_accounts()?; | ||
| *last_dir_hash = dir_hash; |
There was a problem hiding this comment.
This could be set before reloading to prevent parallel reloads.
There was a problem hiding this comment.
What? Set and release a lock before actual reloading?
There was a problem hiding this comment.
hmm, yeah, disregard that :)
NikVolf
added
A3-inprogress ⏳
NikVolf
added
A0-pleasereview 🤓
|
@rphmeier updated using simple std hasher & addressed another issue |
| }; | ||
| store.reload_accounts()?; | ||
| Ok(store) | ||
| } | ||
|
|
||
| fn reload_if_changed(&self) -> Result<(), Error> { | ||
| let mut last_dir_hash = self.dir_hash.lock(); |
There was a problem hiding this comment.
this will work for root dir only (not for vaults), but maybe that's a good idea for next PR :)
There was a problem hiding this comment.
@svyatonik
yeah but i guess users are not supposed to mess with the vault directories themselves
There was a problem hiding this comment.
they can use ethstore cli utility to modify vaults
svyatonik
added
A8-looksgood 🦄
Only reloads if something changed in the directory (including from outside of the app)
addresses #4443