Fix key collision for child trie (#4162)

* In progress, runtime io must switch to future proof root +
child_specific (unique id) + u32 type.

* Switch interface, sr-io seems ok, rpc could use similar interface to
sr-io, genesis json broken if there is child trie in existing encoding
genesis.

* test from previous implementation.

* fix proving test.

* Restore Keyspacedb from other branch, only apply to child trie.

* Removing unneeded child_info from child root (child info are stored
if things changed, otherwhise the root does not change).

* Switch rpc to use same format as ext: more future proof.

* use root from child info for trie backend essence.

* Breaking long lines.

* Update doc and clean pr a bit.

* fix error type

* Restore removed doc on merge and update sr-io doc.

* Switch child storage api to use directly unique id, if managed id
where to be put in place, the api will change at this time.

* Clean deprecated host interface from child.

* Removing assertion on child info (can fail depending on root
memoization).

* merging child info in the overlay when possible.

* child iteration by prefix using child_info.

* Using ChainInfo in frame support. ChainInfo gets redesign to avoid
buffers allocation on every calls.

* Add length of root to the data of child info.

* comments

* Encode compact.

* Remove child info with root.

* Fix try_update condition.

* Comment Ext child root caching.

* Replace tuples by struct with field

* remove StorageTuple alias.

* Fix doc tests, and remove StorageOverlay and ChildStorageOverlay
aliases.

substrate/client/src/cht.rs

@@ -98,9 +98,9 @@ pub fn build_proof<Header, Hasher, BlocksI, HashesI>(
 {
 	let transaction = build_pairs::<Header, _>(cht_size, cht_num, hashes)?
 		.into_iter()
-		.map(|(k, v)| (None, k, Some(v)))
+		.map(|(k, v)| (k, Some(v)))
 		.collect::<Vec<_>>();
-	let mut storage = InMemoryState::<Hasher>::default().update(transaction);
+	let mut storage = InMemoryState::<Hasher>::default().update(vec![(None, transaction)]);
 	let trie_storage = storage.as_trie_backend()
 		.expect("InMemoryState::as_trie_backend always returns Some; qed");
 	prove_read_on_trie_backend(

substrate/client/src/client.rs

@@ -28,7 +28,7 @@ use hash_db::{Hasher, Prefix};
 use primitives::{
 	Blake2Hasher, H256, ChangesTrieConfiguration, convert_hash,
 	NeverNativeValue, ExecutionContext, NativeOrEncoded,
-	storage::{StorageKey, StorageData, well_known_keys},
+	storage::{StorageKey, StorageData, well_known_keys, ChildInfo},
 	traits::CodeExecutor,
 };
 use sc_telemetry::{telemetry, SUBSTRATE_INFO};
@@ -199,10 +199,10 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		execution_extensions: ExecutionExtensions<Block>,
 	) -> sp_blockchain::Result<Self> {
 		if backend.blockchain().header(BlockId::Number(Zero::zero()))?.is_none() {
-			let (genesis_storage, children_genesis_storage) = build_genesis_storage.build_storage()?;
+			let genesis_storage = build_genesis_storage.build_storage()?;
 			let mut op = backend.begin_operation()?;
 			backend.begin_state_operation(&mut op, BlockId::Hash(Default::default()))?;
-			let state_root = op.reset_storage(genesis_storage, children_genesis_storage)?;
+			let state_root = op.reset_storage(genesis_storage)?;
 			let genesis_block = genesis::construct_genesis_block::<Block>(state_root.into());
 			info!("Initializing Genesis block/state (state: {}, header-hash: {})",
 				genesis_block.header().state_root(),
@@ -267,10 +267,11 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		&self,
 		id: &BlockId<Block>,
 		child_storage_key: &StorageKey,
+		child_info: ChildInfo,
 		key_prefix: &StorageKey
 	) -> sp_blockchain::Result<Vec<StorageKey>> {
 		let keys = self.state_at(id)?
-			.child_keys(&child_storage_key.0, &key_prefix.0)
+			.child_keys(&child_storage_key.0, child_info, &key_prefix.0)
 			.into_iter()
 			.map(StorageKey)
 			.collect();
@@ -281,11 +282,13 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 	pub fn child_storage(
 		&self,
 		id: &BlockId<Block>,
-		child_storage_key: &StorageKey,
+		storage_key: &StorageKey,
+		child_info: ChildInfo,
 		key: &StorageKey
 	) -> sp_blockchain::Result<Option<StorageData>> {
 		Ok(self.state_at(id)?
-			.child_storage(&child_storage_key.0, &key.0).map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))?
+			.child_storage(&storage_key.0, child_info, &key.0)
+			.map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))?
 			.map(StorageData))
 	}
 
@@ -293,11 +296,13 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 	pub fn child_storage_hash(
 		&self,
 		id: &BlockId<Block>,
-		child_storage_key: &StorageKey,
+		storage_key: &StorageKey,
+		child_info: ChildInfo,
 		key: &StorageKey
 	) -> sp_blockchain::Result<Option<Block::Hash>> {
 		Ok(self.state_at(id)?
-			.child_storage_hash(&child_storage_key.0, &key.0).map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))?
+			.child_storage_hash(&storage_key.0, child_info, &key.0)
+			.map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))?
 		)
 	}
 
@@ -334,13 +339,14 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		&self,
 		id: &BlockId<Block>,
 		storage_key: &[u8],
+		child_info: ChildInfo,
 		keys: I,
 	) -> sp_blockchain::Result<StorageProof> where
 		I: IntoIterator,
 		I::Item: AsRef<[u8]>,
 	{
 		self.state_at(id)
-			.and_then(|state| prove_child_read(state, storage_key, keys)
+			.and_then(|state| prove_child_read(state, storage_key, child_info, keys)
 				.map_err(Into::into))
 	}
 
@@ -1012,7 +1018,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 				overlay.commit_prospective();
 
 				let (top, children) = overlay.into_committed();
-				let children = children.map(|(sk, it)| (sk, it.collect())).collect();
+				let children = children.map(|(sk, it)| (sk, it.0.collect())).collect();
 				if import_headers.post().state_root() != &storage_update.1 {
 					return Err(sp_blockchain::Error::InvalidStateRoot);
 				}

substrate/client/src/genesis.rs

@@ -53,7 +53,7 @@ mod tests {
 		runtime::{Hash, Transfer, Block, BlockNumber, Header, Digest},
 		AccountKeyring, Sr25519Keyring,
 	};
-	use primitives::{Blake2Hasher, map};
+	use primitives::Blake2Hasher;
 	use hex_literal::*;
 
 	native_executor_instance!(
@@ -154,8 +154,7 @@ mod tests {
 			vec![AccountKeyring::One.into(), AccountKeyring::Two.into()],
 			1000,
 			None,
-			map![],
-			map![],
+			Default::default(),
 		).genesis_map();
 		let genesis_hash = insert_genesis_block(&mut storage);
 
@@ -183,8 +182,7 @@ mod tests {
 			vec![AccountKeyring::One.into(), AccountKeyring::Two.into()],
 			1000,
 			None,
-			map![],
-			map![],
+			Default::default(),
 		).genesis_map();
 		let genesis_hash = insert_genesis_block(&mut storage);
 
@@ -212,8 +210,7 @@ mod tests {
 			vec![AccountKeyring::One.into(), AccountKeyring::Two.into()],
 			68,
 			None,
-			map![],
-			map![],
+			Default::default(),
 		).genesis_map();
 		let genesis_hash = insert_genesis_block(&mut storage);
 

substrate/client/src/in_mem.rs

@@ -25,7 +25,7 @@ use primitives::offchain::storage::{
 };
 use sp_runtime::generic::{BlockId, DigestItem};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT, Zero, NumberFor};
-use sp_runtime::{Justification, StorageOverlay, ChildrenStorageOverlay};
+use sp_runtime::{Justification, Storage};
 use state_machine::backend::{Backend as StateBackend, InMemory};
 use state_machine::{self, InMemoryChangesTrieStorage, ChangesTrieAnchorBlockId, ChangesTrieTransaction};
 use hash_db::{Hasher, Prefix};
@@ -505,15 +505,15 @@ where
 		Ok(())
 	}
 
-	fn reset_storage(&mut self, top: StorageOverlay, children: ChildrenStorageOverlay) -> sp_blockchain::Result<H::Out> {
-		check_genesis_storage(&top, &children)?;
+	fn reset_storage(&mut self, storage: Storage) -> sp_blockchain::Result<H::Out> {
+		check_genesis_storage(&storage)?;
 
-		let child_delta = children.into_iter()
-			.map(|(storage_key, child_overlay)|
-				(storage_key, child_overlay.into_iter().map(|(k, v)| (k, Some(v)))));
+		let child_delta = storage.children.into_iter()
+			.map(|(storage_key, child_content)|
+				(storage_key, child_content.data.into_iter().map(|(k, v)| (k, Some(v))), child_content.child_info));
 
 		let (root, transaction) = self.old_state.full_storage_root(
-			top.into_iter().map(|(k, v)| (k, Some(v))),
+			storage.top.into_iter().map(|(k, v)| (k, Some(v))),
 			child_delta
 		);
 
@@ -796,12 +796,12 @@ impl<Block, H> state_machine::ChangesTrieStorage<H, NumberFor<Block>> for Change
 }
 
 /// Check that genesis storage is valid.
-pub fn check_genesis_storage(top: &StorageOverlay, children: &ChildrenStorageOverlay) -> sp_blockchain::Result<()> {
-	if top.iter().any(|(k, _)| well_known_keys::is_child_storage_key(k)) {
+pub fn check_genesis_storage(storage: &Storage) -> sp_blockchain::Result<()> {
+	if storage.top.iter().any(|(k, _)| well_known_keys::is_child_storage_key(k)) {
 		return Err(sp_blockchain::Error::GenesisInvalid.into());
 	}
 
-	if children.keys().any(|child_key| !well_known_keys::is_child_storage_key(&child_key)) {
+	if storage.children.keys().any(|child_key| !well_known_keys::is_child_storage_key(&child_key)) {
 		return Err(sp_blockchain::Error::GenesisInvalid.into());
 	}
 

substrate/client/src/lib.rs

@@ -48,7 +48,7 @@
 //! use std::sync::Arc;
 //! use sc_client::{Client, in_mem::Backend, LocalCallExecutor};
 //! use primitives::Blake2Hasher;
-//! use sp_runtime::{StorageOverlay, ChildrenStorageOverlay};
+//! use sp_runtime::Storage;
 //! use executor::{NativeExecutor, WasmExecutionMethod};
 //!
 //! // In this example, we're using the `Block` and `RuntimeApi` types from the
@@ -65,7 +65,7 @@
 //! 		NativeExecutor::<LocalExecutor>::new(WasmExecutionMethod::Interpreted, None),
 //!		),
 //! 	// This parameter provides the storage for the chain genesis.
-//! 	<(StorageOverlay, ChildrenStorageOverlay)>::default(),
+//! 	<Storage>::default(),
 //! 	Default::default(),
 //! 	Default::default(),
 //! );

substrate/client/src/light/backend.rs

@@ -21,11 +21,12 @@ use std::collections::HashMap;
 use std::sync::Arc;
 use parking_lot::RwLock;
 
+use primitives::storage::{ChildInfo, OwnedChildInfo};
 use state_machine::{
 	Backend as StateBackend, TrieBackend, backend::InMemory as InMemoryState, ChangesTrieTransaction
 };
 use primitives::offchain::storage::InMemOffchainStorage;
-use sp_runtime::{generic::BlockId, Justification, StorageOverlay, ChildrenStorageOverlay};
+use sp_runtime::{generic::BlockId, Justification, Storage};
 use sp_runtime::traits::{Block as BlockT, NumberFor, Zero, Header};
 use crate::in_mem::{self, check_genesis_storage};
 use sp_blockchain::{ Error as ClientError, Result as ClientResult };
@@ -280,22 +281,21 @@ where
 		Ok(())
 	}
 
-	fn reset_storage(&mut self, top: StorageOverlay, children: ChildrenStorageOverlay) -> ClientResult<H::Out> {
-		check_genesis_storage(&top, &children)?;
+	fn reset_storage(&mut self, input: Storage) -> ClientResult<H::Out> {
+		check_genesis_storage(&input)?;
 
 		// this is only called when genesis block is imported => shouldn't be performance bottleneck
-		let mut storage: HashMap<Option<Vec<u8>>, StorageOverlay> = HashMap::new();
-		storage.insert(None, top);
+		let mut storage: HashMap<Option<(Vec<u8>, OwnedChildInfo)>, _> = HashMap::new();
+		storage.insert(None, input.top);
 
 		// create a list of children keys to re-compute roots for
-		let child_delta = children.keys()
-			.cloned()
-			.map(|storage_key| (storage_key, None))
+		let child_delta = input.children.iter()
+			.map(|(storage_key, storage_child)| (storage_key.clone(), None, storage_child.child_info.clone()))
 			.collect::<Vec<_>>();
 
 		// make sure to persist the child storage
-		for (child_key, child_storage) in children {
-			storage.insert(Some(child_key), child_storage);
+		for (child_key, storage_child) in input.children {
+			storage.insert(Some((child_key, storage_child.child_info)), storage_child.data);
 		}
 
 		let storage_update: InMemoryState<H> = storage.into();
@@ -357,10 +357,15 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
 		}
 	}
 
-	fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> ClientResult<Option<Vec<u8>>> {
+	fn child_storage(
+		&self,
+		storage_key: &[u8],
+		child_info: ChildInfo,
+		key: &[u8],
+	) -> ClientResult<Option<Vec<u8>>> {
 		match *self {
 			GenesisOrUnavailableState::Genesis(ref state) =>
-				Ok(state.child_storage(storage_key, key).expect(IN_MEMORY_EXPECT_PROOF)),
+				Ok(state.child_storage(storage_key, child_info, key).expect(IN_MEMORY_EXPECT_PROOF)),
 			GenesisOrUnavailableState::Unavailable => Err(ClientError::NotAvailableOnLightClient),
 		}
 	}
@@ -373,10 +378,17 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
 		}
 	}
 
-	fn next_child_storage_key(&self, storage_key: &[u8], key: &[u8]) -> Result<Option<Vec<u8>>, Self::Error> {
+	fn next_child_storage_key(
+		&self,
+		storage_key: &[u8],
+		child_info: ChildInfo,
+		key: &[u8],
+	) -> Result<Option<Vec<u8>>, Self::Error> {
 		match *self {
-			GenesisOrUnavailableState::Genesis(ref state) =>
-				Ok(state.next_child_storage_key(storage_key, key).expect(IN_MEMORY_EXPECT_PROOF)),
+			GenesisOrUnavailableState::Genesis(ref state) => Ok(
+				state.next_child_storage_key(storage_key, child_info, key)
+					.expect(IN_MEMORY_EXPECT_PROOF)
+			),
 			GenesisOrUnavailableState::Unavailable => Err(ClientError::NotAvailableOnLightClient),
 		}
 	}
@@ -395,10 +407,15 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
 		}
 	}
 
-
-	fn for_keys_in_child_storage<A: FnMut(&[u8])>(&self, storage_key: &[u8], action: A) {
+	fn for_keys_in_child_storage<A: FnMut(&[u8])>(
+		&self,
+		storage_key: &[u8],
+		child_info: ChildInfo,
+		action: A,
+	) {
 		match *self {
-			GenesisOrUnavailableState::Genesis(ref state) => state.for_keys_in_child_storage(storage_key, action),
+			GenesisOrUnavailableState::Genesis(ref state) =>
+				state.for_keys_in_child_storage(storage_key, child_info, action),
 			GenesisOrUnavailableState::Unavailable => (),
 		}
 	}
@@ -406,12 +423,13 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
 	fn for_child_keys_with_prefix<A: FnMut(&[u8])>(
 		&self,
 		storage_key: &[u8],
+		child_info: ChildInfo,
 		prefix: &[u8],
 		action: A,
 	) {
 		match *self {
 			GenesisOrUnavailableState::Genesis(ref state) =>
-				state.for_child_keys_with_prefix(storage_key, prefix, action),
+				state.for_child_keys_with_prefix(storage_key, child_info, prefix, action),
 			GenesisOrUnavailableState::Unavailable => (),
 		}
 	}
@@ -427,13 +445,18 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
 		}
 	}
 
-	fn child_storage_root<I>(&self, key: &[u8], delta: I) -> (H::Out, bool, Self::Transaction)
+	fn child_storage_root<I>(
+		&self,
+		storage_key: &[u8],
+		child_info: ChildInfo,
+		delta: I,
+	) -> (H::Out, bool, Self::Transaction)
 	where
 		I: IntoIterator<Item=(Vec<u8>, Option<Vec<u8>>)>
 	{
 		match *self {
 			GenesisOrUnavailableState::Genesis(ref state) => {
-				let (root, is_equal, _) = state.child_storage_root(key, delta);
+				let (root, is_equal, _) = state.child_storage_root(storage_key, child_info, delta);
 				(root, is_equal, ())
 			},
 			GenesisOrUnavailableState::Unavailable => (H::Out::default(), true, ()),
@@ -478,7 +501,7 @@ mod tests {
 		let backend: Backend<_, Blake2Hasher> = Backend::new(Arc::new(DummyBlockchain::new(DummyStorage::new())));
 		let mut op = backend.begin_operation().unwrap();
 		op.set_block_data(header0, None, None, NewBlockState::Final).unwrap();
-		op.reset_storage(Default::default(), Default::default()).unwrap();
+		op.reset_storage(Default::default()).unwrap();
 		backend.commit_operation(op).unwrap();
 
 		match backend.state_at(BlockId::Number(0)).unwrap() {

substrate/client/src/light/fetcher.rs

@@ -339,11 +339,13 @@ pub mod tests {
 	use crate::light::fetcher::{FetchChecker, LightDataChecker, RemoteHeaderRequest};
 	use crate::light::blockchain::tests::{DummyStorage, DummyBlockchain};
 	use primitives::{blake2_256, Blake2Hasher, H256};
-	use primitives::storage::{well_known_keys, StorageKey};
+	use primitives::storage::{well_known_keys, StorageKey, ChildInfo};
 	use sp_runtime::generic::BlockId;
 	use state_machine::Backend;
 	use super::*;
 
+	const CHILD_INFO_1: ChildInfo<'static> = ChildInfo::new_default(b"unique_id_1");
+
 	type TestChecker = LightDataChecker<
 		NativeExecutor<test_client::LocalExecutor>,
 		Blake2Hasher,
@@ -394,8 +396,12 @@ pub mod tests {
 		use test_client::TestClientBuilderExt;
 		// prepare remote client
 		let remote_client = test_client::TestClientBuilder::new()
-			.add_extra_child_storage(b":child_storage:default:child1".to_vec(), b"key1".to_vec(), b"value1".to_vec())
-			.build();
+			.add_extra_child_storage(
+				b":child_storage:default:child1".to_vec(),
+				CHILD_INFO_1,
+				b"key1".to_vec(),
+				b"value1".to_vec(),
+			).build();
 		let remote_block_id = BlockId::Number(0);
 		let remote_block_hash = remote_client.block_hash(0).unwrap().unwrap();
 		let mut remote_block_header = remote_client.header(&remote_block_id).unwrap().unwrap();
@@ -406,12 +412,14 @@ pub mod tests {
 		let child_value = remote_client.child_storage(
 			&remote_block_id,
 			&StorageKey(b":child_storage:default:child1".to_vec()),
+			CHILD_INFO_1,
 			&StorageKey(b"key1".to_vec()),
 		).unwrap().unwrap().0;
 		assert_eq!(b"value1"[..], child_value[..]);
 		let remote_read_proof = remote_client.read_child_proof(
 			&remote_block_id,
 			b":child_storage:default:child1",
+			CHILD_INFO_1,
 			&[b"key1"],
 		).unwrap();
 
@@ -487,11 +495,14 @@ pub mod tests {
 			remote_read_proof,
 			result,
 		) = prepare_for_read_child_proof_check();
+		let child_infos = CHILD_INFO_1.info();
 		assert_eq!((&local_checker as &dyn FetchChecker<Block>).check_read_child_proof(
 			&RemoteReadChildRequest::<Header> {
 				block: remote_block_header.hash(),
 				header: remote_block_header,
 				storage_key: b":child_storage:default:child1".to_vec(),
+				child_info: child_infos.0.to_vec(),
+				child_type: child_infos.1,
 				keys: vec![b"key1".to_vec()],
 				retry_count: None,
 			},