Miner tweaks (#1797) (#1862)

* 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.

* Fixed tests

db/src/database.rs

@@ -26,12 +26,6 @@ use std::mem;
 use ipc::binary::BinaryConvertError;
 use std::collections::{VecDeque, HashMap, BTreeMap};
 
-impl From<String> for Error {
-	fn from(s: String) -> Error {
-		Error::RocksDb(s)
-	}
-}
-
 enum WriteCacheEntry {
 	Remove,
 	Write(Vec<u8>),

db/src/traits.rs

@@ -31,8 +31,8 @@ pub struct KeyValue {
 	pub value: Vec<u8>,
 }
 
-	#[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<String> 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>;
 

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<TraceError> for Error {
@@ -14,10 +19,18 @@ impl From<TraceError> for Error {
 	}
 }
 
+impl From<UtilError> 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),
 		}
 	}
 }

ethcore/src/miner/external.rs

@@ -15,9 +15,9 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use std::collections::HashMap;
-use std::sync::{Arc, RwLock};
-use util::numbers::U256;
-use util::hash::H256;
+use std::sync::{Arc};
+use std::time::{Instant, Duration};
+use util::{Mutex, U256, H256};
 
 /// External miner interface.
 pub trait ExternalMinerService: Send + Sync {
@@ -26,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<RwLock<HashMap<H256, U256>>>,
+	hashrates: Arc<Mutex<HashMap<H256, (Instant, U256)>>>,
 }
 
 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<RwLock<HashMap<H256, U256>>>) -> Self {
+	pub fn new(hashrates: Arc<Mutex<HashMap<H256, (Instant, U256)>>>) -> 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().unwrap().insert(id, hashrate);
+		self.hashrates.lock().unwrap().insert(id, (Instant::now() + Duration::from_secs(ENTRY_TIMEOUT), hashrate));
 	}
 
 	fn hashrate(&self) -> U256 {
-		self.hashrates.read().unwrap().iter().fold(U256::from(0), |sum, (_, v)| sum + *v)
-	}
-
-	fn is_mining(&self) -> bool {
-		!self.hashrates.read().unwrap().is_empty()
+		let mut hashrates = self.hashrates.lock().unwrap();
+		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 {
@@ -77,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]

ethcore/src/miner/miner.rs

@@ -689,6 +689,10 @@ impl MinerService for Miner {
 		}
 	}
 
+	fn is_sealing(&self) -> bool {
+		self.sealing_work.lock().unwrap().queue.is_in_use()
+	}
+
 	fn map_sealing_work<F, T>(&self, chain: &MiningBlockChainClient, f: F) -> Option<T> where F: FnOnce(&ClosedBlock) -> T {
 		trace!(target: "miner", "map_sealing_work: entering");
 		self.enable_and_prepare_sealing(chain);

ethcore/src/miner/mod.rs

@@ -148,6 +148,9 @@ pub trait MinerService : Send + Sync {
 	/// Returns highest transaction nonce for given address.
 	fn last_nonce(&self, address: &Address) -> Option<U256>;
 
+	/// Is it currently sealing?
+	fn is_sealing(&self) -> bool;
+
 	/// Suggested gas price.
 	fn sensible_gas_price(&self) -> U256 { 20000000000u64.into() }
 

rpc/src/v1/impls/eth.rs

@@ -283,7 +283,7 @@ impl<C, S, M, EM> Eth for EthClient<C, S, M, EM> where
 
 	fn is_mining(&self, params: Params) -> Result<Value, Error> {
 		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())
 		}
 	}

rpc/src/v1/tests/helpers/miner_service.rs

@@ -205,6 +205,10 @@ impl MinerService for TestMinerService {
 		self.last_nonces.read().unwrap().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<Bytes>) -> Result<(), Error> {

rpc/src/v1/tests/mocked/eth.rs

@@ -16,7 +16,8 @@
 
 use std::str::FromStr;
 use std::collections::HashMap;
-use std::sync::{Arc, RwLock};
+use std::sync::{Arc, RwLock, Mutex};
+use std::time::{Instant, Duration};
 use jsonrpc_core::IoHandler;
 use util::hash::{Address, H256, FixedHash};
 use util::numbers::{Uint, U256};
@@ -55,8 +56,8 @@ struct EthTester {
 	pub client: Arc<TestBlockChainClient>,
 	pub sync: Arc<TestSyncProvider>,
 	pub accounts_provider: Arc<AccountProvider>,
-	miner: Arc<TestMinerService>,
-	hashrates: Arc<RwLock<HashMap<H256, U256>>>,
+	pub miner: Arc<TestMinerService>,
+	hashrates: Arc<Mutex<HashMap<H256, (Instant, U256)>>>,
 	pub io: IoHandler,
 }
 
@@ -66,7 +67,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();
@@ -132,9 +133,9 @@ fn rpc_eth_syncing() {
 #[test]
 fn rpc_eth_hashrate() {
 	let tester = EthTester::default();
-	tester.hashrates.write().unwrap().insert(H256::from(0), U256::from(0xfffa));
-	tester.hashrates.write().unwrap().insert(H256::from(0), U256::from(0xfffb));
-	tester.hashrates.write().unwrap().insert(H256::from(1), U256::from(0x1));
+	tester.hashrates.lock().unwrap().insert(H256::from(0), (Instant::now() + Duration::from_secs(2), U256::from(0xfffa)));
+	tester.hashrates.lock().unwrap().insert(H256::from(0), (Instant::now() + Duration::from_secs(2), U256::from(0xfffb)));
+	tester.hashrates.lock().unwrap().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}"#;
@@ -157,8 +158,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().unwrap().get(&H256::from("0x59daa26581d0acd1fce254fb7e85952f4c09d0915afd33d3886cd914bc7d283c")).cloned(),
-		Some(U256::from(0x500_000)));
+	assert_eq!(tester.hashrates.lock().unwrap().get(&H256::from("0x59daa26581d0acd1fce254fb7e85952f4c09d0915afd33d3886cd914bc7d283c")).cloned().unwrap().1,
+		U256::from(0x500_000));
 }
 
 #[test]
@@ -209,16 +210,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().unwrap().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]

util/src/using_queue.rs

@@ -70,6 +70,9 @@ impl<T> UsingQueue<T> 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;