This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Closed
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min
Closed

avoid key collision on child trie and proof on child trie#2209
cheme wants to merge 142 commits into
paritytech:masterfrom
cheme:child-trie-soft-min

Conversation

@cheme

@chemecheme commented Apr 4, 2019

Copy link
Copy Markdown
Contributor
  • Initialissue

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).

Comment threadcore/trie/src/lib.rs Outdated
Comment threadsrml/contract/src/account_db.rs Outdated
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

@chemechemeApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 .

@gui1117gui1117Apr 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadcore/executor/src/wasm_executor.rs
Comment threadcore/state-machine/src/backend.rs Outdated
use trie::{TrieDBMut, TrieMut, MemoryDB, trie_root, child_trie_root, default_child_trie_root, KeySpacedDBMut};
use heapsize::HeapSizeOf;
use primitives::subtrie::{KeySpace, SubTrie};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadcore/trie/src/lib.rs Outdated
Comment threadcore/primitives/src/subtrie.rs Outdated
@chemecheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 4, 2019
@chemecheme removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 5, 2019
cheme added 6 commits August 8, 2019 22:56
- 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).
Comment threadcore/primitives/src/child_trie.rs
Comment threadcore/sr-io/with_std.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs
@chemecheme added A4-gotissues and removed A0-please_review Pull request needs code review. labels Sep 6, 2019
@cheme

cheme commented Sep 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
Member

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cheme@sorpaas@gavofyork@pepyakin@gui1117@jimpo@Demi-Marie@devops-parity