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

Support multi-hash in multi-trie via PlainDB #1106

Merged
merged 15 commits into from
Feb 6, 2019
Merged

Support multi-hash in multi-trie via PlainDB #1106

merged 15 commits into from
Feb 6, 2019

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Nov 13, 2018

Requires paritytech/trie#2, solves #872 (comment)

This adds multiple hash algorithm support for multi-trie. See paritytech/trie#2 description for how this will work.

  • Ephemeral implements PlainDB traits. PlainDB is HashDB with hashing stripped. Unlike HashDB, PlainDB will not check for null node, and it's expected that wrapper of PlainDB handles that.
  • For substrate-trie, hash_db::HashDB/PlainDB are taken into function as generic (instead of trait objects). This change is because Rust does not support super-trait upcasting, but for functions like child_delta_trie_root, we need DB to be HashDB + PlainDB.

@sorpaas sorpaas added the A0-please_review Pull request needs code review. label Nov 13, 2018
I: IntoIterator<Item = (A, Option<B>)>,
A: AsRef<[u8]> + Ord,
B: AsRef<[u8]>,
DB: hash_db::HashDB<H, trie_db::DBValue> + hash_db::PlainDB<H::Out, trie_db::DBValue>,
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like can't use the HashDB type alias defined above because Rust treats type alias and traits differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

there has been an RFC for trait aliases for ages but not happening anytime soon sadly. type aliases of traits are actually for the unsized values with the type of the trait.

We always check overlay first for a storage fetch, which already checked null data. Using PlainDB here makes it work
nicer with other PlainDB overlays.
@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Nov 14, 2018
@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 14, 2018
@sorpaas
Copy link
Member Author

sorpaas commented Nov 14, 2018

Added two new traits in hash-db: PlainDBRef and HashDBRef. The reason for this is that sometimes we pass down immutable reference of PlainDB, and the user may want to wrap them and create new instance of HashDB. In this case, we need to implement immutable and mutable reference wrapper separately (because there's no generic over reference in Rust). For immutable reference, all the mutable functions still appear in the trait, and if without PlainDBRef and HashDBRef, they would need to be marked as unreachable!(), which is not nice.

@sorpaas sorpaas mentioned this pull request Dec 18, 2018
@gavofyork gavofyork added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Jan 7, 2019
@gavofyork gavofyork added this to the 1.0gamma milestone Jan 11, 2019
@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Jan 22, 2019
@gavofyork
Copy link
Member

would be great to get some more eyes on this as it may become relevant for a smart-contract chain with storage-rent.

@svyatonik will this present a big issue for light client and/or change trie?
@arkpar any interactions/significant maintenance burden for client/db?
@rphmeier any other issues?

@gavofyork gavofyork added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Feb 1, 2019
@svyatonik
Copy link
Contributor

For light client, imo, that wouldn't be a problem iff there'll be some strict correspondence (known to light client) between child storage (identified by storage_key) and the hashing algorithm that it uses. Would be good to see example of how it'll work for full client first, though. Right now I could see that child storage is completely unavailable on light clients. This could be fixed easily (without multi hashing).

Changes trie is a separate trie, which could use its own hashing algorithm (i.e. Blake2 for all child storages). But there's another issue - we need to emit separate DigestItem::ChangesTrieRoot for every child storage in every block if we want they to be covered by changes tries. I'm not sure it is a good idea. Right now changes tries are only covering top-level storage changes - probably we should leave it as is?

@gavofyork
Copy link
Member

probably we should leave it as is?

Sounds reasonable, yeah.

@rphmeier @arkpar thoughts?

@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Feb 5, 2019
@arkpar arkpar merged commit dd3fdee into master Feb 6, 2019
@arkpar arkpar deleted the sp-multihash branch February 6, 2019 10:16
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* Temporarily pin trie to #2

* Use generic and delay trait object casting

Rust does not support super-trait upcasting

* Add PlainDB impl for Ephemeral

* Add PlainDB trait alias for completeness

* Use PlainDB for test TrieBackendStorage fetch

We always check overlay first for a storage fetch, which already checked null data. Using PlainDB here makes it work
nicer with other PlainDB overlays.

* Update trie reference

* Use HashDBRef in places when approriate

* Use PlainDBRef in places when approriate

* Update trie crate reference

* Remove unused HashDB::keys

* Patch dependencies

* Fix cargolock

* Update cargo lock again
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants