From a7521fc80b0130fd80160b188e147ac04bd254ef Mon Sep 17 00:00:00 2001 From: SeongChan Lee Date: Tue, 10 Dec 2019 16:39:40 +0900 Subject: [PATCH 1/3] Use genesis state to read network_id --- core/src/client/client.rs | 13 +++++---- core/src/client/mod.rs | 4 +-- core/src/client/test_client.rs | 8 +++--- core/src/consensus/tendermint/worker.rs | 2 +- foundry/run_node.rs | 2 +- rpc/src/v1/impls/account.rs | 18 ++++++------- rpc/src/v1/impls/chain.rs | 36 ++++++++----------------- rpc/src/v1/impls/engine.rs | 2 +- rpc/src/v1/impls/mempool.rs | 8 +++--- 9 files changed, 37 insertions(+), 56 deletions(-) diff --git a/core/src/client/client.rs b/core/src/client/client.rs index c5dbf07bdb..f99e76098f 100644 --- a/core/src/client/client.rs +++ b/core/src/client/client.rs @@ -481,6 +481,10 @@ impl StateInfo for Client { } impl EngineInfo for Client { + fn network_id(&self) -> NetworkId { + self.common_params(BlockId::Earliest).expect("Genesis state must exist").network_id() + } + fn common_params(&self, block_id: BlockId) -> Option { self.state_info(block_id.into()).map(|state| { state @@ -518,7 +522,7 @@ impl EngineInfo for Client { } fn possible_authors(&self, block_number: Option) -> Result>, EngineError> { - let network_id = self.common_params(BlockId::Latest).unwrap().network_id(); + let network_id = self.network_id(); if block_number == Some(0) { let genesis_author = self.block_header(&0.into()).expect("genesis block").author(); return Ok(Some(vec![PlatformAddress::new_v1(network_id, genesis_author)])) @@ -572,8 +576,7 @@ impl BlockChainTrait for Client { } fn genesis_accounts(&self) -> Vec { - // XXX: What should we do if the network id has been changed - let network_id = self.common_params(BlockId::Latest).unwrap().network_id(); + let network_id = self.network_id(); self.genesis_accounts.iter().map(|addr| PlatformAddress::new_v1(network_id, *addr)).collect() } @@ -887,10 +890,6 @@ impl MiningBlockChainClient for Client { fn register_immune_users(&self, immune_user_vec: Vec
) { self.importer.miner.register_immune_users(immune_user_vec) } - - fn get_network_id(&self) -> NetworkId { - self.common_params(BlockId::Latest).unwrap().network_id() - } } impl ChainTimeInfo for Client { diff --git a/core/src/client/mod.rs b/core/src/client/mod.rs index dd42b863d1..f331bb90e3 100644 --- a/core/src/client/mod.rs +++ b/core/src/client/mod.rs @@ -87,6 +87,7 @@ pub trait BlockChainTrait { } pub trait EngineInfo: Send + Sync { + fn network_id(&self) -> NetworkId; fn common_params(&self, block_id: BlockId) -> Option; fn metadata_seq(&self, block_id: BlockId) -> Option; fn block_reward(&self, block_number: u64) -> u64; @@ -279,9 +280,6 @@ pub trait MiningBlockChainClient: BlockChainClient + BlockProducer + FindActionH /// Append designated users to the immune user list. fn register_immune_users(&self, immune_user_vec: Vec
); - - /// Returns network id. - fn get_network_id(&self) -> NetworkId; } /// Provides methods to access database. diff --git a/core/src/client/test_client.rs b/core/src/client/test_client.rs index f8b0252119..0ef27d3b19 100644 --- a/core/src/client/test_client.rs +++ b/core/src/client/test_client.rs @@ -380,10 +380,6 @@ impl MiningBlockChainClient for TestBlockChainClient { fn register_immune_users(&self, immune_user_vec: Vec
) { self.miner.register_immune_users(immune_user_vec) } - - fn get_network_id(&self) -> NetworkId { - NetworkId::default() - } } impl AccountData for TestBlockChainClient { @@ -632,6 +628,10 @@ impl super::EngineClient for TestBlockChainClient { } impl EngineInfo for TestBlockChainClient { + fn network_id(&self) -> NetworkId { + self.scheme.engine.machine().genesis_common_params().network_id() + } + fn common_params(&self, _block_id: BlockId) -> Option { Some(*self.scheme.engine.machine().genesis_common_params()) } diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index 5956ad6d9a..f6fa86ce10 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -1446,7 +1446,7 @@ impl Worker { } fn report_double_vote(&self, double: &DoubleVote) { - let network_id = self.client().common_params(BlockId::Latest).unwrap().network_id(); + let network_id = self.client().network_id(); let seq = match self.signer.address() { Some(address) => self.client().latest_seq(address), None => { diff --git a/foundry/run_node.rs b/foundry/run_node.rs index 2fa44e59ba..51c0782513 100644 --- a/foundry/run_node.rs +++ b/foundry/run_node.rs @@ -262,7 +262,7 @@ pub fn run_node(matches: &ArgMatches<'_>) -> Result<(), String> { let network_config = config.network_config()?; // XXX: What should we do if the network id has been changed. let c = client.client(); - let network_id = c.common_params(BlockId::Latest).unwrap().network_id(); + let network_id = c.network_id(); let routing_table = RoutingTable::new(); let service = network_start(network_id, timer_loop, &network_config, Arc::clone(&routing_table))?; diff --git a/rpc/src/v1/impls/account.rs b/rpc/src/v1/impls/account.rs index a4c16207e9..fa10fbc063 100644 --- a/rpc/src/v1/impls/account.rs +++ b/rpc/src/v1/impls/account.rs @@ -18,8 +18,8 @@ use std::convert::TryInto; use std::sync::Arc; use std::time::Duration; -use ccore::{AccountData, AccountProvider, BlockId, EngineInfo, MinerService, MiningBlockChainClient, TermInfo}; -use ckey::{NetworkId, Password, PlatformAddress, Signature}; +use ccore::{AccountData, AccountProvider, EngineInfo, MinerService, MiningBlockChainClient, TermInfo}; +use ckey::{Password, PlatformAddress, Signature}; use ctypes::transaction::IncompleteTransaction; use jsonrpc_core::Result; use parking_lot::Mutex; @@ -46,11 +46,6 @@ where miner, } } - - fn network_id(&self) -> NetworkId { - // XXX: What should we do if the network id has been changed - self.client.common_params(BlockId::Latest).unwrap().network_id() - } } impl Account for AccountClient @@ -62,7 +57,10 @@ where self.account_provider .get_list() .map(|addresses| { - addresses.into_iter().map(|address| PlatformAddress::new_v1(self.network_id(), address)).collect() + addresses + .into_iter() + .map(|address| PlatformAddress::new_v1(self.client.network_id(), address)) + .collect() }) .map_err(account_provider) } @@ -70,13 +68,13 @@ where fn create_account(&self, passphrase: Option) -> Result { let (address, _) = self.account_provider.new_account_and_public(&passphrase.unwrap_or_default()).map_err(account_provider)?; - Ok(PlatformAddress::new_v1(self.network_id(), address)) + Ok(PlatformAddress::new_v1(self.client.network_id(), address)) } fn create_account_from_secret(&self, secret: H256, passphrase: Option) -> Result { self.account_provider .insert_account(secret.into(), &passphrase.unwrap_or_default()) - .map(|address| PlatformAddress::new_v1(self.network_id(), address)) + .map(|address| PlatformAddress::new_v1(self.client.network_id(), address)) .map_err(account_provider) } diff --git a/rpc/src/v1/impls/chain.rs b/rpc/src/v1/impls/chain.rs index 576e571d00..d9a3797f0c 100644 --- a/rpc/src/v1/impls/chain.rs +++ b/rpc/src/v1/impls/chain.rs @@ -128,10 +128,11 @@ where return Ok(None) } let block_id = block_number.map(BlockId::from).unwrap_or(BlockId::Latest); - Ok(self.client.get_text(transaction_hash, block_id).map_err(errors::transaction_state)?.map(|text| { - let parent_block_id = block_number.map(|n| (n - 1).into()).unwrap_or(BlockId::ParentOfLatest); - Text::from_core(text, self.client.common_params(parent_block_id).unwrap().network_id()) - })) + Ok(self + .client + .get_text(transaction_hash, block_id) + .map_err(errors::transaction_state)? + .map(|text| Text::from_core(text, self.client.network_id()))) } fn get_asset( @@ -178,8 +179,7 @@ where fn get_regular_key_owner(&self, public: Public, block_number: Option) -> Result> { let block_id = block_number.map(BlockId::Number).unwrap_or(BlockId::Latest); Ok(self.client.regular_key_owner(&public_to_address(&public), block_id.into()).and_then(|address| { - let parent_block_id = block_number.map(|n| (n - 1).into()).unwrap_or(BlockId::ParentOfLatest); - let network_id = self.client.common_params(parent_block_id).unwrap().network_id(); + let network_id = self.client.network_id(); Some(PlatformAddress::new_v1(network_id, address)) })) } @@ -206,8 +206,7 @@ where fn get_shard_owners(&self, shard_id: ShardId, block_number: Option) -> Result>> { let block_id = block_number.map(BlockId::Number).unwrap_or(BlockId::Latest); Ok(self.client.shard_owners(shard_id, block_id.into()).map(|owners| { - let parent_block_id = block_number.map(|n| (n - 1).into()).unwrap_or(BlockId::ParentOfLatest); - let network_id = self.client.common_params(parent_block_id).unwrap().network_id(); + let network_id = self.client.network_id(); owners.into_iter().map(|owner| PlatformAddress::new_v1(network_id, owner)).collect() })) } @@ -215,8 +214,7 @@ where fn get_shard_users(&self, shard_id: ShardId, block_number: Option) -> Result>> { let block_id = block_number.map(BlockId::Number).unwrap_or(BlockId::Latest); Ok(self.client.shard_users(shard_id, block_id.into()).map(|users| { - let parent_block_id = block_number.map(|n| (n - 1).into()).unwrap_or(BlockId::ParentOfLatest); - let network_id = self.client.common_params(parent_block_id).unwrap().network_id(); + let network_id = self.client.network_id(); users.into_iter().map(|user| PlatformAddress::new_v1(network_id, user)).collect() })) } @@ -239,26 +237,14 @@ where fn get_block_by_number(&self, block_number: u64) -> Result> { let id = BlockId::Number(block_number); - Ok(self.client.block(&id).map(|block| { - let block_id_to_read_params = if block_number == 0 { - 0.into() - } else { - (block_number - 1).into() - }; - Block::from_core(block.decode(), self.client.common_params(block_id_to_read_params).unwrap().network_id()) - })) + Ok(self.client.block(&id).map(|block| Block::from_core(block.decode(), self.client.network_id()))) } fn get_block_by_hash(&self, block_hash: BlockHash) -> Result> { let id = BlockId::Hash(block_hash); Ok(self.client.block(&id).map(|block| { let block = block.decode(); - let block_id_to_read_params = if block.header.number() == 0 { - 0.into() - } else { - (*block.header.parent_hash()).into() - }; - Block::from_core(block, self.client.common_params(block_id_to_read_params).unwrap().network_id()) + Block::from_core(block, self.client.network_id()) })) } @@ -301,7 +287,7 @@ where } fn get_network_id(&self) -> Result { - Ok(self.client.common_params(BlockId::Latest).unwrap().network_id()) + Ok(self.client.network_id()) } fn get_common_params(&self, block_number: Option) -> Result> { diff --git a/rpc/src/v1/impls/engine.rs b/rpc/src/v1/impls/engine.rs index f106f29346..730ec09073 100644 --- a/rpc/src/v1/impls/engine.rs +++ b/rpc/src/v1/impls/engine.rs @@ -62,7 +62,7 @@ where Ok(None) } else { // XXX: What should we do if the network id has been changed - let network_id = self.client.common_params(BlockId::Latest).unwrap().network_id(); + let network_id = self.client.network_id(); Ok(Some(PlatformAddress::new_v1(network_id, author))) } } diff --git a/rpc/src/v1/impls/mempool.rs b/rpc/src/v1/impls/mempool.rs index ed021f1c8d..ab78c740e8 100644 --- a/rpc/src/v1/impls/mempool.rs +++ b/rpc/src/v1/impls/mempool.rs @@ -16,7 +16,7 @@ use std::sync::Arc; -use ccore::{BlockChainClient, MiningBlockChainClient, SignedTransaction}; +use ccore::{BlockChainClient, EngineInfo, MiningBlockChainClient, SignedTransaction}; use cjson::bytes::Bytes; use ckey::{Address, PlatformAddress}; use ctypes::{Tracker, TxHash}; @@ -42,7 +42,7 @@ impl MempoolClient { impl Mempool for MempoolClient where - C: BlockChainClient + MiningBlockChainClient + 'static, + C: BlockChainClient + MiningBlockChainClient + EngineInfo + 'static, { fn send_signed_transaction(&self, raw: Bytes) -> Result { Rlp::new(&raw.into_vec()) @@ -82,7 +82,7 @@ where fn get_banned_accounts(&self) -> Result> { let malicious_user_vec = self.client.get_malicious_users(); - let network_id = self.client.get_network_id(); + let network_id = self.client.network_id(); Ok(malicious_user_vec.into_iter().map(|address| PlatformAddress::new_v1(network_id, address)).collect()) } @@ -102,7 +102,7 @@ where fn get_immune_accounts(&self) -> Result> { let immune_user_vec = self.client.get_immune_users(); - let network_id = self.client.get_network_id(); + let network_id = self.client.network_id(); Ok(immune_user_vec.into_iter().map(|address| PlatformAddress::new_v1(network_id, address)).collect()) } From 797f7dd1c201300e5ab18c05990e993f2639ff51 Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Fri, 27 Dec 2019 06:38:39 +0900 Subject: [PATCH 2/3] Fix tendermint to use CurrentValidators CurrentValidators was introduced to tendermint, but it wasn't used by some part of our implementation. This patch doesn't fix all of them, but important parts are filled in. --- core/src/consensus/stake/action_data.rs | 4 ++ core/src/consensus/tendermint/engine.rs | 12 ++++-- core/src/consensus/tendermint/worker.rs | 15 ++++---- .../validator_set/dynamic_validator.rs | 38 +++++++++++++++++++ 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/core/src/consensus/stake/action_data.rs b/core/src/consensus/stake/action_data.rs index 7c59dc9135..34bc90ab72 100644 --- a/core/src/consensus/stake/action_data.rs +++ b/core/src/consensus/stake/action_data.rs @@ -433,6 +433,10 @@ impl CurrentValidators { pub fn update(&mut self, validators: Vec) { self.0 = validators; } + + pub fn addresses(&self) -> Vec
{ + self.0.iter().rev().map(|v| public_to_address(&v.pubkey)).collect() + } } impl Deref for CurrentValidators { diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 67a83dc4d1..46daad7259 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -389,20 +389,24 @@ fn aggregate_work_info( end_of_the_last_term + 1 }; let mut header = start_of_the_next_term_header; - let mut parent_validators = validators.current_addresses(&header.parent_hash()); + // FIXME: Use method from `ValidatorSet` interface + let mut current_validators = { + let state = chain.state_at(header.parent_hash().into()).expect("The block's state must exist"); + stake::CurrentValidators::load_from_state(&state)?.addresses() + }; while start_of_the_current_term != header.number() { for index in TendermintSealView::new(&header.seal()).bitset()?.true_index_iter() { - let signer = *parent_validators.get(index).expect("The seal must be the signature of the validator"); + let signer = *current_validators.get(index).expect("The seal must be the signature of the validator"); work_info.entry(signer).or_default().signed += 1; } header = chain.block_header(&header.parent_hash().into()).unwrap(); - parent_validators = validators.current_addresses(&header.parent_hash()); + current_validators = validators.current_addresses(&header.parent_hash()); let author = header.author(); let info = work_info.entry(author).or_default(); info.proposed += 1; - info.missed += parent_validators.len() - TendermintSealView::new(&header.seal()).bitset()?.count(); + info.missed += current_validators.len() - TendermintSealView::new(&header.seal()).bitset()?.count(); } Ok(work_info) diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index f6fa86ce10..75e7d65294 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -1219,13 +1219,14 @@ impl Worker { }; let mut voted_validators = BitSet::new(); - let grand_parent_hash = self - .client() - .block_header(&(*header.parent_hash()).into()) - .expect("The parent block must exist") - .parent_hash(); + let parent_hash = header.parent_hash(); + let grand_parent_hash = + self.client().block_header(&(*parent_hash).into()).expect("The parent block must exist").parent_hash(); for (bitset_index, signature) in seal_view.signatures()? { - let public = self.validators.get(&grand_parent_hash, bitset_index); + let public = match self.validators.get_current(header.parent_hash(), bitset_index) { + Some(p) => p, + None => self.validators.get(&grand_parent_hash, bitset_index), + }; if !verify_schnorr(&public, &signature, &precommit_vote_on.hash())? { let address = public_to_address(&public); return Err(EngineError::BlockNotAuthorized(address.to_owned()).into()) @@ -1238,7 +1239,7 @@ impl Worker { if header.number() == 1 { return Ok(()) } - self.validators.check_enough_votes(&grand_parent_hash, &voted_validators)?; + self.validators.check_enough_votes_with_current(&parent_hash, &voted_validators)?; Ok(()) } diff --git a/core/src/consensus/validator_set/dynamic_validator.rs b/core/src/consensus/validator_set/dynamic_validator.rs index 19fa775db8..63ae1ffdd5 100644 --- a/core/src/consensus/validator_set/dynamic_validator.rs +++ b/core/src/consensus/validator_set/dynamic_validator.rs @@ -104,6 +104,44 @@ impl DynamicValidator { (prev_proposer_index + proposed_view + 1) % num_validators } } + + pub fn get_current(&self, hash: &BlockHash, index: usize) -> Option { + let validators = self.current_validators_pubkey(*hash)?; + let n_validators = validators.len(); + Some(*validators.get(index % n_validators).unwrap()) + } + + pub fn check_enough_votes_with_current(&self, hash: &BlockHash, votes: &BitSet) -> Result<(), EngineError> { + if let Some(validators) = self.current_validators(*hash) { + let mut voted_delegation = 0u64; + let n_validators = validators.len(); + for index in votes.true_index_iter() { + assert!(index < n_validators); + let validator = validators.get(index).ok_or_else(|| { + EngineError::ValidatorNotExist { + height: 0, // FIXME + index, + } + })?; + voted_delegation += validator.delegation(); + } + let total_delegation: u64 = validators.iter().map(Validator::delegation).sum(); + if voted_delegation * 3 > total_delegation * 2 { + Ok(()) + } else { + let threshold = total_delegation as usize * 2 / 3; + Err(EngineError::BadSealFieldSize(OutOfBounds { + min: Some(threshold), + max: Some(total_delegation as usize), + found: voted_delegation as usize, + })) + } + } else { + let client = self.client.read().as_ref().and_then(Weak::upgrade).expect("Client is not initialized"); + let header = client.block_header(&(*hash).into()).unwrap(); + self.check_enough_votes(&header.parent_hash(), votes) + } + } } impl ValidatorSet for DynamicValidator { From ae8afc6dd8825b8c2d8dfc6c72344928e76c8cfa Mon Sep 17 00:00:00 2001 From: Joonmo Yang Date: Fri, 27 Dec 2019 06:43:12 +0900 Subject: [PATCH 3/3] Don't do anything at the 1th term's open 1th term open doesn't require any reward distribution since the 0th term is static validator consensus --- core/src/consensus/tendermint/engine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 46daad7259..24b10b70a3 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -160,7 +160,7 @@ impl ConsensusEngine for Tendermint { if block_number == metadata.last_term_finished_block_num() + 1 { match term { - 0 => {} + 0 | 1 => {} _ => { let rewards = stake::drain_current_rewards(block.state_mut())?; let banned = stake::Banned::load_from_state(block.state())?;