Deprecate ValidateUnsigned and prevent duplicate heartbeats (#3975)

* Add pre-dispatch checks for ValidateUnsigned

* Deprecate ValidateUnsigned.

* Bump specversion.

* Fix test.

substrate/core/sr-primitives/src/generic/checked_extrinsic.rs

@@ -18,8 +18,10 @@
 //! stage.
 
 use crate::traits::{
-	self, Member, MaybeDisplay, SignedExtension, Dispatchable, ValidateUnsigned,
+	self, Member, MaybeDisplay, SignedExtension, Dispatchable,
 };
+#[allow(deprecated)]
+use crate::traits::ValidateUnsigned;
 use crate::weights::{GetDispatchInfo, DispatchInfo};
 use crate::transaction_validity::TransactionValidity;
 
@@ -51,6 +53,7 @@ where
 		self.signed.as_ref().map(|x| &x.0)
 	}
 
+	#[allow(deprecated)] // Allow ValidateUnsigned
 	fn validate<U: ValidateUnsigned<Call = Self::Call>>(
 		&self,
 		info: DispatchInfo,
@@ -60,11 +63,13 @@ where
 			Extra::validate(extra, id, &self.function, info, len)
 		} else {
 			let valid = Extra::validate_unsigned(&self.function, info, len)?;
-			Ok(valid.combine_with(U::validate_unsigned(&self.function)?))
+			let unsigned_validation = U::validate_unsigned(&self.function)?;
+			Ok(valid.combine_with(unsigned_validation))
 		}
 	}
 
-	fn apply(
+	#[allow(deprecated)] // Allow ValidateUnsigned
+	fn apply<U: ValidateUnsigned<Call=Self::Call>>(
 		self,
 		info: DispatchInfo,
 		len: usize,
@@ -74,6 +79,7 @@ where
 			(Some(id), pre)
 		} else {
 			let pre = Extra::pre_dispatch_unsigned(&self.function, info, len)?;
+			U::pre_dispatch(&self.function)?;
 			(None, pre)
 		};
 		let res = self.function.dispatch(Origin::from(maybe_who));

substrate/core/sr-primitives/src/testing.rs

@@ -20,9 +20,11 @@ use serde::{Serialize, Serializer, Deserialize, de::Error as DeError, Deserializ
 use std::{fmt::Debug, ops::Deref, fmt, cell::RefCell};
 use crate::codec::{Codec, Encode, Decode};
 use crate::traits::{
-	self, Checkable, Applyable, BlakeTwo256, OpaqueKeys, ValidateUnsigned,
+	self, Checkable, Applyable, BlakeTwo256, OpaqueKeys,
 	SignedExtension, Dispatchable,
 };
+#[allow(deprecated)]
+use crate::traits::ValidateUnsigned;
 use crate::{generic, KeyTypeId, ApplyResult};
 use crate::weights::{GetDispatchInfo, DispatchInfo};
 pub use primitives::{H256, sr25519};
@@ -323,6 +325,7 @@ impl<Origin, Call, Extra> Applyable for TestXt<Call, Extra> where
 	fn sender(&self) -> Option<&Self::AccountId> { self.0.as_ref().map(|x| &x.0) }
 
 	/// Checks to see if this is a valid *transaction*. It returns information on it if so.
+	#[allow(deprecated)] // Allow ValidateUnsigned
 	fn validate<U: ValidateUnsigned<Call=Self::Call>>(
 		&self,
 		_info: DispatchInfo,
@@ -333,7 +336,8 @@ impl<Origin, Call, Extra> Applyable for TestXt<Call, Extra> where
 
 	/// Executes all necessary logic needed prior to dispatch and deconstructs into function call,
 	/// index and sender.
-	fn apply(
+	#[allow(deprecated)] // Allow ValidateUnsigned
+	fn apply<U: ValidateUnsigned<Call=Self::Call>>(
 		self,
 		info: DispatchInfo,
 		len: usize,

substrate/core/sr-primitives/src/traits.rs

@@ -755,9 +755,6 @@ pub trait SignedExtension: Codec + Debug + Sync + Send + Clone + Eq + PartialEq
 
 	/// Validate an unsigned transaction for the transaction queue.
 	///
-	/// Normally the default implementation is fine since `ValidateUnsigned`
-	/// is a better way of recognising and validating unsigned transactions.
-	///
 	/// This function can be called frequently by the transaction queue,
 	/// to obtain transaction validity against current state.
 	/// It should perform all checks that determine a valid unsigned transaction,
@@ -889,6 +886,7 @@ pub trait Applyable: Sized + Send + Sync {
 	fn sender(&self) -> Option<&Self::AccountId>;
 
 	/// Checks to see if this is a valid *transaction*. It returns information on it if so.
+	#[allow(deprecated)] // Allow ValidateUnsigned
 	fn validate<V: ValidateUnsigned<Call=Self::Call>>(
 		&self,
 		info: DispatchInfo,
@@ -897,7 +895,8 @@ pub trait Applyable: Sized + Send + Sync {
 
 	/// Executes all necessary logic needed prior to dispatch and deconstructs into function call,
 	/// index and sender.
-	fn apply(
+	#[allow(deprecated)] // Allow ValidateUnsigned
+	fn apply<V: ValidateUnsigned<Call=Self::Call>>(
 		self,
 		info: DispatchInfo,
 		len: usize,
@@ -966,10 +965,27 @@ pub trait RuntimeApiInfo {
 /// the transaction for the transaction pool.
 /// During block execution phase one need to perform the same checks anyway,
 /// since this function is not being called.
+#[deprecated(note = "Use SignedExtensions instead.")]
 pub trait ValidateUnsigned {
 	/// The call to validate
 	type Call;
 
+	/// Validate the call right before dispatch.
+	///
+	/// This method should be used to prevent transactions already in the pool
+	/// (i.e. passing `validate_unsigned`) from being included in blocks
+	/// in case we know they now became invalid.
+	///
+	/// By default it's a good idea to call `validate_unsigned` from within
+	/// this function again to make sure we never include an invalid transaction.
+	///
+	/// Changes made to storage WILL be persisted if the call returns `Ok`.
+	fn pre_dispatch(call: &Self::Call) -> Result<(), crate::ApplyError> {
+		Self::validate_unsigned(call)
+			.map(|_| ())
+			.map_err(Into::into)
+	}
+
 	/// Return the validity of the call
 	///
 	/// This doesn't execute any side-effects; it merely checks

substrate/srml/executive/src/lib.rs

@@ -60,7 +60,9 @@
 //! # pub type AllModules = u64;
 //! # pub enum Runtime {};
 //! # use sr_primitives::transaction_validity::{TransactionValidity, UnknownTransaction};
+//!	# #[allow(deprecated)]
 //! # use sr_primitives::traits::ValidateUnsigned;
+//!	# #[allow(deprecated)]
 //! # impl ValidateUnsigned for Runtime {
 //! # 	type Call = ();
 //! #
@@ -79,10 +81,12 @@ use sr_primitives::{
 	generic::Digest, ApplyResult, weights::GetDispatchInfo,
 	traits::{
 		self, Header, Zero, One, Checkable, Applyable, CheckEqual, OnFinalize, OnInitialize,
-		NumberFor, Block as BlockT, OffchainWorker, ValidateUnsigned, Dispatchable
+		NumberFor, Block as BlockT, OffchainWorker, Dispatchable,
 	},
 	transaction_validity::TransactionValidity,
 };
+#[allow(deprecated)]
+use sr_primitives::traits::ValidateUnsigned;
 use codec::{Codec, Encode};
 use system::{extrinsics_root, DigestOf};
 
@@ -100,6 +104,7 @@ pub struct Executive<System, Block, Context, UnsignedValidator, AllModules>(
 	PhantomData<(System, Block, Context, UnsignedValidator, AllModules)>
 );
 
+#[allow(deprecated)] // Allow ValidateUnsigned, remove the attribute when the trait is removed.
 impl<
 	System: system::Trait,
 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
@@ -119,6 +124,7 @@ where
 	}
 }
 
+#[allow(deprecated)] // Allow ValidateUnsigned, remove the attribute when the trait is removed.
 impl<
 	System: system::Trait,
 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
@@ -242,7 +248,7 @@ where
 
 		// Decode parameters and dispatch
 		let dispatch_info = xt.get_dispatch_info();
-		let r = Applyable::apply(xt, dispatch_info, encoded_len)?;
+		let r = Applyable::apply::<UnsignedValidator>(xt, dispatch_info, encoded_len)?;
 
 		<system::Module<System>>::note_applied_extrinsic(&r, encoded_len as u32);
 
@@ -326,7 +332,6 @@ mod tests {
 		}
 	}
 
-	// Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 	#[derive(Clone, Eq, PartialEq)]
 	pub struct Runtime;
 	parameter_types! {
@@ -382,9 +387,14 @@ mod tests {
 		type FeeMultiplierUpdate = ();
 	}
 
+	#[allow(deprecated)]
 	impl ValidateUnsigned for Runtime {
 		type Call = Call;
 
+		fn pre_dispatch(_call: &Self::Call) -> Result<(), ApplyError> {
+			Ok(())
+		}
+
 		fn validate_unsigned(call: &Self::Call) -> TransactionValidity {
 			match call {
 				Call::Balances(BalancesCall::set_balance(_, _, _)) => Ok(Default::default()),

substrate/srml/im-online/src/lib.rs

@@ -88,7 +88,7 @@ use sr_staking_primitives::{
 	offence::{ReportOffence, Offence, Kind},
 };
 use support::{
-	decl_module, decl_event, decl_storage, print, ensure, Parameter, debug
+	decl_module, decl_event, decl_storage, print, Parameter, debug
 };
 use system::ensure_none;
 use system::offchain::SubmitUnsignedTransaction;
@@ -243,24 +243,20 @@ decl_module! {
 		fn heartbeat(
 			origin,
 			heartbeat: Heartbeat<T::BlockNumber>,
-			signature: <T::AuthorityId as RuntimeAppPublic>::Signature
+			// since signature verification is done in `validate_unsigned`
+			// we can skip doing it here again.
+			_signature: <T::AuthorityId as RuntimeAppPublic>::Signature
 		) {
 			ensure_none(origin)?;
 
 			let current_session = <session::Module<T>>::current_index();
-			ensure!(current_session == heartbeat.session_index, "Outdated heartbeat received.");
 			let exists = <ReceivedHeartbeats>::exists(
 				&current_session,
 				&heartbeat.authority_index
 			);
 			let keys = Keys::<T>::get();
-			let maybe_public = keys.get(heartbeat.authority_index as usize);
+			let public = keys.get(heartbeat.authority_index as usize);
-			if let (false, Some(public)) = (exists, maybe_public) {
+			if let (false, Some(public)) = (exists, public) {
-				let signature_valid = heartbeat.using_encoded(|encoded_heartbeat| {
-					public.verify(&encoded_heartbeat, &signature)
-				});
-				ensure!(signature_valid, "Invalid heartbeat signature.");
-
 				Self::deposit_event(Event::<T>::HeartbeatReceived(public.clone()));
 
 				let network_state = heartbeat.network_state.encode();
@@ -563,6 +559,7 @@ impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
 	}
 }
 
+#[allow(deprecated)]
 impl<T: Trait> support::unsigned::ValidateUnsigned for Module<T> {
 	type Call = Call<T>;
 

substrate/srml/im-online/src/tests.rs

@@ -103,6 +103,9 @@ fn heartbeat(
 	authority_index: u32,
 	id: UintAuthorityId,
 ) -> dispatch::Result {
+	#[allow(deprecated)]
+	use support::unsigned::ValidateUnsigned;
+
 	let heartbeat = Heartbeat {
 		block_number,
 		network_state: OpaqueNetworkState {
@@ -114,6 +117,8 @@ fn heartbeat(
 	};
 	let signature = id.sign(&heartbeat.encode()).unwrap();
 
+	#[allow(deprecated)] // Allow ValidateUnsigned
+	ImOnline::pre_dispatch(&crate::Call::heartbeat(heartbeat.clone(), signature.clone()))?;
 	ImOnline::heartbeat(
 		Origin::system(system::RawOrigin::None),
 		heartbeat,
@@ -170,8 +175,8 @@ fn late_heartbeat_should_fail() {
 		assert_eq!(Session::validators(), vec![1, 2, 3]);
 
 		// when
-		assert_noop!(heartbeat(1, 3, 0, 1.into()), "Outdated heartbeat received.");
+		assert_noop!(heartbeat(1, 3, 0, 1.into()), "Transaction is outdated");
-		assert_noop!(heartbeat(1, 1, 0, 1.into()), "Outdated heartbeat received.");
+		assert_noop!(heartbeat(1, 1, 0, 1.into()), "Transaction is outdated");
 	});
 }
 

substrate/srml/support/src/unsigned.rs

@@ -15,6 +15,7 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 #[doc(hidden)]
+#[allow(deprecated)]
 pub use crate::sr_primitives::traits::ValidateUnsigned;
 #[doc(hidden)]
 pub use crate::sr_primitives::transaction_validity::{TransactionValidity, UnknownTransaction};
@@ -65,9 +66,20 @@ macro_rules! impl_outer_validate_unsigned {
 			$( $module:ident )*
 		}
 	) => {
+		#[allow(deprecated)] // Allow ValidateUnsigned
 		impl $crate::unsigned::ValidateUnsigned for $runtime {
 			type Call = Call;
 
+			fn pre_dispatch(call: &Self::Call) -> Result<(), $crate::unsigned::ApplyError> {
+				#[allow(unreachable_patterns)]
+				match call {
+					$( Call::$module(inner_call) => $module::pre_dispatch(inner_call), )*
+					// pre-dispatch should not stop inherent extrinsics, validation should prevent
+					// including arbitrary (non-inherent) extrinsics to blocks.
+					_ => Ok(()),
+				}
+			}
+
 			fn validate_unsigned(call: &Self::Call) -> $crate::unsigned::TransactionValidity {
 				#[allow(unreachable_patterns)]
 				match call {
@@ -97,6 +109,7 @@ mod test_partial_and_full_call {
 	pub mod timestamp {
 		pub struct Module;
 
+		#[allow(deprecated)] // Allow ValidateUnsigned
 		impl super::super::ValidateUnsigned for Module {
 			type Call = Call;