Multiple storage root support #902

sorpaas · 2018-10-12T11:17:25Z

This adds support for multiple storage root. The basic idea is that we compute multiple tries (maybe of different types) for the same runtime, and in the end, merge the changeset to commit the results. This allows children trie to get its own storage root.

The top-level storage contains a WELL_KNOWN_KEYS prefix :child_storage:, within which we store all child storage roots. A child storage root is updated whenever Ext::child_storage_root is called, or Ext::storage_root is called. Ext::place_storage and other mutation operations are forbidden to modify storage values under :child_storage:.

Currently it's a non-generic version, but it already works -- even for multiple trie types. (I'm hoping to get a generic version, together with generic tries, in a separate PR.) The trick is that one can specify [patch] for substrate-trie to overwrite the default implementation, and then write a customized child_delta_trie_root. The idea is that that function should dispatch and use different trie types to compute the transaction based on the child "storage key". A customized implementation can also use is_child_trie_key_valid to make it so that only a subset of the :child_storage: namespace is valid.

For the default implementation right now in substrate-trie, it builds all child tries using the same trie type.

bkchr · 2018-10-15T07:21:26Z

core/state-machine/src/ext.rs

+	/// Fetch child storage root together with its transaction.
+	fn child_storage_root_transaction(&mut self, storage_key: &[u8]) -> (Vec<u8>, B::Transaction) {
+		self.mark_dirty();
+		let storage_key = storage_key.to_vec();


Why do you convert it to an Vec at this position?
The Vec is required in sync_child_storage_root and this function clones the incoming value. So, we make one allocation that is not required at all. If you would call to_vec in sync_child_storage_root instead of clone(). You could reduce the number of allocations to 1.

bkchr · 2018-10-15T07:30:55Z

core/state-machine/src/overlayed_changes.rs

+				.insert(extrinsic);
+		}
+
+		for (_, entry) in map_entry.1.iter_mut() {


Nitpick:
Could be changed to:
map_entry.1.values_mut().for_each(|e| e = None)

core/state-machine/src/overlayed_changes.rs

svyatonik

lgtm

svyatonik · 2018-10-18T07:25:32Z

core/state-machine/src/ext.rs

+				.into_iter()
+				.flat_map(|map| map.1.iter().map(|(k, v)| (k.clone(), v.clone())))
+				.chain(self.overlay.prospective.children.get(storage_key)
+					   .into_iter()


spaces -> tabs

bkchr · 2018-10-18T07:44:47Z

core/executor/src/wasm_executor.rs

+			if let Some(ref b) = maybe_value {
+				format!("{}", HexDisplay::from(b))
+			} else {
+				"<empty>".to_owned()


I think you can remove the .to_owned() when you change the first if branch to &format!("{}", HexDisplay::from(b)).
So you don't need to allocate in the else branch :)

bkchr · 2018-10-18T07:48:13Z

core/executor/src/wasm_executor.rs

+			if let Some(ref b) = maybe_value {
+				format!("{}", HexDisplay::from(b))
+			} else {
+				"<empty>".to_owned()


Same as below.

bkchr · 2018-10-18T07:48:58Z

core/executor/src/wasm_executor.rs

+		let key = this.memory.get(key_data, key_len as usize).map_err(|_| UserError("Invalid attempt to determine key in ext_set_child_storage"))?;
+		let value = this.memory.get(value_data, value_len as usize).map_err(|_| UserError("Invalid attempt to determine value in ext_set_child_storage"))?;
+		if let Some(_preimage) = this.hash_lookup.get(&key) {
+			debug_trace!(target: "wasm-trace", "*** Setting child storage: {} -> %{} -> {}   [k={}]", ::primitives::hexdisplay::ascii_format(&storage_key), ::primitives::hexdisplay::ascii_format(&_preimage), HexDisplay::from(&value), HexDisplay::from(&key));


Could you maybe break these two debug_trace! into multiple lines?

bkchr · 2018-10-18T07:51:46Z

core/sr-io/with_std.rs

+	ext::with(|ext| ext.child_storage(storage_key, key).map(|value| {
+		let value = &value[value_offset..];
+		let written = ::std::cmp::min(value.len(), value_out.len());
+		value_out[0..written].copy_from_slice(&value[0..written]);


Nitpick: You can remove the leading 0 in [0..written].

bkchr · 2018-10-18T07:56:31Z

core/state-machine/src/lib.rs

 	/// Set storage entry `key` of current contract being called (effective immediately).
 	fn set_storage(&mut self, key: Vec<u8>, value: Vec<u8>) {
 		self.place_storage(key, Some(value));
 	}

+	/// Set child storage entry `key` of current contract being called (effective immediately).
+	fn set_child_storage(&mut self, storage_key: Vec<u8>, key: Vec<u8>, value: Vec<u8>) -> bool {


Maybe taking just a slice of u8 for each of these values would be better.
You have cases calling this function, where the data is not required to be an Vec<u8> and so we could reduce the number allocations.

The only one I found where it may be redundant is kill_child_storage, which I think would be fine because it doesn't mean to be a fast operation.

I would actually hope to keep this to use Vec<u8> because it is a public interface. We also have set_storage and many other interface here that use Vec<u8>.

bkchr · 2018-10-18T08:10:39Z

core/sr-io/without_std.rs

+		if length == u32::max_value() {
+			None
+		} else {
+			Some(Vec::from_raw_parts(ptr, length as usize, length as usize))


Same here, as above.

core/state-machine/src/backend.rs

bkchr · 2018-10-18T08:19:59Z

core/state-machine/src/backend.rs

+		}
+		let root = match root {
+			Some(root) => root,
+			None => insert_into_memory_db::<H, _>(&mut mdb, HashMap::new().into_iter())?,


I think HashMap::new().into_iter() can be replaced by https://doc.rust-lang.org/std/iter/fn.empty.html

core/state-machine/src/ext.rs

… sp-multi-trie2

pepyakin · 2018-10-18T09:36:20Z

core/executor/src/wasm_executor.rs

+		Ok(())
+	},
+	ext_clear_child_storage(storage_key_data: *const u8, storage_key_len: u32, key_data: *const u8, key_len: u32) => {
+		let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_clear_child_storage"))?;


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-18T09:37:07Z

core/executor/src/wasm_executor.rs

@@ -196,11 +221,21 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
 		let key = this.memory.get(key_data, key_len as usize).map_err(|_| UserError("Invalid attempt to determine key in ext_exists_storage"))?;
 		Ok(if this.ext.exists_storage(&key) { 1 } else { 0 })
 	},
+	ext_exists_child_storage(storage_key_data: *const u8, storage_key_len: u32, key_data: *const u8, key_len: u32) -> u32 => {
+		let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_exists_child_storage"))?;


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-18T09:37:24Z

core/executor/src/wasm_executor.rs

 	ext_clear_prefix(prefix_data: *const u8, prefix_len: u32) => {
 		let prefix = this.memory.get(prefix_data, prefix_len as usize).map_err(|_| UserError("Invalid attempt to determine prefix in ext_clear_prefix"))?;
 		this.ext.clear_prefix(&prefix);
 		Ok(())
 	},
+	ext_kill_child_storage(storage_key_data: *const u8, storage_key_len: u32) => {
+		let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_kill_child_storage"))?;


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-18T09:37:50Z

core/executor/src/wasm_executor.rs

@@ -232,6 +267,39 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
 			Ok(0)
 		}
 	},
+	// return 0 and place u32::max_value() into written_out if no value exists for the key.
+	ext_get_allocated_child_storage(storage_key_data: *const u8, storage_key_len: u32, key_data: *const u8, key_len: u32, written_out: *mut u32) -> *mut u8 => {
+		let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_get_allocated_child_storage"))?;


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-18T09:38:15Z

core/executor/src/wasm_executor.rs

@@ -259,11 +327,55 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
 			Ok(u32::max_value())
 		}
 	},
+	// return u32::max_value() if no value exists for the key.
+	ext_get_child_storage_into(storage_key_data: *const u8, storage_key_len: u32, key_data: *const u8, key_len: u32, value_data: *mut u8, value_len: u32, value_offset: u32) -> u32 => {
+		let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_get_child_storage_into"))?;


It looks like this code is wider than 120 maximum. Can we wrap this code?

#856 (comment)

bkchr · 2018-10-18T10:31:47Z

core/sr-io/without_std.rs

+		if length == u32::max_value() {
+			None
+		} else {
+			Some(slice::from_raw_parts(ptr, length as usize).to_vec())


One last time ^^
@pepyakin can't we call ext_free here for freeing ptr?
If that not works, I would at least propose to add TODO over these lines that we do not forget these memory leaks.

Yes, this is the only possible way, as long as ext_* make allocations with it.

… sp-multi-trie2

bkchr

Looks good to me :)

sorpaas added 15 commits October 12, 2018 03:07

Implement a non generic version of child delta trie

ad8ecaa

Use delta_trie_root in state_machine

b4ae088

Expand InMemory backend to support multi-storage

134ad78

Create Consolidate trait

48040c7

Fix all crate compile and remove unused OverlayedChanges::drain

307406f

Implement child storage root support and overlay changes

252befb

Add child storage reader

bc88cd5

Add child storage writer

74c2c1e

Implement child storage cleaning

925cace

Fix light backend compile

f257e4c

Add all required ext functions for wasm executor

7664f74

Add ext def to io

ceb03a2

Add all io functions

073f8a3

Fix nostd compile

7b061ae

Add simple test

00e757b

sorpaas added the A0-please_review Pull request needs code review. label Oct 12, 2018

gavofyork requested a review from arkpar October 14, 2018 10:29

bkchr reviewed Oct 15, 2018

View reviewed changes

core/state-machine/src/overlayed_changes.rs Show resolved Hide resolved

sorpaas added 3 commits October 15, 2018 16:56

Remove unnecessary vec copy in child_storage_root_transaction

a450d4f

Use values_mut/for_each to make it shorter

8791a4c

Use extend to shorter a for loop

8708012

svyatonik approved these changes Oct 18, 2018

View reviewed changes

svyatonik added F1-panic and removed A0-please_review Pull request needs code review. F1-panic labels Oct 18, 2018

bkchr reviewed Oct 18, 2018

View reviewed changes

sorpaas added 8 commits October 18, 2018 16:43

Merge branch 'master' of https://github.com/paritytech/substrate into…

44c22d3

… sp-multi-trie2

Move record_all_keys to trie so it's easier to generic them later

5c8cc89

space -> tab

e4ec3a5

Remove to_owned in debug format

2cb62ba

Clean out all to_owned

33afc57

Break debug_trace to multiple lines

4d3ee4f

Remove 0..

7a2981c

UserError copy/paste typo

577e32e

pepyakin reviewed Oct 18, 2018

View reviewed changes

sorpaas added 2 commits October 18, 2018 17:46

Replace Vec::from_raw_parts by slice::from_raw_parts

81685c4

Use iter::empty()

d132d05

pepyakin mentioned this pull request Oct 18, 2018

Better allocator for Wasm #300

Closed

sorpaas added 3 commits October 18, 2018 17:55

Wrap some long lines

b76e574

Wrap a missing line

ac697ce

Remove unnecessary map

29c20a6

#856 (comment)

bkchr reviewed Oct 18, 2018

View reviewed changes

sorpaas added 3 commits October 18, 2018 18:45

Call ext_free after from_raw_parts

b73f25c

Fix tests in other crates

69d1840

Merge branch 'master' of https://github.com/paritytech/substrate into…

d28f3c0

… sp-multi-trie2

bkchr approved these changes Oct 18, 2018

View reviewed changes

gavofyork merged commit 234e31d into master Oct 18, 2018

bkchr deleted the sp-multi-trie2 branch October 18, 2018 19:43

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020

Bump versions & Substrate. (paritytech#902)

5356c4d

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

change node version to 1.1.0 (paritytech#902)

3321681

sorpaas mentioned this pull request Oct 24, 2022

Membership request polkadot-fellows/seeding#50

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple storage root support #902

Multiple storage root support #902

sorpaas commented Oct 12, 2018 •

edited

Loading

bkchr Oct 15, 2018

bkchr Oct 15, 2018

svyatonik left a comment

svyatonik Oct 18, 2018

bkchr Oct 18, 2018

bkchr Oct 18, 2018

bkchr Oct 18, 2018

bkchr Oct 18, 2018

bkchr Oct 18, 2018

sorpaas Oct 18, 2018

bkchr Oct 18, 2018

bkchr Oct 18, 2018

pepyakin Oct 18, 2018

pepyakin Oct 18, 2018

pepyakin Oct 18, 2018

pepyakin Oct 18, 2018

pepyakin Oct 18, 2018

bkchr Oct 18, 2018

pepyakin Oct 18, 2018

bkchr left a comment

Multiple storage root support #902

Multiple storage root support #902

Conversation

sorpaas commented Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svyatonik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

sorpaas commented Oct 12, 2018 •

edited

Loading