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

Default keys to something enumerable #4610

Open
gavofyork opened this issue Jan 13, 2020 · 5 comments
Open

Default keys to something enumerable #4610

gavofyork opened this issue Jan 13, 2020 · 5 comments

Comments

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Jan 13, 2020

Right now, we default to using Blake2-256 for maps and double-maps. This guarantees security but comes at a cost of being unable to enumerate the keys (we can enumerate the values but only the hashes of the keys).

We should replace this default with Blake2-128-Concat. This provides 128-bits of security against trie-unbalancing attacks which should be just as secure as Blake2-256 since we were never going to be worrying about all 256 bits being equal, while at the same time, is secure against birthday attacks as it concatenates the preimage and also (for the same reason) includes the key.

First PR (pre-2.0):

  • Alter the pallets that are relying on defaults to explicitly specify Blake2-256 hasher.
  • Change the default to Blake2-128-Concat

Second PR (post-2.0)

  • Prepare migration code to allow Blake2-256-based maps to be translated to Blake-128-Concat. This will be a multi-phase migration where initially the key preimages of the origin Blake2-256 map are injected, then later (in an initialize_block, along with an upgrade) they are moved wholesale into the Blake2-128-Concat map and the original map destroyed.
@gavofyork gavofyork added this to the 2.0 milestone Jan 13, 2020
@gavofyork gavofyork added this to To Do in Runtime via automation Jan 13, 2020
@bkchr

This comment has been minimized.

Copy link
Contributor

@bkchr bkchr commented Jan 13, 2020

Why not provide some new parameter to a decl storage line that stores the key with the value in a tuple? For the user it would make no difference, up to the point where he wants to iterate the keys. We could implement an extra trait, if the new attribute is set.

@gavofyork

This comment has been minimized.

Copy link
Member Author

@gavofyork gavofyork commented Jan 13, 2020

that's another way, yeah. in both cases there should be no significant change for the user (in both cases, the default should be to include the key and the user should need to take action for anything else).

this way is slightly more space efficient (fewer bytes stored as it's 16 + len(key) + len(value) rather than 32 + len(key) + len(value) and it may be slightly faster since we're only computing a hash of half the size.

@gavofyork gavofyork modified the milestones: 2.0, 3.0 Jan 15, 2020
@thiolliere thiolliere self-assigned this Jan 27, 2020
@thiolliere

This comment has been minimized.

Copy link
Contributor

@thiolliere thiolliere commented Jan 27, 2020

I think it should be better make the transition be removing default hasher and reintroducing later.
Otherwise once this PR is merged, some ppl could merge master without seeing this change no ?

@thiolliere thiolliere removed their assignment Jan 29, 2020
@thiolliere

This comment has been minimized.

Copy link
Contributor

@thiolliere thiolliere commented Jan 29, 2020

adding the new default can be done whenever people think it is ready, the change is very similar to the code before the PR that removed the default, you just have to change 2 lines in the parsing in decl_storage

@bkchr

This comment has been minimized.

Copy link
Contributor

@bkchr bkchr commented Jan 29, 2020

I think we should just stick to no default hasher as it is done now. Being explicit for this is probably better anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Runtime
  
To Do
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.