Revert "Build block without checking signatures (#4916)" (#5159)

* Revert "Build block without checking signatures (#4916)"

This reverts commit e50f610907.

* Some further clean ups

substrate/primitives/runtime/src/generic/mod.rs

@@ -39,15 +39,6 @@ pub use self::digest::{
 use crate::codec::Encode;
 use sp_std::prelude::*;
 
-/// Perform singature check.
-#[derive(PartialEq, Eq, Clone, Copy)]
-pub enum CheckSignature {
-	/// Perform.
-	Yes,
-	/// Don't perform.
-	No,
-}
-
 fn encode_with_vec_prefix<T: Encode, F: Fn(&mut Vec<u8>)>(encoder: F) -> Vec<u8> {
 	let size = ::sp_std::mem::size_of::<T>();
 	let reserve = match size {

substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs

@@ -24,8 +24,7 @@ use crate::{
 		self, Member, MaybeDisplay, SignedExtension, Checkable, Extrinsic, ExtrinsicMetadata,
 		IdentifyAccount,
 	},
-	generic::{CheckSignature, CheckedExtrinsic}, 
-	transaction_validity::{TransactionValidityError, InvalidTransaction},
+	generic::CheckedExtrinsic, transaction_validity::{TransactionValidityError, InvalidTransaction},
 };
 
 const TRANSACTION_VERSION: u8 = 4;
@@ -121,26 +120,18 @@ where
 {
 	type Checked = CheckedExtrinsic<AccountId, Call, Extra>;
 
-	fn check(self, check_signature: CheckSignature, lookup: &Lookup) -> Result<Self::Checked, TransactionValidityError> {
+	fn check(self, lookup: &Lookup) -> Result<Self::Checked, TransactionValidityError> {
 		Ok(match self.signature {
 			Some((signed, signature, extra)) => {
 				let signed = lookup.lookup(signed)?;
+				let raw_payload = SignedPayload::new(self.function, extra)?;
+				if !raw_payload.using_encoded(|payload| {
+					signature.verify(payload, &signed)
+				}) {
+					return Err(InvalidTransaction::BadProof.into())
+				}
 
-				let (function, extra) = if let CheckSignature::No = check_signature {
-					(self.function, extra)
-				} else {
-					let raw_payload = SignedPayload::new(self.function, extra)?;
-
-					if !raw_payload.using_encoded(|payload| {
-						signature.verify(payload, &signed)
-					}) {
-						return Err(InvalidTransaction::BadProof.into())
-					}
-					let (function, extra, _) = raw_payload.deconstruct();
-
-					(function, extra)
-				};
-
+				let (function, extra, _) = raw_payload.deconstruct();
 				CheckedExtrinsic {
 					signed: Some((signed, extra)),
 					function,
@@ -331,7 +322,6 @@ mod tests {
 	use sp_io::hashing::blake2_256;
 	use crate::codec::{Encode, Decode};
 	use crate::traits::{SignedExtension, IdentifyAccount, IdentityLookup};
-	use crate::generic::CheckSignature;
 	use serde::{Serialize, Deserialize};
 
 	type TestContext = IdentityLookup<u64>;
@@ -412,7 +402,7 @@ mod tests {
 	fn unsigned_check_should_work() {
 		let ux = Ex::new_unsigned(vec![0u8; 0]);
 		assert!(!ux.is_signed().unwrap_or(false));
-		assert!(<Ex as Checkable<TestContext>>::check(ux, CheckSignature::Yes, &Default::default()).is_ok());
+		assert!(<Ex as Checkable<TestContext>>::check(ux, &Default::default()).is_ok());
 	}
 
 	#[test]
@@ -425,7 +415,7 @@ mod tests {
 		);
 		assert!(ux.is_signed().unwrap_or(false));
 		assert_eq!(
-			<Ex as Checkable<TestContext>>::check(ux, CheckSignature::Yes, &Default::default()),
+			<Ex as Checkable<TestContext>>::check(ux, &Default::default()),
 			Err(InvalidTransaction::BadProof.into()),
 		);
 	}
@@ -440,7 +430,7 @@ mod tests {
 		);
 		assert!(ux.is_signed().unwrap_or(false));
 		assert_eq!(
-			<Ex as Checkable<TestContext>>::check(ux, CheckSignature::Yes, &Default::default()),
+			<Ex as Checkable<TestContext>>::check(ux, &Default::default()),
 			Ok(CEx { signed: Some((TEST_ACCOUNT, TestExtra)), function: vec![0u8; 0] }),
 		);
 	}

substrate/primitives/runtime/src/testing.rs

@@ -24,10 +24,11 @@ use crate::traits::{
 	SignedExtension, Dispatchable,
 };
 use crate::traits::ValidateUnsigned;
-use crate::{generic::{self, CheckSignature}, KeyTypeId, ApplyExtrinsicResult};
+use crate::{generic, KeyTypeId, ApplyExtrinsicResult};
 pub use sp_core::{H256, sr25519};
 use sp_core::{crypto::{CryptoType, Dummy, key_types, Public}, U256};
-use crate::transaction_validity::{TransactionValidity, TransactionValidityError, InvalidTransaction};
+use crate::transaction_validity::{TransactionValidity, TransactionValidityError};
+
 /// Authority Id
 #[derive(Default, PartialEq, Eq, Clone, Encode, Decode, Debug, Hash, Serialize, Deserialize, PartialOrd, Ord)]
 pub struct UintAuthorityId(pub u64);
@@ -293,69 +294,24 @@ impl<'a, Xt> Deserialize<'a> for Block<Xt> where Block<Xt>: Decode {
 	}
 }
 
-/// Test validity.
-#[derive(PartialEq, Eq, Clone, Encode, Decode)]
-pub enum TestValidity {
-	/// Valid variant that will pass all checks.
-	Valid,
-	/// Variant with invalid signature.
-	///
-	/// Will fail signature check.
-	SignatureInvalid(TransactionValidityError),
-	/// Variant with invalid logic.
-	///
-	/// Will fail all checks.
-	OtherInvalid(TransactionValidityError),
-}
-
-/// Test transaction.
+/// Test transaction, tuple of (sender, call, signed_extra)
+/// with index only used if sender is some.
 ///
-/// Used to mock actual transaction.
+/// If sender is some then the transaction is signed otherwise it is unsigned.
 #[derive(PartialEq, Eq, Clone, Encode, Decode)]
 pub struct TestXt<Call, Extra> {
-	/// Signature with extra.
-	///
-	/// if some, then the transaction is signed. Transaction is unsigned otherwise.
+	/// Signature of the extrinsic.
 	pub signature: Option<(u64, Extra)>,
-	/// Validity.
-	///
-	/// Instantiate invalid variant and transaction will fail correpsonding checks.
-	pub validity: TestValidity,
-	/// Call.
+	/// Call of the extrinsic.
 	pub call: Call,
 }
 
 impl<Call, Extra> TestXt<Call, Extra> {
-	/// New signed test `TextXt`.
-	pub fn new_signed(signature: (u64, Extra), call: Call) -> Self {
-		TestXt {
-			signature: Some(signature),
-			validity: TestValidity::Valid,
-			call,
-		}
+	/// Create a new `TextXt`.
+	pub fn new(call: Call, signature: Option<(u64, Extra)>) -> Self {
+		Self { call, signature }
 	}
-
-	/// New unsigned test `TextXt`.
-	pub fn new_unsigned(call: Call) -> Self {
-		TestXt {
-			signature: None,
-			validity: TestValidity::Valid,
-			call,
-		}
-	}
-
-	/// Build invalid variant of `TestXt`.
-	pub fn invalid(mut self, err: TransactionValidityError) -> Self {
-		self.validity = TestValidity::OtherInvalid(err);
-		self
-	}
-
-	/// Build badly signed variant of `TestXt`.
-	pub fn badly_signed(mut self) -> Self {
-		self.validity = TestValidity::SignatureInvalid(TransactionValidityError::Invalid(InvalidTransaction::BadProof));
-		self
-	}
- }
+}
 
 // Non-opaque extrinsics always 0.
 parity_util_mem::malloc_size_of_is_0!(any: TestXt<Call, Extra>);
@@ -368,29 +324,14 @@ impl<Call, Extra> Serialize for TestXt<Call, Extra> where TestXt<Call, Extra>: E
 
 impl<Call, Extra> Debug for TestXt<Call, Extra> {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		write!(f, "TestXt({:?}, {}, ...)",
-			self.signature.as_ref().map(|x| &x.0),
-			if let TestValidity::Valid = self.validity { "valid" } else { "invalid" }
-		)
+		write!(f, "TestXt({:?}, ...)", self.signature.as_ref().map(|x| &x.0))
 	}
 }
 
 impl<Call: Codec + Sync + Send, Context, Extra> Checkable<Context> for TestXt<Call, Extra> {
 	type Checked = Self;
-	fn check(self, signature: CheckSignature, _: &Context) -> Result<Self::Checked, TransactionValidityError> {
-		match self.validity {
-			TestValidity::Valid => Ok(self),
-			TestValidity::SignatureInvalid(e) =>
-				if let CheckSignature::No = signature {
-					Ok(self)
-				} else {
-					Err(e)
-				},
-			TestValidity::OtherInvalid(e)  => Err(e),
-		}
-	 }
+	fn check(self, _: &Context) -> Result<Self::Checked, TransactionValidityError> { Ok(self) }
 }
-
 impl<Call: Codec + Sync + Send, Extra> traits::Extrinsic for TestXt<Call, Extra> {
 	type Call = Call;
 	type SignaturePayload = (u64, Extra);
@@ -399,8 +340,8 @@ impl<Call: Codec + Sync + Send, Extra> traits::Extrinsic for TestXt<Call, Extra>
 		Some(self.signature.is_some())
 	}
 
-	fn new(call: Call, signature: Option<Self::SignaturePayload>) -> Option<Self> {
-		Some(TestXt { signature, call, validity: TestValidity::Valid })
+	fn new(c: Call, sig: Option<Self::SignaturePayload>) -> Option<Self> {
+		Some(TestXt { signature: sig, call: c })
 	}
 }
 

substrate/primitives/runtime/src/traits.rs

@@ -30,7 +30,7 @@ use crate::codec::{Codec, Encode, Decode};
 use crate::transaction_validity::{
 	ValidTransaction, TransactionValidity, TransactionValidityError, UnknownTransaction,
 };
-use crate::generic::{Digest, DigestItem, CheckSignature};
+use crate::generic::{Digest, DigestItem};
 pub use sp_arithmetic::traits::{
 	AtLeast32Bit, UniqueSaturatedInto, UniqueSaturatedFrom, Saturating, SaturatedConversion,
 	Zero, One, Bounded, CheckedAdd, CheckedSub, CheckedMul, CheckedDiv,
@@ -647,7 +647,7 @@ pub trait Checkable<Context>: Sized {
 	type Checked;
 
 	/// Check self, given an instance of Context.
-	fn check(self, signature: CheckSignature, c: &Context) -> Result<Self::Checked, TransactionValidityError>;
+	fn check(self, c: &Context) -> Result<Self::Checked, TransactionValidityError>;
 }
 
 /// A "checkable" piece of information, used by the standard Substrate Executive in order to
@@ -659,15 +659,15 @@ pub trait BlindCheckable: Sized {
 	type Checked;
 
 	/// Check self.
-	fn check(self, signature: CheckSignature) -> Result<Self::Checked, TransactionValidityError>;
+	fn check(self) -> Result<Self::Checked, TransactionValidityError>;
 }
 
 // Every `BlindCheckable` is also a `StaticCheckable` for arbitrary `Context`.
 impl<T: BlindCheckable, Context> Checkable<Context> for T {
 	type Checked = <Self as BlindCheckable>::Checked;
 
-	fn check(self, signature: CheckSignature, _c: &Context) -> Result<Self::Checked, TransactionValidityError> {
-		BlindCheckable::check(self, signature)
+	fn check(self, _c: &Context) -> Result<Self::Checked, TransactionValidityError> {
+		BlindCheckable::check(self)
 	}
 }