ethcore: fix pow difficulty validation (#9328)

* ethcore: fix pow difficulty validation

* ethcore: validate difficulty is not zero

* ethcore: add issue link to regression test

* ethcore: fix tests

* ethcore: move difficulty_to_boundary to ethash crate

* ethcore: reuse difficulty_to_boundary and boundary_to_difficulty

* ethcore: fix grumbles in difficulty_to_boundary_aux

Author: andresilva

Cargo.lock

@@ -6,13 +6,14 @@ authors = ["Parity Technologies <admin@parity.io>"]
 [lib]
 
 [dependencies]
-log = "0.4"
-keccak-hash = { git = "https://github.com/paritytech/parity-common" }
-primal = "0.2.3"
-parking_lot = "0.6"
 crunchy = "0.1.0"
-memmap = "0.6"
 either = "1.0.0"
+ethereum-types = "0.3"
+keccak-hash = { git = "https://github.com/paritytech/parity-common" }
+log = "0.4"
+memmap = "0.6"
+parking_lot = "0.6"
+primal = "0.2.3"
 
 [dev-dependencies]
 tempdir = "0.3"

ethash/Cargo.toml

@@ -16,10 +16,11 @@
 
 #![cfg_attr(feature = "benches", feature(test))]
 
-extern crate primal;
-extern crate parking_lot;
 extern crate either;
+extern crate ethereum_types;
 extern crate memmap;
+extern crate parking_lot;
+extern crate primal;
 
 #[macro_use]
 extern crate crunchy;
@@ -38,6 +39,7 @@ mod shared;
 pub use cache::{NodeCacheBuilder, OptimizeFor};
 pub use compute::{ProofOfWork, quick_get_difficulty, slow_hash_block_number};
 use compute::Light;
+use ethereum_types::{U256, U512};
 use keccak::H256;
 use parking_lot::Mutex;
 pub use seed_compute::SeedHashCompute;
@@ -136,6 +138,29 @@ impl EthashManager {
 	}
 }
 
+/// Convert an Ethash boundary to its original difficulty. Basically just `f(x) = 2^256 / x`.
+pub fn boundary_to_difficulty(boundary: &ethereum_types::H256) -> U256 {
+	difficulty_to_boundary_aux(&**boundary)
+}
+
+/// Convert an Ethash difficulty to the target boundary. Basically just `f(x) = 2^256 / x`.
+pub fn difficulty_to_boundary(difficulty: &U256) -> ethereum_types::H256 {
+	difficulty_to_boundary_aux(difficulty).into()
+}
+
+fn difficulty_to_boundary_aux<T: Into<U512>>(difficulty: T) -> ethereum_types::U256 {
+	let difficulty = difficulty.into();
+
+	assert!(!difficulty.is_zero());
+
+	if difficulty == U512::one() {
+		U256::max_value()
+	} else {
+		// difficulty > 1, so result should never overflow 256 bits
+		U256::from((U512::one() << 256) / difficulty)
+	}
+}
+
 #[test]
 fn test_lru() {
 	use tempdir::TempDir;
@@ -155,6 +180,43 @@ fn test_lru() {
 	assert_eq!(ethash.cache.lock().prev_epoch.unwrap(), 0);
 }
 
+#[test]
+fn test_difficulty_to_boundary() {
+	use ethereum_types::H256;
+	use std::str::FromStr;
+
+	assert_eq!(difficulty_to_boundary(&U256::from(1)), H256::from(U256::max_value()));
+	assert_eq!(difficulty_to_boundary(&U256::from(2)), H256::from_str("8000000000000000000000000000000000000000000000000000000000000000").unwrap());
+	assert_eq!(difficulty_to_boundary(&U256::from(4)), H256::from_str("4000000000000000000000000000000000000000000000000000000000000000").unwrap());
+	assert_eq!(difficulty_to_boundary(&U256::from(32)), H256::from_str("0800000000000000000000000000000000000000000000000000000000000000").unwrap());
+}
+
+#[test]
+fn test_difficulty_to_boundary_regression() {
+	use ethereum_types::H256;
+
+	// the last bit was originally being truncated when performing the conversion
+	// https://github.com/paritytech/parity-ethereum/issues/8397
+	for difficulty in 1..9 {
+		assert_eq!(U256::from(difficulty), boundary_to_difficulty(&difficulty_to_boundary(&difficulty.into())));
+		assert_eq!(H256::from(difficulty), difficulty_to_boundary(&boundary_to_difficulty(&difficulty.into())));
+		assert_eq!(U256::from(difficulty), boundary_to_difficulty(&boundary_to_difficulty(&difficulty.into()).into()));
+		assert_eq!(H256::from(difficulty), difficulty_to_boundary(&difficulty_to_boundary(&difficulty.into()).into()));
+	}
+}
+
+#[test]
+#[should_panic]
+fn test_difficulty_to_boundary_panics_on_zero() {
+	difficulty_to_boundary(&U256::from(0));
+}
+
+#[test]
+#[should_panic]
+fn test_boundary_to_difficulty_panics_on_zero() {
+	boundary_to_difficulty(&ethereum_types::H256::from(0));
+}
+
 #[cfg(feature = "benches")]
 mod benchmarks {
 	extern crate test;

ethash/src/lib.rs

@@ -20,7 +20,7 @@ use std::collections::BTreeMap;
 use std::sync::Arc;
 use hash::{KECCAK_EMPTY_LIST_RLP};
 use engines::block_reward::{self, RewardKind};
-use ethash::{quick_get_difficulty, slow_hash_block_number, EthashManager, OptimizeFor};
+use ethash::{self, quick_get_difficulty, slow_hash_block_number, EthashManager, OptimizeFor};
 use ethereum_types::{H256, H64, U256, Address};
 use unexpected::{OutOfBounds, Mismatch};
 use block::*;
@@ -302,7 +302,7 @@ impl Engine<EthereumMachine> for Arc<Ethash> {
 			return Err(From::from(BlockError::DifficultyOutOfBounds(OutOfBounds { min: Some(min_difficulty), max: None, found: header.difficulty().clone() })))
 		}
 
-		let difficulty = Ethash::boundary_to_difficulty(&H256(quick_get_difficulty(
+		let difficulty = ethash::boundary_to_difficulty(&H256(quick_get_difficulty(
 			&header.bare_hash().0,
 			seal.nonce.low_u64(),
 			&seal.mix_hash.0
@@ -324,7 +324,7 @@ impl Engine<EthereumMachine> for Arc<Ethash> {
 
 		let result = self.pow.compute_light(header.number() as u64, &header.bare_hash().0, seal.nonce.low_u64());
 		let mix = H256(result.mix_hash);
-		let difficulty = Ethash::boundary_to_difficulty(&H256(result.value));
+		let difficulty = ethash::boundary_to_difficulty(&H256(result.value));
 		trace!(target: "miner", "num: {num}, seed: {seed}, h: {h}, non: {non}, mix: {mix}, res: {res}",
 			   num = header.number() as u64,
 			   seed = H256(slow_hash_block_number(header.number() as u64)),
@@ -447,25 +447,6 @@ impl Ethash {
 		}
 		target
 	}
-
-	/// Convert an Ethash boundary to its original difficulty. Basically just `f(x) = 2^256 / x`.
-	pub fn boundary_to_difficulty(boundary: &H256) -> U256 {
-		let d = U256::from(*boundary);
-		if d <= U256::one() {
-			U256::max_value()
-		} else {
-			((U256::one() << 255) / d) << 1
-		}
-	}
-
-	/// Convert an Ethash difficulty to the target boundary. Basically just `f(x) = 2^256 / x`.
-	pub fn difficulty_to_boundary(difficulty: &U256) -> H256 {
-		if *difficulty <= U256::one() {
-			U256::max_value().into()
-		} else {
-			(((U256::one() << 255) / *difficulty) << 1).into()
-		}
-	}
 }
 
 fn ecip1017_eras_block_reward(era_rounds: u64, mut reward: U256, block_number:u64) -> (u64, U256) {
@@ -766,16 +747,6 @@ mod tests {
 		}
 	}
 
-	#[test]
-	fn test_difficulty_to_boundary() {
-		// result of f(0) is undefined, so do not assert the result
-		let _ = Ethash::difficulty_to_boundary(&U256::from(0));
-		assert_eq!(Ethash::difficulty_to_boundary(&U256::from(1)), H256::from(U256::max_value()));
-		assert_eq!(Ethash::difficulty_to_boundary(&U256::from(2)), H256::from_str("8000000000000000000000000000000000000000000000000000000000000000").unwrap());
-		assert_eq!(Ethash::difficulty_to_boundary(&U256::from(4)), H256::from_str("4000000000000000000000000000000000000000000000000000000000000000").unwrap());
-		assert_eq!(Ethash::difficulty_to_boundary(&U256::from(32)), H256::from_str("0800000000000000000000000000000000000000000000000000000000000000").unwrap());
-	}
-
 	#[test]
 	fn difficulty_frontier() {
 		let machine = new_homestead_test_machine();

ethcore/src/ethereum/ethash.rs

@@ -22,8 +22,7 @@ use std::fmt;
 
 use client::{Client, ImportSealedBlock};
 use ethereum_types::{H64, H256, clean_0x, U256};
-use ethereum::ethash::Ethash;
-use ethash::SeedHashCompute;
+use ethash::{self, SeedHashCompute};
 #[cfg(feature = "work-notify")]
 use ethcore_miner::work_notify::NotifyWork;
 #[cfg(feature = "work-notify")]
@@ -167,7 +166,7 @@ impl StratumJobDispatcher {
 	/// Serializes payload for stratum service
 	fn payload(&self, pow_hash: H256, difficulty: U256, number: u64) -> String {
 		// TODO: move this to engine
-		let target = Ethash::difficulty_to_boundary(&difficulty);
+		let target = ethash::difficulty_to_boundary(&difficulty);
 		let seed_hash = &self.seed_compute.lock().hash_block_number(number);
 		let seed_hash = H256::from_slice(&seed_hash[..]);
 		format!(

ethcore/src/miner/stratum.rs

@@ -24,6 +24,7 @@ use hash::Address;
 pub struct EthashParams {
 	/// See main EthashParams docs.
 	#[serde(rename="minimumDifficulty")]
+	#[serde(deserialize_with="uint::validate_non_zero")]
 	pub minimum_difficulty: Uint,
 	/// See main EthashParams docs.
 	#[serde(rename="difficultyBoundDivisor")]

json/src/spec/ethash.rs

@@ -67,19 +67,10 @@ impl WorkPoster {
 	}
 }
 
-/// Convert an Ethash difficulty to the target boundary. Basically just `f(x) = 2^256 / x`.
-fn difficulty_to_boundary(difficulty: &U256) -> H256 {
-	if *difficulty <= U256::one() {
-		U256::max_value().into()
-	} else {
-		(((U256::one() << 255) / *difficulty) << 1).into()
-	}
-}
-
 impl NotifyWork for WorkPoster {
 	fn notify(&self, pow_hash: H256, difficulty: U256, number: u64) {
 		// TODO: move this to engine
-		let target = difficulty_to_boundary(&difficulty);
+		let target = ethash::difficulty_to_boundary(&difficulty);
 		let seed_hash = &self.seed_compute.lock().hash_block_number(number);
 		let seed_hash = H256::from_slice(&seed_hash[..]);
 		let body = format!(

miner/src/work_notify.rs

@@ -24,10 +24,9 @@ use rlp::{self, Rlp};
 use ethereum_types::{U256, H64, H256, Address};
 use parking_lot::Mutex;
 
-use ethash::SeedHashCompute;
+use ethash::{self, SeedHashCompute};
 use ethcore::account_provider::AccountProvider;
 use ethcore::client::{BlockChainClient, BlockId, TransactionId, UncleId, StateOrBlock, StateClient, StateInfo, Call, EngineInfo};
-use ethcore::ethereum::Ethash;
 use ethcore::filter::Filter as EthcoreFilter;
 use ethcore::header::{BlockNumber as EthBlockNumber};
 use ethcore::log_entry::LogEntry;
@@ -758,7 +757,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
 		})?;
 
 		let (pow_hash, number, timestamp, difficulty) = work;
-		let target = Ethash::difficulty_to_boundary(&difficulty);
+		let target = ethash::difficulty_to_boundary(&difficulty);
 		let seed_hash = self.seed_compute.lock().hash_block_number(number);
 
 		let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs();