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

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Nov 1, 2019

Context

Prefixed map is a new storage available in decl_storage.

it differ from map as it stores it keys at twox_256(prefix)++map_hasher(key) instead of map_hasher(prefix ++ key).

Thus it allows to remove all key of the map using sr-io::kill_prefix.

But the final goal is to make it also iterable using #3883

In the end this should replace linked_map as iteration is more efficient (just there is no longer ordering). and also Maps as it allows to have more functionality

To discuss:

  • should we directly put this in a child trie instead of behind a prefix in top trie ?

  • the value is stored at twox_256(prefix)++map_hasher(key):

    • the first part is using twox_256 for hashing the prefix because we have to be sure that no other value share the same prefix. in case we only use twox_128 then an attacker could try to craft a key which have blake2_256(key)[0..16] == twox_128(prefix). Thus inserting a wrong value into this map.

      Introducing this map has the consequence to limit the size of our hashers. introducing a hasher with an output longer than 256 bit is no longer relevant as it can already collision with this map.

      If this is an issue then it can be solve in two way:

      • waiting for child trie
      • allow to remove key with a given prefix and a given size only. thus we would call sr-io::kill_prefix_with_size(MAP_PREFIX, 32+size_of_map_hasher)
    • the second part is map_hasher(key). currently a hasher is mandatory though it seems it could be interesting to introduce a no_hasher variant (or hasher=identity). thus value would be stored at twox_256(prefix) ++ key.encode(). The only interest I see from this is that it would avoid some hasher call.

Prefixed map is a new storage available in decl_storage, it differ from
map as it stores it keys at `twox_256(prefix)++map_hasher(key)` instead
of `map_hasher(prefix ++ key)`. Thus it allows to remove all key of the
map using `sr-io::kill_prefix`, but the final goal is to also allow to
iter on the map once sr-io make such call available.
@xlc
Copy link
Contributor

xlc commented Nov 2, 2019

This looks like double_map with with first key of ()?

EventTopics get(fn event_topics): double_map hasher(blake2_256) (), blake2_256(T::Hash)

@gavofyork
Copy link
Member

It's not so great to have a long, insecure hash as the first key as it's not inconceivable that someone could craft a module which intentionally has storage keys that collided with another module. A better way of forming the first key could be to simply append a short twox hash (8 byte, perhaps) of the item name to either the module name or a twox hash of it. This more or less guarantees it being unrealistic to craft a storage item collision, at least making it super-obvious in the module name.

@gui1117
Copy link
Contributor Author

gui1117 commented Nov 13, 2019

So finally we will do instead of a new storage type just an optional attribute on linked_map

MyLinkedMap prefixed(): linked_map u32 => u32

stored at twox(ModuleName)++twox(MyLinkedMap)++hasher(key).
This would implemente the same trait StorageLinkedMap (though with change of the generator to have this two prefixes), also in the metadata we need a new field for the additional prefix as well (or a complete new storage variant)

(Or otherwise we could also make use of blake2hasher if we can ensure this is done at compile time but I prefer the first way)

I'll work on this starting monday next week after my days off

@gavofyork
Copy link
Member

sounds good.

@bkchr
Copy link
Member

bkchr commented Nov 15, 2019

As we want to remove LinkedMap in the future anyway. I would propose that you add the new prefixed() attribute to StorageMap.
If it is a prefixed StorageMap, we also implement Iterator or a custom trait to make it iterable like a LinkedMap.

@gavofyork
Copy link
Member

how far off is this? i could do with it for a PR i'm working on...

@gavofyork
Copy link
Member

in particular, i could do with iteration on a double_map - is that already prototyped?

@gui1117
Copy link
Contributor Author

gui1117 commented Nov 20, 2019

for double_map you want to iterate over all values?

I can implement Iterator<Item=(Key1, Key2, Value)> similarly to the incoming prefixed map it seems.

But if you want to iterate on first keys only it seems it requires new function in sr-io.

@xlc xlc mentioned this pull request Nov 22, 2019
@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 22, 2019
@gui1117
Copy link
Contributor Author

gui1117 commented Nov 22, 2019

superseeded by #4185

@gui1117 gui1117 closed this Nov 22, 2019
@gui1117 gui1117 deleted the gui-prefixed-map branch November 22, 2019 13:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-in_progress Pull request is in progress. No review needed at this stage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants