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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3803305
expand disclaimer, reference from relevant sections
lamafab Nov 7, 2022
44c39d1
add section on Substrate assumptions
lamafab Nov 7, 2022
a4a95c5
resentence, expand a little
lamafab Nov 7, 2022
b0c3337
expand on child storage root behavior
lamafab Nov 7, 2022
d715c23
formatting
lamafab Nov 7, 2022
0320fc0
update sentence
lamafab Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ab_host-api/child_storage.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@

Interface for accessing the child storage from within the runtime.

IMPORTANT: Do note that this API must implement some specific behaviors,
described further in <<sect-storage-assumptions>>

[#defn-child-storage-type]
.<<defn-child-storage-type, Child Storage>>
====
Expand All @@ -25,6 +28,7 @@ child storage key (<<defn-child-storage-type>>).
* `key`: a pointer-size (<<defn-runtime-pointer-size>>) to the key.
* `value`: a pointer-size (<<defn-runtime-pointer-size>>) to the value.

[#sect-ext-default-child-storage-get]
==== `ext_default_child_storage_get`
Retrieves the value associated with the given key from the child storage.

Expand Down Expand Up @@ -212,6 +216,7 @@ where _0_ indicates that all keys of the child storage have been removed,
followed by the number of removed keys, stem:[c]. The variant _1_ indicates that
there are remaining keys, followed by the number of removed keys.

[#ext-default-child-storage-root]
==== `ext_default_child_storage_root`

Commits all existing operations and computes the resulting child storage
Expand Down
57 changes: 52 additions & 5 deletions ab_host-api/storage.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,47 @@

Interface for accessing the storage from within the runtime.

IMPORTANT: As of now, the storage API should silently ignore any keys that start
with the `:child_storage:default:` prefix. This applies to reading and writing.
If the function expects a return value, then _None_ (<<defn-option-type>>)
should be returned. See
https://github.com/paritytech/substrate/issues/12461[substrate issue #12461].
[#sect-storage-assumptions]
==== Implementation Assumptions
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
Comment on lines +8 to +12
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 •
edited
Loading

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?

namespaced respectively segregated by attaching the `:child_storage:default:`
prefix to every child storage keys, followed by `:<KEY>` for the key within that
child storage. However, the storage API described in this section and the child
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 •
edited
Loading

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?

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.

`ext_default_child_storage_get_version_1`
(<<sect-ext-default-child-storage-get>>) with child storage key `some_child` and
key `some_key`.

Importantly, the storage API can only _read_ child storage entries, but must
implement write protection. Any function that modifies data must **silently ignore**
any keys that start with the `:child_storage:` prefix. For
`ext_storage_clear_prefix` (<<sect-ext-storage-clear-prefix>>) specifically,
Comment on lines +27 to +28
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?

this also applies to any key that is a substring of the `:child_storage:` prefix,
such as `:child_sto` (but not `:other`, for example). If the function expects a
return value, then _None_ (<<defn-option-type>>) should be returned.

Additionally, calling `ext_storage_get_version_1` on a child storage key
directly (without a key within the child storage), such as
`:child_storage:defaut:some_child`, the function returns the root of the child
storage from when `ext_default_child_storage_root`
(<<ext-default-child-storage-root>>) has been **last called** or _None_
(<<defn-option-type>>) if it has never been called. Respectively, that root
value is cached. Calling `ext_default_child_storage_root` directly always
recomputes the current root value.

See the following issues for more information:

* https://github.com/w3f/polkadot-spec/issues/575
* https://github.com/w3f/polkadot-spec/issues/577
* https://github.com/paritytech/substrate/issues/12461
Comment on lines +44 to +46
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.


[#defn-state-version]
.<<defn-state-version, State Version>>
Expand All @@ -29,6 +65,9 @@ merkle proof (<<defn-hashed-subvalue>>).
==== `ext_storage_set`
Sets the value under a given key into storage.

IMPORTANT: Do note that this API must implement some specific behaviors,
described further in <<sect-storage-assumptions>>

===== Version 1 - Prototype
----
(func $ext_storage_set_version_1
Expand All @@ -40,9 +79,13 @@ Arguments::
* `value`: a pointer-size (<<defn-runtime-pointer-size>>) containing the
value.

[#sect-ext-storage-get]
==== `ext_storage_get`
Retrieves the value associated with the given key from storage.

IMPORTANT: Do note that this API must implement some specific behaviors,
described further in <<sect-storage-assumptions>>

===== Version 1 - Prototype
----
(func $ext_storage_get_version_1
Expand Down Expand Up @@ -107,11 +150,15 @@ Arguments::
* `return`: an i32 integer value equal to _1_ if the key exists or a value equal
to _0_ if otherwise.

[#sect-ext-storage-clear-prefix]
==== `ext_storage_clear_prefix`

Clear the storage of each key/value pair where the key starts with the given
prefix.

IMPORTANT: Do note that this API must implement some specific behaviors,
described further in <<sect-storage-assumptions>>

===== Version 1 - Prototype
----
(func $ext_storage_clear_prefix_version_1
Expand Down