best_containing operations (issue 603) (#740)

* add stub for Client.best_chain_containing_block_hash

* add fn blockchain::Backend::leaf_hashes

* fix typo

* sketch out Client.best_chain_containing_block_hash

* fix indent

* Blockchain.leaf_hashes -> Blockchain.leaves

* add unimplemented! stub impls for Blockchain.leaves

* start impl of Blockchain.leaves for in-memory client db

* Client.best_chain_containing...: check canonical first and make compile

* first rough attempt at maintaining leaf list in in-memory db

* fix tab indent

* add test best_chain_containing_single_block

* add failing test best_chain_containing_with_fork

* pub use client::blockchain; in test-client to prevent circular dep in client tests

* best_chain_containing_with_single_block: improve and test leaves

* far improve in-memory Backend::leaves impl

* test blockchain::Backend::leaves more thoroughly

* handle more edge cases in blockchain::Backend::leaves impl for in memory

* fix test best_chain_containing_with_fork (two distinct test blocks had same hash)

* make best_chain_containing_block_hash pass existing tests

* improve docstring for Blockchain::leaves

* Client.best_chain_containing: some cleanup. support max_block_number

* best_chain_containing: remove broken outcommented fast check for best = canonical

* remove blank line

* best_block_containing: return None if target_hash not found

* best_chain_containing: add unreachable! at end of function

* improve tests for best_chain_containing

* renames

* more elaborate test scenario for best_containing

* best_containing: fix restriction of search through maybe_max_number

* best_containing: tests for restriction of search

* get rid of unnecessary clones

* replace Client::new_in_mem by new_with_backend which is useful for testing backends

* add test_client::new_with_backend for testing different backend impls

* add test for in_mem::Backend::leaves

* remove unused imports

* in_mem test_leaves: simplify

* flesh out tests for in_mem leaves impl

* remove tests for leaves from client which are now covered in implementing module

* improve comment

* add Client.new_in_mem again

* unwrap in test_client::new_with_backend

* make test_client::BlockBuilderExt work not just with in-mem backend

* make test client ext not just work with in mem backend

* add failing Backend.leaves test for client-db

* update Cargo.lock

* replace KeccakHasher with Blake2Hasher

* refactor

address grumble https://github.com/paritytech/substrate/pull/740#discussion_r217822862

* refactor using NumberFor

address grumble https://github.com/paritytech/substrate/pull/740#discussion_r217823341

* add test that exposes possible problem

* update docstring for Client.best_containing

* extract test for Backend.leaves for reuse

* improve test blockchain_header_and_hash_return_blocks_from_canonical_chain_given_block_numbers

* extract test_blockchain_query_by_number_gets_canonical to easily test multiple impls

* remove whitespace

* remove todo

* Client.best_containing: pre-empt search loop when target in canonical

* best_containing: prevent race condition by holding import lock

* add todo

* extract leaf list update code into function

* add comment

* client-db: use in-memory-kvdb for tests

* use BTreeSet to store leaves for in-mem which is faster and simpler

* add docstring

* add comments and fix formatting

* add initial raw version of LeafSet

* remove Client::update_leaves which has been superceded by LeafSet

* use LeafSet in in-mem backend

* keccak -> blake2

* don't reexport codec traits in primitives

addresses https://github.com/paritytech/substrate/pull/740#discussion_r219538185

* fix rebase mistake

* improve LeafSet and use it in state-db

* correct Transfer nonces to fix ApplyExtinsicFailed(Stale)

* use given backend in canoncal test

* kill dead tree-route code in util

* fix warnings

* tests for leafset

* reorganizations in in_mem backend

* fix reorganization canon block logic

* DB commit and safe reversion on write error

* fix style nits

substrate/core/test-client/src/block_builder_ext.rs

@@ -21,7 +21,6 @@ use client;
 use keyring;
 use runtime;
 
-use {Backend, Executor};
 use primitives::{Blake2Hasher};
 
 /// Extension trait for test block builder.
@@ -30,7 +29,11 @@ pub trait BlockBuilderExt {
 	fn push_transfer(&mut self, transfer: runtime::Transfer) -> Result<(), client::error::Error>;
 }
 
-impl BlockBuilderExt for client::block_builder::BlockBuilder<Backend, Executor, runtime::Block, Blake2Hasher> {
+impl<B, E> BlockBuilderExt for client::block_builder::BlockBuilder<B, E, runtime::Block, Blake2Hasher>
+    where
+        B: client::backend::Backend<runtime::Block, Blake2Hasher>,
+        E: client::CallExecutor<runtime::Block, Blake2Hasher> + Clone,
+{
 	fn push_transfer(&mut self, transfer: runtime::Transfer) -> Result<(), client::error::Error> {
 		self.push(sign_tx(transfer))
 	}

substrate/core/test-client/src/client_ext.rs

@@ -18,18 +18,13 @@
 
 use client::{self, Client};
 use keyring::Keyring;
-use runtime_primitives::{generic::BlockId, StorageMap};
-use runtime::genesismap::{GenesisConfig, additional_storage_with_genesis};
-use executor::NativeExecutor;
+use runtime_primitives::generic::BlockId;
+use primitives::Blake2Hasher;
 use runtime;
 use bft;
-use {Backend, Executor};
 
 /// Extension trait for a test client.
 pub trait TestClient {
-	/// Crates new client instance for tests.
-	fn new_for_tests() -> Self;
-
 	/// Justify and import block to the chain. No finality.
 	fn justify_and_import(&self, origin: client::BlockOrigin, block: runtime::Block) -> client::error::Result<()>;
 
@@ -40,11 +35,11 @@ pub trait TestClient {
 	fn genesis_hash(&self) -> runtime::Hash;
 }
 
-impl TestClient for Client<Backend, Executor, runtime::Block> {
-	fn new_for_tests() -> Self {
-		client::new_in_mem(NativeExecutor::new(), genesis_storage()).unwrap()
-	}
-
+impl<B, E> TestClient for Client<B, E, runtime::Block>
+    where
+        B: client::backend::Backend<runtime::Block, Blake2Hasher>,
+        E: client::CallExecutor<runtime::Block, Blake2Hasher>
+{
 	fn justify_and_import(&self, origin: client::BlockOrigin, block: runtime::Block) -> client::error::Result<()> {
 		let justification = fake_justify(&block.header);
 		let justified = self.check_justification(block.header, justification)?;
@@ -93,18 +88,3 @@ fn fake_justify(header: &runtime::Header) -> bft::UncheckedJustification<runtime
 		1,
 	)
 }
-
-fn genesis_config() -> GenesisConfig {
-	GenesisConfig::new_simple(vec![
-		Keyring::Alice.to_raw_public().into(),
-		Keyring::Bob.to_raw_public().into(),
-		Keyring::Charlie.to_raw_public().into(),
-	], 1000)
-}
-
-fn genesis_storage() -> StorageMap {
-		let mut storage = genesis_config().genesis_map();
-		let block: runtime::Block = client::genesis::construct_genesis_block(&storage);
-		storage.extend(additional_storage_with_genesis(&block));
-		storage
-}

substrate/core/test-client/src/lib.rs

@@ -33,13 +33,22 @@ pub extern crate substrate_client as client;
 pub extern crate substrate_keyring as keyring;
 pub extern crate substrate_test_runtime as runtime;
 
-mod client_ext;
+pub mod client_ext;
+pub mod trait_tests;
 mod block_builder_ext;
 
+use std::sync::Arc;
+
 pub use client_ext::TestClient;
 pub use block_builder_ext::BlockBuilderExt;
+pub use client::blockchain;
+pub use client::backend;
+pub use executor::NativeExecutor;
 
-use primitives::{Blake2Hasher};
+use primitives::Blake2Hasher;
+use runtime_primitives::StorageMap;
+use runtime::genesismap::{GenesisConfig, additional_storage_with_genesis};
+use keyring::Keyring;
 
 mod local_executor {
 	#![allow(missing_docs)]
@@ -62,5 +71,30 @@ pub type Executor = client::LocalCallExecutor<
 
 /// Creates new client instance used for tests.
 pub fn new() -> client::Client<Backend, Executor, runtime::Block> {
-	TestClient::new_for_tests()
+	new_with_backend(Arc::new(Backend::new()))
+}
+
+/// Creates new client instance used for tests with an explicitely provided backend.
+/// This is useful for testing backend implementations.
+pub fn new_with_backend<B>(backend: Arc<B>) -> client::Client<B, client::LocalCallExecutor<B, executor::NativeExecutor<LocalExecutor>>, runtime::Block>
+	where
+		B: backend::LocalBackend<runtime::Block, Blake2Hasher>,
+{
+	let executor = NativeExecutor::new();
+	client::new_with_backend(backend, executor, genesis_storage()).unwrap()
+}
+
+fn genesis_config() -> GenesisConfig {
+	GenesisConfig::new_simple(vec![
+		Keyring::Alice.to_raw_public().into(),
+		Keyring::Bob.to_raw_public().into(),
+		Keyring::Charlie.to_raw_public().into(),
+	], 1000)
+}
+
+fn genesis_storage() -> StorageMap {
+	let mut storage = genesis_config().genesis_map();
+	let block: runtime::Block = client::genesis::construct_genesis_block(&storage);
+	storage.extend(additional_storage_with_genesis(&block));
+	storage
 }

substrate/core/test-client/src/trait_tests.rs

@@ -0,0 +1,241 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! tests that should hold for all implementations of certain traits.
+//! to test implementations without duplication.
+
+#![allow(missing_docs)]
+
+use std::sync::Arc;
+use keyring::Keyring;
+use client::BlockOrigin;
+use primitives::Blake2Hasher;
+use ::TestClient;
+use runtime_primitives::traits::Block as BlockT;
+use backend;
+use blockchain::{Backend as BlockChainBackendT, HeaderBackend};
+use ::BlockBuilderExt;
+use runtime::{self, Transfer};
+use runtime_primitives::generic::BlockId;
+
+/// helper to test the `leaves` implementation for various backends
+pub fn test_leaves_for_backend<B>(backend: Arc<B>) where
+	B: backend::LocalBackend<runtime::Block, Blake2Hasher>,
+{
+	// block tree:
+	// G -> A1 -> A2 -> A3 -> A4 -> A5
+	//		A1 -> B2 -> B3 -> B4
+	//			  B2 -> C3
+	//		A1 -> D2
+
+	let client = ::new_with_backend(backend.clone());
+
+	let genesis_hash = client.info().unwrap().chain.genesis_hash;
+
+	assert_eq!(
+		client.backend().blockchain().leaves().unwrap(),
+		vec![genesis_hash]);
+
+	// G -> A1
+	let a1 = client.new_block().unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a1.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a1.hash()]);
+
+	// A1 -> A2
+	let a2 = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a2.clone()).unwrap();
+	assert_eq!(
+		client.backend().blockchain().leaves().unwrap(),
+		vec![a2.hash()]);
+
+	// A2 -> A3
+	let a3 = client.new_block_at(&BlockId::Hash(a2.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a3.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a3.hash()]);
+
+	// A3 -> A4
+	let a4 = client.new_block_at(&BlockId::Hash(a3.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a4.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a4.hash()]);
+
+	// A4 -> A5
+	let a5 = client.new_block_at(&BlockId::Hash(a4.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a5.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash()]);
+
+	// A1 -> B2
+	let mut builder = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap();
+	// this push is required as otherwise B2 has the same hash as A2 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 41,
+		nonce: 0,
+	}).unwrap();
+	let b2 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b2.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash(), b2.hash()]);
+
+	// B2 -> B3
+	let b3 = client.new_block_at(&BlockId::Hash(b2.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b3.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash(), b3.hash()]);
+
+	// B3 -> B4
+	let b4 = client.new_block_at(&BlockId::Hash(b3.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b4.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash(), b4.hash()]);
+
+	// // B2 -> C3
+	let mut builder = client.new_block_at(&BlockId::Hash(b2.hash())).unwrap();
+	// this push is required as otherwise C3 has the same hash as B3 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 1,
+		nonce: 1,
+	}).unwrap();
+	let c3 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, c3.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash(), b4.hash(), c3.hash()]);
+
+	// A1 -> D2
+	let mut builder = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap();
+	// this push is required as otherwise D2 has the same hash as B2 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 1,
+		nonce: 0,
+	}).unwrap();
+	let d2 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, d2.clone()).unwrap();
+	assert_eq!(
+		backend.blockchain().leaves().unwrap(),
+		vec![a5.hash(), b4.hash(), c3.hash(), d2.hash()]);
+}
+
+
+pub fn test_blockchain_query_by_number_gets_canonical<B>(backend: Arc<B>) where
+	B: backend::LocalBackend<runtime::Block, Blake2Hasher>,
+{
+	// block tree:
+	// G -> A1 -> A2 -> A3 -> A4 -> A5
+	//		A1 -> B2 -> B3 -> B4
+	//			  B2 -> C3
+	//		A1 -> D2
+	let client = ::new_with_backend(backend);
+
+	// G -> A1
+	let a1 = client.new_block().unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a1.clone()).unwrap();
+
+	// A1 -> A2
+	let a2 = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a2.clone()).unwrap();
+
+	// A2 -> A3
+	let a3 = client.new_block_at(&BlockId::Hash(a2.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a3.clone()).unwrap();
+
+	// A3 -> A4
+	let a4 = client.new_block_at(&BlockId::Hash(a3.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a4.clone()).unwrap();
+
+	// A4 -> A5
+	let a5 = client.new_block_at(&BlockId::Hash(a4.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, a5.clone()).unwrap();
+
+	// A1 -> B2
+	let mut builder = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap();
+	// this push is required as otherwise B2 has the same hash as A2 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 41,
+		nonce: 0,
+	}).unwrap();
+	let b2 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b2.clone()).unwrap();
+
+	// B2 -> B3
+	let b3 = client.new_block_at(&BlockId::Hash(b2.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b3.clone()).unwrap();
+
+	// B3 -> B4
+	let b4 = client.new_block_at(&BlockId::Hash(b3.hash())).unwrap().bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, b4.clone()).unwrap();
+
+	// // B2 -> C3
+	let mut builder = client.new_block_at(&BlockId::Hash(b2.hash())).unwrap();
+	// this push is required as otherwise C3 has the same hash as B3 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 1,
+		nonce: 1,
+	}).unwrap();
+	let c3 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, c3.clone()).unwrap();
+
+	// A1 -> D2
+	let mut builder = client.new_block_at(&BlockId::Hash(a1.hash())).unwrap();
+	// this push is required as otherwise D2 has the same hash as B2 and won't get imported
+	builder.push_transfer(Transfer {
+		from: Keyring::Alice.to_raw_public().into(),
+		to: Keyring::Ferdie.to_raw_public().into(),
+		amount: 1,
+		nonce: 0,
+	}).unwrap();
+	let d2 = builder.bake().unwrap();
+	client.justify_and_import(BlockOrigin::Own, d2.clone()).unwrap();
+
+	let genesis_hash = client.info().unwrap().chain.genesis_hash;
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(0)).unwrap().unwrap().hash(), genesis_hash);
+	assert_eq!(client.backend().blockchain().hash(0).unwrap().unwrap(), genesis_hash);
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(1)).unwrap().unwrap().hash(), a1.hash());
+	assert_eq!(client.backend().blockchain().hash(1).unwrap().unwrap(), a1.hash());
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(2)).unwrap().unwrap().hash(), a2.hash());
+	assert_eq!(client.backend().blockchain().hash(2).unwrap().unwrap(), a2.hash());
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(3)).unwrap().unwrap().hash(), a3.hash());
+	assert_eq!(client.backend().blockchain().hash(3).unwrap().unwrap(), a3.hash());
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(4)).unwrap().unwrap().hash(), a4.hash());
+	assert_eq!(client.backend().blockchain().hash(4).unwrap().unwrap(), a4.hash());
+
+	assert_eq!(client.backend().blockchain().header(BlockId::Number(5)).unwrap().unwrap().hash(), a5.hash());
+	assert_eq!(client.backend().blockchain().hash(5).unwrap().unwrap(), a5.hash());
+}