Merge branch 'feature/term_common_params' into feature/era-test

foriequal0 · foriequal0 · commit 148f1d0e4e1f · 2019-11-28T11:13:12.000+09:00
diff --git 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)]
@@ -504,16 +505,7 @@ pub fn enact<C: ChainTimeInfo + EngineInfo + FindActionHandler + TermInfo>(
     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())
-            .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, &parent_common_params, term_common_params.as_ref())
 }
 
diff --git a/core/src/client/client.rs b/core/src/client/client.rs
@@ -826,6 +826,23 @@ 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<CommonParams> {
+        let state = self.state_at(id)?;
+        let metadata = state.metadata().unwrap().expect("Metadata always exist");
+
+        if let Some(term_params) = metadata.term_params() {
+            Some(*term_params)
+        } else {
+            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
@@ -119,6 +119,7 @@ pub trait ConsensusClient: BlockChainClient + EngineClient + EngineInfo + TermIn
 pub trait TermInfo {
     fn last_term_finished_block_num(&self, id: BlockId) -> Option<BlockNumber>;
     fn current_term_id(&self, id: BlockId) -> Option<u64>;
+    fn term_common_params(&self, id: BlockId) -> Option<CommonParams>;
 }
 
 /// Provides methods to access account info
diff --git 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<u64> {
         self.term_id
     }
+
+    fn term_common_params(&self, _id: BlockId) -> Option<CommonParams> {
+        None
+    }
 }
 
 impl StateInfo for TestBlockChainClient {
diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs
@@ -24,7 +24,7 @@ use std::sync::{Arc, Weak};
 use ckey::{public_to_address, Address};
 use cnetwork::NetworkService;
 use crossbeam_channel as crossbeam;
-use cstate::{ActionHandler, TopStateView};
+use cstate::{ActionHandler, TopState, TopStateView};
 use ctypes::{BlockHash, CommonParams, Header};
 use num_rational::Ratio;
 
@@ -238,6 +238,14 @@ impl ConsensusEngine for Tendermint {
 
         stake::on_term_close(block.state_mut(), block_number, &inactive_validators)?;
 
+        match term {
+            0 => {}
+            _ => match term_common_params.expect("Term common params should exist").era() {
+                0 => {}
+                1 => block.state_mut().snapshot_term_params()?,
+                _ => unimplemented!("It is not decided how we handle this"),
+            },
+        }
         Ok(())
     }
 
diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs
@@ -604,16 +604,7 @@ impl Miner {
             (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())
-                .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, &parent_common_params, term_common_params.as_ref())?;
 
         let fetch_seq = |p: &Public| {
diff --git a/state/src/impls/top_level.rs b/state/src/impls/top_level.rs
@@ -999,6 +999,12 @@ impl TopState for TopLevelState {
         metadata.increase_seq();
         Ok(())
     }
+
+    fn snapshot_term_params(&mut self) -> StateResult<()> {
+        let mut metadata = self.get_metadata_mut()?;
+        metadata.snapshot_term_params();
+        Ok(())
+    }
 }
 
 fn is_active_account(state: &dyn TopStateView, address: &Address) -> TrieResult<bool> {
diff --git a/state/src/item/metadata.rs b/state/src/item/metadata.rs
@@ -34,6 +34,7 @@ pub struct Metadata {
     term: TermMetadata,
     seq: u64,
     params: Option<CommonParams>,
+    term_params: Option<CommonParams>,
 }
 
 impl Metadata {
@@ -45,6 +46,7 @@ impl Metadata {
             term: Default::default(),
             seq: 0,
             params: None,
+            term_params: None,
         }
     }
 
@@ -93,6 +95,14 @@ impl Metadata {
         self.params = Some(params);
     }
 
+    pub fn term_params(&self) -> Option<&CommonParams> {
+        self.term_params.as_ref()
+    }
+
+    pub fn snapshot_term_params(&mut self) {
+        self.term_params = self.params;
+    }
+
     pub fn increase_term_id(&mut self, last_term_finished_block_num: u64) {
         assert!(self.term.last_term_finished_block_num < last_term_finished_block_num);
         self.term.last_term_finished_block_num = last_term_finished_block_num;
@@ -124,25 +134,31 @@ impl CacheableItem for Metadata {
 
 const PREFIX: u8 = super::METADATA_PREFIX;
 
+const INITIAL_LEN: usize = 4;
+const TERM_LEN: usize = INITIAL_LEN + 2;
+const PARAMS_LEN: usize = TERM_LEN + 2;
+const TERM_PARAMS_LEN: usize = PARAMS_LEN + 1;
+const VALID_LEN: &[usize] = &[INITIAL_LEN, TERM_LEN, PARAMS_LEN, TERM_PARAMS_LEN];
+
 impl Encodable for Metadata {
     fn rlp_append(&self, s: &mut RlpStream) {
-        const INITIAL_LEN: usize = 4;
-        const TERM_LEN: usize = 2;
-        const PARAMS_LEN: usize = 2;
-        let mut len = INITIAL_LEN;
-
         let term_changed = self.term != Default::default();
-        if term_changed {
-            len += TERM_LEN;
-        }
-
         let params_changed = self.seq != 0;
-        if params_changed {
-            if !term_changed {
-                len += TERM_LEN;
+        let term_params_exist = self.term_params.is_some();
+
+        let len = if term_params_exist {
+            if !params_changed {
+                panic!("Term params only can be changed if params changed");
             }
-            len += PARAMS_LEN;
-        }
+            TERM_PARAMS_LEN
+        } else if params_changed {
+            PARAMS_LEN
+        } else if term_changed {
+            TERM_LEN
+        } else {
+            INITIAL_LEN
+        };
+
         s.begin_list(len)
             .append(&PREFIX)
             .append(&self.number_of_shards)
@@ -159,48 +175,63 @@ impl Encodable for Metadata {
             }
             s.append(&self.seq).append(self.params.as_ref().unwrap());
         }
+        if term_params_exist {
+            if !params_changed {
+                unreachable!("Term params only can be changed if params changed");
+            }
+            s.append(self.term_params.as_ref().unwrap());
+        }
     }
 }
 
 impl Decodable for Metadata {
     fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
-        let (term, seq, params) = match rlp.item_count()? {
-            4 => (TermMetadata::default(), 0, None),
-            6 => (
-                TermMetadata {
-                    last_term_finished_block_num: rlp.val_at(4)?,
-                    current_term_id: rlp.val_at(5)?,
-                },
-                0,
-                None,
-            ),
-            8 => (
-                TermMetadata {
-                    last_term_finished_block_num: rlp.val_at(4)?,
-                    current_term_id: rlp.val_at(5)?,
-                },
-                rlp.val_at(6)?,
-                Some(rlp.val_at(7)?),
-            ),
-            item_count => {
-                return Err(DecoderError::RlpInvalidLength {
-                    got: item_count,
-                    expected: 4,
-                })
-            }
-        };
+        let item_count = rlp.item_count()?;
+        if !VALID_LEN.contains(&item_count) {
+            return Err(DecoderError::RlpInvalidLength {
+                got: item_count,
+                expected: 4,
+            })
+        }
+
         let prefix = rlp.val_at::<u8>(0)?;
         if PREFIX != prefix {
             cdebug!(STATE, "{} is not an expected prefix for asset", prefix);
             return Err(DecoderError::Custom("Unexpected prefix"))
         }
+        let number_of_shards = rlp.val_at(1)?;
+        let number_of_initial_shards = rlp.val_at(2)?;
+        let hashes = rlp.list_at(3)?;
+
+        let term = if item_count >= TERM_LEN {
+            TermMetadata {
+                last_term_finished_block_num: rlp.val_at(4)?,
+                current_term_id: rlp.val_at(5)?,
+            }
+        } else {
+            TermMetadata::default()
+        };
+
+        let (seq, params) = if item_count >= PARAMS_LEN {
+            (rlp.val_at(6)?, Some(rlp.val_at(7)?))
+        } else {
+            Default::default()
+        };
+
+        let term_params = if item_count >= TERM_PARAMS_LEN {
+            Some(rlp.val_at(8)?)
+        } else {
+            Default::default()
+        };
+
         Ok(Self {
-            number_of_shards: rlp.val_at(1)?,
-            number_of_initial_shards: rlp.val_at(2)?,
-            hashes: rlp.list_at(3)?,
+            number_of_shards,
+            number_of_initial_shards,
+            hashes,
             term,
             seq,
             params,
+            term_params,
         })
     }
 }
@@ -266,6 +297,7 @@ mod tests {
             term: Default::default(),
             seq: 0,
             params: None,
+            term_params: None,
         };
         let mut rlp = RlpStream::new_list(4);
         rlp.append(&PREFIX).append(&10u16).append(&1u16).append_list::<H256, H256>(&[]);
@@ -281,6 +313,7 @@ mod tests {
             term: Default::default(),
             seq: 3,
             params: Some(CommonParams::default_for_test()),
+            term_params: Some(CommonParams::default_for_test()),
         };
         rlp_encode_and_decode_test!(metadata);
     }
@@ -297,6 +330,7 @@ mod tests {
             },
             seq: 0,
             params: None,
+            term_params: None,
         };
         rlp_encode_and_decode_test!(metadata);
     }
@@ -313,6 +347,24 @@ mod tests {
             },
             seq: 3,
             params: Some(CommonParams::default_for_test()),
+            term_params: Some(CommonParams::default_for_test()),
+        };
+        rlp_encode_and_decode_test!(metadata);
+    }
+
+    #[test]
+    fn metadata_with_term_and_seq_but_not_term_params() {
+        let metadata = Metadata {
+            number_of_shards: 10,
+            number_of_initial_shards: 1,
+            hashes: vec![],
+            term: TermMetadata {
+                last_term_finished_block_num: 1,
+                current_term_id: 100,
+            },
+            seq: 3,
+            params: Some(CommonParams::default_for_test()),
+            term_params: None,
         };
         rlp_encode_and_decode_test!(metadata);
     }
diff --git a/state/src/traits.rs b/state/src/traits.rs
@@ -183,6 +183,7 @@ pub trait TopState {
     fn remove_action_data(&mut self, key: &H256);
 
     fn update_params(&mut self, metadata_seq: u64, params: CommonParams) -> StateResult<()>;
+    fn snapshot_term_params(&mut self) -> StateResult<()>;
 }
 
 pub trait StateWithCache {

Original file line number	Diff line number	Diff line change
`@@ -119,6 +119,7 @@ pub trait ConsensusClient: BlockChainClient + EngineClient + EngineInfo + TermIn`
`119`	`119`	`pub trait TermInfo {`
`120`	`120`	`fn last_term_finished_block_num(&self, id: BlockId) -> Option<BlockNumber>;`
`121`	`121`	`fn current_term_id(&self, id: BlockId) -> Option<u64>;`
	`122`	`+ fn term_common_params(&self, id: BlockId) -> Option<CommonParams>;`
`122`	`123`	`}`
`123`	`124`
`124`	`125`	`/// Provides methods to access account info`
Original file line number	Diff line number	Diff line change
`@@ -679,6 +679,10 @@ impl TermInfo for TestBlockChainClient {`
`679`	`679`	`fn current_term_id(&self, _id: BlockId) -> Option<u64> {`
`680`	`680`	`self.term_id`
`681`	`681`	`}`
	`682`	`+`
	`683`	`+ fn term_common_params(&self, _id: BlockId) -> Option<CommonParams> {`
	`684`	`+ None`
	`685`	`+ }`
`682`	`686`	`}`
`683`	`687`
`684`	`688`	`impl StateInfo for TestBlockChainClient {`
Original file line number	Diff line number	Diff line change
`@@ -999,6 +999,12 @@ impl TopState for TopLevelState {`
`999`	`999`	`metadata.increase_seq();`
`1000`	`1000`	`Ok(())`
`1001`	`1001`	`}`
	`1002`	`+`
	`1003`	`+ fn snapshot_term_params(&mut self) -> StateResult<()> {`
	`1004`	`+ let mut metadata = self.get_metadata_mut()?;`
	`1005`	`+ metadata.snapshot_term_params();`
	`1006`	`+ Ok(())`
	`1007`	`+ }`
`1002`	`1008`	`}`
`1003`	`1009`
`1004`	`1010`	`fn is_active_account(state: &dyn TopStateView, address: &Address) -> TrieResult<bool> {`
Original file line number	Diff line number	Diff line change
`@@ -183,6 +183,7 @@ pub trait TopState {`
`183`	`183`	`fn remove_action_data(&mut self, key: &H256);`
`184`	`184`
`185`	`185`	`fn update_params(&mut self, metadata_seq: u64, params: CommonParams) -> StateResult<()>;`
	`186`	`+ fn snapshot_term_params(&mut self) -> StateResult<()>;`
`186`	`187`	`}`
`187`	`188`
`188`	`189`	`pub trait StateWithCache {`