From cf298ca2a4b402a27efc9c9396dbdb33de0b197c Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 19 Nov 2019 10:23:43 +0900 Subject: [PATCH 1/4] 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 2389a88235..32cdb03506 100644 --- a/core/src/client/client.rs +++ b/core/src/client/client.rs @@ -826,6 +826,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| { From fc79c4561e40bb2ee4cfc3bc7bce830d08c0ec12 Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 26 Nov 2019 11:21:29 +0900 Subject: [PATCH 2/4] Add term_params to Metadata `term_params` should be snapshot every `on_term_close` --- state/src/item/metadata.rs | 136 +++++++++++++++++++++++++------------ 1 file changed, 94 insertions(+), 42 deletions(-) diff --git a/state/src/item/metadata.rs b/state/src/item/metadata.rs index 6ad74076b3..39651888a4 100644 --- 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, + term_params: Option, } 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 { - 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::(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::(&[]); @@ -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); } From c165bb48542b3f2b6c319abd594d77c2e71d7a52 Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 26 Nov 2019 14:43:37 +0900 Subject: [PATCH 3/4] Snapshot term_metadata on every term close for the era > 0 --- core/src/consensus/tendermint/engine.rs | 10 +++++++++- state/src/impls/top_level.rs | 6 ++++++ state/src/traits.rs | 1 + 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 3ea3282c7a..f5263796c1 100644 --- 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; @@ -235,6 +235,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/state/src/impls/top_level.rs b/state/src/impls/top_level.rs index b871efaa58..4d51ec0b3f 100644 --- 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 { diff --git a/state/src/traits.rs b/state/src/traits.rs index 40d94688f7..c9414611d1 100644 --- 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 { From cd9a669f6e7f6e4dd40338cce153d12ad5f74b3e Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 26 Nov 2019 17:10:12 +0900 Subject: [PATCH 4/4] Read term_params from the snapshot --- core/src/client/client.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/core/src/client/client.rs b/core/src/client/client.rs index 32cdb03506..eb1d15ff05 100644 --- a/core/src/client/client.rs +++ b/core/src/client/client.rs @@ -828,11 +828,19 @@ impl TermInfo for Client { } 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 + 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 { - Some(self.common_params((block_number).into()).expect("Common params should exist")) + 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")) + } } } }