From 9744a93b0184a77d138490ff0abda02bb7075779 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Wed, 3 Jun 2020 17:14:13 +0900 Subject: [PATCH 1/3] Introduce NextAllowedReseal NextAllowedReseal limits the lifetime of the inner lock. --- core/src/miner/miner.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 2dd666c3b9..7a6992cf15 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -86,7 +86,7 @@ pub struct AuthoringParams { pub struct Miner { mem_pool: Arc>, - next_allowed_reseal: Mutex, + next_allowed_reseal: NextAllowedReseal, params: RwLock, engine: Arc, options: MinerOptions, @@ -98,6 +98,26 @@ pub struct Miner { immune_users: RwLock>, } +struct NextAllowedReseal { + instant: Mutex, +} + +impl NextAllowedReseal { + pub fn new(instant: Instant) -> Self { + Self { + instant: Mutex::new(instant), + } + } + + pub fn get(&self) -> Instant { + *self.instant.lock() + } + + pub fn set(&self, instant: Instant) { + *self.instant.lock() = instant; + } +} + impl Miner { pub fn new( options: MinerOptions, @@ -128,7 +148,7 @@ impl Miner { Self { mem_pool, - next_allowed_reseal: Mutex::new(Instant::now()), + next_allowed_reseal: NextAllowedReseal::new(Instant::now()), params: RwLock::new(AuthoringParams::default()), engine: scheme.engine.clone(), options, @@ -419,7 +439,7 @@ impl Miner { /// Are we allowed to do a non-mandatory reseal? fn transaction_reseal_allowed(&self) -> bool { - self.sealing_enabled.load(Ordering::Relaxed) && (Instant::now() > *self.next_allowed_reseal.lock()) + self.sealing_enabled.load(Ordering::Relaxed) && (Instant::now() > self.next_allowed_reseal.get()) } } @@ -529,7 +549,7 @@ impl MinerService for Miner { } // Sealing successful - *self.next_allowed_reseal.lock() = Instant::now() + self.options.reseal_min_period; + self.next_allowed_reseal.set(Instant::now() + self.options.reseal_min_period); chain.set_min_timer(); } From 9c42b0232028ed679e69a666428b6dbd179fc312 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Wed, 3 Jun 2020 17:21:39 +0900 Subject: [PATCH 2/3] Introduce Params struct in miner.rs Params limits the lifetime of the inner lock. --- core/src/miner/miner.rs | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 7a6992cf15..7f24a35bcc 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -87,7 +87,7 @@ pub struct AuthoringParams { pub struct Miner { mem_pool: Arc>, next_allowed_reseal: NextAllowedReseal, - params: RwLock, + params: Params, engine: Arc, options: MinerOptions, @@ -98,6 +98,29 @@ pub struct Miner { immune_users: RwLock>, } +struct Params { + params: RwLock, +} + +impl Params { + pub fn new(params: AuthoringParams) -> Self { + Self { + params: RwLock::new(params), + } + } + + pub fn get(&self) -> AuthoringParams { + self.params.read().clone() + } + + pub fn apply(&self, f: F) + where + F: FnOnce(&mut AuthoringParams) -> (), { + let mut params = self.params.write(); + f(&mut params); + } +} + struct NextAllowedReseal { instant: Mutex, } @@ -149,7 +172,7 @@ impl Miner { Self { mem_pool, next_allowed_reseal: NextAllowedReseal::new(Instant::now()), - params: RwLock::new(AuthoringParams::default()), + params: Params::new(AuthoringParams::default()), engine: scheme.engine.clone(), options, sealing_enabled: AtomicBool::new(true), @@ -269,7 +292,7 @@ impl Miner { ) -> Result, Error> { let (transactions, mut open_block, block_number, block_tx_signer, block_tx_seq) = { ctrace!(MINER, "prepare_block: No existing work - making new block"); - let params = self.params.read().clone(); + let params = self.params.get(); let open_block = chain.prepare_open_block(parent_block_id, params.author, params.extra_data); let (block_number, parent_hash) = { let header = open_block.block().header(); @@ -455,11 +478,11 @@ impl MinerService for Miner { } fn authoring_params(&self) -> AuthoringParams { - self.params.read().clone() + self.params.get() } fn set_author(&self, pubkey: Public) -> Result<(), AccountProviderError> { - self.params.write().author = pubkey; + self.params.apply(|params| params.author = pubkey); if self.engine_type().need_signer_key() { ctrace!(MINER, "Set author to {:?}", pubkey); @@ -471,11 +494,11 @@ impl MinerService for Miner { } fn get_author(&self) -> Public { - self.params.read().author + self.params.get().author } fn set_extra_data(&self, extra_data: Bytes) { - self.params.write().extra_data = extra_data; + self.params.apply(|params| params.extra_data = extra_data); } fn transactions_limit(&self) -> usize { From 03e30af09317fa8a85a3902d81fd01b47ec165f4 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Wed, 3 Jun 2020 17:30:44 +0900 Subject: [PATCH 3/3] Introduce Users struct in miner.rs Users limits the lifetime of the inner lock. --- core/src/miner/miner.rs | 77 ++++++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/core/src/miner/miner.rs b/core/src/miner/miner.rs index 7f24a35bcc..fa4c158040 100644 --- a/core/src/miner/miner.rs +++ b/core/src/miner/miner.rs @@ -39,7 +39,6 @@ use rlp::Encodable; use std::borrow::Borrow; use std::collections::HashSet; use std::convert::TryInto; -use std::iter::FromIterator; use std::ops::Range; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; @@ -94,8 +93,46 @@ pub struct Miner { sealing_enabled: AtomicBool, accounts: Arc, - malicious_users: RwLock>, - immune_users: RwLock>, + malicious_users: Users, + immune_users: Users, +} + +struct Users { + users: RwLock>, +} + +impl Users { + pub fn new() -> Self { + Self { + users: RwLock::new(HashSet::new()), + } + } + + pub fn cloned(&self) -> Vec { + self.users.read().iter().map(Clone::clone).collect() + } + + pub fn contains(&self, public: &Public) -> bool { + self.users.read().contains(public) + } + + pub fn insert(&self, public: Public) -> bool { + self.users.write().insert(public) + } + + pub fn insert_users(&self, publics: impl Iterator) { + let mut users = self.users.write(); + for public in publics { + users.insert(public); + } + } + + pub fn remove_users<'a>(&self, publics: impl Iterator) { + let mut users = self.users.write(); + for public in publics { + users.remove(public); + } + } } struct Params { @@ -177,8 +214,8 @@ impl Miner { options, sealing_enabled: AtomicBool::new(true), accounts, - malicious_users: RwLock::new(HashSet::new()), - immune_users: RwLock::new(HashSet::new()), + malicious_users: Users::new(), + immune_users: Users::new(), } } @@ -211,7 +248,7 @@ impl Miner { // FIXME: Refactoring is needed. recover_public is calling in verify_transaction_unordered. let signer_public = tx.signer_public(); if default_origin.is_local() { - self.immune_users.write().insert(signer_public); + self.immune_users.insert(signer_public); } let origin = if self.accounts.has_account(&signer_public).unwrap_or_default() { @@ -220,7 +257,7 @@ impl Miner { default_origin }; - if self.malicious_users.read().contains(&signer_public) { + if self.malicious_users.contains(&signer_public) { // FIXME: just to skip, think about another way. return Ok(()) } @@ -228,7 +265,6 @@ impl Miner { cdebug!(MINER, "Rejected transaction {:?}: already in the blockchain", hash); return Err(HistoryError::TransactionAlreadyImported.into()) } - let immune_users = self.immune_users.read(); let tx: VerifiedTransaction = tx .verify_basic() .map_err(From::from) @@ -239,8 +275,8 @@ impl Miner { .and_then(|_| Ok(tx.try_into()?)) .map_err(|e| { match e { - Error::Syntax(_) if !origin.is_local() && !immune_users.contains(&signer_public) => { - self.malicious_users.write().insert(signer_public); + Error::Syntax(_) if !origin.is_local() && !self.immune_users.contains(&signer_public) => { + self.malicious_users.insert(signer_public); } _ => {} } @@ -356,7 +392,7 @@ impl Miner { for tx in transactions { let signer_public = tx.signer_public(); - if self.malicious_users.read().contains(&signer_public) { + if self.malicious_users.contains(&signer_public) { invalid_transactions.push(tx.hash()); continue } @@ -730,32 +766,23 @@ impl MinerService for Miner { } fn get_malicious_users(&self) -> Vec { - Vec::from_iter(self.malicious_users.read().iter().map(Clone::clone)) + self.malicious_users.cloned() } fn release_malicious_users(&self, prisoners: Vec) { - let mut malicious_users = self.malicious_users.write(); - for prisoner in &prisoners { - malicious_users.remove(prisoner); - } + self.malicious_users.remove_users(prisoners.iter()) } fn imprison_malicious_users(&self, prisoners: Vec) { - let mut malicious_users = self.malicious_users.write(); - for prisoner in prisoners { - malicious_users.insert(prisoner); - } + self.malicious_users.insert_users(prisoners.into_iter()); } fn get_immune_users(&self) -> Vec { - Vec::from_iter(self.immune_users.read().iter().map(Clone::clone)) + self.immune_users.cloned() } fn register_immune_users(&self, immune_users: Vec) { - let mut immune_users_lock = self.immune_users.write(); - for user in immune_users { - immune_users_lock.insert(user); - } + self.immune_users.insert_users(immune_users.into_iter()) } }