From 9de579366a058b0f6690f819a6dd9b8c95545da2 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 2 Aug 2016 17:53:32 +0100 Subject: [PATCH] Miner tweaks (#1797) * Mining fixes. - Use queue to determine whether we're mining - Kick stale hash rates Fixes #1794 Fixes #1641 * Fix tests. * Address grumbles. --- db/src/database.rs | 6 ---- db/src/traits.rs | 12 +++++-- ethcore/src/client/client.rs | 6 ++-- ethcore/src/client/error.rs | 15 ++++++++- ethcore/src/miner/external.rs | 39 ++++++++++++----------- ethcore/src/miner/miner.rs | 4 +++ ethcore/src/miner/mod.rs | 3 ++ rpc/src/v1/impls/eth.rs | 2 +- rpc/src/v1/tests/helpers/miner_service.rs | 4 +++ rpc/src/v1/tests/mocked/eth.rs | 24 ++++++-------- util/using_queue/src/lib.rs | 3 ++ 11 files changed, 72 insertions(+), 46 deletions(-) diff --git a/db/src/database.rs b/db/src/database.rs index 777ec3bbc..185618f99 100644 --- a/db/src/database.rs +++ b/db/src/database.rs @@ -25,12 +25,6 @@ use std::mem; use ipc::binary::BinaryConvertError; use std::collections::{VecDeque, HashMap, BTreeMap}; -impl From for Error { - fn from(s: String) -> Error { - Error::RocksDb(s) - } -} - enum WriteCacheEntry { Remove, Write(Vec), diff --git a/db/src/traits.rs b/db/src/traits.rs index dd5743fe5..bab132450 100644 --- a/db/src/traits.rs +++ b/db/src/traits.rs @@ -31,8 +31,8 @@ pub struct KeyValue { pub value: Vec, } - #[derive(Debug, Binary)] - pub enum Error { +#[derive(Debug, Binary)] +pub enum Error { AlreadyOpen, IsClosed, RocksDb(String), @@ -41,6 +41,12 @@ pub struct KeyValue { UncommitedTransactions, } +impl From for Error { + fn from(s: String) -> Error { + Error::RocksDb(s) + } +} + /// Database configuration #[derive(Binary)] pub struct DatabaseConfig { @@ -68,7 +74,7 @@ impl DatabaseConfig { } } - pub trait DatabaseService : Sized { +pub trait DatabaseService : Sized { /// Opens database in the specified path fn open(&self, config: DatabaseConfig, path: String) -> Result<(), Error>; diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index bb878b834..7c66cc5d3 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -178,15 +178,15 @@ impl Client { db_config.compaction = config.db_compaction.compaction_profile(); db_config.wal = config.db_wal; - let db = Arc::new(Database::open(&db_config, &path.to_str().unwrap()).expect("Error opening database")); + let db = Arc::new(try!(Database::open(&db_config, &path.to_str().unwrap()).map_err(ClientError::Database))); let chain = Arc::new(BlockChain::new(config.blockchain, &gb, db.clone())); let tracedb = Arc::new(try!(TraceDB::new(config.tracing, db.clone(), chain.clone()))); let mut state_db = journaldb::new(db.clone(), config.pruning, DB_COL_STATE); if state_db.is_empty() && spec.ensure_db_good(state_db.as_hashdb_mut()) { let batch = DBTransaction::new(&db); - state_db.commit(&batch, 0, &spec.genesis_header().hash(), None).expect("Error commiting genesis state to state DB"); - db.write(batch).expect("Error writing genesis state to state DB"); + try!(state_db.commit(&batch, 0, &spec.genesis_header().hash(), None)); + try!(db.write(batch).map_err(ClientError::Database)); } if !chain.block_header(&chain.best_block_hash()).map_or(true, |h| state_db.contains(h.state_root())) { diff --git a/ethcore/src/client/error.rs b/ethcore/src/client/error.rs index fde22cb10..4c5b465b6 100644 --- a/ethcore/src/client/error.rs +++ b/ethcore/src/client/error.rs @@ -1,4 +1,5 @@ use trace::Error as TraceError; +use util::UtilError; use std::fmt::{Display, Formatter, Error as FmtError}; /// Client configuration errors. @@ -6,6 +7,10 @@ use std::fmt::{Display, Formatter, Error as FmtError}; pub enum Error { /// TraceDB configuration error. Trace(TraceError), + /// Database error + Database(String), + /// Util error + Util(UtilError), } impl From for Error { @@ -14,10 +19,18 @@ impl From for Error { } } +impl From for Error { + fn from(err: UtilError) -> Self { + Error::Util(err) + } +} + impl Display for Error { fn fmt(&self, f: &mut Formatter) -> Result<(), FmtError> { match *self { - Error::Trace(ref err) => write!(f, "{}", err) + Error::Trace(ref err) => write!(f, "{}", err), + Error::Util(ref err) => write!(f, "{}", err), + Error::Database(ref s) => write!(f, "Database error: {}", s), } } } diff --git a/ethcore/src/miner/external.rs b/ethcore/src/miner/external.rs index ef6875930..c3fcf723f 100644 --- a/ethcore/src/miner/external.rs +++ b/ethcore/src/miner/external.rs @@ -16,7 +16,8 @@ use std::collections::HashMap; use std::sync::Arc; -use util::{RwLock, U256, H256}; +use std::time::{Instant, Duration}; +use util::{Mutex, U256, H256}; /// External miner interface. pub trait ExternalMinerService: Send + Sync { @@ -25,50 +26,50 @@ pub trait ExternalMinerService: Send + Sync { /// Total hashrate. fn hashrate(&self) -> U256; - - /// Returns true if external miner is mining. - fn is_mining(&self) -> bool; } /// External Miner. pub struct ExternalMiner { - hashrates: Arc>>, + hashrates: Arc>>, } impl Default for ExternalMiner { fn default() -> Self { ExternalMiner { - hashrates: Arc::new(RwLock::new(HashMap::new())), + hashrates: Arc::new(Mutex::new(HashMap::new())), } } } impl ExternalMiner { /// Creates new external miner with prefilled hashrates. - pub fn new(hashrates: Arc>>) -> Self { + pub fn new(hashrates: Arc>>) -> Self { ExternalMiner { - hashrates: hashrates + hashrates: hashrates, } } } +const ENTRY_TIMEOUT: u64 = 2; + impl ExternalMinerService for ExternalMiner { fn submit_hashrate(&self, hashrate: U256, id: H256) { - self.hashrates.write().insert(id, hashrate); + self.hashrates.lock().insert(id, (Instant::now() + Duration::from_secs(ENTRY_TIMEOUT), hashrate)); } fn hashrate(&self) -> U256 { - self.hashrates.read().iter().fold(U256::from(0), |sum, (_, v)| sum + *v) - } - - fn is_mining(&self) -> bool { - !self.hashrates.read().is_empty() + let mut hashrates = self.hashrates.lock(); + let h = hashrates.drain().filter(|&(_, (t, _))| t > Instant::now()).collect(); + *hashrates = h; + hashrates.iter().fold(U256::from(0), |sum, (_, &(_, v))| sum + v) } } #[cfg(test)] mod tests { use super::*; + use std::thread::sleep; + use std::time::Duration; use util::{H256, U256}; fn miner() -> ExternalMiner { @@ -76,16 +77,18 @@ mod tests { } #[test] - fn should_return_that_is_mining_if_there_is_at_least_one_entry() { + fn it_should_forget_old_hashrates() { // given let m = miner(); - assert_eq!(m.is_mining(), false); + assert_eq!(m.hashrate(), U256::from(0)); + m.submit_hashrate(U256::from(10), H256::from(1)); + assert_eq!(m.hashrate(), U256::from(10)); // when - m.submit_hashrate(U256::from(10), H256::from(1)); + sleep(Duration::from_secs(3)); // then - assert_eq!(m.is_mining(), true); + assert_eq!(m.hashrate(), U256::from(0)); } #[test] diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 1ebae894e..cc2e6d1e6 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -780,6 +780,10 @@ impl MinerService for Miner { } } + fn is_sealing(&self) -> bool { + self.sealing_work.lock().queue.is_in_use() + } + fn map_sealing_work(&self, chain: &MiningBlockChainClient, f: F) -> Option where F: FnOnce(&ClosedBlock) -> T { trace!(target: "miner", "map_sealing_work: entering"); self.enable_and_prepare_sealing(chain); diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 4b8f01c0e..15933901b 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -150,6 +150,9 @@ pub trait MinerService : Send + Sync { /// Returns highest transaction nonce for given address. fn last_nonce(&self, address: &Address) -> Option; + /// Is it currently sealing? + fn is_sealing(&self) -> bool; + /// Suggested gas price. fn sensible_gas_price(&self) -> U256 { 20000000000u64.into() } diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 4ad66e082..f773429e8 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -316,7 +316,7 @@ impl Eth for EthClient where fn is_mining(&self, params: Params) -> Result { try!(self.active()); match params { - Params::None => to_value(&self.external_miner.is_mining()), + Params::None => to_value(&(take_weak!(self.miner).is_sealing())), _ => Err(Error::invalid_params()) } } diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 21b4d5bba..3d83f934f 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -205,6 +205,10 @@ impl MinerService for TestMinerService { self.last_nonces.read().get(address).cloned() } + fn is_sealing(&self) -> bool { + false + } + /// Submit `seal` as a valid solution for the header of `pow_hash`. /// Will check the seal, but not actually insert the block into the chain. fn submit_seal(&self, _chain: &MiningBlockChainClient, _pow_hash: H256, _seal: Vec) -> Result<(), Error> { diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 0a9275bd8..e0c55f126 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -17,10 +17,11 @@ use std::str::FromStr; use std::collections::HashMap; use std::sync::Arc; +use std::time::{Instant, Duration}; use jsonrpc_core::IoHandler; use util::hash::{Address, H256, FixedHash}; use util::numbers::{Uint, U256}; -use util::RwLock; +use util::Mutex; use ethcore::account_provider::AccountProvider; use ethcore::client::{TestBlockChainClient, EachBlockWith, Executed, TransactionID}; use ethcore::log_entry::{LocalizedLogEntry, LogEntry}; @@ -57,7 +58,7 @@ struct EthTester { pub sync: Arc, pub accounts_provider: Arc, pub miner: Arc, - hashrates: Arc>>, + hashrates: Arc>>, pub io: IoHandler, } @@ -67,7 +68,7 @@ impl Default for EthTester { let sync = sync_provider(); let ap = accounts_provider(); let miner = miner_service(); - let hashrates = Arc::new(RwLock::new(HashMap::new())); + let hashrates = Arc::new(Mutex::new(HashMap::new())); let external_miner = Arc::new(ExternalMiner::new(hashrates.clone())); let eth = EthClient::new(&client, &sync, &ap, &miner, &external_miner, true).to_delegate(); let sign = EthSigningUnsafeClient::new(&client, &ap, &miner).to_delegate(); @@ -133,9 +134,9 @@ fn rpc_eth_syncing() { #[test] fn rpc_eth_hashrate() { let tester = EthTester::default(); - tester.hashrates.write().insert(H256::from(0), U256::from(0xfffa)); - tester.hashrates.write().insert(H256::from(0), U256::from(0xfffb)); - tester.hashrates.write().insert(H256::from(1), U256::from(0x1)); + tester.hashrates.lock().insert(H256::from(0), (Instant::now() + Duration::from_secs(2), U256::from(0xfffa))); + tester.hashrates.lock().insert(H256::from(0), (Instant::now() + Duration::from_secs(2), U256::from(0xfffb))); + tester.hashrates.lock().insert(H256::from(1), (Instant::now() + Duration::from_secs(2), U256::from(0x1))); let request = r#"{"jsonrpc": "2.0", "method": "eth_hashrate", "params": [], "id": 1}"#; let response = r#"{"jsonrpc":"2.0","result":"0xfffc","id":1}"#; @@ -158,8 +159,8 @@ fn rpc_eth_submit_hashrate() { let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; assert_eq!(tester.io.handle_request(request), Some(response.to_owned())); - assert_eq!(tester.hashrates.read().get(&H256::from("0x59daa26581d0acd1fce254fb7e85952f4c09d0915afd33d3886cd914bc7d283c")).cloned(), - Some(U256::from(0x500_000))); + assert_eq!(tester.hashrates.lock().get(&H256::from("0x59daa26581d0acd1fce254fb7e85952f4c09d0915afd33d3886cd914bc7d283c")).cloned().unwrap().1, + U256::from(0x500_000)); } #[test] @@ -210,16 +211,11 @@ fn rpc_eth_author() { #[test] fn rpc_eth_mining() { let tester = EthTester::default(); + tester.miner.set_author(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()); let request = r#"{"jsonrpc": "2.0", "method": "eth_mining", "params": [], "id": 1}"#; let response = r#"{"jsonrpc":"2.0","result":false,"id":1}"#; assert_eq!(tester.io.handle_request(request), Some(response.to_owned())); - - tester.hashrates.write().insert(H256::from(1), U256::from(0x1)); - - let request = r#"{"jsonrpc": "2.0", "method": "eth_mining", "params": [], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; - assert_eq!(tester.io.handle_request(request), Some(response.to_owned())); } #[test] diff --git a/util/using_queue/src/lib.rs b/util/using_queue/src/lib.rs index 0daa66f59..24dae94b9 100644 --- a/util/using_queue/src/lib.rs +++ b/util/using_queue/src/lib.rs @@ -71,6 +71,9 @@ impl UsingQueue where T: Clone { self.pending = Some(b); } + /// Is there anything in the queue currently? + pub fn is_in_use(&self) -> bool { self.in_use.len() > 0 } + /// Clears everything; the queue is entirely reset. pub fn reset(&mut self) { self.pending = None;