Fix block detail updating (#11015)

* Add finality parameter to `null engine`

* Add testcase for finalization marking in `ethcore` client

* Add double cache read for db

* Prevent lost update of block details

* Read with pending update for block details in batch

ethcore/blockchain/src/blockchain.rs

@@ -198,6 +198,12 @@ pub trait BlockProvider {
 		where F: Fn(&LogEntry) -> bool + Send + Sync, Self: Sized;
 }
 
+/// Interface for querying blocks with pending db transaction by hash and by number.
+trait InTransactionBlockProvider {
+	/// Get the familial details concerning a block.
+	fn uncommitted_block_details(&self, hash: &H256) -> Option<BlockDetails>;
+}
+
 #[derive(Debug, Hash, Eq, PartialEq, Clone)]
 enum CacheId {
 	BlockHeader(H256),
@@ -427,6 +433,19 @@ impl BlockProvider for BlockChain {
 	}
 }
 
+impl InTransactionBlockProvider for BlockChain {
+	fn uncommitted_block_details(&self, hash: &H256) -> Option<BlockDetails> {
+		let result = self.db.key_value().read_with_two_layer_cache(
+			db::COL_EXTRA,
+			&self.pending_block_details,
+			&self.block_details,
+			hash
+		)?;
+		self.cache_man.lock().note_used(CacheId::BlockDetails(*hash));
+		Some(result)
+	}
+}
+
 /// An iterator which walks the blockchain towards the genesis.
 #[derive(Clone)]
 pub struct AncestryIter<'a> {
@@ -795,7 +814,7 @@ impl BlockChain {
 		batch.put(db::COL_HEADERS, hash.as_bytes(), &compressed_header);
 		batch.put(db::COL_BODIES, hash.as_bytes(), &compressed_body);
 
-		let maybe_parent = self.block_details(&block_parent_hash);
+		let maybe_parent = self.uncommitted_block_details(&block_parent_hash);
 
 		if let Some(parent_details) = maybe_parent {
 			// parent known to be in chain.
@@ -1047,7 +1066,7 @@ impl BlockChain {
 	///
 	/// Used in snapshots to glue the chunks together at the end.
 	pub fn add_child(&self, batch: &mut DBTransaction, block_hash: H256, child_hash: H256) {
-		let mut parent_details = self.block_details(&block_hash)
+		let mut parent_details = self.uncommitted_block_details(&block_hash)
 			.unwrap_or_else(|| panic!("Invalid block hash: {:?}", block_hash));
 
 		parent_details.children.push(child_hash);
@@ -1154,7 +1173,7 @@ impl BlockChain {
 	/// Mark a block to be considered finalized. Returns `Some(())` if the operation succeeds, and `None` if the block
 	/// hash is not found.
 	pub fn mark_finalized(&self, batch: &mut DBTransaction, block_hash: H256) -> Option<()> {
-		let mut block_details = self.block_details(&block_hash)?;
+		let mut block_details = self.uncommitted_block_details(&block_hash)?;
 		block_details.is_finalized = true;
 
 		self.update_block_details(batch, block_hash, block_details);
@@ -1347,7 +1366,7 @@ impl BlockChain {
 	/// Uses the given parent details or attempts to load them from the database.
 	fn prepare_block_details_update(&self, parent_hash: H256, info: &BlockInfo, is_finalized: bool) -> HashMap<H256, BlockDetails> {
 		// update parent
-		let mut parent_details = self.block_details(&parent_hash).unwrap_or_else(|| panic!("Invalid parent hash: {:?}", parent_hash));
+		let mut parent_details = self.uncommitted_block_details(&parent_hash).unwrap_or_else(|| panic!("Invalid parent hash: {:?}", parent_hash));
 		parent_details.children.push(info.hash);
 
 		// create current block details.
@@ -1653,7 +1672,7 @@ mod tests {
 		let fork_choice = {
 			let header = block.header_view();
 			let parent_hash = header.parent_hash();
-			let parent_details = bc.block_details(&parent_hash).unwrap_or_else(|| panic!("Invalid parent hash: {:?}", parent_hash));
+			let parent_details = bc.uncommitted_block_details(&parent_hash).unwrap_or_else(|| panic!("Invalid parent hash: {:?}", parent_hash));
 			let block_total_difficulty = parent_details.total_difficulty + header.difficulty();
 			if block_total_difficulty > bc.best_block_total_difficulty() {
 				common_types::engines::ForkChoice::New

ethcore/db/src/db.rs

@@ -190,6 +190,21 @@ pub trait Readable {
 		})
 	}
 
+	/// Returns value for given key either in two-layered cache or in database.
+	fn read_with_two_layer_cache<K, T, C>(&self, col: Option<u32>, l1_cache: &RwLock<C>, l2_cache: &RwLock<C>, key: &K) -> Option<T> where
+		K: Key<T> + Eq + Hash + Clone,
+		T: Clone + rlp::Decodable,
+		C: Cache<K, T> {
+		{
+			let read = l1_cache.read();
+			if let Some(v) = read.get(key) {
+				return Some(v.clone());
+			}
+		}
+
+		self.read_with_cache(col, l2_cache, key)
+	}
+
 	/// Returns true if given value exists.
 	fn exists<T, R>(&self, col: Option<u32>, key: &dyn Key<T, Target = R>) -> bool where R: AsRef<[u8]>;
 

ethcore/engines/null-engine/src/lib.rs

@@ -27,19 +27,26 @@ use machine::{
 	ExecutedBlock,
 	Machine,
 };
-use common_types::snapshot::Snapshotting;
+use common_types::{
+	ancestry_action::AncestryAction,
+	header::ExtendedHeader,
+	snapshot::Snapshotting
+};
 
 /// Params for a null engine.
 #[derive(Clone, Default)]
 pub struct NullEngineParams {
 	/// base reward for a block.
 	pub block_reward: U256,
+	/// Immediate finalization.
+	pub immediate_finalization: bool
 }
 
 impl From<ethjson::spec::NullEngineParams> for NullEngineParams {
 	fn from(p: ethjson::spec::NullEngineParams) -> Self {
 		NullEngineParams {
 			block_reward: p.block_reward.map_or_else(Default::default, Into::into),
+			immediate_finalization: p.immediate_finalization.unwrap_or(false)
 		}
 	}
 }
@@ -108,4 +115,13 @@ impl Engine for NullEngine {
 	fn params(&self) -> &CommonParams {
 		self.machine.params()
 	}
+
+	fn ancestry_actions(&self, _header: &Header, ancestry: &mut dyn Iterator<Item=ExtendedHeader>) -> Vec<AncestryAction> {
+		if self.params.immediate_finalization {
+			// always mark parent finalized
+			ancestry.take(1).map(|e| AncestryAction::MarkFinalized(e.header.hash())).collect()
+		} else {
+			Vec::new()
+		}
+	}
 }

ethcore/res/null_morden_with_finality.json

@@ -0,0 +1,38 @@
+{
+	"name": "Morden",
+	"engine": {
+		"null": {
+			"params": {
+				"immediateFinalization": true
+			}
+		}
+	},
+	"params": {
+		"gasLimitBoundDivisor": "0x0400",
+		"accountStartNonce": "0x0",
+		"maximumExtraDataSize": "0x20",
+		"minGasLimit": "0x1388",
+		"networkID" : "0x2"
+	},
+	"genesis": {
+		"seal": {
+			"ethereum": {
+				"nonce": "0x00006d6f7264656e",
+				"mixHash": "0x00000000000000000000000000000000000000647572616c65787365646c6578"
+			}
+		},
+		"difficulty": "0x20000",
+		"author": "0x0000000000000000000000000000000000000000",
+		"timestamp": "0x00",
+		"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
+		"extraData": "0x",
+		"gasLimit": "0x2fefd8"
+	},
+	"accounts": {
+		"0000000000000000000000000000000000000001": { "balance": "1", "nonce": "1048576", "builtin": { "name": "ecrecover", "pricing": { "linear": { "base": 3000, "word": 0 } } } },
+		"0000000000000000000000000000000000000002": { "balance": "1", "nonce": "1048576", "builtin": { "name": "sha256", "pricing": { "linear": { "base": 60, "word": 12 } } } },
+		"0000000000000000000000000000000000000003": { "balance": "1", "nonce": "1048576", "builtin": { "name": "ripemd160", "pricing": { "linear": { "base": 600, "word": 120 } } } },
+		"0000000000000000000000000000000000000004": { "balance": "1", "nonce": "1048576", "builtin": { "name": "identity", "pricing": { "linear": { "base": 15, "word": 3 } } } },
+		"102e61f5d8f9bc71d0ad4a084df4e65e05ce0e1c": { "balance": "1606938044258990275541962092341162602522202993782792835301376", "nonce": "1048576" }
+	}
+}

ethcore/spec/src/chain.rs

@@ -102,6 +102,7 @@ bundle_test_spec! {
 	"null" => new_null,
 	"null_morden" => new_test,
 	"null_morden_with_reward" => new_test_with_reward,
+	"null_morden_with_finality" => new_test_with_finality,
 	"validator_contract" => new_validator_contract,
 	"validator_multi" => new_validator_multi,
 	"validator_safe_contract" => new_validator_safe_contract

ethcore/src/client/client.rs

@@ -2644,13 +2644,13 @@ mod tests {
 		receipt::{Receipt, LocalizedReceipt, TransactionOutcome},
 		transaction::{Transaction, LocalizedTransaction, Action},
 	};
-	use test_helpers::{generate_dummy_client, get_good_dummy_block_hash, generate_dummy_client_with_data};
+	use test_helpers::{generate_dummy_client, generate_dummy_client_with_data, generate_dummy_client_with_spec_and_data, get_good_dummy_block_hash};
 	use std::thread;
 	use std::time::Duration;
 	use std::sync::Arc;
 	use std::sync::atomic::{AtomicBool, Ordering};
 	use kvdb::DBTransaction;
-	use blockchain::ExtrasInsert;
+	use blockchain::{BlockProvider, ExtrasInsert};
 	use hash::keccak;
 	use super::transaction_receipt;
 	use ethkey::KeyPair;
@@ -2785,4 +2785,22 @@ mod tests {
 			outcome: TransactionOutcome::StateRoot(state_root),
 		});
 	}
+
+	#[test]
+	fn should_mark_finalization_correctly_for_parent() {
+		let client = generate_dummy_client_with_spec_and_data(spec::new_test_with_finality, 2, 0, &[]);
+		let chain = client.chain();
+
+		let block1_details = chain.block_hash(1).and_then(|h| chain.block_details(&h));
+		assert!(block1_details.is_some());
+		let block1_details = block1_details.unwrap();
+		assert_eq!(block1_details.children.len(), 1);
+		assert!(block1_details.is_finalized);
+
+		let block2_details = chain.block_hash(2).and_then(|h| chain.block_details(&h));
+		assert!(block2_details.is_some());
+		let block2_details = block2_details.unwrap();
+		assert_eq!(block2_details.children.len(), 0);
+		assert!(!block2_details.is_finalized);
+	}
 }

json/src/spec/null_engine.rs

@@ -25,6 +25,8 @@ use uint::Uint;
 pub struct NullEngineParams {
 	/// Block reward.
 	pub block_reward: Option<Uint>,
+	/// Immediate finalization.
+	pub immediate_finalization: Option<bool>
 }
 
 /// Null engine descriptor