Signature cleanup (#1921)

* Address renamed to H160 at bigint library level

* moved uint specific test from util to bigint library

* naming

* unifing hashes in progress

* unifing hashes

* cleanup redundant unwraps in tests

* Removing util/crypto in progress.

* fixed compiling

* signature cleanup in progress

* new module - ethcrypto used by ethstore and ethcore-network

* fixed compiling

* fixed compiling

* fixed merge

ethcore/Cargo.toml

@@ -33,6 +33,7 @@ ethcore-devtools = { path = "../devtools" }
 ethjson = { path = "../json" }
 ethcore-ipc = { path = "../ipc/rpc" }
 ethstore = { path = "../ethstore" }
+ethkey = { path = "../ethkey" }
 ethcore-ipc-nano = { path = "../ipc/nano" }
 rand = "0.3"
 

ethcore/src/builtin.rs

@@ -14,10 +14,11 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use util::*;
 use crypto::sha2::Sha256;
 use crypto::ripemd160::Ripemd160;
 use crypto::digest::Digest;
+use util::*;
+use ethkey::{Signature, recover};
 use ethjson;
 
 /// Definition of a contract whose implementation is built-in.
@@ -92,19 +93,19 @@ pub fn new_builtin_exec(name: &str) -> Box<Fn(&[u8], &mut [u8])> {
 		}),
 		"ecrecover" => Box::new(move|input: &[u8], output: &mut[u8]| {
 			#[repr(packed)]
-			#[derive(Debug)]
+			#[derive(Debug, Default)]
 			struct InType {
 				hash: H256,
 				v: H256,
 				r: H256,
 				s: H256,
 			}
-			let mut it: InType = InType { hash: H256::new(), v: H256::new(), r: H256::new(), s: H256::new() };
+			let mut it = InType::default();
 			it.copy_raw(input);
 			if it.v == H256::from(&U256::from(27)) || it.v == H256::from(&U256::from(28)) {
-				let s = signature_from_rsv(&it.r, &it.s, it.v[31] - 27);
-				if ec::is_valid(&s) {
-					if let Ok(p) = ec::recover(&s, &it.hash) {
+				let s = Signature::from_rsv(&it.r, &it.s, it.v[31] - 27);
+				if s.is_valid() {
+					if let Ok(p) = recover(&s, &it.hash) {
 						let r = p.as_slice().sha3();
 						// NICE: optimise and separate out into populate-like function
 						for i in 0..min(32, output.len()) {

ethcore/src/client/test_client.rs

@@ -18,6 +18,7 @@
 
 use std::sync::atomic::{AtomicUsize, Ordering as AtomicOrder};
 use util::*;
+use ethkey::{Generator, Random};
 use devtools::*;
 use transaction::{Transaction, LocalizedTransaction, SignedTransaction, Action};
 use blockchain::TreeRoute;
@@ -188,7 +189,7 @@ impl TestBlockChainClient {
 			let txs = match with {
 				EachBlockWith::Transaction | EachBlockWith::UncleAndTransaction => {
 					let mut txs = RlpStream::new_list(1);
-					let keypair = KeyPair::create().unwrap();
+					let keypair = Random.generate().unwrap();
 					// Update nonces value
 					self.nonces.write().insert(keypair.address(), U256::one());
 					let tx = Transaction {

ethcore/src/engines/basic_authority.rs

@@ -17,6 +17,7 @@
 //! A blockchain engine that supports a basic, non-BFT proof-of-authority.
 
 use common::*;
+use ethkey::{recover, public_to_address};
 use account_provider::AccountProvider;
 use block::*;
 use spec::CommonParams;
@@ -133,7 +134,7 @@ impl Engine for BasicAuthority {
 	fn verify_block_unordered(&self, header: &Header, _block: Option<&[u8]>) -> result::Result<(), Error> {
 		// check the signature is legit.
 		let sig = try!(UntrustedRlp::new(&header.seal[0]).as_val::<H520>());
-		let signer = Address::from(try!(ec::recover(&sig, &header.bare_hash())).sha3());
+		let signer = public_to_address(&try!(recover(&sig.into(), &header.bare_hash())));
 		if !self.our_params.authorities.contains(&signer) {
 			return try!(Err(BlockError::InvalidSeal));
 		}
@@ -228,15 +229,10 @@ mod tests {
 	fn can_do_signature_verification_fail() {
 		let engine = new_test_authority().engine;
 		let mut header: Header = Header::default();
-		header.set_seal(vec![rlp::encode(&Signature::zero()).to_vec()]);
+		header.set_seal(vec![rlp::encode(&H520::default()).to_vec()]);
 
 		let verify_result = engine.verify_block_unordered(&header, None);
-
-		match verify_result {
-			Err(Error::Util(UtilError::Crypto(CryptoError::InvalidSignature))) => {},
-			Err(_) => { panic!("should be block difficulty error (got {:?})", verify_result); },
-			_ => { panic!("Should be error, got Ok"); },
-		}
+		assert!(verify_result.is_err());
 	}
 
 	#[test]

ethcore/src/engines/instant_seal.rs

@@ -100,7 +100,7 @@ mod tests {
 
 		assert!(engine.verify_block_basic(&header, None).is_ok());
 
-		header.set_seal(vec![rlp::encode(&Signature::zero()).to_vec()]);
+		header.set_seal(vec![rlp::encode(&H520::default()).to_vec()]);
 
 		assert!(engine.verify_block_unordered(&header, None).is_ok());
 	}

ethcore/src/error.rs

@@ -24,6 +24,7 @@ use client::Error as ClientError;
 use ipc::binary::{BinaryConvertError, BinaryConvertable};
 use types::block_import_error::BlockImportError;
 use snapshot::Error as SnapshotError;
+use ethkey::Error as EthkeyError;
 
 pub use types::executed::{ExecutionError, CallError};
 
@@ -238,6 +239,8 @@ pub enum Error {
 	Snappy(::util::snappy::InvalidInput),
 	/// Snapshot error.
 	Snapshot(SnapshotError),
+	/// Ethkey error.
+	Ethkey(EthkeyError),
 }
 
 impl fmt::Display for Error {
@@ -258,6 +261,7 @@ impl fmt::Display for Error {
 			Error::StdIo(ref err) => err.fmt(f),
 			Error::Snappy(ref err) => err.fmt(f),
 			Error::Snapshot(ref err) => err.fmt(f),
+			Error::Ethkey(ref err) => err.fmt(f),
 		}
 	}
 }
@@ -298,12 +302,6 @@ impl From<ExecutionError> for Error {
 	}
 }
 
-impl From<CryptoError> for Error {
-	fn from(err: CryptoError) -> Error {
-		Error::Util(UtilError::Crypto(err))
-	}
-}
-
 impl From<DecoderError> for Error {
 	fn from(err: DecoderError) -> Error {
 		Error::Util(UtilError::Decoder(err))
@@ -361,6 +359,12 @@ impl From<SnapshotError> for Error {
 	}
 }
 
+impl From<EthkeyError> for Error {
+	fn from(err: EthkeyError) -> Error {
+		Error::Ethkey(err)
+	}
+}
+
 impl<E> From<Box<E>> for Error where Error: From<E> {
 	fn from(err: Box<E>) -> Error {
 		Error::from(*err)

ethcore/src/executive.rs

@@ -483,6 +483,7 @@ impl<'a> Executive<'a> {
 #[cfg(test)]
 #[allow(dead_code)]
 mod tests {
+	use ethkey::{Generator, Random};
 	use super::*;
 	use common::*;
 	use evm::{Factory, VMType};
@@ -1002,7 +1003,7 @@ mod tests {
 	// TODO: fix (preferred) or remove
 	evm_test_ignore!{test_transact_simple: test_transact_simple_jit, test_transact_simple_int}
 	fn test_transact_simple(factory: Factory) {
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let t = Transaction {
 			action: Action::Create,
 			value: U256::from(17),
@@ -1069,7 +1070,7 @@ mod tests {
 
 	evm_test!{test_transact_invalid_nonce: test_transact_invalid_nonce_jit, test_transact_invalid_nonce_int}
 	fn test_transact_invalid_nonce(factory: Factory) {
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let t = Transaction {
 			action: Action::Create,
 			value: U256::from(17),
@@ -1102,7 +1103,7 @@ mod tests {
 
 	evm_test!{test_transact_gas_limit_reached: test_transact_gas_limit_reached_jit, test_transact_gas_limit_reached_int}
 	fn test_transact_gas_limit_reached(factory: Factory) {
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let t = Transaction {
 			action: Action::Create,
 			value: U256::from(17),
@@ -1137,7 +1138,7 @@ mod tests {
 	evm_test!{test_not_enough_cash: test_not_enough_cash_jit, test_not_enough_cash_int}
 	fn test_not_enough_cash(factory: Factory) {
 
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let t = Transaction {
 			action: Action::Create,
 			value: U256::from(18),

ethcore/src/lib.rs

@@ -96,6 +96,7 @@ extern crate bloomchain;
 extern crate rayon;
 extern crate hyper;
 extern crate ethash;
+extern crate ethkey;
 pub extern crate ethstore;
 extern crate semver;
 extern crate ethcore_ipc_nano as nanoipc;

ethcore/src/miner/miner.rs

@@ -911,6 +911,7 @@ mod tests {
 	use super::super::MinerService;
 	use super::*;
 	use util::*;
+	use ethkey::{Generator, Random};
 	use client::{TestBlockChainClient, EachBlockWith};
 	use client::{TransactionImportResult};
 	use types::transaction::{Transaction, Action};
@@ -975,7 +976,7 @@ mod tests {
 		let client = TestBlockChainClient::default();
 		let miner = miner();
 		let transaction = {
-			let keypair = KeyPair::create().unwrap();
+			let keypair = Random.generate().unwrap();
 			Transaction {
 				action: Action::Create,
 				value: U256::zero(),
@@ -1005,7 +1006,7 @@ mod tests {
 		let client = TestBlockChainClient::default();
 		let miner = miner();
 		let transaction = {
-			let keypair = KeyPair::create().unwrap();
+			let keypair = Random.generate().unwrap();
 			Transaction {
 				action: Action::Create,
 				value: U256::zero(),

ethcore/src/miner/transaction_queue.rs

@@ -26,16 +26,17 @@
 //! ```rust
 //! extern crate ethcore_util as util;
 //! extern crate ethcore;
+//! extern crate ethkey;
 //! extern crate rustc_serialize;
 //!
-//!	use util::crypto::KeyPair;
 //! use util::{Uint, U256, Address};
+//! use ethkey::{Random, Generator};
 //!	use ethcore::miner::{TransactionQueue, AccountDetails, TransactionOrigin};
 //!	use ethcore::transaction::*;
 //!	use rustc_serialize::hex::FromHex;
 //!
 //! fn main() {
-//!		let key = KeyPair::create().unwrap();
+//!		let key = Random.generate().unwrap();
 //!		let t1 = Transaction { action: Action::Create, value: U256::from(100), data: "3331600055".from_hex().unwrap(),
 //!			gas: U256::from(100_000), gas_price: U256::one(), nonce: U256::from(10) };
 //!		let t2 = Transaction { action: Action::Create, value: U256::from(100), data: "3331600055".from_hex().unwrap(),
@@ -233,7 +234,7 @@ struct TransactionSet {
 impl TransactionSet {
 	/// Inserts `TransactionOrder` to this set. Transaction does not need to be unique -
 	/// the same transaction may be validly inserted twice. Any previous transaction that
-	/// it replaces (i.e. with the same `sender` and `nonce`) should be returned. 
+	/// it replaces (i.e. with the same `sender` and `nonce`) should be returned.
 	fn insert(&mut self, sender: Address, nonce: U256, order: TransactionOrder) -> Option<TransactionOrder> {
 		if !self.by_priority.insert(order.clone()) {
 			return Some(order.clone());
@@ -313,7 +314,7 @@ impl TransactionSet {
 	}
 
 	/// Get the minimum gas price that we can accept into this queue that wouldn't cause the transaction to
-	/// immediately be dropped. 0 if the queue isn't at capacity; 1 plus the lowest if it is. 
+	/// immediately be dropped. 0 if the queue isn't at capacity; 1 plus the lowest if it is.
 	fn gas_price_entry_limit(&self) -> U256 {
 		match self.by_gas_price.keys().next() {
 			Some(k) if self.by_priority.len() >= self.limit => *k + 1.into(),
@@ -340,7 +341,7 @@ impl TransactionSet {
 				return false;
 			}
 		} else {
-			// Operation failed: gas-price not found in Map. 
+			// Operation failed: gas-price not found in Map.
 			return false;
 		}
 		// Operation maybe ok: only if hash not found in gas-price Set.
@@ -869,6 +870,7 @@ mod test {
 	extern crate rustc_serialize;
 	use util::table::*;
 	use util::*;
+	use ethkey::{Random, Generator};
 	use transaction::*;
 	use error::{Error, TransactionError};
 	use super::*;
@@ -897,7 +899,7 @@ mod test {
 	}
 
 	fn new_tx(nonce: U256, gas_price: U256) -> SignedTransaction {
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		new_unsigned_tx(nonce, gas_price).sign(keypair.secret())
 	}
 
@@ -916,7 +918,7 @@ mod test {
 		let tx1 = new_unsigned_tx(nonce, gas_price);
 		let tx2 = new_unsigned_tx(nonce + nonce_increment, gas_price + gas_price_increment);
 
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let secret = &keypair.secret();
 		(tx1.sign(secret), tx2.sign(secret))
 	}
@@ -1373,7 +1375,7 @@ mod test {
 	fn should_move_transactions_if_gap_filled() {
 		// given
 		let mut txq = TransactionQueue::new();
-		let kp = KeyPair::create().unwrap();
+		let kp = Random.generate().unwrap();
 		let secret = kp.secret();
 		let tx = new_unsigned_tx(123.into(), 1.into()).sign(secret);
 		let tx1 = new_unsigned_tx(124.into(), 1.into()).sign(secret);
@@ -1397,7 +1399,7 @@ mod test {
 	fn should_remove_transaction() {
 		// given
 		let mut txq2 = TransactionQueue::new();
-		let (tx, tx2) = new_tx_pair_default(3.into(), 0.into()); 
+		let (tx, tx2) = new_tx_pair_default(3.into(), 0.into());
 		txq2.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap();
 		txq2.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap();
 		assert_eq!(txq2.status().pending, 1);
@@ -1582,7 +1584,7 @@ mod test {
 		init_log();
 		// given
 		let mut txq = TransactionQueue::new();
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let tx = new_unsigned_tx(123.into(), 1.into()).sign(keypair.secret());
 		let tx2 = {
 			let mut tx2 = (*tx).clone();
@@ -1605,7 +1607,7 @@ mod test {
 	fn should_replace_same_transaction_when_importing_to_futures() {
 		// given
 		let mut txq = TransactionQueue::new();
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 		let tx0 = new_unsigned_tx(123.into(), 1.into()).sign(keypair.secret());
 		let tx1 = {
 			let mut tx1 = (*tx0).clone();
@@ -1758,7 +1760,7 @@ mod test {
 		// given
 		let mut txq = TransactionQueue::new();
 		let (tx1, tx2, tx2_2, tx3) = {
-			let keypair = KeyPair::create().unwrap();
+			let keypair = Random.generate().unwrap();
 			let secret = &keypair.secret();
 			let nonce = 123.into();
 			let tx = new_unsigned_tx(nonce, 1.into());

ethcore/src/tests/helpers.rs

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
+use ethkey::KeyPair;
 use io::*;
 use client::{BlockChainClient, Client, ClientConfig};
 use common::*;
@@ -145,7 +146,7 @@ pub fn generate_dummy_client_with_spec_and_data<F>(get_test_spec: F, block_numbe
 	let mut last_hashes = vec![];
 	let mut last_header = genesis_header.clone();
 
-	let kp = KeyPair::from_secret("".sha3()).unwrap()	;
+	let kp = KeyPair::from_secret("".sha3()).unwrap();
 	let author = kp.address();
 
 	let mut n = 0;

ethcore/src/types/transaction.rs

@@ -16,18 +16,16 @@
 
 //! Transaction data structure.
 
-use util::{H256, Address, U256, H520};
 use std::ops::Deref;
-use util::rlp::*;
-use util::sha3::*;
-use util::{UtilError, CryptoError, Bytes, Signature, Secret, ec};
-use util::crypto::{signature_from_rsv, signature_to_rsv};
 use std::cell::*;
+use util::rlp::*;
+use util::sha3::Hashable;
+use util::{H256, Address, U256, Bytes};
+use ethkey::{Signature, sign, Secret, recover, public_to_address, Error as EthkeyError};
 use error::*;
 use evm::Schedule;
 use header::BlockNumber;
 use ethjson;
-use ethstore::ethkey::Signature as EthkeySignature;
 
 #[derive(Debug, Clone, PartialEq, Eq, Binary)]
 /// Transaction action type.
@@ -139,19 +137,17 @@ impl Transaction {
 
 	/// Signs the transaction as coming from `sender`.
 	pub fn sign(self, secret: &Secret) -> SignedTransaction {
-		let sig = ec::sign(secret, &self.hash()).unwrap();
-		self.with_signature(sig.into())
+		let sig = sign(secret, &self.hash()).unwrap();
+		self.with_signature(sig)
 	}
 
 	/// Signs the transaction with signature.
-	pub fn with_signature(self, sig: EthkeySignature) -> SignedTransaction {
-		let sig: H520 = sig.into();
-		let (r, s, v) = signature_to_rsv(&sig);
+	pub fn with_signature(self, sig: Signature) -> SignedTransaction {
 		SignedTransaction {
 			unsigned: self,
-			r: r,
-			s: s,
-			v: v + 27,
+			r: sig.r().into(),
+			s: sig.s().into(),
+			v: sig.v() + 27,
 			hash: Cell::new(None),
 			sender: Cell::new(None),
 		}
@@ -290,12 +286,14 @@ impl SignedTransaction {
 	pub fn standard_v(&self) -> u8 { match self.v { 27 => 0, 28 => 1, _ => 4 } }
 
 	/// Construct a signature object from the sig.
-	pub fn signature(&self) -> Signature { signature_from_rsv(&From::from(&self.r), &From::from(&self.s), self.standard_v()) }
+	pub fn signature(&self) -> Signature {
+		Signature::from_rsv(&self.r.into(), &self.s.into(), self.standard_v())
+	}
 
 	/// Checks whether the signature has a low 's' value.
 	pub fn check_low_s(&self) -> Result<(), Error> {
-		if !ec::is_low_s(&self.s) {
-			Err(Error::Util(UtilError::Crypto(CryptoError::InvalidSignature)))
+		if !self.signature().is_low_s() {
+			Err(EthkeyError::InvalidSignature.into())
 		} else {
 			Ok(())
 		}
@@ -307,7 +305,7 @@ impl SignedTransaction {
 		match sender {
 			Some(s) => Ok(s),
 			None => {
-				let s = Address::from(try!(ec::recover(&self.signature(), &self.unsigned.hash())).sha3());
+				let s = public_to_address(&try!(recover(&self.signature(), &self.unsigned.hash())));
 				self.sender.set(Some(s));
 				Ok(s)
 			}
@@ -319,8 +317,8 @@ impl SignedTransaction {
 	#[cfg(test)]
 	#[cfg(feature = "json-tests")]
 	pub fn validate(self, schedule: &Schedule, require_low: bool) -> Result<SignedTransaction, Error> {
-		if require_low && !ec::is_low_s(&self.s) {
-			return Err(Error::Util(UtilError::Crypto(CryptoError::InvalidSignature)));
+		if require_low && !self.signature().is_low_s() {
+			return Err(EthkeyError::InvalidSignature.into())
 		}
 		try!(self.sender());
 		if self.gas < U256::from(self.gas_required(&schedule)) {
@@ -368,7 +366,9 @@ fn sender_test() {
 
 #[test]
 fn signing() {
-	let key = ::util::crypto::KeyPair::create().unwrap();
+	use ethkey::{Random, Generator};
+
+	let key = Random.generate().unwrap();
 	let t = Transaction {
 		action: Action::Create,
 		nonce: U256::from(42),

ethcore/src/verification/verification.rs

@@ -228,6 +228,7 @@ fn verify_block_integrity(block: &[u8], transactions_root: &H256, uncles_hash: &
 #[cfg(test)]
 mod tests {
 	use util::*;
+	use ethkey::{Random, Generator};
 	use header::*;
 	use verification::*;
 	use blockchain::extras::*;
@@ -355,7 +356,7 @@ mod tests {
 		good.timestamp = 40;
 		good.number = 10;
 
-		let keypair = KeyPair::create().unwrap();
+		let keypair = Random.generate().unwrap();
 
 		let tr1 = Transaction {
 			action: Action::Create,