Do not check for root in TrieDB and TrieDBMut constructors
#155
Conversation
This leads to one extra storage lookup before we are doing the actual key lookup. This leads to reading the `root` twice when just wanting to lookup one key in the trie. We also return an error on lookup anyway if `root` doesn't exists, so there is really no need to do this twice.
|
I like the idea of not checking root (a trie is valid with any root and should be buildable, then missing is something else). Probably the initial design was to fail early when state was not available: in system where the presence of root indicate that the whole state for the trie exists (I am thinking maybe previous ethereum code). Making the function a combination of 'new' and '~check_existing_state'. Could failing early in substrate be good: we query one node and avoid all processing between triedb instantiation and actual first triedb access. But if the state is correct, we should not have this error ever. Maybe we need to keep the old instantiation for compatibility (changing name and indicating it in the https://github.com/paritytech/trie/blob/master/trie-db/CHANGELOG.md), or add a small 'exists_root' function so previous. |
|
Also I did find it inconvenient a few time to have to handle this error, so probably I would suggest adding a good CHANGELOG and this will be good for me. |
| where | ||
| L: TrieLayout + 'db, | ||
| { | ||
| impl TrieFactory { |
There was a problem hiding this comment.
TrieFactory is not used by substrate, but I feel like the change is good to have. Worth mentioning in CHANGELOG though.
There was a problem hiding this comment.
Here I just removed the warning of the unused layout
There was a problem hiding this comment.
I mean api did change (better new).
|
|
||
| /// `new_with_layout`, but do not check root presence, if missing | ||
| /// this will fail at first node access. | ||
| pub fn new_unchecked(db: &'db dyn HashDBRef<L::Hash, DBValue>, root: &'db TrieHash<L>) -> Self { |
There was a problem hiding this comment.
Oh we actually have the unchecked version already. I never use it so new being unchecked by default makes sense.
Changelog could simply be new_unchecked replace new.
Yeah that sounds reasonable |
This leads to one extra storage lookup before we are doing the actual key lookup. This leads to reading the
roottwicewhen just wanting to lookup one key in the trie. We also return an error on lookup anyway if
rootdoesn't exists, sothere is really no need to do this twice.