Refactor on_close_block, take #2

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<ClosedBlock, Error> {
         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<LockedBlock, Error> {
-        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<C: ChainTimeInfo + EngineInfo + FindActionHandler + TermInfo>(
     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<C: ChainTimeInfo + EngineInfo + FindActionHandler + TermInfo>(
             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![]);
     }
 }

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<CommonParams> {
-        unimplemented!()
+        Some(*self.scheme.engine.machine().genesis_common_params())
     }
 
     fn metadata_seq(&self, _block_id: BlockId) -> Option<u64> {

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();

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());
     }
 

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(())

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) = {

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());
         }

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<Option<Weak<dyn ConsensusClient>>>,
     params: SoloParams,
     machine: CodeChainMachine,
     action_handlers: Vec<Arc<dyn ActionHandler>>,
@@ -47,11 +50,16 @@ impl Solo {
         action_handlers.push(Arc::new(stake::Stake::new(params.genesis_stakes.clone())));
 
         Solo {
+            client: Default::default(),
             params,
             machine,
             action_handlers,
         }
     }
+
+    fn client(&self) -> Option<Arc<dyn ConsensusClient>> {
+        self.client.read().as_ref()?.upgrade()
+    }
 }
 
 impl ConsensusEngine for Solo {
@@ -78,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();
@@ -111,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(())
             }
@@ -127,6 +138,10 @@ impl ConsensusEngine for Solo {
         Ok(())
     }
 
+    fn register_client(&self, client: Weak<dyn ConsensusClient>) {
+        *self.client.write() = Some(Weak::clone(&client));
+    }
+
     fn block_reward(&self, _block_number: u64) -> u64 {
         self.params.block_reward
     }
@@ -146,25 +161,29 @@ 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 parent_common_params = CommonParams::default_for_test();
+        let client = Arc::new(TestBlockChainClient::new_with_scheme(scheme));
+        let engine = client.scheme.engine.clone();
+        engine.register_client(Arc::downgrade(&(client.clone() as Arc<dyn ConsensusClient>)));
+        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 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());
+            assert!(b.try_seal(&*engine, seal).is_ok());
         }
     }
 

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(())
     }
 }

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,13 +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
-                    .read()
-                    .as_ref()
-                    .ok_or(EngineError::CannotOpenBlock)?
-                    .upgrade()
-                    .ok_or(EngineError::CannotOpenBlock)?;
 
                 if term > 1 {
                     let start_of_the_previous_term = {
@@ -273,7 +270,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 +314,7 @@ impl ConsensusEngine for Tendermint {
     }
 
     fn possible_authors(&self, block_number: Option<u64>) -> Result<Option<Vec<Address>>, 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