remove null signatures (#11335)

* tx: clean up legacy eip-86 based null signature

* tx: add a test for null signature rejection

* tx: revert json txn changes

* fix evmbin bench build

* tx: put UNSIGNED_SENDER behind 'test-helpers' feature

* Revert "tx: put UNSIGNED_SENDER behind 'test-helpers' feature"

This reverts commit 1dde478.

* tx: add comment for null_sign

* even more cleanup

* Revert "even more cleanup"

This reverts commit be29f03.

ethcore/src/client/evm_test_client.rs

@@ -245,7 +245,7 @@ impl<'a> EvmTestClient<'a> {
 	) -> std::result::Result<TransactSuccess<T::Output, V::Output>, TransactErr> {
 		let initial_gas = transaction.gas;
 		// Verify transaction
-		let is_ok = transaction.verify_basic(true, None, false);
+		let is_ok = transaction.verify_basic(true, None);
 		if let Err(error) = is_ok {
 			return Err(
 				TransactErr{

ethcore/src/machine.rs

@@ -367,7 +367,7 @@ impl Machine {
 		} else {
 			None
 		};
-		t.verify_basic(check_low_s, chain_id, false)?;
+		t.verify_basic(check_low_s, chain_id)?;
 
 		Ok(())
 	}

ethcore/types/src/transaction/transaction.rs

@@ -138,6 +138,7 @@ impl Transaction {
 	}
 }
 
+#[cfg(any(test, feature = "test-helpers"))]
 impl From<ethjson::transaction::Transaction> for SignedTransaction {
 	fn from(t: ethjson::transaction::Transaction) -> Self {
 		let to: Option<ethjson::hash::Address> = t.to.into();
@@ -237,7 +238,10 @@ impl Transaction {
 		}
 	}
 
-	/// Add EIP-86 compatible empty signature.
+	/// Legacy EIP-86 compatible empty signature.
+	/// This method is used in json tests as well as
+	/// signature verification tests.
+	#[cfg(any(test, feature = "test-helpers"))]
 	pub fn null_sign(self, chain_id: u64) -> SignedTransaction {
 		SignedTransaction {
 			transaction: UnverifiedTransaction {
@@ -295,7 +299,7 @@ impl rlp::Decodable for UnverifiedTransaction {
 			v: d.val_at(6)?,
 			r: d.val_at(7)?,
 			s: d.val_at(8)?,
-			hash: hash,
+			hash,
 		})
 	}
 }
@@ -312,7 +316,7 @@ impl UnverifiedTransaction {
 		self
 	}
 
-	/// Checks is signature is empty.
+	/// Checks if the signature is empty.
 	pub fn is_unsigned(&self) -> bool {
 		self.r.is_zero() && self.s.is_zero()
 	}
@@ -386,17 +390,12 @@ impl UnverifiedTransaction {
 	}
 
 	/// Verify basic signature params. Does not attempt sender recovery.
-	pub fn verify_basic(&self, check_low_s: bool, chain_id: Option<u64>, allow_empty_signature: bool) -> Result<(), error::Error> {
-		if check_low_s && !(allow_empty_signature && self.is_unsigned()) {
-			self.check_low_s()?;
-		}
-		// Disallow unsigned transactions in case EIP-86 is disabled.
-		if !allow_empty_signature && self.is_unsigned() {
+	pub fn verify_basic(&self, check_low_s: bool, chain_id: Option<u64>) -> Result<(), error::Error> {
+		if self.is_unsigned() {
 			return Err(ethkey::Error::InvalidSignature.into());
 		}
-		// EIP-86: Transactions of this form MUST have gasprice = 0, nonce = 0, value = 0, and do NOT increment the nonce of account 0.
-		if allow_empty_signature && self.is_unsigned() && !(self.gas_price.is_zero() && self.value.is_zero() && self.nonce.is_zero()) {
-			return Err(ethkey::Error::InvalidSignature.into())
+		if check_low_s {
+			self.check_low_s()?;
 		}
 		match (self.chain_id(), chain_id) {
 			(None, _) => {},
@@ -436,20 +435,15 @@ impl SignedTransaction {
 	/// Try to verify transaction and recover sender.
 	pub fn new(transaction: UnverifiedTransaction) -> Result<Self, ethkey::Error> {
 		if transaction.is_unsigned() {
-			Ok(SignedTransaction {
-				transaction: transaction,
-				sender: UNSIGNED_SENDER,
-				public: None,
-			})
-		} else {
-			let public = transaction.recover_public()?;
-			let sender = public_to_address(&public);
-			Ok(SignedTransaction {
-				transaction: transaction,
-				sender: sender,
-				public: Some(public),
-			})
+			return Err(ethkey::Error::InvalidSignature);
 		}
+		let public = transaction.recover_public()?;
+		let sender = public_to_address(&public);
+		Ok(SignedTransaction {
+			transaction,
+			sender,
+			public: Some(public),
+		})
 	}
 
 	/// Returns transaction sender.
@@ -640,6 +634,24 @@ mod tests {
 		assert_eq!(t.chain_id(), None);
 	}
 
+	#[test]
+	fn should_reject_null_signature() {
+		let t = Transaction {
+			nonce: U256::zero(),
+			gas_price: U256::from(10000000000u64),
+			gas: U256::from(21000),
+			action: Action::Call(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()),
+			value: U256::from(1),
+			data: vec![]
+		}.null_sign(1);
+
+		let res = SignedTransaction::new(t.transaction);
+		match res {
+			Err(parity_crypto::publickey::Error::InvalidSignature) => {}
+			_ => panic!("null signature should be rejected"),
+		}
+	}
+
 	#[test]
 	fn should_recover_from_chain_specific_signing() {
 		use ethkey::{Random, Generator};

evmbin/Cargo.toml

@@ -9,7 +9,7 @@ name = "parity-evm"
 path = "./src/main.rs"
 
 [dependencies]
-common-types = { path = "../ethcore/types" }
+common-types = { path = "../ethcore/types", features = ["test-helpers"] }
 docopt = "1.0"
 env_logger = "0.5"
 ethcore = { path = "../ethcore", features = ["test-helpers", "json-tests", "to-pod-full"] }

json/src/spec/builtin.rs

@@ -203,6 +203,28 @@ mod tests {
 		]);
 	}
 
+	#[test]
+	fn deserialization_alt_bn128_const_operations() {
+		let s = r#"{
+			"name": "alt_bn128_mul",
+			"pricing": {
+				"100500": {
+					"price": { "alt_bn128_const_operations": { "price": 123 }}
+				}
+			}
+		}"#;
+		let builtin: Builtin = serde_json::from_str::<BuiltinCompat>(s).unwrap().into();
+		assert_eq!(builtin.name, "alt_bn128_mul");
+		assert_eq!(builtin.pricing, map![
+			100500 => PricingAt {
+				info: None,
+				price: Pricing::AltBn128ConstOperations(AltBn128ConstOperations { 
+					price: 123,
+				}),
+			}
+		]);
+	}
+
 	#[test]
 	fn activate_at() {
 		let s = r#"{