avoid key collision on child trie and proof on child trie #2209

cheme · 2019-04-04T14:08:34Z

Initial issue

initial issue is that child trie key/value stored in rocksdb are all build the same way for all child trie,
therefore two identical child trie will get all their key duplicated and the pruning will break one of the two tries (only child trie with guaranty of not having same key/value content can currently be use with pruning).

Keyspace change

This PR solves the initial issue by prepending a unique id to the keyvalue db keys. It uses a HashDB implementation KeySpaceDB to prepend a unique trieid.

! with current implementation it is only prepended if PrefixedMemoryDB is use.
Long term design should move that keyspace information to the db layer (have keyvalue db that manage two level of collections: the heavy one and the light one).

Those prefix should also allow more efficient operation on child trie.

accessing all child trie keyvalue db key from rocksdb, allowing to skip what would otherwhise be a query of every nodes of the child trie for every of its states (blocks).
related to this access we can have efficient deletion.
related to this access we can export/import trie between chain (the prefix will need to be rewritten as it is only unique for a chain context).

Api change

This PR also contains a change of child trie api: do not do operation depending on storage_key but do operation depending on child_trie state.

This can allow efficient child trie usage (no need to query parent trie on every operation).

Recently I tried to split this PR in two (keyspace first, api second), but this api change is needed when a child trie does not exists (having the child trie in parameter makes things easier).

branch

try_to backend meth (seems like proof should not work without it). Super awkward memory struct, keep this refacto for after pr (keep pr simple).

huge performance cost for wasm but need to get feedback on best/safe wasm design).

TODO wasm, then SubTrie sanitize field access (ensure no use of key without prefix), then add tests.

runtime anyway??).

`proof_recorded_and_checked_with_child` test.

core/trie/src/lib.rs

cheme · 2019-04-04T14:22:07Z

srml/contract/src/account_db.rs

@@ -62,7 +63,11 @@ pub trait AccountDb<T: Trait> {
 pub struct DirectAccountDb;
 impl<T: Trait> AccountDb<T> for DirectAccountDb {
 	fn get_storage(&self, _account: &T::AccountId, trie_id: Option<&TrieId>, location: &StorageKey) -> Option<Vec<u8>> {
-		trie_id.and_then(|id| child::get_raw(id, location))
+		// TODO pass optional SubTrie or change def to use subtrie (put the subtrie in cache (rc one of


I did use account_db trie_id (see other comments) as keyspace (and as subtrie parent location).
So there is a need for caching SubTrie struct. TODO issue for that ?
There is a possible optimization by putting SubTrie directly at Account_id location, this would only be possible by:

storing account infos at the same location as subtrie (in subtrie prefix)

changing this pr to allow storing subtrie at any location
For both point it requires implementing a way to store subtrie with different encoder (and with additional info).

@thiolliere maybe this pr (not sure it will get merged there might be better way of fixing the collision), could be of interest regarding #1882 or #1883 .

So there is a need for caching SubTrie struct. TODO issue for that ?

hmm I can also think of another anwser. The trie_id: Option<&TrieId> is already an optimisation that says: if there is a trie_id in direct_storage then you have to give me that I won't do the lookup from AccountId, so you can cache it for me.
This thoses changes you introduce we can just say: if there is a subtrie associated to the account already then give it I won't do the lookup.

Having subtrie in AccoundInfo seems cool though but not mandatory here.

@thiolliere maybe this pr (not sure it will get merged there might be better way of fixing the collision), could be of interest regarding 1882 or 1883 .

I don't know in which context this collision can happen I design things without taking this into consideration though. But interesting thanks

Yes I see your optim, it is better design (that is the reason why I pinged you :)

I don't in which context this collision can happen I design things without taking this into consideration though. But interesting thanks

without this pr it happens whenever two subtries are similar and content got pruned (for contract it should happen a lot). It only requires that both subtries got same branch path (also content (same proof would be more correct)) to the deleted value.

Having subtrie in AccoundInfo seems cool though but not mandatory here.

As long as we use a trie_id as subtrie key from account_id (merging account info and trie info) there is no need to take this into consideration.

core/executor/src/wasm_executor.rs

cheme · 2019-04-04T14:32:16Z

core/state-machine/src/backend.rs

 use heapsize::HeapSizeOf;
+use primitives::subtrie::{KeySpace, SubTrie};
+


Backend struct renamed as MapTransaction and VecTransaction are really suitable with SubTrie struct: especially the usage of Option<SubTrie> seems awkward at some points).

core/trie/src/lib.rs

core/primitives/src/subtrie.rs

…child-trie-soft-min

- use keyspace instead of storage key (situation before was an issue and gave the wrong impression). Subscription rpc for child does not exists but shall use storage_key (we may keep keyspace internally). - add children change to change set (probably break subscription rpc format).

core/primitives/src/child_trie.rs

core/sr-io/with_std.rs

core/executor/src/wasm_executor.rs

cheme · 2019-09-06T11:02:22Z

I did merge the pr with master.
There is an unsolve issue: actions on child trie (set_child_trie) do not manage their extrinsics.
It simply requires another extrinsics counter.

Also I did not reassert the logic of using keyspace either: there might be some place where storage key should be kept.

Seeing how stale this PR is, I will refer to my previous comment #2209 (comment) (see missing point) and conclude this is a wrong approach.

For instance having the technical keyspace in the state will make some rather complicated specification, when an implementation using reference counted keyvalue do not need it at all.

Therefore I am switching this PR to 'A4-Got issue' (could also be close (will require an issue creation first)) in favor of another approach (a bit more involved):

create an offstate storage, something similar to aux_storage but that is aligned with the block chain state (meaning that it needs to be change from overlay layer and needs pruning to: very similar to trie state).
This offstate storage may have also some similarity with offchain local storage but it is unclear at this point.
make a pr
use similar keyspace as in this pr for child tries, but do not touch child api (keep using storage key only and
manage some caching of storage_key -> keyspace in the new offstate storage).
make a pr
maybe try to get into some api change to avoid all those redundant query of child trie state by using a different api (notably splitting the child trie proof in two part as it is the case in this pr). At this point things could be on par with this pr (except there is no keyspace in child trie structure). + child trie structure could spawn from current description (not serialized and type of child trie contain in path).
make a pr

gavofyork · 2019-09-19T08:11:17Z

@cheme please make an issue for it and close this when done.

cheme added 16 commits March 26, 2019 10:15

Straight forward move of trie related only change from child-trie-soft

f6c4bb2

branch

merge backend stuff from old branch, add insertion of child trie root in

878a7ae

try_to backend meth (seems like proof should not work without it). Super awkward memory struct, keep this refacto for after pr (keep pr simple).

commit before wasm_executor change (we will keep old interface for now:

a464936

huge performance cost for wasm but need to get feedback on best/safe wasm design).

tabify

4de73d7

Things compile (account change is unoptimized), not for wasm.

be0e340

TODO wasm, then SubTrie sanitize field access (ensure no use of key without prefix), then add tests.

Make SutrieField non public

7f3a282

Compile no std child trie with subtrie query (super costy: do we trust

4d208db

runtime anyway??).

Make the single child trie test pass.

6d7f355

Proving synch child trie content (TODO it requires deletion tests).

25bcb4c

restore test

9eaef35

Merge branch 'master' into child-trie-soft-min

37d9536

Fix test error (wrong vec alloc).

67c03a2

tests for no key collision

99dbb5a

Merge branch 'master' into child-trie-soft-min

3be1802

Revert storage_root using child, create a variant for it: see

fec73d0

`proof_recorded_and_checked_with_child` test.

indentation and remove comment

20d168d

devops-parity added the A4-gotissues label Apr 4, 2019

cheme commented Apr 4, 2019

View reviewed changes

core/trie/src/lib.rs Outdated Show resolved Hide resolved

cheme commented Apr 4, 2019

View reviewed changes

core/executor/src/wasm_executor.rs Show resolved Hide resolved

cheme commented Apr 4, 2019

View reviewed changes

core/trie/src/lib.rs Outdated Show resolved Hide resolved

cheme commented Apr 4, 2019

View reviewed changes

core/primitives/src/subtrie.rs Outdated Show resolved Hide resolved

Remove some TODOs, fix compile error

d797bd0

cheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019

plaindb does not need to be keyspaceddb

27096aa

cheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019

cheme added 2 commits April 5, 2019 17:54

Merge branch 'master' into child-trie-soft-min

3560acc

bump impl_version

c3fc432

cheme mentioned this pull request Apr 15, 2019

Adds first version of validate_block paritytech/cumulus#3

Merged

cheme added 6 commits August 8, 2019 22:56

Remove unused method (in favor of assimilate).

7d96338

Merge branch 'child-trie-soft-min' of github.com:cheme/polkadot into …

ba7bdcb

…child-trie-soft-min

Merge branch 'master' into child-trie-soft-min

aacea85

merge fix.

6789641

Rename MapTransaction to StorageContent.

2b37160

cheme commented Aug 16, 2019

View reviewed changes

core/primitives/src/child_trie.rs Show resolved Hide resolved

cheme added 2 commits August 22, 2019 10:06

Merge branch 'master' into child-trie-soft-min

bc5653c

update to master.

5ecfd19

Demi-Marie reviewed Aug 25, 2019

View reviewed changes

core/sr-io/with_std.rs Outdated Show resolved Hide resolved

core/executor/src/wasm_executor.rs Show resolved Hide resolved

cheme added 3 commits August 26, 2019 08:23

Cast explicitelly for readability.

baf89c8

bump spec version.

0f8bff9

Merge branch 'master' into child-trie-soft-min

5a8576b

cheme requested review from andresilva, kianenigma and tomusdrw as code owners August 29, 2019 08:48

cheme added 2 commits August 29, 2019 15:15

Merge branch 'master' into child-trie-soft-min

045ee32

Fix compilation.

58e6e41

4meta5 mentioned this pull request Aug 31, 2019

Generalized Child Tries (storage) JoshOrndorff/substrate-recipes#35

Closed

cheme added 2 commits September 5, 2019 15:25

Merge branch 'master' into child-trie-soft-min with conflicts.

70555c5

build passing, regression on extrinsics for a set_child (see new TODO).

130e5e4

cheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019

cheme mentioned this pull request Sep 19, 2019

Possible child trie key collision when pruning #3648

Closed

cheme closed this Sep 19, 2019

This was referenced Oct 7, 2019

Chain key value storage #3774

Closed

Avoid key collision for child trie #3804

Closed

cheme mentioned this pull request Nov 20, 2019

Fix key collision for child trie #4162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid key collision on child trie and proof on child trie #2209

avoid key collision on child trie and proof on child trie #2209

cheme commented Apr 4, 2019 •

edited

Loading

cheme Apr 4, 2019 •

edited

Loading

gui1117 Apr 5, 2019 •

edited

Loading

cheme Apr 5, 2019

cheme Apr 4, 2019

cheme commented Sep 6, 2019

gavofyork commented Sep 19, 2019

		use heapsize::HeapSizeOf;
		use primitives::subtrie::{KeySpace, SubTrie};

avoid key collision on child trie and proof on child trie #2209

avoid key collision on child trie and proof on child trie #2209

Conversation

cheme commented Apr 4, 2019 • edited Loading

cheme Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

gui1117 Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

cheme Apr 5, 2019

Choose a reason for hiding this comment

cheme Apr 4, 2019

Choose a reason for hiding this comment

cheme commented Sep 6, 2019

gavofyork commented Sep 19, 2019

cheme commented Apr 4, 2019 •

edited

Loading

cheme Apr 4, 2019 •

edited

Loading

gui1117 Apr 5, 2019 •

edited

Loading