Apply pending block details on commit (#2254)

* failing test

* Cache pending details

* [ci skip] updated comment

ethcore/src/blockchain/blockchain.rs

@@ -196,6 +196,7 @@ pub struct BlockChain {
 
 	pending_best_block: RwLock<Option<BestBlock>>,
 	pending_block_hashes: RwLock<HashMap<BlockNumber, H256>>,
+	pending_block_details: RwLock<HashMap<H256, BlockDetails>>,
 	pending_transaction_addresses: RwLock<HashMap<H256, Option<TransactionAddress>>>,
 }
 
@@ -439,6 +440,7 @@ impl BlockChain {
 			cache_man: Mutex::new(cache_man),
 			pending_best_block: RwLock::new(None),
 			pending_block_hashes: RwLock::new(HashMap::new()),
+			pending_block_details: RwLock::new(HashMap::new()),
 			pending_transaction_addresses: RwLock::new(HashMap::new()),
 		};
 
@@ -895,17 +897,6 @@ impl BlockChain {
 
 	/// Prepares extras update.
 	fn prepare_update(&self, batch: &mut DBTransaction, update: ExtrasUpdate, is_best: bool) {
-		{
-			let block_hashes: Vec<_> = update.block_details.keys().cloned().collect();
-
-			let mut write_details = self.block_details.write();
-			batch.extend_with_cache(db::COL_EXTRA, &mut *write_details, update.block_details, CacheUpdatePolicy::Overwrite);
-
-			let mut cache_man = self.cache_man.lock();
-			for hash in block_hashes {
-				cache_man.note_used(CacheID::BlockDetails(hash));
-			}
-		}
 
 		{
 			let mut write_receipts = self.block_receipts.write();
@@ -917,7 +908,7 @@ impl BlockChain {
 			batch.extend_with_cache(db::COL_EXTRA, &mut *write_blocks_blooms, update.blocks_blooms, CacheUpdatePolicy::Remove);
 		}
 
-		// These cached values must be updated last with all three locks taken to avoid
+		// These cached values must be updated last with all four locks taken to avoid
 		// cache decoherence
 		{
 			let mut best_block = self.pending_best_block.write();
@@ -935,8 +926,10 @@ impl BlockChain {
 				},
 			}
 			let mut write_hashes = self.pending_block_hashes.write();
+			let mut write_details = self.pending_block_details.write();
 			let mut write_txs = self.pending_transaction_addresses.write();
 
+			batch.extend_with_cache(db::COL_EXTRA, &mut *write_details, update.block_details, CacheUpdatePolicy::Overwrite);
 			batch.extend_with_cache(db::COL_EXTRA, &mut *write_hashes, update.block_hashes, CacheUpdatePolicy::Overwrite);
 			batch.extend_with_option_cache(db::COL_EXTRA, &mut *write_txs, update.transactions_addresses, CacheUpdatePolicy::Overwrite);
 		}
@@ -946,9 +939,11 @@ impl BlockChain {
 	pub fn commit(&self) {
 		let mut pending_best_block = self.pending_best_block.write();
 		let mut pending_write_hashes = self.pending_block_hashes.write();
+		let mut pending_block_details = self.pending_block_details.write();
 		let mut pending_write_txs = self.pending_transaction_addresses.write();
 
 		let mut best_block = self.best_block.write();
+		let mut write_block_details = self.block_details.write();
 		let mut write_hashes = self.block_hashes.write();
 		let mut write_txs = self.transaction_addresses.write();
 		// update best block
@@ -961,9 +956,11 @@ impl BlockChain {
 
 		let pending_hashes_keys: Vec<_> = pending_write_hashes.keys().cloned().collect();
 		let enacted_txs_keys: Vec<_> = enacted_txs.keys().cloned().collect();
+		let pending_block_hashes: Vec<_> = pending_block_details.keys().cloned().collect();
 
 		write_hashes.extend(mem::replace(&mut *pending_write_hashes, HashMap::new()));
 		write_txs.extend(enacted_txs.into_iter().map(|(k, v)| (k, v.expect("Transactions were partitioned; qed"))));
+		write_block_details.extend(mem::replace(&mut *pending_block_details, HashMap::new()));
 
 		for hash in retracted_txs.keys() {
 			write_txs.remove(hash);
@@ -977,6 +974,10 @@ impl BlockChain {
 		for hash in enacted_txs_keys {
 			cache_man.note_used(CacheID::TransactionAddresses(hash));
 		}
+
+		for hash in pending_block_hashes {
+			cache_man.note_used(CacheID::BlockDetails(hash));
+		}
 	}
 
 	/// Iterator that lists `first` and then all of `first`'s ancestors, by hash.
@@ -1297,6 +1298,11 @@ impl BlockChain {
 			ancient_block_number: best_ancient_block.as_ref().map(|b| b.number),
 		}
 	}
+
+	#[cfg(test)]
+	pub fn db(&self) -> &Arc<Database> {
+		&self.db
+	}
 }
 
 #[cfg(test)]

ethcore/src/client/client.rs

@@ -1216,3 +1216,33 @@ impl MayPanic for Client {
 		self.panic_handler.on_panic(closure);
 	}
 }
+
+
+#[test]
+fn should_not_cache_details_before_commit() {
+	use tests::helpers::*;
+	use std::thread;
+	use std::time::Duration;
+	use std::sync::atomic::{AtomicBool, Ordering};
+
+	let client = generate_dummy_client(0);
+	let genesis = client.chain_info().best_block_hash;
+	let (new_hash, new_block) = get_good_dummy_block_hash();
+
+	let go = {
+		// Separate thread uncommited transaction
+		let go = Arc::new(AtomicBool::new(false));
+		let go_thread = go.clone();
+		let another_client = client.reference().clone();
+		thread::spawn(move || {
+			let mut batch = DBTransaction::new(&*another_client.chain.read().db().clone());
+			another_client.chain.read().insert_block(&mut batch, &new_block, Vec::new());
+			go_thread.store(true, Ordering::SeqCst);
+		});
+		go
+	};
+
+	while !go.load(Ordering::SeqCst) { thread::park_timeout(Duration::from_millis(5)); }
+
+	assert!(client.tree_route(&genesis, &new_hash).is_none());
+}

ethcore/src/tests/helpers.rs

@@ -388,7 +388,7 @@ pub fn get_good_dummy_block_fork_seq(start_number: usize, count: usize, parent_h
 	r
 }
 
-pub fn get_good_dummy_block() -> Bytes {
+pub fn get_good_dummy_block_hash() -> (H256, Bytes) {
 	let mut block_header = Header::new();
 	let test_spec = get_test_spec();
 	let test_engine = &test_spec.engine;
@@ -399,7 +399,12 @@ pub fn get_good_dummy_block() -> Bytes {
 	block_header.set_parent_hash(test_spec.genesis_header().hash());
 	block_header.set_state_root(test_spec.genesis_header().state_root().clone());
 
-	create_test_block(&block_header)
+	(block_header.hash(), create_test_block(&block_header))
+}
+
+pub fn get_good_dummy_block() -> Bytes {
+	let (_, bytes) = get_good_dummy_block_hash();
+	bytes
 }
 
 pub fn get_bad_state_dummy_block() -> Bytes {