Chain key value storage #3774
Chain key value storage #3774
Conversation
complete some test method by providing child access to values.
is. Need to use historied values, maybe remove pinned, and put some context as parameter for get_offstate
probably need to store state, apply gc on state and depending on result restore state or actually gc historied values.
should get updated.
for gc. Need to change commit set to contain an history of values and adjust test db.
cheme
added
the
A3-in_progress
| /// Internal buffer, it is either a readonly view other the | ||
| /// serialized data or the pending changes. | ||
| type SerializedBuff<'a> = Cow<'a, [u8]>; |
There was a problem hiding this comment.
does that means that if cow is borrowed variant then this is a readonly view other the serialized data and if it is owned then it is a pending change ?
There was a problem hiding this comment.
Actually doesn't seems to have more than Cow semantic, the type renaming is for me more confusing than helpful
There was a problem hiding this comment.
yeah, removing the alias could be better, will do.
| pub fn into_owned(self) -> Serialized<'static, F> { | ||
| Serialized(Cow::from(self.0.into_owned()), PhantomData) | ||
| } |
There was a problem hiding this comment.
then this contradict my understanding of serialized, this is owned variant and this is not a readonly view over some serialized data ? no ?
There was a problem hiding this comment.
also this function seem never used, nor it is documented, also Serialized implements Clone and the only difference between this and clone is that internally the value is owned instead of borrowed but user of linear don't access this so from his point of view there is no difference
| // truncate here can be bad | ||
| self.0.to_mut().truncate(start_ix + SIZE_BYTE_LEN); |
There was a problem hiding this comment.
not to much :), I dropped the comment (just rewrite with copy_within)
| self.0.to_mut().copy_within(start_from..start_from + size, start_to); | ||
| } | ||
|
|
||
| // Usize encoded as le u64 (for historical value). |
There was a problem hiding this comment.
| // Usize encoded as le u64 (for historical value). | |
| // Read u64 encoded as le (for history index). |
what do you mean by Usize encoded as le u64, history index is u64 itself
Co-Authored-By: thiolliere <gui.thiolliere@gmail.com>
Break big line.
| /// Trait defining a state for querying or modifying a tree. | ||
| /// This is a collection of branches index, corresponding | ||
| /// to a tree path. | ||
| pub trait BranchesStateTrait<S, I, BI> { |
There was a problem hiding this comment.
I don't see the point of having S generic if it only implemented for boolean
| /// Trait defining a state for querying or modifying a branch. | ||
| /// This is therefore the representation of a branch state. | ||
| pub trait BranchStateTrait<S, I> { | ||
|
|
||
| /// Get state for node at a given index. | ||
| fn get_node(&self, i: I) -> S; | ||
|
|
||
| /// Get the last index for the state, inclusive. | ||
| fn last_index(&self) -> I; | ||
| } | ||
|
|
||
| /// This is a simple range, end non inclusive. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct BranchRange { | ||
| pub start: u64, | ||
| pub end: u64, | ||
| } |
There was a problem hiding this comment.
note: maybe this could make use of standard Range structures and traits as S is always a boolean
| I: Copy + Eq + TryFrom<u64> + TryInto<u64>, | ||
| BI: Copy + Eq + TryFrom<u64> + TryInto<u64>, | ||
| { | ||
| if let Some((state_branch, state_index)) = state.iter().next() { | ||
| let state_index_u64 = saturating_into(state_index); |
There was a problem hiding this comment.
I'm not sure of the interest of having I being generic if is it to saturate it anyway.
cheme
added
A3-in_progress
core/state-machine/src/backend.rs
Outdated
| /// In-memory backend. Fully recomputes tries on each commit but useful for | ||
| /// tests. | ||
| pub struct InMemory<H: Hasher> { | ||
| inner: HashMap<Option<Vec<u8>>, HashMap<Vec<u8>, Vec<u8>>>, |
There was a problem hiding this comment.
must be renamed trie_inner or trie
core/state-machine/src/backend.rs
Outdated
| pub struct InMemory<H: Hasher> { | ||
| inner: HashMap<Option<Vec<u8>>, HashMap<Vec<u8>, Vec<u8>>>, | ||
| trie: Option<TrieBackend<MemoryDB<H>, H>>, | ||
| kv: Option<InMemoryKvBackend>, |
There was a problem hiding this comment.
must be renamed kv_inner or kv
core/state-machine/src/backend.rs
Outdated
| inner: HashMap<Option<Vec<u8>>, HashMap<Vec<u8>, Vec<u8>>>, | ||
| trie: Option<TrieBackend<MemoryDB<H>, H>>, | ||
| kv: Option<InMemoryKvBackend>, | ||
| state: Option<StateBackend<MemoryDB<H>, H, InMemoryKvBackend>>, |
There was a problem hiding this comment.
once you merge master you could renamed it with a more obvious name like as_state_backend_place_holder but I can't really find a good name, but at least the comment
| kv | ||
| } else { | ||
| self.state.as_ref().unwrap().kv_backend() | ||
| } |
There was a problem hiding this comment.
this else condition doesn't make sense, state is only used in the function for returning the immutable reference in as_state_backend.
There was a problem hiding this comment.
I remember why I use it this way, it was simply to avoid copy when producing the 'as_state_backend' : instead of cloning the values as we do for the trie part it is moving the container.
So after using as_state_backend this is getting use instead of the kv field. I could probably switch to simply cloning the kv field.
There was a problem hiding this comment.
would be good to have some comment about this complex behavior
core/state-machine/src/backend.rs
Outdated
| inner: inner, | ||
| trie: None, | ||
| state: None, | ||
| kv: Some(Default::default()), |
There was a problem hiding this comment.
All this from sounds now a bit weird to me because it is not obvious if it should make an InMemory with the kv storage or without the kv storage. usually I prefer a new function that makes it obvious. However I'm ok with this. But if we really want to make kv optional then add a doc in the from implementation stating that kv is activated
There was a problem hiding this comment.
Yes, it is only to avoid cloning it, would probably be better with a box. But yes the option here does not have meaning, in current state you need either kv being Some or state being Some.
But yes I could probably just clone.
| /// in the backend, and produce a "transaction" that can be used to commit. | ||
| /// Does include child storage updates. | ||
| fn full_storage_root<I1, I2i, I2>( | ||
| fn full_storage_root<I1, I2i, I2, I3>( |
There was a problem hiding this comment.
why does the storage root takes kv_deltas ? kv shouldn't influe the root in any way no ?
There was a problem hiding this comment.
Yes kv does not influe in the root, it is mainly here to complete the transaction.
Ideally we could have a function that does only the root: this would be good from my point of view (there is quite a few place where we do not use the produced transaction).
| pub struct BlockImportOperation<Block: BlockT, H: Hasher> { | ||
| old_state: CachingState<Blake2Hasher, RefTrackingState<Block>, Block>, | ||
| db_updates: PrefixedMemoryDB<H>, | ||
| db_updates: (PrefixedMemoryDB<H>, InMemoryKvBackend), |
There was a problem hiding this comment.
maybe it is better to avoid tuple here ? you could do trie_db_update and kv_db_update.
or maybe create a new struct ?
There was a problem hiding this comment.
Yes, that is a bit of refactoring, but those key value pr should try to get as decoupled as possible from the trie state (the opposite of what I did).
That would be a big but probably needed refactoring, I am switching those PR state to in_progress thus.
| if change.is_some() { | ||
| (Ser::default(), true) | ||
| } else { | ||
| break; |
There was a problem hiding this comment.
This is an error isn't it ? it mustn't happen that a key doesn't exist in db while being in the commit marked at unchanged, maybe
| break; | |
| break; // This is an invalid state, but nothing we can do |
or better log something
There was a problem hiding this comment.
yes it is the case of deleting a value that is not here.
and few doc and naming changes.
|
Closing this PR because Child trie associated issue will be work around in a way that does not require this. |
3 participants
This is ongoing work on a new storage kind for substrate. I do open the PR to follow the progress, and get feedback.
The amount of code start to grow, and I am at a point where I want to stabilize, clean and document code, for this putting some roadmap in place will help.
Seems also like a good time to start design discussion.
Description
Some info needs to be in synch with blockchain state (information accessible or any block), but does not require the proof property.
This PR intends to have such storage. Design goals are:
Basically the same thing as the existing state trie storage without the trie proof overhead.
Use case
Child trie key isolation can be based on parent storage path, but this does not play well with move or delete child trie operations. So associating a unique id with every child trie seems needed (see #2209). Yet those uniqueids are only an implementation details and should not be part of the state (and in case of full storage with pruning it could not (this case does not exists at this time but indicate a severe design limitation of #2209)).
Using this storage for those child trie unique id (or keyspace) seems fine. For instance, a child trie deletion and then re-creation (at the same address in the same transaction) is doable : deletion register the old unique id for pruning and delete the association from the new storage, creation get a new unique id for the new child trie so no delta for pruning is needed (global id for full history with pruning or index from the same storage if only using canonical).
An alternative to get back to #2209 state would be to use storage cache, but it is a bit less flexible (no direct access from statemachine overlay, so it would be temporary code until move or delete operation are really needed), and it is subject to exploit (as his the current implementation) : stores one rocksdb key value per block history as a linked list.
cc\ @todr , iirc there is such local storage in offchain storage api but not yet implemented.
It seems to me this replies the the local offchain storage, it will requires exposing the extrinsic (see TODOs), and probably handling bigger content (see TODOs on indexing and maybe handling file as content). Also maybe adding the per key lock that is in place in offchain storage and not needed in others use cases.
any technical info that can be different between client, eg current use cases of storage-cache.
info that should remain identical between client but does not fit a trie storage (we still need to synch some proof on the trie). can even be extended to state storage with lazy proof access (or only a few recalculated trie levels).
TODOs in this PR
TODOs for other PR
Those TODOs even if a bit to much to be reviewable in this pr are strictly needed for this storage.
- linear: size limite range index and value: linked list of range with possibly included values in range.
- mmr: range with indexing of range as in mmr allows quick access for latest blocks and reduce significantly access for very big history (eg a block index). Size limits similar to previous impl (may need to allow splitting mmr nodes index too).
- bench with different size limits.
- serialized with history: likely a first indexing by branch index and canonical only indexing behind it.
- storing association block -> branch index : that was done in one of my previous branch (client-db-ix) where I mistakenly implements branch index management at client level.
- storing last branch ix at client level.
- transmitting branchix in commitset of state-db and initiating state-db at right branch index
- this mode is way less efficient than standard canonical only mode so there could be a plugged canonical only storage mode (same as current) with complement to the full trie indexed one.
Optional TODOs
Api similar to offchain, add a prefix, have reserved columns for some static prefix (or add both column and prefix).
- means that result transaction will also be grouped by those.
- means adding extrinsics to query the storage (such ext will probably be needed soon for testing (but gated behind a feature).
full history with pruning: currently all non canonical state is only use in state-db in memory. Implementing full history for this storage means basically that those key do not have to be in memory until canonicalisation. This is not possible currently for state db trie because it will result in key collision. Still a costy approach would be to extend trie library to add the couple branch number block number to the encoded nodes in the underlying db (we already prefix with the node partial path). Then stored branch will becomes [Encoded branch ++ array of child (branch ix + block ix)]. This will not change hash calculation, but would certainly be a bit costy memory wise (two u64 per child, a bit less if we do some small optim like last childs if undefined are same as previous). This could be use along with a canonical compact encoding, but it would involve a pretty heavy cannonicalisation process (rewrite all branch pointer and node index for canonicalized content).
Also this pr could be split for review:
This could also be organized in a clean 3 commits history.