From 111c87bf4414a6396b22f49354fbb525f7e5ca3d Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Mon, 25 Nov 2019 21:03:09 +0900 Subject: [PATCH 1/9] Remove redundant variable in tendermint/engine --- core/src/consensus/tendermint/engine.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 3261e087a5..6039bd647c 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -173,8 +173,7 @@ impl ConsensusEngine for Tendermint { return Ok(()) } - let block_author = *block.header().author(); - stake::update_validator_weights(&mut block.state_mut(), &block_author)?; + stake::update_validator_weights(&mut block.state_mut(), &author)?; stake::add_intermediate_rewards(block.state_mut(), author, block_author_reward)?; From 769c11d022a5f09ced702a4721305d0dbade923e Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Mon, 25 Nov 2019 21:05:59 +0900 Subject: [PATCH 2/9] Change block_number_if_term_changed to is_term_changed `block_number_if_term_changed` was returning the current block number if the term changed, which could be calculated in the outer scope. --- core/src/consensus/tendermint/engine.rs | 37 +++++++++---------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 6039bd647c..188e554899 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -25,7 +25,7 @@ use ckey::{public_to_address, Address}; use cnetwork::NetworkService; use crossbeam_channel as crossbeam; use cstate::{ActionHandler, TopStateView}; -use ctypes::{BlockHash, BlockNumber, CommonParams, Header}; +use ctypes::{BlockHash, CommonParams, Header}; use num_rational::Ratio; use super::super::stake; @@ -141,10 +141,11 @@ impl ConsensusEngine for Tendermint { parent_common_params: &CommonParams, term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { + let block_number = block.header().number(); let author = *block.header().author(); let (total_reward, total_min_fee) = { let transactions = block.transactions(); - let block_reward = self.block_reward(block.header().number()); + let block_reward = self.block_reward(block_number); let total_min_fee: u64 = transactions.iter().map(|tx| tx.fee).sum(); let min_fee = transactions.iter().map(|tx| CodeChainMachine::min_cost(&parent_common_params, &tx.action)).sum(); @@ -164,9 +165,7 @@ impl ConsensusEngine for Tendermint { if metadata.current_term_id() == 0 { self.machine.add_balance(block, &author, block_author_reward)?; - if let Some(block_number) = - block_number_if_term_changed(block.header(), parent_header, parent_common_params) - { + if is_term_changed(block.header(), parent_header, parent_common_params) { // First term change stake::on_term_close(block.state_mut(), block_number, &[])?; } @@ -178,13 +177,9 @@ impl ConsensusEngine for Tendermint { stake::add_intermediate_rewards(block.state_mut(), author, block_author_reward)?; let term_common_params = term_common_params.expect("TermCommonParams should exist"); - let last_term_finished_block_num = if let Some(block_number) = - block_number_if_term_changed(block.header(), parent_header, term_common_params) - { - block_number - } else { + if !is_term_changed(block.header(), parent_header, term_common_params) { return Ok(()) - }; + } let rewards = stake::drain_previous_rewards(&mut block.state_mut())?; let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; @@ -213,7 +208,7 @@ impl ConsensusEngine for Tendermint { }; let banned = stake::Banned::load_from_state(block.state())?; - let start_of_the_current_term_header = if block.header().number() == start_of_the_current_term { + let start_of_the_current_term_header = if block_number == start_of_the_current_term { encoded::Header::new(block.header().clone().rlp_bytes().to_vec()) } else { client.block_header(&start_of_the_current_term.into()).unwrap() @@ -241,7 +236,7 @@ impl ConsensusEngine for Tendermint { }; stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; - stake::on_term_close(block.state_mut(), last_term_finished_block_num, &inactive_validators)?; + stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?; Ok(()) } @@ -342,22 +337,16 @@ impl ConsensusEngine for Tendermint { } } -fn block_number_if_term_changed( - header: &Header, - parent_header: &Header, - common_params: &CommonParams, -) -> Option { +fn is_term_changed(header: &Header, parent: &Header, common_params: &CommonParams) -> bool { let term_seconds = common_params.term_seconds(); if term_seconds == 0 { - return None + return false } let current_term_period = header.timestamp() / term_seconds; - let parent_term_period = parent_header.timestamp() / term_seconds; - if current_term_period == parent_term_period { - return None - } - Some(header.number()) + let parent_term_period = parent.timestamp() / term_seconds; + + current_term_period != parent_term_period } fn inactive_validators( From 3d461104eb325e4dac0eb5e4716a02075740354d Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Mon, 25 Nov 2019 21:13:28 +0900 Subject: [PATCH 3/9] Refactor `is_term_changed` check in on_close_block --- core/src/consensus/tendermint/engine.rs | 149 +++++++++++++----------- 1 file changed, 79 insertions(+), 70 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 188e554899..fb76d2baf3 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -162,82 +162,92 @@ impl ConsensusEngine for Tendermint { let block_author_reward = total_reward - total_min_fee + distributor.remaining_fee(); let metadata = block.state().metadata()?.expect("Metadata must exist"); - if metadata.current_term_id() == 0 { - self.machine.add_balance(block, &author, block_author_reward)?; + let term = metadata.current_term_id(); + let term_seconds = match term { + 0 => parent_common_params.term_seconds(), + _ => term_common_params.expect("TermCommonParams should exist").term_seconds(), + }; - if is_term_changed(block.header(), parent_header, parent_common_params) { - // First term change - stake::on_term_close(block.state_mut(), block_number, &[])?; + match term { + 0 => { + self.machine.add_balance(block, &author, block_author_reward)?; + } + _ => { + stake::update_validator_weights(&mut block.state_mut(), &author)?; + stake::add_intermediate_rewards(block.state_mut(), author, block_author_reward)?; } - return Ok(()) } - stake::update_validator_weights(&mut block.state_mut(), &author)?; - - stake::add_intermediate_rewards(block.state_mut(), author, block_author_reward)?; - - let term_common_params = term_common_params.expect("TermCommonParams should exist"); - if !is_term_changed(block.header(), parent_header, term_common_params) { + if !is_term_changed(block.header(), parent_header, term_seconds) { return Ok(()) } - let rewards = stake::drain_previous_rewards(&mut block.state_mut())?; - let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; - let client = self - .client - .read() - .as_ref() - .ok_or(EngineError::CannotOpenBlock)? - .upgrade() - .ok_or(EngineError::CannotOpenBlock)?; - - let inactive_validators = if metadata.current_term_id() == 1 { - assert!(rewards.is_empty()); - - let validators = stake::Validators::load_from_state(block.state())? - .into_iter() - .map(|val| public_to_address(val.pubkey())) - .collect(); - inactive_validators(&*client, start_of_the_current_term, block.header(), validators) - } else { - let start_of_the_previous_term = { - let end_of_the_two_level_previous_term = - client.last_term_finished_block_num((metadata.last_term_finished_block_num() - 1).into()).unwrap(); - - end_of_the_two_level_previous_term + 1 - }; - - let banned = stake::Banned::load_from_state(block.state())?; - let start_of_the_current_term_header = if block_number == start_of_the_current_term { - encoded::Header::new(block.header().clone().rlp_bytes().to_vec()) - } else { - client.block_header(&start_of_the_current_term.into()).unwrap() - }; - - let pending_rewards = calculate_pending_rewards_of_the_previous_term( - &*client, - &*self.validators, - rewards, - start_of_the_current_term, - start_of_the_current_term_header, - start_of_the_previous_term, - &banned, - )?; - - for (address, reward) in pending_rewards { - self.machine.add_balance(block, &address, reward)?; + match term { + 0 => { + // First term change + stake::on_term_close(block.state_mut(), block_number, &[])?; } - - let validators = stake::Validators::load_from_state(block.state())? - .into_iter() - .map(|val| public_to_address(val.pubkey())) - .collect(); - inactive_validators(&*client, start_of_the_current_term, block.header(), validators) - }; - - stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; - stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?; - + _ => { + let rewards = stake::drain_previous_rewards(&mut block.state_mut())?; + + let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; + let client = self + .client + .read() + .as_ref() + .ok_or(EngineError::CannotOpenBlock)? + .upgrade() + .ok_or(EngineError::CannotOpenBlock)?; + + let inactive_validators = if term == 1 { + assert!(rewards.is_empty()); + + let validators = stake::Validators::load_from_state(block.state())? + .into_iter() + .map(|val| public_to_address(val.pubkey())) + .collect(); + inactive_validators(&*client, start_of_the_current_term, block.header(), validators) + } else { + let start_of_the_previous_term = { + let end_of_the_two_level_previous_term = client + .last_term_finished_block_num((metadata.last_term_finished_block_num() - 1).into()) + .unwrap(); + + end_of_the_two_level_previous_term + 1 + }; + + let banned = stake::Banned::load_from_state(block.state())?; + let start_of_the_current_term_header = if block_number == start_of_the_current_term { + encoded::Header::new(block.header().clone().rlp_bytes().to_vec()) + } else { + client.block_header(&start_of_the_current_term.into()).unwrap() + }; + + let pending_rewards = calculate_pending_rewards_of_the_previous_term( + &*client, + &*self.validators, + rewards, + start_of_the_current_term, + start_of_the_current_term_header, + start_of_the_previous_term, + &banned, + )?; + + for (address, reward) in pending_rewards { + self.machine.add_balance(block, &address, reward)?; + } + + let validators = stake::Validators::load_from_state(block.state())? + .into_iter() + .map(|val| public_to_address(val.pubkey())) + .collect(); + inactive_validators(&*client, start_of_the_current_term, block.header(), validators) + }; + + stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; + stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?; + } + } Ok(()) } @@ -337,8 +347,7 @@ impl ConsensusEngine for Tendermint { } } -fn is_term_changed(header: &Header, parent: &Header, common_params: &CommonParams) -> bool { - let term_seconds = common_params.term_seconds(); +fn is_term_changed(header: &Header, parent: &Header, term_seconds: u64) -> bool { if term_seconds == 0 { return false } From 45a7001d0c03ec6af58d7eca7465f42ea8f926fa Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Mon, 25 Nov 2019 21:18:36 +0900 Subject: [PATCH 4/9] Refactor `on_term_close` call in on_close_block --- core/src/consensus/tendermint/engine.rs | 38 +++++++++---------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index fb76d2baf3..1f78c80b23 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -182,14 +182,10 @@ impl ConsensusEngine for Tendermint { return Ok(()) } - match term { - 0 => { - // First term change - stake::on_term_close(block.state_mut(), block_number, &[])?; - } + let inactive_validators = match term { + 0 => Vec::new(), _ => { let rewards = stake::drain_previous_rewards(&mut block.state_mut())?; - let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; let client = self .client @@ -199,15 +195,7 @@ impl ConsensusEngine for Tendermint { .upgrade() .ok_or(EngineError::CannotOpenBlock)?; - let inactive_validators = if term == 1 { - assert!(rewards.is_empty()); - - let validators = stake::Validators::load_from_state(block.state())? - .into_iter() - .map(|val| public_to_address(val.pubkey())) - .collect(); - inactive_validators(&*client, start_of_the_current_term, block.header(), validators) - } else { + if term > 1 { let start_of_the_previous_term = { let end_of_the_two_level_previous_term = client .last_term_finished_block_num((metadata.last_term_finished_block_num() - 1).into()) @@ -236,18 +224,20 @@ impl ConsensusEngine for Tendermint { for (address, reward) in pending_rewards { self.machine.add_balance(block, &address, reward)?; } - - let validators = stake::Validators::load_from_state(block.state())? - .into_iter() - .map(|val| public_to_address(val.pubkey())) - .collect(); - inactive_validators(&*client, start_of_the_current_term, block.header(), validators) - }; + } stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; - stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?; + + let validators = stake::Validators::load_from_state(block.state())? + .into_iter() + .map(|val| public_to_address(val.pubkey())) + .collect(); + inactive_validators(&*client, start_of_the_current_term, block.header(), validators) } - } + }; + + stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?; + Ok(()) } From 4b187858015b748413440bf99a23cfff9674c7e4 Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Mon, 25 Nov 2019 21:24:27 +0900 Subject: [PATCH 5/9] Remove unnecessary mutable borrow --- core/src/consensus/tendermint/engine.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 1f78c80b23..4cb410751a 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -173,7 +173,7 @@ impl ConsensusEngine for Tendermint { self.machine.add_balance(block, &author, block_author_reward)?; } _ => { - stake::update_validator_weights(&mut block.state_mut(), &author)?; + stake::update_validator_weights(block.state_mut(), &author)?; stake::add_intermediate_rewards(block.state_mut(), author, block_author_reward)?; } } @@ -185,7 +185,7 @@ impl ConsensusEngine for Tendermint { let inactive_validators = match term { 0 => Vec::new(), _ => { - let rewards = stake::drain_previous_rewards(&mut block.state_mut())?; + let rewards = stake::drain_previous_rewards(block.state_mut())?; let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; let client = self .client @@ -226,7 +226,7 @@ impl ConsensusEngine for Tendermint { } } - stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; + stake::move_current_to_previous_intermediate_rewards(block.state_mut())?; let validators = stake::Validators::load_from_state(block.state())? .into_iter() From b200c857243450ecce566475c3ea95072342ab30 Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Tue, 26 Nov 2019 17:43:42 +0900 Subject: [PATCH 6/9] Include ConsensusClient in Solo engine --- core/src/consensus/solo/mod.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/core/src/consensus/solo/mod.rs b/core/src/consensus/solo/mod.rs index 6faf5dc0d5..1a50b18920 100644 --- a/core/src/consensus/solo/mod.rs +++ b/core/src/consensus/solo/mod.rs @@ -16,22 +16,25 @@ mod params; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use ckey::Address; use cstate::{ActionHandler, HitHandler}; use ctypes::{CommonParams, Header}; +use parking_lot::RwLock; use self::params::SoloParams; use super::stake; use super::{ConsensusEngine, Seal}; use crate::block::{ExecutedBlock, IsBlock}; +use crate::client::ConsensusClient; use crate::codechain_machine::CodeChainMachine; use crate::consensus::{EngineError, EngineType}; use crate::error::Error; /// A consensus engine which does not provide any consensus mechanism. pub struct Solo { + client: RwLock>>, params: SoloParams, machine: CodeChainMachine, action_handlers: Vec>, @@ -47,6 +50,7 @@ impl Solo { action_handlers.push(Arc::new(stake::Stake::new(params.genesis_stakes.clone()))); Solo { + client: Default::default(), params, machine, action_handlers, @@ -127,6 +131,10 @@ impl ConsensusEngine for Solo { Ok(()) } + fn register_client(&self, client: Weak) { + *self.client.write() = Some(Weak::clone(&client)); + } + fn block_reward(&self, _block_number: u64) -> u64 { self.params.block_reward } @@ -146,25 +154,30 @@ impl ConsensusEngine for Solo { #[cfg(test)] mod tests { + use std::sync::Arc; + use ctypes::{CommonParams, Header}; use primitives::H520; use crate::block::{IsBlock, OpenBlock}; + use crate::client::{ConsensusClient, TestBlockChainClient}; use crate::scheme::Scheme; use crate::tests::helpers::get_temp_state_db; #[test] fn seal() { let scheme = Scheme::new_test_solo(); - let engine = &*scheme.engine; - let db = scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); - let genesis_header = scheme.genesis_header(); - let b = OpenBlock::try_new(engine, db, &genesis_header, Default::default(), vec![]).unwrap(); + let client = Arc::new(TestBlockChainClient::new_with_scheme(scheme)); + let engine = client.scheme.engine.clone(); + engine.register_client(Arc::downgrade(&(client.clone() as Arc))); + let db = client.scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); + let genesis_header = client.scheme.genesis_header(); + let b = OpenBlock::try_new(&*engine, db, &genesis_header, Default::default(), vec![]).unwrap(); let parent_common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); let b = b.close_and_lock(&genesis_header, &parent_common_params, Some(&term_common_params)).unwrap(); if let Some(seal) = engine.generate_seal(Some(b.block()), &genesis_header).seal_fields() { - assert!(b.try_seal(engine, seal).is_ok()); + assert!(b.try_seal(&*engine, seal).is_ok()); } } From b398eb12a4515a461c23e0738e853723d725e4e3 Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Tue, 26 Nov 2019 18:17:08 +0900 Subject: [PATCH 7/9] Add a helper function for obtaining client --- core/src/consensus/tendermint/engine.rs | 18 +++--------------- core/src/consensus/tendermint/mod.rs | 4 ++++ 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 4cb410751a..b6ff3dcdea 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -187,13 +187,7 @@ impl ConsensusEngine for Tendermint { _ => { let rewards = stake::drain_previous_rewards(block.state_mut())?; let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; - let client = self - .client - .read() - .as_ref() - .ok_or(EngineError::CannotOpenBlock)? - .upgrade() - .ok_or(EngineError::CannotOpenBlock)?; + let client = self.client().ok_or(EngineError::CannotOpenBlock)?; if term > 1 { let start_of_the_previous_term = { @@ -273,7 +267,7 @@ impl ConsensusEngine for Tendermint { let inner = self.inner.clone(); let extension = service.register_extension(move |api| TendermintExtension::new(inner, timeouts, api)); - let client = Weak::clone(self.client.read().as_ref().unwrap()); + let client = Arc::downgrade(&self.client().unwrap()); self.extension_initializer.send((extension, client)).unwrap(); let (result, receiver) = crossbeam::bounded(1); @@ -317,13 +311,7 @@ impl ConsensusEngine for Tendermint { } fn possible_authors(&self, block_number: Option) -> Result>, EngineError> { - let client = self - .client - .read() - .as_ref() - .ok_or(EngineError::CannotOpenBlock)? - .upgrade() - .ok_or(EngineError::CannotOpenBlock)?; + let client = self.client().ok_or(EngineError::CannotOpenBlock)?; let block_hash = match block_number { None => { client.block_header(&BlockId::Latest).expect("latest block must exist").hash() // the latest block diff --git a/core/src/consensus/tendermint/mod.rs b/core/src/consensus/tendermint/mod.rs index 85edcd6a9a..73f1c5fc2a 100644 --- a/core/src/consensus/tendermint/mod.rs +++ b/core/src/consensus/tendermint/mod.rs @@ -115,6 +115,10 @@ impl Tendermint { has_signer: false.into(), }) } + + fn client(&self) -> Option> { + self.client.read().as_ref()?.upgrade() + } } const SEAL_FIELDS: usize = 4; From 73f0a761148ec9c9ed578e5def5deccacd6ce7a3 Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Tue, 26 Nov 2019 18:35:17 +0900 Subject: [PATCH 8/9] Derive parent block information inside consensus engine --- core/src/block.rs | 36 +++++++++---------------- core/src/client/test_client.rs | 2 +- core/src/consensus/blake_pow/mod.rs | 2 -- core/src/consensus/cuckoo/mod.rs | 12 +-------- core/src/consensus/mod.rs | 2 -- core/src/consensus/null_engine/mod.rs | 4 +-- core/src/consensus/simple_poa/mod.rs | 5 +--- core/src/consensus/solo/mod.rs | 16 +++++++---- core/src/consensus/stake/mod.rs | 8 +----- core/src/consensus/tendermint/engine.rs | 13 +++++---- core/src/consensus/tendermint/mod.rs | 3 +-- core/src/miner/miner.rs | 3 +-- core/src/miner/sealing_queue.rs | 3 +-- state/src/action_handler/hit.rs | 8 +----- state/src/action_handler/mod.rs | 8 +----- 15 files changed, 41 insertions(+), 84 deletions(-) diff --git a/core/src/block.rs b/core/src/block.rs index 103d5903a7..7d5a5dea68 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -220,25 +220,20 @@ impl<'x> OpenBlock<'x> { pub fn close( mut self, parent_header: &Header, - parent_common_params: &CommonParams, term_common_params: Option<&CommonParams>, ) -> Result { let unclosed_state = self.block.state.clone(); - if let Err(e) = - self.engine.on_close_block(&mut self.block, parent_header, parent_common_params, term_common_params) - { + if let Err(e) = self.engine.on_close_block(&mut self.block, term_common_params) { warn!("Encountered error on closing the block: {}", e); return Err(e) } let header = self.block.header().clone(); for handler in self.engine.action_handlers() { - handler.on_close_block(self.block.state_mut(), &header, parent_header, parent_common_params).map_err( - |e| { - warn!("Encountered error in {}::on_close_block", handler.name()); - e - }, - )?; + handler.on_close_block(self.block.state_mut(), &header).map_err(|e| { + warn!("Encountered error in {}::on_close_block", handler.name()); + e + })?; } let state_root = self.block.state.commit().map_err(|e| { @@ -262,23 +257,18 @@ impl<'x> OpenBlock<'x> { pub fn close_and_lock( mut self, parent_header: &Header, - parent_common_params: &CommonParams, term_common_params: Option<&CommonParams>, ) -> Result { - if let Err(e) = - self.engine.on_close_block(&mut self.block, parent_header, parent_common_params, term_common_params) - { + if let Err(e) = self.engine.on_close_block(&mut self.block, term_common_params) { warn!("Encountered error on closing the block: {}", e); return Err(e) } let header = self.block.header().clone(); for handler in self.engine.action_handlers() { - handler.on_close_block(self.block.state_mut(), &header, parent_header, parent_common_params).map_err( - |e| { - warn!("Encountered error in {}::on_close_block", handler.name()); - e - }, - )?; + handler.on_close_block(self.block.state_mut(), &header).map_err(|e| { + warn!("Encountered error in {}::on_close_block", handler.name()); + e + })?; } let state_root = self.block.state.commit().map_err(|e| { @@ -503,7 +493,6 @@ pub fn enact( b.populate_from(header); b.push_transactions(transactions, client, parent.number(), parent.timestamp())?; - let parent_common_params = client.common_params((*header.parent_hash()).into()).unwrap(); let term_common_params = { let block_number = client .last_term_finished_block_num((*header.parent_hash()).into()) @@ -514,7 +503,7 @@ pub fn enact( Some(client.common_params((block_number).into()).expect("Common params should exist")) } }; - b.close_and_lock(parent, &parent_common_params, term_common_params.as_ref()) + b.close_and_lock(parent, term_common_params.as_ref()) } #[cfg(test)] @@ -532,9 +521,8 @@ mod tests { let genesis_header = scheme.genesis_header(); let db = scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); let b = OpenBlock::try_new(&*scheme.engine, db, &genesis_header, Address::default(), vec![]).unwrap(); - let parent_common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); - let b = b.close_and_lock(&genesis_header, &parent_common_params, Some(&term_common_params)).unwrap(); + let b = b.close_and_lock(&genesis_header, Some(&term_common_params)).unwrap(); let _ = b.seal(&*scheme.engine, vec![]); } } diff --git a/core/src/client/test_client.rs b/core/src/client/test_client.rs index 0a279626e7..31775914f8 100644 --- a/core/src/client/test_client.rs +++ b/core/src/client/test_client.rs @@ -645,7 +645,7 @@ impl super::EngineClient for TestBlockChainClient { impl EngineInfo for TestBlockChainClient { fn common_params(&self, _block_id: BlockId) -> Option { - unimplemented!() + Some(*self.scheme.engine.machine().genesis_common_params()) } fn metadata_seq(&self, _block_id: BlockId) -> Option { diff --git a/core/src/consensus/blake_pow/mod.rs b/core/src/consensus/blake_pow/mod.rs index 8e0b20a6e3..010fa35ae6 100644 --- a/core/src/consensus/blake_pow/mod.rs +++ b/core/src/consensus/blake_pow/mod.rs @@ -163,8 +163,6 @@ impl ConsensusEngine for BlakePoW { fn on_close_block( &self, block: &mut ExecutedBlock, - _parent_header: &Header, - _parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { let author = *block.header().author(); diff --git a/core/src/consensus/cuckoo/mod.rs b/core/src/consensus/cuckoo/mod.rs index ad088f2454..d904120d67 100644 --- a/core/src/consensus/cuckoo/mod.rs +++ b/core/src/consensus/cuckoo/mod.rs @@ -173,8 +173,6 @@ impl ConsensusEngine for Cuckoo { fn on_close_block( &self, block: &mut ExecutedBlock, - _parent_header: &Header, - _parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { let author = *block.header().author(); @@ -261,21 +259,13 @@ mod tests { #[test] fn on_close_block() { let scheme = Scheme::new_test_cuckoo(); - let genesis_header = scheme.genesis_header(); let engine = &*scheme.engine; let db = scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); let header = Header::default(); let block = OpenBlock::try_new(engine, db, &header, Default::default(), vec![]).unwrap(); let mut executed_block = block.block().clone(); - assert!(engine - .on_close_block( - &mut executed_block, - &genesis_header, - &CommonParams::default_for_test(), - Some(&CommonParams::default_for_test()) - ) - .is_ok()); + assert!(engine.on_close_block(&mut executed_block, Some(&CommonParams::default_for_test())).is_ok()); assert_eq!(0xd, engine.machine().balance(&executed_block, header.author()).unwrap()); } diff --git a/core/src/consensus/mod.rs b/core/src/consensus/mod.rs index 6841b859d4..c0d78b1485 100644 --- a/core/src/consensus/mod.rs +++ b/core/src/consensus/mod.rs @@ -225,8 +225,6 @@ pub trait ConsensusEngine: Sync + Send { fn on_close_block( &self, _block: &mut ExecutedBlock, - _parent_header: &Header, - _parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { Ok(()) diff --git a/core/src/consensus/null_engine/mod.rs b/core/src/consensus/null_engine/mod.rs index 832f6165b7..26f40199d1 100644 --- a/core/src/consensus/null_engine/mod.rs +++ b/core/src/consensus/null_engine/mod.rs @@ -17,7 +17,7 @@ mod params; use ckey::Address; -use ctypes::{CommonParams, Header}; +use ctypes::CommonParams; use self::params::NullEngineParams; use super::ConsensusEngine; @@ -58,8 +58,6 @@ impl ConsensusEngine for NullEngine { fn on_close_block( &self, block: &mut ExecutedBlock, - _parent_header: &Header, - _parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { let (author, total_reward) = { diff --git a/core/src/consensus/simple_poa/mod.rs b/core/src/consensus/simple_poa/mod.rs index 848af86b70..a6b72120b8 100644 --- a/core/src/consensus/simple_poa/mod.rs +++ b/core/src/consensus/simple_poa/mod.rs @@ -121,8 +121,6 @@ impl ConsensusEngine for SimplePoA { fn on_close_block( &self, block: &mut ExecutedBlock, - _parent_header: &Header, - _parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { let author = *block.header().author(); @@ -186,9 +184,8 @@ mod tests { let db = scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); let genesis_header = scheme.genesis_header(); let b = OpenBlock::try_new(engine, db, &genesis_header, Default::default(), vec![]).unwrap(); - let parent_common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); - let b = b.close_and_lock(&genesis_header, &parent_common_params, Some(&term_common_params)).unwrap(); + let b = b.close_and_lock(&genesis_header, Some(&term_common_params)).unwrap(); if let Some(seal) = engine.generate_seal(Some(b.block()), &genesis_header).seal_fields() { assert!(b.try_seal(engine, seal).is_ok()); } diff --git a/core/src/consensus/solo/mod.rs b/core/src/consensus/solo/mod.rs index 1a50b18920..414e45a00f 100644 --- a/core/src/consensus/solo/mod.rs +++ b/core/src/consensus/solo/mod.rs @@ -56,6 +56,10 @@ impl Solo { action_handlers, } } + + fn client(&self) -> Option> { + self.client.read().as_ref()?.upgrade() + } } impl ConsensusEngine for Solo { @@ -82,10 +86,13 @@ impl ConsensusEngine for Solo { fn on_close_block( &self, block: &mut ExecutedBlock, - parent_header: &Header, - parent_common_params: &CommonParams, _term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { + let client = self.client().ok_or(EngineError::CannotOpenBlock)?; + + let parent_hash = *block.header().parent_hash(); + let parent = client.block_header(&parent_hash.into()).expect("Parent header must exist"); + let parent_common_params = client.common_params(parent_hash.into()).expect("CommonParams of parent must exist"); let author = *block.header().author(); let (total_reward, total_min_fee) = { let transactions = block.transactions(); @@ -115,7 +122,7 @@ impl ConsensusEngine for Solo { let last_term_finished_block_num = { let header = block.header(); let current_term_period = header.timestamp() / term_seconds; - let parent_term_period = parent_header.timestamp() / term_seconds; + let parent_term_period = parent.timestamp() / term_seconds; if current_term_period == parent_term_period { return Ok(()) } @@ -173,9 +180,8 @@ mod tests { let db = client.scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); let genesis_header = client.scheme.genesis_header(); let b = OpenBlock::try_new(&*engine, db, &genesis_header, Default::default(), vec![]).unwrap(); - let parent_common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); - let b = b.close_and_lock(&genesis_header, &parent_common_params, Some(&term_common_params)).unwrap(); + let b = b.close_and_lock(&genesis_header, Some(&term_common_params)).unwrap(); if let Some(seal) = engine.generate_seal(Some(b.block()), &genesis_header).seal_fields() { assert!(b.try_seal(&*engine, seal).is_ok()); } diff --git a/core/src/consensus/stake/mod.rs b/core/src/consensus/stake/mod.rs index cf0a07022a..02789499fe 100644 --- a/core/src/consensus/stake/mod.rs +++ b/core/src/consensus/stake/mod.rs @@ -156,13 +156,7 @@ impl ActionHandler for Stake { action.verify(current_params, client, validators) } - fn on_close_block( - &self, - _state: &mut TopLevelState, - _header: &Header, - _parent_header: &Header, - _parent_common_params: &CommonParams, - ) -> StateResult<()> { + fn on_close_block(&self, _state: &mut TopLevelState, _header: &Header) -> StateResult<()> { Ok(()) } } diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index b6ff3dcdea..3ea3282c7a 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -137,12 +137,16 @@ impl ConsensusEngine for Tendermint { fn on_close_block( &self, block: &mut ExecutedBlock, - parent_header: &Header, - parent_common_params: &CommonParams, term_common_params: Option<&CommonParams>, ) -> Result<(), Error> { - let block_number = block.header().number(); + let client = self.client().ok_or(EngineError::CannotOpenBlock)?; + + let parent_hash = *block.header().parent_hash(); + let parent = client.block_header(&parent_hash.into()).expect("Parent header must exist").decode(); + let parent_common_params = client.common_params(parent_hash.into()).expect("CommonParams of parent must exist"); let author = *block.header().author(); + let block_number = block.header().number(); + let (total_reward, total_min_fee) = { let transactions = block.transactions(); let block_reward = self.block_reward(block_number); @@ -178,7 +182,7 @@ impl ConsensusEngine for Tendermint { } } - if !is_term_changed(block.header(), parent_header, term_seconds) { + if !is_term_changed(block.header(), &parent, term_seconds) { return Ok(()) } @@ -187,7 +191,6 @@ impl ConsensusEngine for Tendermint { _ => { let rewards = stake::drain_previous_rewards(block.state_mut())?; let start_of_the_current_term = metadata.last_term_finished_block_num() + 1; - let client = self.client().ok_or(EngineError::CannotOpenBlock)?; if term > 1 { let start_of_the_previous_term = { diff --git a/core/src/consensus/tendermint/mod.rs b/core/src/consensus/tendermint/mod.rs index 73f1c5fc2a..721ce1cb34 100644 --- a/core/src/consensus/tendermint/mod.rs +++ b/core/src/consensus/tendermint/mod.rs @@ -161,9 +161,8 @@ mod tests { let genesis_header = scheme.genesis_header(); let b = OpenBlock::try_new(scheme.engine.as_ref(), db, &genesis_header, proposer, vec![]).unwrap(); let seal = scheme.engine.generate_seal(None, &genesis_header).seal_fields().unwrap(); - let common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); - let b = b.close(&genesis_header, &common_params, Some(&term_common_params)).unwrap(); + let b = b.close(&genesis_header, Some(&term_common_params)).unwrap(); (b, seal) } diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 8c7c446262..6dc64bb987 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -603,7 +603,6 @@ impl Miner { let parent_header = chain.block_header(&parent_hash.into()).expect("Parent header MUST exist"); (parent_header.decode(), parent_hash) }; - let parent_common_params = chain.common_params(parent_hash.into()).unwrap(); let term_common_params = { let block_number = chain .last_term_finished_block_num(parent_hash.into()) @@ -614,7 +613,7 @@ impl Miner { Some(chain.common_params((block_number).into()).expect("Common params should exist")) } }; - let block = open_block.close(&parent_header, &parent_common_params, term_common_params.as_ref())?; + let block = open_block.close(&parent_header, term_common_params.as_ref())?; let fetch_seq = |p: &Public| { let address = public_to_address(p); diff --git a/core/src/miner/sealing_queue.rs b/core/src/miner/sealing_queue.rs index 3866274d04..16f282367e 100644 --- a/core/src/miner/sealing_queue.rs +++ b/core/src/miner/sealing_queue.rs @@ -87,9 +87,8 @@ mod tests { let genesis_header = scheme.genesis_header(); let db = scheme.ensure_genesis_state(get_temp_state_db()).unwrap(); let b = OpenBlock::try_new(&*scheme.engine, db, &genesis_header, address, vec![]).unwrap(); - let common_params = CommonParams::default_for_test(); let term_common_params = CommonParams::default_for_test(); - b.close(&genesis_header, &common_params, Some(&term_common_params)).unwrap() + b.close(&genesis_header, Some(&term_common_params)).unwrap() } #[test] diff --git a/state/src/action_handler/hit.rs b/state/src/action_handler/hit.rs index 407524fe1f..7891b99e36 100644 --- a/state/src/action_handler/hit.rs +++ b/state/src/action_handler/hit.rs @@ -86,13 +86,7 @@ impl ActionHandler for HitHandler { Ok(()) } - fn on_close_block( - &self, - state: &mut TopLevelState, - _header: &Header, - _parent_header: &Header, - _parent_common_params: &CommonParams, - ) -> StateResult<()> { + fn on_close_block(&self, state: &mut TopLevelState, _header: &Header) -> StateResult<()> { let address = self.close_count(); let action_data = state.action_data(&address)?.unwrap_or_default(); let prev_counter: u32 = rlp::decode(&*action_data).unwrap(); diff --git a/state/src/action_handler/mod.rs b/state/src/action_handler/mod.rs index 519f8e62df..dd7c1da1ac 100644 --- a/state/src/action_handler/mod.rs +++ b/state/src/action_handler/mod.rs @@ -47,13 +47,7 @@ pub trait ActionHandler: Send + Sync { Ok(some_action_data) } - fn on_close_block( - &self, - state: &mut TopLevelState, - header: &Header, - parent_header: &Header, - parent_common_params: &CommonParams, - ) -> StateResult<()>; + fn on_close_block(&self, state: &mut TopLevelState, header: &Header) -> StateResult<()>; } pub trait FindActionHandler { From c66fb4bc73cf5704f73be95c523eacb9a8c03a60 Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 19 Nov 2019 10:23:43 +0900 Subject: [PATCH 9/9] Refactor term_common_params method --- core/src/block.rs | 12 ++---------- core/src/client/client.rs | 9 +++++++++ core/src/client/mod.rs | 1 + core/src/client/test_client.rs | 4 ++++ core/src/miner/miner.rs | 11 +---------- 5 files changed, 17 insertions(+), 20 deletions(-) diff --git a/core/src/block.rs b/core/src/block.rs index 7d5a5dea68..9a6584d090 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -33,6 +33,7 @@ use crate::client::{EngineInfo, TermInfo}; use crate::consensus::CodeChainEngine; use crate::error::{BlockError, Error}; use crate::transaction::{SignedTransaction, UnverifiedTransaction}; +use crate::BlockId; /// A block, encoded as it is on the block chain. #[derive(Debug, Clone, PartialEq)] @@ -493,16 +494,7 @@ pub fn enact( b.populate_from(header); b.push_transactions(transactions, client, parent.number(), parent.timestamp())?; - let term_common_params = { - let block_number = client - .last_term_finished_block_num((*header.parent_hash()).into()) - .expect("The block of the parent hash should exist"); - if block_number == 0 { - None - } else { - Some(client.common_params((block_number).into()).expect("Common params should exist")) - } - }; + let term_common_params = client.term_common_params(BlockId::Hash(*header.parent_hash())); b.close_and_lock(parent, term_common_params.as_ref()) } diff --git a/core/src/client/client.rs b/core/src/client/client.rs index d0617431c1..6bb39e5543 100644 --- a/core/src/client/client.rs +++ b/core/src/client/client.rs @@ -813,6 +813,15 @@ impl TermInfo for Client { .map(|state| state.metadata().unwrap().expect("Metadata always exist")) .map(|metadata| metadata.current_term_id()) } + + fn term_common_params(&self, id: BlockId) -> Option { + let block_number = self.last_term_finished_block_num(id).expect("The block of the parent hash should exist"); + if block_number == 0 { + None + } else { + Some(self.common_params((block_number).into()).expect("Common params should exist")) + } + } } impl AccountData for Client { diff --git a/core/src/client/mod.rs b/core/src/client/mod.rs index eb3adaf3b3..edb8abc76f 100644 --- a/core/src/client/mod.rs +++ b/core/src/client/mod.rs @@ -119,6 +119,7 @@ pub trait ConsensusClient: BlockChainClient + EngineClient + EngineInfo + TermIn pub trait TermInfo { fn last_term_finished_block_num(&self, id: BlockId) -> Option; fn current_term_id(&self, id: BlockId) -> Option; + fn term_common_params(&self, id: BlockId) -> Option; } /// Provides methods to access account info diff --git a/core/src/client/test_client.rs b/core/src/client/test_client.rs index 31775914f8..f14c2bf80a 100644 --- a/core/src/client/test_client.rs +++ b/core/src/client/test_client.rs @@ -679,6 +679,10 @@ impl TermInfo for TestBlockChainClient { fn current_term_id(&self, _id: BlockId) -> Option { self.term_id } + + fn term_common_params(&self, _id: BlockId) -> Option { + None + } } impl StateInfo for TestBlockChainClient { diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 6dc64bb987..08a2ba0a99 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -603,16 +603,7 @@ impl Miner { let parent_header = chain.block_header(&parent_hash.into()).expect("Parent header MUST exist"); (parent_header.decode(), parent_hash) }; - let term_common_params = { - let block_number = chain - .last_term_finished_block_num(parent_hash.into()) - .expect("The block of the parent hash should exist"); - if block_number == 0 { - None - } else { - Some(chain.common_params((block_number).into()).expect("Common params should exist")) - } - }; + let term_common_params = chain.term_common_params(parent_hash.into()); let block = open_block.close(&parent_header, term_common_params.as_ref())?; let fetch_seq = |p: &Public| {