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

Inner hash of value support. #134

Closed
wants to merge 168 commits into from
Closed

Conversation

cheme
Copy link
Contributor

@cheme cheme commented May 28, 2021

This PR allows Inner hash of value support and metadata storage.

Inner hash of value

The triedbmut code change to allow hash of value instead of value:
enum Value replaces Option<DBValue>.

When accessing hash, then HashDB trait allows querying for value through
access_from hashdb new function, but at this point it only acts as
callback of value access (since in all hashdb implementation values kept
being stored with their encoded node).

Notes for reviewers

  • A big part of test code is the actual implementation and description are in Inner hashing of value in state trie (chainspec versioning). substrate#8931, not sure if its review should be done here or on the substrate pr.
  • triedbmut changes include Fix prefix for extension implementation #133 which are easier to review there (I think there is not Fix read access of changed value in triedbmut. #132, mentioning it just in case).
  • iter_build is not active in other project (trie_root still use in substrate).
  • there is a lot of changes in test, but usually is just removing redundant code, register_proof_without_value of triedbmut is new though.
  • some triedbmut types were switch to use triedb layout (there was a meta associated type at some point). I did not switch back those changes, they are a bit verbose but does not cost much and may be suitable in the future.

Variant branch: cheme/trie@hashed-value-simple4...cheme:hashed-value-simple4-meta-trait

TODOs

  • versioning is not applied in the current pr state, but should be all +0.1.0 (hash is a bit on top of the dependency tree).
  • support added to avoid including value in iteration by using a specific key iterator, but no support yet for 'exists' in substrate by adding a exists primitive to lookup that do not flag value as accessed.

@cheme cheme changed the title Inner hash of value support and metadata storage. Inner hash of value support. Jun 21, 2021
@@ -1625,11 +1776,4 @@ mod tests {
test_comb((0, &a), (1, &b), (1, &[0x01, 0x23, 0x46, 0x78][..]));
test_comb((1, &a), (1, &b), (0, &[0x23, 0x46, 0x78][..]));
}

#[test]
fn nice_debug_for_node() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed due to absence of an easy way to have a Layout with a u32 hash.
(could switch back to having Node instead of Node, but switching to Node is something I did a few time in the past so I did not revert those changes, but doable).

@cheme cheme requested a review from arkpar June 21, 2021 16:03
@cheme
Copy link
Contributor Author

cheme commented Aug 17, 2021

To reviewers, looking at current changes, 'Meta' in trielayout seems a bit awkward, I therefore recreate a branch with a inner meta type:
cheme/trie@hashed-value-simple4...cheme:hashed-value-simple4-meta-trait

This is more changes, but I generally find the typing better.
So opinion on this could be interesting (I will maintain both branches).

@arkpar
Copy link
Member

arkpar commented Sep 2, 2021

After an internal discussion we've agreed to try a different approach. Values will be stored as plain data in HashDb and not embedded in value nodes. This will make implementation much more simple as no special handling in HashDb would be required.

@qdm12
Copy link

qdm12 commented May 23, 2022

Hello everyone, I'm just curious if this PR is still relevant? 🤔 Thanks!!

@cheme
Copy link
Contributor Author

cheme commented May 23, 2022

No, it was superseded by #142, thanks for noticing it.

@cheme cheme closed this May 23, 2022
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.

3 participants