From 95b8fd61c3e6d0e30362897bd206f4556ec5734b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 29 Oct 2021 18:00:49 +0200 Subject: [PATCH] pallet-scheduler: Introduce `OriginPrivilegeCmp` (#10078) * pallet-scheduler: Introduce `OriginPrivilegeCmp` When a scheduled task should be canceled, the origin that tries to cancel the task is compared to the origin the task should be executed with. Before this pr this check only allowed that both origins are equal. However, this is problematic as this means that for example a council origin it needs to be have the same amount of yes votes to cancel the scheduled task. While a council origin with more yes votes should be able to cancel this task. This happened recently on Kusama and lead to a failed cancelation of a scheduled task. With this pr the two origins are compared and the cancelling origin needs to have greater or equal privileges as the origin that scheduled the task. What a greater, equal or less privilege is, can be configured in the runtime. For simplicity, a `EqualPrivilegeOnly` implementation is provided that only checks if two origins are equal. So, this mimics the old behaviour. * FMT * fix import * Small optimizations Co-authored-by: Shawn Tabrizi --- substrate/bin/node/runtime/src/lib.rs | 5 +++-- substrate/frame/democracy/src/tests.rs | 3 ++- substrate/frame/scheduler/src/lib.rs | 26 +++++++++++++++++----- substrate/frame/support/src/traits.rs | 8 +++---- substrate/frame/support/src/traits/misc.rs | 22 +++++++++++++++++- 5 files changed, 51 insertions(+), 13 deletions(-) diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index c7920629bf..0638e62faa 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -26,8 +26,8 @@ use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ construct_runtime, parameter_types, traits::{ - Currency, Everything, Imbalance, InstanceFilter, KeyOwnerProofSystem, LockIdentifier, - Nothing, OnUnbalanced, U128CurrencyToVote, + Currency, EqualPrivilegeOnly, Everything, Imbalance, InstanceFilter, KeyOwnerProofSystem, + LockIdentifier, Nothing, OnUnbalanced, U128CurrencyToVote, }, weights::{ constants::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight, WEIGHT_PER_SECOND}, @@ -345,6 +345,7 @@ impl pallet_scheduler::Config for Runtime { type ScheduleOrigin = EnsureRoot; type MaxScheduledPerBlock = MaxScheduledPerBlock; type WeightInfo = pallet_scheduler::weights::SubstrateWeight; + type OriginPrivilegeCmp = EqualPrivilegeOnly; } parameter_types! { diff --git a/substrate/frame/democracy/src/tests.rs b/substrate/frame/democracy/src/tests.rs index f56667e909..06c4ac666c 100644 --- a/substrate/frame/democracy/src/tests.rs +++ b/substrate/frame/democracy/src/tests.rs @@ -22,7 +22,7 @@ use crate as pallet_democracy; use codec::Encode; use frame_support::{ assert_noop, assert_ok, ord_parameter_types, parameter_types, - traits::{Contains, GenesisBuild, OnInitialize, SortedMembers}, + traits::{Contains, EqualPrivilegeOnly, GenesisBuild, OnInitialize, SortedMembers}, weights::Weight, }; use frame_system::{EnsureRoot, EnsureSignedBy}; @@ -118,6 +118,7 @@ impl pallet_scheduler::Config for Test { type ScheduleOrigin = EnsureRoot; type MaxScheduledPerBlock = (); type WeightInfo = (); + type OriginPrivilegeCmp = EqualPrivilegeOnly; } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/substrate/frame/scheduler/src/lib.rs b/substrate/frame/scheduler/src/lib.rs index ca9e15812a..d59c42cc85 100644 --- a/substrate/frame/scheduler/src/lib.rs +++ b/substrate/frame/scheduler/src/lib.rs @@ -58,7 +58,7 @@ use frame_support::{ dispatch::{DispatchError, DispatchResult, Dispatchable, Parameter}, traits::{ schedule::{self, DispatchTime}, - EnsureOrigin, Get, IsType, OriginTrait, + EnsureOrigin, Get, IsType, OriginTrait, PrivilegeCmp, }, weights::{GetDispatchInfo, Weight}, }; @@ -69,7 +69,7 @@ use sp_runtime::{ traits::{BadOrigin, One, Saturating, Zero}, RuntimeDebug, }; -use sp_std::{borrow::Borrow, marker::PhantomData, prelude::*}; +use sp_std::{borrow::Borrow, cmp::Ordering, marker::PhantomData, prelude::*}; pub use weights::WeightInfo; /// Just a simple index for naming period tasks. @@ -160,6 +160,15 @@ pub mod pallet { /// Required origin to schedule or cancel calls. type ScheduleOrigin: EnsureOrigin<::Origin>; + /// Compare the privileges of origins. + /// + /// This will be used when canceling a task, to ensure that the origin that tries + /// to cancel has greater or equal privileges as the origin that created the scheduled task. + /// + /// For simplicity the [`EqualPrivilegeOnly`](frame_support::traits::EqualPrivilegeOnly) can + /// be used. This will only check if two given origins are equal. + type OriginPrivilegeCmp: PrivilegeCmp; + /// The maximum number of scheduled calls in the queue for a single block. /// Not strictly enforced, but used for weight estimation. #[pallet::constant] @@ -614,7 +623,10 @@ impl Pallet { Ok(None), |s| -> Result>, DispatchError> { if let (Some(ref o), Some(ref s)) = (origin, s.borrow()) { - if *o != s.origin { + if matches!( + T::OriginPrivilegeCmp::cmp_privilege(o, &s.origin), + Some(Ordering::Less) | None + ) { return Err(BadOrigin.into()) } }; @@ -709,7 +721,10 @@ impl Pallet { Agenda::::try_mutate(when, |agenda| -> DispatchResult { if let Some(s) = agenda.get_mut(i) { if let (Some(ref o), Some(ref s)) = (origin, s.borrow()) { - if *o != s.origin { + if matches!( + T::OriginPrivilegeCmp::cmp_privilege(o, &s.origin), + Some(Ordering::Less) | None + ) { return Err(BadOrigin.into()) } } @@ -832,7 +847,7 @@ mod tests { use crate as scheduler; use frame_support::{ assert_err, assert_noop, assert_ok, ord_parameter_types, parameter_types, - traits::{Contains, OnFinalize, OnInitialize}, + traits::{Contains, EqualPrivilegeOnly, OnFinalize, OnInitialize}, weights::constants::RocksDbWeight, Hashable, }; @@ -980,6 +995,7 @@ mod tests { type ScheduleOrigin = EnsureOneOf, EnsureSignedBy>; type MaxScheduledPerBlock = MaxScheduledPerBlock; type WeightInfo = (); + type OriginPrivilegeCmp = EqualPrivilegeOnly; } pub type LoggerCall = logger::Call; diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index 43eadb3a05..bb990e2564 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -50,10 +50,10 @@ pub use filter::{ClearFilterGuard, FilterStack, FilterStackGuard, InstanceFilter mod misc; pub use misc::{ - Backing, ConstU32, EnsureInherentsAreFirst, EstimateCallFee, ExecuteBlock, ExtrinsicCall, Get, - GetBacking, GetDefault, HandleLifetime, IsSubType, IsType, Len, OffchainWorker, - OnKilledAccount, OnNewAccount, SameOrOther, Time, TryDrop, UnixTime, WrapperKeepOpaque, - WrapperOpaque, + Backing, ConstU32, EnsureInherentsAreFirst, EqualPrivilegeOnly, EstimateCallFee, ExecuteBlock, + ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime, IsSubType, IsType, Len, + OffchainWorker, OnKilledAccount, OnNewAccount, PrivilegeCmp, SameOrOther, Time, TryDrop, + UnixTime, WrapperKeepOpaque, WrapperOpaque, }; mod stored_map; diff --git a/substrate/frame/support/src/traits/misc.rs b/substrate/frame/support/src/traits/misc.rs index 6587945604..0a3fb045d6 100644 --- a/substrate/frame/support/src/traits/misc.rs +++ b/substrate/frame/support/src/traits/misc.rs @@ -21,7 +21,7 @@ use crate::dispatch::Parameter; use codec::{CompactLen, Decode, DecodeAll, Encode, EncodeLike, Input, MaxEncodedLen}; use scale_info::{build::Fields, meta_type, Path, Type, TypeInfo, TypeParameter}; use sp_runtime::{traits::Block as BlockT, DispatchError}; -use sp_std::prelude::*; +use sp_std::{cmp::Ordering, prelude::*}; /// Anything that can have a `::len()` method. pub trait Len { @@ -289,6 +289,26 @@ pub trait ExecuteBlock { fn execute_block(block: Block); } +/// Something that can compare privileges of two origins. +pub trait PrivilegeCmp { + /// Compare the `left` to the `right` origin. + /// + /// The returned ordering should be from the pov of the `left` origin. + /// + /// Should return `None` when it can not compare the given origins. + fn cmp_privilege(left: &Origin, right: &Origin) -> Option; +} + +/// Implementation of [`PrivilegeCmp`] that only checks for equal origins. +/// +/// This means it will either return [`Origin::Equal`] or `None`. +pub struct EqualPrivilegeOnly; +impl PrivilegeCmp for EqualPrivilegeOnly { + fn cmp_privilege(left: &Origin, right: &Origin) -> Option { + (left == right).then(|| Ordering::Equal) + } +} + /// Off-chain computation trait. /// /// Implementing this trait on a module allows you to perform long-running tasks