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

Document storage key generating algorithm #2443

Closed
xlc opened this issue May 1, 2019 · 7 comments

Comments

@xlc
Copy link
Contributor

commented May 1, 2019

Is there any documents of how storage key is been generated?

I spent a lot of time debugging a double map breaking change caused by #2268.
I simply change the hasher for prefix + key 1 from twox128 to blake2 256 and it doesn't work.

I had a hard time to debug macro generated code trying to figure out why simply change the hasher is not enough and then finally found out an extra using_encoded is performed so I have to add length prefix. I don't know if this is intended because I cannot find any documents and spec about this.

The old algorithm for double map with twox128 for key 2 hasher:
twox128(encode(prefix) + encode(key1)) + twox128(key2)

The new algorithm for double map with twox128 for key 2 hasher:
blake2_256(encode(encode(prefix) + encode(key1))) + twox128(key2)

The extra encode seems redundant to me.

I also cannot find any tests for double map storage key generation. There should be some to ensure the generated key won't change unexpectedly.

@rphmeier

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@rphmeier rphmeier added this to the 1.x series milestone May 2, 2019

@thiolliere thiolliere self-assigned this May 2, 2019

@thiolliere

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

the documentation of decl_storage say:

$hash(module_name ++ storage_name ++ first_key) ++ $hash2(second_key)

it was intended to be understood as

$hash(module_name ++ storage_name ++ encode(first_key)) ++ $hash2(encode(second_key))

but you are write there is an unintended encoding which is very wrong as the tree will be unbalanced.

This diff should solve it:
EDIT: no it does not but the issue is here.

diff --git a/srml/support/procedural/src/storage/impls.rs b/srml/support/procedural/src/storage/impls.rs
index 2708fdb21..b9e6844ac 100644
--- a/srml/support/procedural/src/storage/impls.rs
+++ b/srml/support/procedural/src/storage/impls.rs
@@ -623,7 +623,7 @@ impl<'a, I: Iterator<Item=syn::Meta>> Impls<'a, I> {
                                fn prefix_for(k1: &#k1ty) -> Vec<u8> {
                                        let mut key = #as_double_map::prefix().to_vec();
                                        #scrate::codec::Encode::encode_to(k1, &mut key);
-                                       #scrate::Hashable::#hasher(&key).to_vec()
+                                       #scrate::Hashable::#hasher(&key[..]).to_vec()
                                }
 
                                fn prefix() -> &'static [u8] {

Though I have to test to be sure. And will update doc and make proper test, thanks

@thiolliere

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

there is no security issue just the encoding is not optimized

@thiolliere

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

so I implemented this way but otherwise we can also just update the doc to specify the current behavior as it doesn't harm that much. ?

@xlc

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Can someone make a decision on this so we know if there is going to be another breaking change in 1.0 branch or not.

@thiolliere

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I don't know about the decision to make in v1.0, but if it is not too late I vote for putting that in.

The PR is #2480

@thiolliere

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Can someone make a decision on this so we know if there is going to be another breaking change in 1.0 branch or not.

this is now merged on 1.0 branch with breaking change and documentation.

@thiolliere thiolliere closed this May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.