paritytech / trie

Base-16 Modified Patricia Merkle Tree (aka Trie)
Apache License 2.0
251 stars 67 forks source link

Add DB generic type instead of dyn Trait to allow use the DB outside of the trie #206

Open AurelienFT opened 9 months ago

AurelienFT commented 9 months ago

Hello,

This crate is awesome and provides a lot of flexibility and cool features. Thanks for this very nice work.

When we used it, we had a problem with the way you hold the DB. In the TrieDBMut for example, you hold a mutable reference over the database that prevents us to have another reference in our code to do custom things such as modifying the DB content (in our case we save all changes that you make in the DB to allow user to rollback or rollforward his trie).

We can have a reference (mutable or not) to our DB using your method db() or db_mut() but it only give us the reference over something that implements HashDB that allows us to use only the methods defined in HashDB trait and not our custom ones.

This PR propose to fix this problem by replacing the dyn HashDB by providing a concrete generic type that needs to implement HashDB trait. This allow us to change db() and db_mut() to return our concrete type and be able to have fully access to the methods we created.

I would love to have your feedback on this. If it's something you already encoured and you have other solutions, I will be very happy to heard them.

Thanks again for your work

cheme commented 7 months ago

Thanks for the pr, sorry for only looking at this now.

I got no strong opinion about this, but it may be that dyn trait are favored here (cc @arkpar , @bkchr maybe). For triedb I think it would be ok to keep two & ref to the db (one dyn and one not dy).

But yes for triedbmut it can be tricky to keep two &mut reference to the db. This forces either using short lived triedbmut instance (it is the case in substrate), or have inner mutability (eg Arc<Mutex> so we can clone it and have two &mut) which if not really elegant is very often the case on db (but may not be for in memory storage).

So I can see the point here, but would want others feedback on it.

bkchr commented 7 months ago

I would argue that the current implementation is written around doing only single operations on the trie and then returning. This is the reason that things like the cache are living outside of the triedb and need to be passed as an argument.

AurelienFT commented 7 months ago

Hello, Thank you for your feedback guys. About your comment @bkchr I don't fully understand, do you mean that we should build a tree and directly drop the structure and so we should never have a case where we need to use the DB outside of the structure?