Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Describe implementation assumptions of Substrate regarding (child) storages #580

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Nov 7, 2022

Fixes #575 and #577 (#540 should be later completed here, too).

@tomaka I would appreciate if you could quickly double check this, especially my claim on how child storage keys are constructed and that get returns None if child_storage_root has never been called. Those are random guesses of mine :)

image

Comment on lines +8 to +12
The storage and child storage (<<sect-child-storage-api>>) API in the Substrate
implementation makes some behavioral assumptions on the underlying storage
architecture. While Polkadot Host implementers can decide for themselves on how
those APIs are implemented, those behaviors must be replicated in order for the
Runtime to be executed deterministically. In Substrate, child storages are
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of this paragraph. You seem to be literally describing what a specification is.

Copy link
Contributor Author

@lamafab lamafab Nov 7, 2022

Choose a reason for hiding this comment

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

So we've been telling the implementers that the "main" storage is fully separated from the child storages and that this functionality can be implemented however they want. But with this new specification we're basically saying that those storages are really not that separate, meaning we kind of insist now that child storage entries are namespaced by a specific prefix and that the Storage API actually can access Child Storages. This paragraph is meant to imply that the Host implementation must simply deal with this "ugliness", but I guess I can clarify this further.

Regarding your other points.

Why is this not explained under ext_storage_get_version_1?
And why "for example"? We want a precise list, not examples?

I put everything into one section so it's clear how the underlying data is structured in the Substrate implementation. Those things must also be considered for read, append, next_key, etc, too. But I should replace the "for example" there with something more formal.

Similarly, why not explain this under ext_storage_clear_prefix?

As mentioned above, it's to keep things packed together; functions that modify data simply check whether the key starts with :child_storage:, but the clear_prefix function makes an exception by also checking whether the input is a substring of :child_storage:. If you think it's more appropriate to move that section to ext_storage_clear_prefix I can do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we've been telling the implementers that the "main" storage is fully separated from the child storages and that this functionality can be implemented however they want. But with this new specification we're basically saying that those storages are really not that separate, meaning we kind of insist now that child storage entries are namespaced by a specific prefix and that the Storage API actually can access Child Storages. This paragraph is meant to imply that the Host implementation must simply deal with this "ugliness", but I guess I can clarify this further.

I still don't understand. You told something to the implementers, and because you told them something you're writing the spec in a different way than you would have if you hadn't told them?

storage API share the same database. This means that the storage API can
retrieve child storage entries.

For example, calling `ext_storage_get_version_1` (<<sect-ext-storage-get>>) with
Copy link
Collaborator

@tomaka tomaka Nov 7, 2022

Choose a reason for hiding this comment

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

Why is this not explained under ext_storage_get_version_1?
And why "for example"? We want a precise list, not examples?

Comment on lines +27 to +28
any keys that start with the `:child_storage:` prefix. For
`ext_storage_clear_prefix` (<<sect-ext-storage-clear-prefix>>) specifically,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, why not explain this under ext_storage_clear_prefix?

Comment on lines +43 to +45
* https://github.com/w3f/polkadot-spec/issues/575
* https://github.com/w3f/polkadot-spec/issues/577
* https://github.com/paritytech/substrate/issues/12461
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't be linking issues in a specification?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This spec isn't supposed to be an explanation of how Substrate works. It is Substrate that is supposed to conform to what this spec says. Linking to an issue saying that we want to remove the feature is IMO not only inappropriate (it places a judgement over how it works) but also doesn't remove the fact that child tries will continue to have to be supported and will forever be part of the Polkadot spec even if we remove its usage from Substrate.

retrieve child storage entries.

For example, calling `ext_storage_get_version_1` (<<sect-ext-storage-get>>) with
the key `:child_storage:default:some_child:some_key` is equivalent to calling
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that this is accurate. As far as I understand, you can only call ext_storage_get_version_1 on :child_storage:default:some_child in order to get the root hash of the child trie, but you can't get the keys of the child trie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document writing/clear_prefix behavior w.r.t. child tries
2 participants