From b3500e933111334e52c14d8c74da665e1feb7457 Mon Sep 17 00:00:00 2001 From: Seulgi Kim Date: Thu, 16 Apr 2020 20:48:37 +0900 Subject: [PATCH 1/3] Merge close_impl and close into one function The close_impl was extracted because close and close_and_lock have duplicate code. Now this is not necessary because close_and_lock is removed. --- core/src/block.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/core/src/block.rs b/core/src/block.rs index e192e4daed..c87ffc75bb 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -208,7 +208,7 @@ impl<'x> OpenBlock<'x> { } /// Turn this into a `ClosedBlock`. - fn close_impl(&mut self) -> Result<(), Error> { + pub fn close(mut self) -> Result { if let Err(e) = self.engine.on_close_block(&mut self.block) { warn!("Encountered error on closing the block: {}", e); return Err(e) @@ -223,13 +223,6 @@ impl<'x> OpenBlock<'x> { let vset = vset_raw.create_compact_validator_set(); self.block.header.set_next_validator_set_hash(vset.hash()); - Ok(()) - } - - /// Turn this into a `ClosedBlock`. - pub fn close(mut self) -> Result { - self.close_impl()?; - if self.block.header.transactions_root() == &BLAKE_NULL_RLP { self.block.header.set_transactions_root(skewed_merkle_root( BLAKE_NULL_RLP, From 4a5dcd4bb871a8767a69f846d235ab5983f20b67 Mon Sep 17 00:00:00 2001 From: Seulgi Kim Date: Thu, 16 Apr 2020 20:59:31 +0900 Subject: [PATCH 2/3] Remove the unused argument of push_transaction The h is always None. --- core/src/block.rs | 5 ++--- core/src/miner/miner.rs | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/core/src/block.rs b/core/src/block.rs index c87ffc75bb..d022bf3adf 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -149,7 +149,6 @@ impl<'x> OpenBlock<'x> { pub fn push_transaction( &mut self, tx: VerifiedTransaction, - h: Option, client: &C, parent_block_number: BlockNumber, parent_block_timestamp: u64, @@ -168,7 +167,7 @@ impl<'x> OpenBlock<'x> { self.block.header.timestamp(), ) { Ok(()) => { - self.block.transactions_set.insert(h.unwrap_or(hash)); + self.block.transactions_set.insert(hash); self.block.transactions.push(tx); None } @@ -194,7 +193,7 @@ impl<'x> OpenBlock<'x> { parent_block_timestamp: u64, ) -> Result<(), Error> { for tx in transactions { - self.push_transaction(tx.clone(), None, client, parent_block_number, parent_block_timestamp)?; + self.push_transaction(tx.clone(), client, parent_block_number, parent_block_timestamp)?; } Ok(()) } diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 5e35f01c06..2013a04f24 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -324,8 +324,7 @@ impl Miner { let hash = tx.hash(); let start = Instant::now(); // Check whether transaction type is allowed for sender - let result = - open_block.push_transaction(tx, None, chain, parent_header.number(), parent_header.timestamp()); + let result = open_block.push_transaction(tx, chain, parent_header.number(), parent_header.timestamp()); match result { // already have transaction - ignore From 8af0c6672341bc67c4c8480e969690a4bf1aba6b Mon Sep 17 00:00:00 2001 From: Seulgi Kim Date: Sun, 12 Apr 2020 14:35:05 +0900 Subject: [PATCH 3/3] Make the miner always have AccountProvider Currently, AccountProvider in the miner is optional to support unit tests who cannot use the account provider which uses the permanent storage. This commit makes the miner uses memory back-end AccountProvider for test. --- core/src/client/test_client.rs | 2 +- core/src/miner/miner.rs | 43 +++++++++++++--------------------- foundry/run_node.rs | 2 +- 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/core/src/client/test_client.rs b/core/src/client/test_client.rs index 805fa4fcd4..d52e1566a6 100644 --- a/core/src/client/test_client.rs +++ b/core/src/client/test_client.rs @@ -143,7 +143,7 @@ impl TestBlockChainClient { seqs: RwLock::new(HashMap::new()), storage: RwLock::new(HashMap::new()), queue_size: AtomicUsize::new(0), - miner: Arc::new(Miner::with_scheme(&scheme, db)), + miner: Arc::new(Miner::with_scheme_for_test(&scheme, db)), scheme, latest_block_timestamp: RwLock::new(10_000_000), history: RwLock::new(None), diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 2013a04f24..6f3b140521 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -98,7 +98,7 @@ pub struct Miner { sealing_enabled: AtomicBool, - accounts: Option>, + accounts: Arc, malicious_users: RwLock>, immune_users: RwLock>, } @@ -107,20 +107,20 @@ impl Miner { pub fn new( options: MinerOptions, scheme: &Scheme, - accounts: Option>, + accounts: Arc, db: Arc, ) -> Arc { Arc::new(Self::new_raw(options, scheme, accounts, db)) } - pub fn with_scheme(scheme: &Scheme, db: Arc) -> Self { - Self::new_raw(Default::default(), scheme, None, db) + pub fn with_scheme_for_test(scheme: &Scheme, db: Arc) -> Self { + Self::new_raw(Default::default(), scheme, AccountProvider::transient_provider(), db) } fn new_raw( options: MinerOptions, scheme: &Scheme, - accounts: Option>, + accounts: Arc, db: Arc, ) -> Self { let mem_limit = options.mem_pool_memory_limit.unwrap_or_else(usize::max_value); @@ -183,15 +183,11 @@ impl Miner { self.immune_users.write().insert(signer_address); } - let origin = self - .accounts - .as_ref() - .and_then(|accounts| match accounts.has_public(&signer_public) { - Ok(true) => Some(TxOrigin::Local), - Ok(false) => None, - Err(_) => None, - }) - .unwrap_or(default_origin); + let origin = if self.accounts.has_public(&signer_public).unwrap_or_default() { + TxOrigin::Local + } else { + default_origin + }; if self.malicious_users.read().contains(&signer_address) { // FIXME: just to skip, think about another way. @@ -406,19 +402,12 @@ impl MinerService for Miner { self.params.write().author = address; if self.engine_type().need_signer_key() { - if let Some(ref ap) = self.accounts { - ctrace!(MINER, "Set author to {:?}", address); - // Sign test message - ap.get_unlocked_account(&address)?.sign(&Default::default())?; - self.engine.set_signer(ap.clone(), address); - Ok(()) - } else { - cwarn!(MINER, "No account provider"); - Err(AccountProviderError::NotFound) - } - } else { - Ok(()) + ctrace!(MINER, "Set author to {:?}", address); + // Sign test message + self.accounts.get_unlocked_account(&address)?.sign(&Default::default())?; + self.engine.set_signer(Arc::clone(&self.accounts), address); } + Ok(()) } fn get_author_address(&self) -> Address { @@ -713,7 +702,7 @@ pub mod test { fn check_add_transactions_result_idx() { let db = Arc::new(kvdb_memorydb::create(NUM_COLUMNS.unwrap())); let scheme = Scheme::new_test(); - let miner = Arc::new(Miner::with_scheme(&scheme, db.clone())); + let miner = Arc::new(Miner::with_scheme_for_test(&scheme, db.clone())); let mut mem_pool = MemPool::with_limits(8192, usize::max_value(), 3, db.clone()); let client = generate_test_client(db, Arc::clone(&miner), &scheme).unwrap(); diff --git a/foundry/run_node.rs b/foundry/run_node.rs index 98200a194f..a2bddd9e0d 100644 --- a/foundry/run_node.rs +++ b/foundry/run_node.rs @@ -123,7 +123,7 @@ fn new_miner( ap: Arc, db: Arc, ) -> Result, String> { - let miner = Miner::new(config.miner_options()?, scheme, Some(ap), db); + let miner = Miner::new(config.miner_options()?, scheme, ap, db); match miner.engine_type() { EngineType::PBFT => match &config.mining.engine_signer {