Child trie api changes BREAKING #4857

cheme · 2020-02-07T19:05:39Z

The child trie api got some questioning recently, so this pr try to make things cleaner and easier to specify.

Allows child trie to be build from their storage key only. This relies on two condition
- storage_key of the child trie is a unique id strong enough to avoid hash collision.
- storage_key of the child trie should only be use once for the child trie (eg if the child trie is deleted, it cannot be created again at the same place).

Contract code falls in this category, polkadot crowdfund to, I think child trie module do also (cc @thiolliere).

Changes the way StorageKey is define to never include ':child_storage:default:' and include it only when needed (when reading or updating parent root).
Separates child trie host function into their own runtime api (so future child trie of different kind could use their own).

So this breaks rpc, with this pr rpc would use storage_key without its prefix and no more child_info and child type fields. cc @Stefie
This PR also do break existing runtime by removing the child info and child type too.

This also changes TrieIdGenerator of contract pallet to stop including the child prefixes for the key, the changes needed are in the pr cc @pepyakin Closes #2325.

So for other kind of child trie new rpc api and runtime host function will be needed.

polkadot companion: paritytech/polkadot#950

prefix will be added at query and on full storage root lazilly. Also note that both type are implementation compatible so we do not need a different well known key.

backend access.

primitives/externalities/src/lib.rs

Stefie · 2020-02-17T16:11:38Z

So this breaks rpc, with this pr rpc would use storage_key without its prefix and default to the child_type 0 with an empty child_info description. cc @Stefie

@cheme: Adding that to the Polkdot JS API shouldn't be much work. So feel free to do these changes any time, I'd just need to change a few lines in the contracts-waterfall repo and the JS API

child_info.

are not general child struct, but default child struct only). Applying merge of ChildInfo and OwnedChildInfo.

bkchr

There is no reason to keep deprecated functions, if they are not used.

bkchr · 2020-04-15T10:13:38Z

primitives/io/src/lib.rs

+
+
+	/// Deprecated, please use dedicated runtime apis.
+	fn child_get(


Is this code used somewhere? I don't think so?

So please remove it, the same goes for all deprecated unused functions.

I did remove them first, but restore them back to retain compatibility with edgeware chain, but I would also really prefer to remove it (and the associated code).
Think we can break that?

Yeah, if we not do it now, we need to keep this code forever.

I did remove the code (it messed up my last master merge and it is in latest master merge commit).

arkpar · 2020-04-15T13:05:30Z

Changes in client/network/src/protocol/message.rs probably require protocol version bump and some code to support backward compatibility

arkpar · 2020-04-15T13:07:29Z

This might be a good time to rename "child" to "subtree" or "substorage". Please vote if you think this is a good idea.

cheme · 2020-04-15T13:13:21Z

This might be a good time to rename "child" to "subtree" or "substorage". Please vote if you think this is a good idea.

My preference goes toward «substorage» (do not have to be a tree, just some storage with a proof system), I could also propose «substate».
This would be a big renaming but I think it will be worth it.

NikVolf

for runtime interface part

cheme · 2020-04-15T15:04:07Z

Changes in client/network/src/protocol/message.rs probably require protocol version bump and some code to support backward compatibility

619b454 is trying to do that, note that it assumes input data is of the right size (I found no other way to remove possible false positive when decoding with the latest encoding without changing to much the way things are encoded).
@arkpar, I do not have code compatibility on the light client handler either, reasoning was that the child trie api should not be use at the moment (one of the latest change remove the sp-io compatibility there). I observe protobuf version is still 1 so it may not be needed (not sure how much support for light client is currently needed).

arkpar · 2020-04-15T15:11:12Z

That's a good point. If it is not used there's no need to bother with compatibility indeed. So 619b454 could be reverted I guess. If not, you could implement custom Decode that would try to decode as previous version and then as new version if it fails. Like here: https://github.com/paritytech/substrate/blob/master/client/network/src/protocol/message.rs#L349

cheme · 2020-04-15T15:16:12Z

Hum custom decoder looks like a better idea indeed (or at least put everything in RemoteReadChildRequest), but I am reverting it right now.

This reverts commit 619b454.

gavofyork · 2020-04-17T10:13:19Z

Needs resolving @cheme

Is there any reason we shouldn't merge this before 2.0?

cheme · 2020-04-17T10:21:08Z

Is there any reason we shouldn't merge this before 2.0?

It is breaking some compatibility so I would think it is better if merge before 2.0. I am resolving the ci failures.

gnunicorn · 2020-04-20T12:26:00Z

@cheme this needs one more conflict resolution and can be merged.

cheme added 8 commits February 6, 2020 17:00

Implementation basis for this PR, note that child storage key default

5151471

prefix will be added at query and on full storage root lazilly. Also note that both type are implementation compatible so we do not need a different well known key.

Resolve prefix from child_info

8715446

Removed all child prefix, next is putting it back on full storage and on

b72cd9c

backend access.

Prefixing child storage key where needed.

b636687

Merge branch 'master' into child_trie_w3_change

e88631c

Remove deprecated check for genesis and fix some test storage builds.

728aedf

Merge branch 'master' into child_trie_w3_change

8329448

Fix contract to stop using the child storage prefix.

864e9ca

cheme added A3-in_progress Pull request is in progress. No review needed at this stage. Z7-question Issue is a question. Closer should answer. labels Feb 7, 2020

drskalman mentioned this pull request Feb 13, 2020

Reply to the child storage questions w3f/polkadot-spec#127

Closed

cheme added 2 commits February 17, 2020 10:02

Merge branch 'master' into child_trie_w3_change

2224e2f

Fixing merge.

c04ba95

Stefie reviewed Feb 17, 2020

View reviewed changes

primitives/externalities/src/lib.rs Outdated Show resolved Hide resolved

cheme mentioned this pull request Feb 17, 2020

Manage child trie content independently #4827

Closed

cheme added 4 commits February 17, 2020 22:39

Switch back to using prefix on host function, remove child_id and

ffaf9f5

child_info.

fix polka ref issue.

87bd97c

Switching back to unprefixed in child info (all temporary struct

17cf613

are not general child struct, but default child struct only). Applying merge of ChildInfo and OwnedChildInfo.

bump version

b3ccc93

paritytech-ci added the B2-breaksapi label Feb 18, 2020

cheme added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. Z7-question Issue is a question. Closer should answer. labels Feb 18, 2020

fix tabs and doc.

afe85ae

cheme marked this pull request as ready for review February 18, 2020 12:38

cheme requested review from kianenigma, pepyakin, gui1117 and tomusdrw as code owners February 18, 2020 12:38

bkchr reviewed Apr 15, 2020

View reviewed changes

sp-io documentation changes.

bc2a198

Merge branch 'master' into child_trie_w3_change

83ff4a0

gnunicorn added this to the 2.0 milestone Apr 15, 2020

Merge branch 'master' into child_trie_w3_change

ecb1c39

NikVolf approved these changes Apr 15, 2020

View reviewed changes

Retain compatibility with network protocol.

619b454

cheme added 2 commits April 15, 2020 17:16

Revert "Retain compatibility with network protocol."

fa52a8c

This reverts commit 619b454.

Merge branch 'master' into child_trie_w3_change

167f20c

arkpar approved these changes Apr 15, 2020

View reviewed changes

Merge branch 'master' into child_trie_w3_change

928fac0

gavofyork added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 17, 2020

fix renamed field related error

69ed118

gnunicorn added the A3-needsresolving label Apr 20, 2020

Merge branch 'master' into child_trie_w3_change

b79bd84

gnunicorn merged commit fc6e2a6 into paritytech:master Apr 20, 2020

Stefie mentioned this pull request May 13, 2020

Implement new childstorage methods paritytech/pallet-contracts-waterfall#73

Merged

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020

Companion to child trie api change paritytech#4857 (paritytech#950)

c79e4bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Child trie api changes BREAKING #4857

Child trie api changes BREAKING #4857

cheme commented Feb 7, 2020 •

edited

Loading

Stefie commented Feb 17, 2020 •

edited

Loading

bkchr left a comment

bkchr Apr 15, 2020

cheme Apr 15, 2020

bkchr Apr 15, 2020

cheme Apr 15, 2020

arkpar commented Apr 15, 2020

arkpar commented Apr 15, 2020

cheme commented Apr 15, 2020

NikVolf left a comment

cheme commented Apr 15, 2020

arkpar commented Apr 15, 2020

cheme commented Apr 15, 2020

gavofyork commented Apr 17, 2020

cheme commented Apr 17, 2020 •

edited

Loading

gnunicorn commented Apr 20, 2020



		/// Deprecated, please use dedicated runtime apis.
		fn child_get(

Child trie api changes BREAKING #4857

Child trie api changes BREAKING #4857

Conversation

cheme commented Feb 7, 2020 • edited Loading

Stefie commented Feb 17, 2020 • edited Loading

bkchr left a comment

Choose a reason for hiding this comment

bkchr Apr 15, 2020

Choose a reason for hiding this comment

cheme Apr 15, 2020

Choose a reason for hiding this comment

bkchr Apr 15, 2020

Choose a reason for hiding this comment

cheme Apr 15, 2020

Choose a reason for hiding this comment

arkpar commented Apr 15, 2020

arkpar commented Apr 15, 2020

cheme commented Apr 15, 2020

NikVolf left a comment

Choose a reason for hiding this comment

cheme commented Apr 15, 2020

arkpar commented Apr 15, 2020

cheme commented Apr 15, 2020

gavofyork commented Apr 17, 2020

cheme commented Apr 17, 2020 • edited Loading

gnunicorn commented Apr 20, 2020

cheme commented Feb 7, 2020 •

edited

Loading

Stefie commented Feb 17, 2020 •

edited

Loading

cheme commented Apr 17, 2020 •

edited

Loading