From ba9906addb3faeff94fbef3fe249114c3715b080 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 09:26:59 +0100 Subject: [PATCH 1/4] Add pre-dispatch checks for ValidateUnsigned --- .../src/generic/checked_extrinsic.rs | 18 ++++++++--- core/sr-primitives/src/testing.rs | 14 +++++++-- core/sr-primitives/src/traits.rs | 30 +++++++++++++++---- srml/executive/src/lib.rs | 10 +++++-- srml/im-online/src/lib.rs | 13 ++++---- srml/support/src/unsigned.rs | 16 ++++++++++ 6 files changed, 79 insertions(+), 22 deletions(-) diff --git a/core/sr-primitives/src/generic/checked_extrinsic.rs b/core/sr-primitives/src/generic/checked_extrinsic.rs index 1e030ea1d8769..158d327b23b8c 100644 --- a/core/sr-primitives/src/generic/checked_extrinsic.rs +++ b/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; @@ -52,7 +54,10 @@ where self.signed.as_ref().map(|x| &x.0) } - fn validate>( + fn validate< + #[allow(deprecated)] + U: ValidateUnsigned + >( &self, info: DispatchInfo, len: usize, @@ -61,11 +66,15 @@ 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( + fn apply< + #[allow(deprecated)] + U: ValidateUnsigned + >( self, info: DispatchInfo, len: usize, @@ -75,6 +84,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)); diff --git a/core/sr-primitives/src/testing.rs b/core/sr-primitives/src/testing.rs index d60e58bab1a5c..e8f390002a0fb 100644 --- a/core/sr-primitives/src/testing.rs +++ b/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,7 +325,10 @@ impl Applyable for TestXt 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. - fn validate>( + fn validate< + #[allow(deprecated)] + U: ValidateUnsigned + >( &self, _info: DispatchInfo, _len: usize, @@ -333,7 +338,10 @@ impl Applyable for TestXt where /// Executes all necessary logic needed prior to dispatch and deconstructs into function call, /// index and sender. - fn apply( + fn apply< + #[allow(deprecated)] + U: ValidateUnsigned + >( self, info: DispatchInfo, len: usize, diff --git a/core/sr-primitives/src/traits.rs b/core/sr-primitives/src/traits.rs index ecaf6a78fcfe4..742b9572932a2 100644 --- a/core/sr-primitives/src/traits.rs +++ b/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,7 +886,10 @@ 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. - fn validate>( + fn validate< + #[allow(deprecated)] + V: ValidateUnsigned + >( &self, info: DispatchInfo, len: usize, @@ -897,7 +897,10 @@ pub trait Applyable: Sized + Send + Sync { /// Executes all necessary logic needed prior to dispatch and deconstructs into function call, /// index and sender. - fn apply( + fn apply< + #[allow(deprecated)] + V: ValidateUnsigned + >( self, info: DispatchInfo, len: usize, @@ -966,10 +969,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 diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index 927a7fef25581..304f2fe9c385f 100644 --- a/srml/executive/src/lib.rs +++ b/srml/executive/src/lib.rs @@ -79,10 +79,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 +102,8 @@ pub struct Executive( PhantomData<(System, Block, Context, UnsignedValidator, AllModules)> ); +// We allow ValidateUnsigned deprecation. Make sure to remove this when we remove `ValidateUnsigned`. +#[allow(deprecated)] impl< System: system::Trait, Block: traits::Block, @@ -119,6 +123,8 @@ where } } +// We allow ValidateUnsigned deprecation. Make sure to remove this when we remove `ValidateUnsigned`. +#[allow(deprecated)] impl< System: system::Trait, Block: traits::Block, @@ -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::(xt, dispatch_info, encoded_len)?; >::note_applied_extrinsic(&r, encoded_len as u32); diff --git a/srml/im-online/src/lib.rs b/srml/im-online/src/lib.rs index 6dcaf1d3abb59..1cdfd4ceabe87 100644 --- a/srml/im-online/src/lib.rs +++ b/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; @@ -233,12 +233,13 @@ decl_module! { fn heartbeat( origin, heartbeat: Heartbeat, - signature: ::Signature + // since signature verification is done in `validate_unsigned` + // we can skip doing it here again. + _signature: ::Signature ) { ensure_none(origin)?; let current_session = >::current_index(); - ensure!(current_session == heartbeat.session_index, "Outdated heartbeat received."); let exists = ::exists( ¤t_session, &heartbeat.authority_index @@ -246,11 +247,6 @@ decl_module! { let keys = Keys::::get(); let public = keys.get(heartbeat.authority_index as usize); if let (true, 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::::HeartbeatReceived(public.clone())); let network_state = heartbeat.network_state.encode(); @@ -500,6 +496,7 @@ impl session::OneSessionHandler for Module { } } +#[allow(deprecated)] impl support::unsigned::ValidateUnsigned for Module { type Call = Call; diff --git a/srml/support/src/unsigned.rs b/srml/support/src/unsigned.rs index 4d2ceddd79f4a..daa8a92cd77dc 100644 --- a/srml/support/src/unsigned.rs +++ b/srml/support/src/unsigned.rs @@ -15,6 +15,7 @@ // along with Substrate. If not, see . #[doc(hidden)] +#[allow(deprecated)] pub use crate::sr_primitives::traits::ValidateUnsigned; #[doc(hidden)] pub use crate::sr_primitives::transaction_validity::{TransactionValidity, UnknownTransaction}; @@ -34,6 +35,10 @@ pub use crate::sr_primitives::ApplyError; /// # impl srml_support::unsigned::ValidateUnsigned for Module { /// # type Call = Call; /// # +/// # fn pre_dispatch(call: &Self::Call) -> Result<(), srml_support::unsigned::ApplyError> { +/// # unimplemented!(); +/// # } +/// # /// # fn validate_unsigned(call: &Self::Call) -> srml_support::unsigned::TransactionValidity { /// # unimplemented!(); /// # } @@ -65,9 +70,20 @@ macro_rules! impl_outer_validate_unsigned { $( $module:ident )* } ) => { + #[allow(deprecated)] 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 { From 6246374245d37d64edc4ac0fe2f73538424df3ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 09:41:54 +0100 Subject: [PATCH 2/4] Deprecate ValidateUnsigned. --- core/sr-primitives/src/generic/checked_extrinsic.rs | 12 ++++-------- core/sr-primitives/src/testing.rs | 12 ++++-------- core/sr-primitives/src/traits.rs | 12 ++++-------- srml/executive/src/lib.rs | 8 ++++---- srml/im-online/src/tests.rs | 9 +++++++-- srml/support/src/unsigned.rs | 4 +++- 6 files changed, 26 insertions(+), 31 deletions(-) diff --git a/core/sr-primitives/src/generic/checked_extrinsic.rs b/core/sr-primitives/src/generic/checked_extrinsic.rs index 158d327b23b8c..90c25ebefaec5 100644 --- a/core/sr-primitives/src/generic/checked_extrinsic.rs +++ b/core/sr-primitives/src/generic/checked_extrinsic.rs @@ -54,10 +54,8 @@ where self.signed.as_ref().map(|x| &x.0) } - fn validate< - #[allow(deprecated)] - U: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn validate>( &self, info: DispatchInfo, len: usize, @@ -71,10 +69,8 @@ where } } - fn apply< - #[allow(deprecated)] - U: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn apply>( self, info: DispatchInfo, len: usize, diff --git a/core/sr-primitives/src/testing.rs b/core/sr-primitives/src/testing.rs index e8f390002a0fb..b1bd94a5461b1 100644 --- a/core/sr-primitives/src/testing.rs +++ b/core/sr-primitives/src/testing.rs @@ -325,10 +325,8 @@ impl Applyable for TestXt 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. - fn validate< - #[allow(deprecated)] - U: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn validate>( &self, _info: DispatchInfo, _len: usize, @@ -338,10 +336,8 @@ impl Applyable for TestXt where /// Executes all necessary logic needed prior to dispatch and deconstructs into function call, /// index and sender. - fn apply< - #[allow(deprecated)] - U: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn apply>( self, info: DispatchInfo, len: usize, diff --git a/core/sr-primitives/src/traits.rs b/core/sr-primitives/src/traits.rs index 742b9572932a2..195fca26fad19 100644 --- a/core/sr-primitives/src/traits.rs +++ b/core/sr-primitives/src/traits.rs @@ -886,10 +886,8 @@ 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. - fn validate< - #[allow(deprecated)] - V: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn validate>( &self, info: DispatchInfo, len: usize, @@ -897,10 +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)] - V: ValidateUnsigned - >( + #[allow(deprecated)] // Allow ValidateUnsigned + fn apply>( self, info: DispatchInfo, len: usize, diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index 304f2fe9c385f..d01ea54c2f92f 100644 --- a/srml/executive/src/lib.rs +++ b/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 = (); //! # @@ -102,8 +104,7 @@ pub struct Executive( PhantomData<(System, Block, Context, UnsignedValidator, AllModules)> ); -// We allow ValidateUnsigned deprecation. Make sure to remove this when we remove `ValidateUnsigned`. -#[allow(deprecated)] +#[allow(deprecated)] // Allow ValidateUnsigned, remove the attribute when the trait is removed. impl< System: system::Trait, Block: traits::Block, @@ -123,8 +124,7 @@ where } } -// We allow ValidateUnsigned deprecation. Make sure to remove this when we remove `ValidateUnsigned`. -#[allow(deprecated)] +#[allow(deprecated)] // Allow ValidateUnsigned, remove the attribute when the trait is removed. impl< System: system::Trait, Block: traits::Block, diff --git a/srml/im-online/src/tests.rs b/srml/im-online/src/tests.rs index 652d751281294..3b6b9cb6dc0fe 100644 --- a/srml/im-online/src/tests.rs +++ b/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, 1, 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()), "Transaction is outdated"); }); } diff --git a/srml/support/src/unsigned.rs b/srml/support/src/unsigned.rs index daa8a92cd77dc..1f27160dc0508 100644 --- a/srml/support/src/unsigned.rs +++ b/srml/support/src/unsigned.rs @@ -32,6 +32,7 @@ pub use crate::sr_primitives::ApplyError; /// # mod timestamp { /// # pub struct Module; /// # +/// # #[allow(deprecated)] /// # impl srml_support::unsigned::ValidateUnsigned for Module { /// # type Call = Call; /// # @@ -70,7 +71,7 @@ macro_rules! impl_outer_validate_unsigned { $( $module:ident )* } ) => { - #[allow(deprecated)] + #[allow(deprecated)] // Allow ValidateUnsigned impl $crate::unsigned::ValidateUnsigned for $runtime { type Call = Call; @@ -113,6 +114,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; From ad09c882a5e39d97efa122d5f79678913ebb0a87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 09:49:09 +0100 Subject: [PATCH 3/4] Bump specversion. --- node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 9bda317c3358b..ec9a99c3f1440 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -81,7 +81,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to equal spec_version. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 190, + spec_version: 191, impl_version: 191, apis: RUNTIME_API_VERSIONS, }; From 1f2f22f2fcfa2990d5e73864383000c0fe77b457 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 31 Oct 2019 10:03:22 +0100 Subject: [PATCH 4/4] Fix test. --- srml/support/src/unsigned.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/srml/support/src/unsigned.rs b/srml/support/src/unsigned.rs index 1f27160dc0508..282f3ac8ae9a8 100644 --- a/srml/support/src/unsigned.rs +++ b/srml/support/src/unsigned.rs @@ -32,14 +32,9 @@ pub use crate::sr_primitives::ApplyError; /// # mod timestamp { /// # pub struct Module; /// # -/// # #[allow(deprecated)] /// # impl srml_support::unsigned::ValidateUnsigned for Module { /// # type Call = Call; /// # -/// # fn pre_dispatch(call: &Self::Call) -> Result<(), srml_support::unsigned::ApplyError> { -/// # unimplemented!(); -/// # } -/// # /// # fn validate_unsigned(call: &Self::Call) -> srml_support::unsigned::TransactionValidity { /// # unimplemented!(); /// # }