From a89bbfe366227599a5beac142c509a398de80873 Mon Sep 17 00:00:00 2001 From: Atkins Date: Fri, 6 Sep 2019 01:27:05 +0800 Subject: [PATCH] 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 | 29 ++++++++++++++--- ethcore/db/src/db.rs | 15 +++++++++ ethcore/engines/null-engine/src/lib.rs | 18 +++++++++- ethcore/res/null_morden_with_finality.json | 38 ++++++++++++++++++++++ ethcore/spec/src/chain.rs | 1 + ethcore/src/client/client.rs | 22 +++++++++++-- json/src/spec/null_engine.rs | 2 ++ 7 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 ethcore/res/null_morden_with_finality.json diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 05aae018a..89f82a8ff 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/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; +} + #[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 { + 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 { // 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 diff --git a/ethcore/db/src/db.rs b/ethcore/db/src/db.rs index 8f5390639..47f3f695f 100644 --- a/ethcore/db/src/db.rs +++ b/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(&self, col: Option, l1_cache: &RwLock, l2_cache: &RwLock, key: &K) -> Option where + K: Key + Eq + Hash + Clone, + T: Clone + rlp::Decodable, + C: Cache { + { + 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(&self, col: Option, key: &dyn Key) -> bool where R: AsRef<[u8]>; diff --git a/ethcore/engines/null-engine/src/lib.rs b/ethcore/engines/null-engine/src/lib.rs index 64b39cc97..8696f0cb3 100644 --- a/ethcore/engines/null-engine/src/lib.rs +++ b/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 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) -> Vec { + if self.params.immediate_finalization { + // always mark parent finalized + ancestry.take(1).map(|e| AncestryAction::MarkFinalized(e.header.hash())).collect() + } else { + Vec::new() + } + } } diff --git a/ethcore/res/null_morden_with_finality.json b/ethcore/res/null_morden_with_finality.json new file mode 100644 index 000000000..ea503ed29 --- /dev/null +++ b/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" } + } +} diff --git a/ethcore/spec/src/chain.rs b/ethcore/spec/src/chain.rs index 4fd9b4d30..b1c5945ab 100644 --- a/ethcore/spec/src/chain.rs +++ b/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 diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 000a0cd75..c41eeb5f7 100644 --- a/ethcore/src/client/client.rs +++ b/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); + } } diff --git a/json/src/spec/null_engine.rs b/json/src/spec/null_engine.rs index bf7574980..37ade4783 100644 --- a/json/src/spec/null_engine.rs +++ b/json/src/spec/null_engine.rs @@ -25,6 +25,8 @@ use uint::Uint; pub struct NullEngineParams { /// Block reward. pub block_reward: Option, + /// Immediate finalization. + pub immediate_finalization: Option } /// Null engine descriptor