move storage maps to blake2_128

[gui1117]

This PR point to v1.0

DONE:

• remove twox_128 default trie hashing in support/storage
• introduce twox_128 and blake2_256 modules in support/storage
• move all maps to blake2_256 modules
• make doublemap first key+prefix using blake2_256
• benchmarks

TODO:

• finish audit of twox_128 as in Usage of twox_128 / XxHash  #1868
• think about breakage

[bkchr]

After a first quick review, do we really need to replicate all this storage stuff? Could we not just take the hashing function as argument for the storage functions?

[gui1117]

hmm yes it seems some refactor are doable:

• at least we can refactor definiton of storages traits:
  write StorageTrait<T: Fn> with type TwoxStorage= StorageTrait<twox_128> same blake2 and unhashed.
• if you are thinking about refactoring storage::twox_128::put get, kill things then it is harder as they are just functions and I don't think we can make type alias with functions
  also we could change it to be method on a structure.

[kirushik]

If 128 bits were decided to be enough before — I think it would even be OK to use blake2_128 instead of 256-bit variant (we have that defined as well, AFAICT).
But if I understand it correctly, the performance difference between 128 and 256-bit variants should be minimal, compared to xxHash vs BLAKE2 difference.

So if we're OK with having our storage tries to be of a larger size now — than this issue indeed solves #1868.

[gui1117]

also cargo run -- --dev:

[thiolliere@localhost substrate]$ cargo run -- --dev
    Finished dev [unoptimized + debuginfo] target(s) in 0.50s
     Running `target/debug/substrate --dev`
2019-04-12 19:21:47 Substrate Node
2019-04-12 19:21:47   version 1.0.0-c24960320-x86_64-linux-gnu
2019-04-12 19:21:47   by Parity Technologies, 2017-2019
2019-04-12 19:21:47 Chain specification: Development
2019-04-12 19:21:47 Node name: null-pail-1977
2019-04-12 19:21:47 Roles: AUTHORITY
Error: Error(Client(Msg("State database error: Error decoding slicable value")), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })

[bkchr]

* if you are thinking about refactoring `storage::twox_128::put` get, kill things then it is harder as they are just functions and I don't think we can make type alias with functions
  also we could change it to be method on a structure.

mod storage {
    fn put(key: &[u8], key_hash: &fn(&[u8]) -> Vec<u8>, value: &[u8]) {}
}

That is what I had in my mind.

[bkchr]

also cargo run -- --dev:

[thiolliere@localhost substrate]$ cargo run -- --dev
    Finished dev [unoptimized + debuginfo] target(s) in 0.50s
     Running `target/debug/substrate --dev`
2019-04-12 19:21:47 Substrate Node
2019-04-12 19:21:47   version 1.0.0-c24960320-x86_64-linux-gnu
2019-04-12 19:21:47   by Parity Technologies, 2017-2019
2019-04-12 19:21:47 Chain specification: Development
2019-04-12 19:21:47 Node name: null-pail-1977
2019-04-12 19:21:47 Roles: AUTHORITY
Error: Error(Client(Msg("State database error: Error decoding slicable value")), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })

Did you purge your db?

[gui1117]

Did you purge your db?

oh no 🤦: here the result

2019-04-13 22:24:22 Substrate Node
2019-04-13 22:24:22   version 1.0.0-c24960320-x86_64-linux-gnu
2019-04-13 22:24:22   by Parity Technologies, 2017-2019
2019-04-13 22:24:22 Chain specification: Development
2019-04-13 22:24:22 Node name: delicate-iron-9744
2019-04-13 22:24:22 Roles: AUTHORITY
2019-04-13 22:24:24 Initializing Genesis block/state (state: 0x901c…6849, header-hash: 0xf24b…6ec5)
2019-04-13 22:24:24 Loaded block-time = 4 seconds from genesis on first-launch
2019-04-13 22:24:24 Loading GRANDPA authority set from genesis on what appears to be first startup.
2019-04-13 22:24:24 Best block: #0
2019-04-13 22:24:24 Local node address is: /ip4/0.0.0.0/tcp/30333/p2p/QmVKy9pTgkkghMfa3gw9yRFMdcbETpReAdXDGRhv6tkQEG
2019-04-13 22:24:24 Libp2p => Random Kademlia query has yielded empty results
2019-04-13 22:24:24 Listening for new connections on 127.0.0.1:9944.
2019-04-13 22:24:24 Using authority key 5FA9nQDVg267DEd8m1ZypXLBnvN7SFxYwV7ndqSYGiN9TmTd
2019-04-13 22:24:24 Running Grandpa session as Authority 5FA9nQDVg267DEd8m1ZypXLBnvN7SFxYwV7ndqSYGiN9TmTd
2019-04-13 22:24:27 Libp2p => Random Kademlia query has yielded empty results
2019-04-13 22:24:28 Starting consensus session on top of parent 0xf24b8c6bcdf152cde8e8c03e47c939e818c46d3bc889654f371076b6003c6ec5
2019-04-13 22:24:28 Prepared block for proposing at 1 [hash: 0x970163e31668b929c2f8bc2013901d6e3562a72e828d57308adb9fc06f851235; parent_hash: 0xf24b…6ec5; extrinsics: [0xc258…1471]]
2019-04-13 22:24:28 Pre-sealed block for proposal at 1. Hash now 0xe0c89f52f380b7f04af5d69a5a0b46c6c2b3c2b916acaaa1795477096bd171cf, previously 0x970163e31668b929c2f8bc2013901d6e3562a72e828d57308adb9fc06f851235.
2019-04-13 22:24:28 Imported #1 (0xe0c8…71cf)
2019-04-13 22:24:29 Idle (0 peers), best: #1 (0xe0c8…71cf), finalized #1 (0xe0c8…71cf), ⬇ 0 ⬆ 0
2019-04-13 22:24:31 Libp2p => Random Kademlia query has yielded empty results
2019-04-13 22:24:32 Starting consensus session on top of parent 0xe0c89f52f380b7f04af5d69a5a0b46c6c2b3c2b916acaaa1795477096bd171cf
2019-04-13 22:24:32 Prepared block for proposing at 2 [hash: 0x76462f552cfc77847aecc645dcdfc9bae61eddabae30e1e7b21fc0047bb919f4; parent_hash: 0xe0c8…71cf; extrinsics: [0xd400…98b8, 0x20ee…7959]]
2019-04-13 22:24:32 Pre-sealed block for proposal at 2. Hash now 0x9566f9b95f8ad21812681aee8d8a6762b5f0355a0a56919b69619377c0be9945, previously 0x76462f552cfc77847aecc645dcdfc9bae61eddabae30e1e7b21fc0047bb919f4.
2019-04-13 22:24:32 Imported #2 (0x9566…9945)
2019-04-13 22:24:34 Idle (0 peers), best: #2 (0x9566…9945), finalized #2 (0x9566…9945), ⬇ 0 ⬆ 0
2019-04-13 22:24:36 Starting consensus session on top of parent 0x9566f9b95f8ad21812681aee8d8a6762b5f0355a0a56919b69619377c0be9945
2019-04-13 22:24:36 Prepared block for proposing at 3 [hash: 0x3222c40a0bf26dc6dd161d2f776d75f633887ef9ca80545728b327b904b0582d; parent_hash: 0x9566…9945; extrinsics: [0x572e…47db, 0x46a6…ca5b]]
2019-04-13 22:24:36 Pre-sealed block for proposal at 3. Hash now 0xb69e4e96dca7675ac1301a0ee23e4b994951ac184b1d8b3ec1de276a846c5430, previously 0x3222c40a0bf26dc6dd161d2f776d75f633887ef9ca80545728b327b904b0582d.

[gui1117]

mod storage {
    fn put(key: &[u8], key_hash: &fn(&[u8]) -> Vec<u8>, value: &[u8]) {}
}

That is what I had in my mind.

Yes ok, I'll go for that, (you mean get ride of all of specialized one isn't it ? not making all specialized pointing to this one)

[bkchr]

Yep, getting rid of all the specializations.

[gui1117]

mod storage {
    fn put(key: &[u8], key_hash: &fn(&[u8]) -> Vec<u8>, value: &[u8]) {}
}

That is what I had in my mind.

Also we could just write

mod storage {
    fn get(key: impl AsRef<[u8]>) {}
}

and let user hash his key. Maybe your proposition put more insistance into using a hasher which is better

[gui1117]

to be honest I can do something like that:

/// Return the value of the item in storage under `key`, or `None` if there is no explicit entry.
pub fn get<T, HashFn, HashType>(hash: &HashFn, key: &[u8]) -> Option<T>
where
	T: Decode + Sized,
	HashFn: Fn(&[u8]) -> HashType,
	HashType: AsRef<[u8]>,
{
	runtime_io::read_storage(hash(key).as_ref(), &mut [0; 0][..], 0).map(|_| {
		let mut input = IncrementalInput {
			key,
			pos: 0,
		};
		Decode::decode(&mut input).expect("storage is not null, therefore must be a valid type")
	})
}

but that feels weird to me for example kill_prefix doesn't really make sense.

[gui1117]

I would prefer to go without hash functions. and people to hash themself actually

[bkchr]

If we go the way with requiring the user to hash its key before, we should probably merge this into master.

but that feels weird to me for example kill_prefix doesn't really make sense.

kill_prefix does not makes any sense with hashed keys and we also don't support this.

And I don't understand your example. First for the hash function pointer, we don't require a generic parameter. This function pointer could be defined as a type once and used in every.

pub fn get<T: Decode + Sized>(key: &[u8]) -> Option<T> {
	unhashed::get(&twox_128(key))
}

This is the current implementation of storage::get which would look like:

pub fn get<T: Decode + Sized>(key: &[u8], hash: HashFn) -> Option<T> {
	unhashed::get(&hash(key))
}

I don't know where you took your example from.

[gui1117]

it now uses blake2_128, for doing I created ext_blake2_128 call.

still have to do refactoring. But benchmark can be started now.

[bkchr]

Hmm, if we need to introduce an ext_* call for doing blake2, we maybe need to rethink the whole interface. @pepyakin do you have some rough estimation what it us cost for doing an ext_* call from wasm?

(In comparison to a normal function call in wasm)

[gui1117]

Hmm, if we need to introduce an ext_* call for doing blake2, we maybe need to rethink the whole interface. @pepyakin do you have some rough estimation what it us cost for doing an ext_* call from wasm?

(In comparison to a normal function call in wasm)

I think no, we were using ext_twox_128 before and ext_blake2_256 in the first implementation of this PR. Just I introduce it because it wasn't there. But nothing has really changed.

[bkchr]

Okay, then forget what I said. I thought that twox_128 did not required an ext_ call. Sorry!

[gui1117]

oh also it breaks UI I think no ?

[bkchr]

Yes this will break the gui.

[gavofyork]

@jacogr note ^^^

[gui1117]

all check have passed, this move all maps to blake2_256,
I manually checked other hasher but haven't write test.

also this update metadata

[gui1117]

ok I merged twox128concat reviewed by Basti and move to twox64concat by introducing ext_twox_64

[gui1117]

blake2_128 previously introduced is not really needed anymore but maybe we can keep it