Client provide uncles by marcio-diaz · Pull Request #1609 · paritytech/substrate

marcio-diaz · 2019-01-29T11:35:00Z

PR for issue #1448

bkchr · 2019-01-29T12:01:12Z

core/client/db/src/lib.rs

 	fn commit_operation(&self, mut operation: Self::BlockImportOperation)
 		-> Result<(), client::error::Error>
 	{
+		println!("db inside commit_operation ----------------------------");


Debug output.

bkchr · 2019-01-29T12:01:20Z

core/client/db/src/lib.rs

 			{
 				let mut leaves = self.blockchain.leaves.write();
 				let displaced_leaf = leaves.import(hash, number, parent_hash);
+				println!("db prepare_transaction --------------- {:?}->{:?}", parent_hash, hash);


Debug output.

bkchr · 2019-01-29T12:01:43Z

core/client/src/children.rs

@@ -0,0 +1,161 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.


bkchr · 2019-01-29T12:02:09Z

core/client/src/children.rs

+    K: Ord + Eq + Hash + Clone + Encode + Decode + Debug,
+    V: Ord + Eq + Hash + Clone + Encode + Decode + Debug,
+{
+    storage: BTreeMap<K, Vec<V>>,


Spaces all over the file.

bkchr · 2019-01-29T12:08:02Z

core/client/src/children.rs

+		for (parent, child) in self.pending_added.drain(..) {
+            parent.using_encoded(|s| buf.extend(s));
+			tx.put_vec(column, &buf[..], child.encode());
+			buf.truncate(prefix.len()); // reuse allocation.


Why do you truncate here?

Because prefix is the same but parent and child change.

also it looks like some spaces got into the indentation for L98 -- please make sure that indentation is done with tabs

bkchr · 2019-01-29T12:08:57Z

core/client/src/children.rs

+		}
+	}
+
+    pub fn hashes(&self, parent_hash: K) -> Vec<V> {


Why do we not return &[V]?

bkchr · 2019-01-29T12:09:14Z

core/client/src/client.rs

 	pub fn lock_import_and_run<R, F: FnOnce(&mut ClientImportOperation<Block, Blake2Hasher, B>) -> error::Result<R>>(
 		&self, f: F
 	) -> error::Result<R> {
+		println!("in lock_import_and_run ------------------------------------------------");


bkchr · 2019-01-29T12:09:37Z

core/client/src/client.rs


+	fn get_descendants(&self, target: Block::Hash) -> Vec<Block::Hash> {
+		let children = self.backend.blockchain().children(target);
+		let mut descendants = vec![];


Suggested change

let mut descendants = vec![];

let mut descendants = Vec::new();

bkchr · 2019-01-29T12:11:22Z

core/client/src/client.rs

+			if ancestors.contains(&uncles[i]) {
+				uncles.remove(i);
+			} else {
+				i += 1;


Why is it only increased in the else branch?

fixed

rphmeier · 2019-02-01T23:29:20Z

core/client/src/children.rs

+	K: Ord + Eq + Hash + Clone + Encode + Decode + Debug,
+	V: Ord + Eq + Hash + Clone + Encode + Decode + Debug,
+{
+	storage: BTreeMap<K, Vec<V>>,


...is this storing all children in memory? that doesn't seem like it will scale very well.
This mapping should be on-disk only -- I don't think emulating the LeavesSet is a good idea for this instance.

I removed the part where it reads all children from db :). I'm adding children everywhere where leaves are being added. Are there other parts of the code where I should add them?

bkchr · 2019-02-07T09:23:30Z

core/client/src/client.rs

+
+	/// Gets the uncles of the block with `target_hash` going back `max_generation` ancestors.
+	pub fn uncles(&self, target_hash: Block::Hash, max_generation: NumberFor<Block>)
+		-> error::Result<Option<Vec<Block::Hash>>>


We do not return any Ok(None), so we probably can remove the Option here.

bkchr · 2019-02-07T09:24:15Z

core/client/src/client.rs

+		-> error::Result<Option<Vec<Block::Hash>>>
+	{
+		let load_header = |id: Block::Hash| {
+			match self.backend.blockchain().header(BlockId::Hash(id)) {


Suggested change

match self.backend.blockchain().header(BlockId::Hash(id)) {

match self.backend.blockchain().header(BlockId::Hash(id))? {

And the you just need to match for Some(_) and None.

bkchr · 2019-02-07T09:24:33Z

core/client/src/client.rs

+		let genesis_hash = self.backend.blockchain().info().unwrap().genesis_hash;
+		let genesis = load_header(genesis_hash)?;
+		let mut current = load_header(target_hash)?;
+		let mut uncles = vec![];


Suggested change

let mut uncles = vec![];

let mut uncles = Vec::new();

gavofyork · 2019-02-12T16:22:33Z

@rphmeier @bkchr further thoughts?

rphmeier · 2019-02-12T20:19:36Z

core/client/src/children.rs

+		match self.storage.get_mut(&parent_hash) {
+			Some(children) => children.push(child_hash),
+			None => { 
+				self.storage.insert(parent_hash, vec![child_hash]);


use entry API. self.storage.entry(parent_hash).or_insert_with(Vec::new).push(child_hash)

rphmeier · 2019-02-12T20:19:46Z

core/client/src/children.rs

+	/// Returns the hashes of the children blocks of the block with `parent_hash`.
+	/// It doesn't read the database.
+	pub fn hashes_from_mem(&self, parent_hash: K) -> Vec<V> {
+		match self.storage.get(&parent_hash) {


use unwrap_or_else

*cloned().unwrap_or_default()

🤷‍♂️ mine is shorter :)

rphmeier · 2019-02-12T20:19:53Z

core/client/src/children.rs

+
+		let mut buf = prefix.to_vec();
+		parent_hash.using_encoded(|s| buf.extend(s));
+		let raw_val = match db.get(column, &buf[..]).unwrap() {


prove or remove unwrap.

rphmeier · 2019-02-12T20:20:56Z

core/sr-primitives/src/traits.rs


 /// Abstraction around hashing
-pub trait Hash: 'static + MaybeSerializeDebug + Clone + Eq + PartialEq {	// Stupid bug in the Rust compiler believes derived
+pub trait Hash: 'static + MaybeSerializeDebug + Clone + Eq + PartialEq + PartialOrd + Ord {	// Stupid bug in the Rust compiler believes derived


that's a bit too strong IMO.

why should hash types always be orderable?

rphmeier · 2019-02-12T20:22:39Z

core/client/src/client.rs

+			}
+		};
+
+		let genesis_hash = self.backend.blockchain().info().unwrap().genesis_hash;


prove or remove unwrap

rphmeier · 2019-02-12T20:24:09Z

core/client/src/client.rs

+		let mut descendants = Vec::new();
+		for child in children {
+			descendants.push(child);
+			let d = self.get_descendants(child, None)?;


won't this recurse all the way to the head of all descendant chains? could be expensive.

rphmeier · 2019-02-12T20:25:27Z

core/client/src/client.rs

+		for _generation in 0..max_generation.as_() {
+			uncles.extend(self.get_descendants(ancestor.hash(), Some(current.hash()))?);
+			current = ancestor;
+			if genesis == current { break; }


just check if current hash == genesis hash and don't load the genesis header. and you can avoid doing the hash calculations in all cases but the first by book-keeping the last parent_hash.

rphmeier

Note that uncles are a direct descendant of an ancestor, not any descendant of an ancestor.

bkchr · 2019-02-13T13:06:29Z

core/client/src/children.rs

+	}
+
+	/// Returns the hashes of the children blocks of the block with `parent_hash`.
+	pub fn hashes(&self, db: &KeyValueDB, column: Option<u32>, prefix: &[u8],


Either all parameters on one line or one parameter per line, please.

bkchr · 2019-02-13T13:11:05Z

core/client/src/children.rs

+		let mut buf = prefix.to_vec();
+		parent_hash.using_encoded(|s| buf.extend(s));
+
+		if let Ok(raw_val_opt) = db.get(column, &buf[..]) {


That is way to complicated. Either split it up across multiple lines with ? or use match.

bkchr · 2019-02-13T13:11:27Z

core/client/src/children.rs

+	/// Returns the hashes of the children blocks of the block with `parent_hash`.
+	/// It doesn't read the database.
+	pub fn hashes_from_mem(&self, parent_hash: K) -> Vec<V> {
+		match self.storage.get(&parent_hash) {


*cloned().unwrap_or_default()

rphmeier · 2019-02-13T16:29:03Z

core/client/src/client.rs

+
+		for _generation in 0..max_generation.as_() {
+			let children = self.get_children(ancestor_hash, Some(current_hash))?;
+			uncles.extend(children);


i think the get_children function could be elided in favor of

let children = self.backend.blockchain().children(ancestor_hash)?; uncles.extend(children.into_iter().filter(|h| h != &current_hash));

this saves a sweep and some potential memmoves.

gavofyork · 2019-02-21T09:09:34Z

Ready for review again?

fixed

marcio-diaz · 2019-02-21T14:19:56Z

Ready for review again?

Yes, I think I handled all the issues in the reviews.

gavofyork · 2019-02-21T18:28:02Z

cool. @bkchr / @rphmeier happy to sign off?

rphmeier · 2019-02-21T18:56:31Z

what has changed since the last review?

marcio-diaz · 2019-02-22T09:29:04Z

what has changed since the last review?

Mainly, I removed the hashmap from the implementation of children as you asked.

I was thinking to create a new issue for this (I took a look to the ethereum repo).
Should I create a meta issue about refactoring db related code?

\cc @rphmeier

core/client/src/in_mem.rs

rphmeier · 2019-02-22T15:41:46Z

core/client/src/children.rs

+
+impl ChildrenMap {
+	/// Returns the hashes of the children blocks of the block with `parent_hash`.
+	pub fn children_hashes<


We could kill the ChildrenMap struct and make these free functions.

bkchr

Looks good, some last minor issues and then we are good to go :)

bkchr · 2019-02-25T08:28:03Z

core/test-client/src/trait_tests.rs

 		vec![a5.hash(), b4.hash(), c3.hash(), d2.hash()]);
 }

+/// helper to test the `leaves` implementation for various backends


Suggested change

/// helper to test the `leaves` implementation for various backends

/// helper to test the `children` implementation for various backends

bkchr · 2019-02-25T08:29:11Z

node/runtime/src/lib.rs

 	authoring_version: 10,
-	spec_version: 30,
-	impl_version: 30,
+	spec_version: 31,


You don't touch the runtime, so you don't need to change the spec_version and impl_version.

…strate into mar-client-provide-uncles

* feat: add children function to backend * feat: add test for children hashes * feat: add uncles function to client * fix: improve uncles function adds few more tests * fix: remove children when reverting * fix: typo and spec version

marcio-diaz added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 29, 2019

bkchr previously requested changes Jan 29, 2019

View reviewed changes

marcio-diaz force-pushed the mar-client-provide-uncles branch from 388f553 to bdcde73 Compare February 1, 2019 14:25

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 1, 2019

rphmeier reviewed Feb 1, 2019

View reviewed changes

gavofyork added A5-grumble and removed A0-please_review Pull request needs code review. labels Feb 3, 2019

marcio-diaz added A0-please_review Pull request needs code review. and removed A5-grumble labels Feb 5, 2019

bkchr reviewed Feb 7, 2019

View reviewed changes

gnunicorn added the A3-needsresolving label Feb 8, 2019

marcio-diaz force-pushed the mar-client-provide-uncles branch from 377c11c to 079c506 Compare February 11, 2019 11:12

devops-parity added the B2-breaksapi label Feb 11, 2019

marcio-diaz force-pushed the mar-client-provide-uncles branch from 079c506 to 3f9dab7 Compare February 11, 2019 12:01

rphmeier reviewed Feb 12, 2019

View reviewed changes

rphmeier previously requested changes Feb 12, 2019

View reviewed changes

rphmeier added A4-gotissues and removed A0-please_review Pull request needs code review. labels Feb 12, 2019

bkchr reviewed Feb 13, 2019

View reviewed changes

marcio-diaz force-pushed the mar-client-provide-uncles branch from ad07fbf to f29f611 Compare February 13, 2019 14:55

rphmeier reviewed Feb 13, 2019

View reviewed changes

marcio-diaz force-pushed the mar-client-provide-uncles branch from b4c0d72 to f587d69 Compare February 15, 2019 14:16

feat: add tests for backend

6b55176

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Feb 18, 2019

Merge branch 'master' into mar-client-provide-uncles

3a0a58b

rphmeier reviewed Feb 22, 2019

View reviewed changes

core/client/src/in_mem.rs Show resolved Hide resolved

rphmeier reviewed Feb 22, 2019

View reviewed changes

rphmeier approved these changes Feb 22, 2019

View reviewed changes

rphmeier added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Feb 22, 2019

bkchr approved these changes Feb 25, 2019

View reviewed changes

seerscode added 4 commits February 25, 2019 09:44

chore: free functions

adfb706

feat: add remove children

bf5a3e8

fix: remove children when reverting

96ce77f

Merge branch 'mar-client-provide-uncles' of github.com:paritytech/sub…

334fc5a

…strate into mar-client-provide-uncles

marcio-diaz removed A6-mustntgrumble labels Feb 25, 2019

fix: typo and spec version

b6fc5bf

marcio-diaz force-pushed the mar-client-provide-uncles branch from cc5b1d0 to b6fc5bf Compare February 25, 2019 10:05

marcio-diaz merged commit 7999743 into master Feb 25, 2019

marcio-diaz deleted the mar-client-provide-uncles branch February 25, 2019 10:21

		@@ -0,0 +1,161 @@
		// Copyright 2018 Parity Technologies (UK) Ltd.

	let mut descendants = vec![];
	let mut descendants = Vec::new();

	match self.backend.blockchain().header(BlockId::Hash(id)) {
	match self.backend.blockchain().header(BlockId::Hash(id))? {

	/// helper to test the `leaves` implementation for various backends
	/// helper to test the `children` implementation for various backends

Conversation

marcio-diaz commented Jan 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Feb 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Feb 21, 2019

Uh oh!

marcio-diaz commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavofyork commented Feb 21, 2019

Uh oh!

rphmeier commented Feb 21, 2019

rphmeier Feb 12, 2019 •

edited

Loading

marcio-diaz commented Feb 21, 2019 •

edited

Loading