Skip to content

Commit

Permalink
pallet-scheduler: Introduce OriginPrivilegeCmp (paritytech#10078)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
  • Loading branch information
bkchr and shawntabrizi authored Oct 29, 2021
1 parent 69fa67d commit 945377f
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 13 deletions.
5 changes: 3 additions & 2 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -345,6 +345,7 @@ impl pallet_scheduler::Config for Runtime {
type ScheduleOrigin = EnsureRoot<AccountId>;
type MaxScheduledPerBlock = MaxScheduledPerBlock;
type WeightInfo = pallet_scheduler::weights::SubstrateWeight<Runtime>;
type OriginPrivilegeCmp = EqualPrivilegeOnly;
}

parameter_types! {
Expand Down
3 changes: 2 additions & 1 deletion frame/democracy/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -118,6 +118,7 @@ impl pallet_scheduler::Config for Test {
type ScheduleOrigin = EnsureRoot<u64>;
type MaxScheduledPerBlock = ();
type WeightInfo = ();
type OriginPrivilegeCmp = EqualPrivilegeOnly;
}
parameter_types! {
pub const ExistentialDeposit: u64 = 1;
Expand Down
26 changes: 21 additions & 5 deletions frame/scheduler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
};
Expand All @@ -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.
Expand Down Expand Up @@ -160,6 +160,15 @@ pub mod pallet {
/// Required origin to schedule or cancel calls.
type ScheduleOrigin: EnsureOrigin<<Self as system::Config>::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<Self::PalletsOrigin>;

/// The maximum number of scheduled calls in the queue for a single block.
/// Not strictly enforced, but used for weight estimation.
#[pallet::constant]
Expand Down Expand Up @@ -614,7 +623,10 @@ impl<T: Config> Pallet<T> {
Ok(None),
|s| -> Result<Option<Scheduled<_, _, _, _>>, 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())
}
};
Expand Down Expand Up @@ -709,7 +721,10 @@ impl<T: Config> Pallet<T> {
Agenda::<T>::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())
}
}
Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -980,6 +995,7 @@ mod tests {
type ScheduleOrigin = EnsureOneOf<u64, EnsureRoot<u64>, EnsureSignedBy<One, u64>>;
type MaxScheduledPerBlock = MaxScheduledPerBlock;
type WeightInfo = ();
type OriginPrivilegeCmp = EqualPrivilegeOnly;
}

pub type LoggerCall = logger::Call<Test>;
Expand Down
8 changes: 4 additions & 4 deletions frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
22 changes: 21 additions & 1 deletion frame/support/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -289,6 +289,26 @@ pub trait ExecuteBlock<Block: BlockT> {
fn execute_block(block: Block);
}

/// Something that can compare privileges of two origins.
pub trait PrivilegeCmp<Origin> {
/// 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<Ordering>;
}

/// Implementation of [`PrivilegeCmp`] that only checks for equal origins.
///
/// This means it will either return [`Origin::Equal`] or `None`.
pub struct EqualPrivilegeOnly;
impl<Origin: PartialEq> PrivilegeCmp<Origin> for EqualPrivilegeOnly {
fn cmp_privilege(left: &Origin, right: &Origin) -> Option<Ordering> {
(left == right).then(|| Ordering::Equal)
}
}

/// Off-chain computation trait.
///
/// Implementing this trait on a module allows you to perform long-running tasks
Expand Down

0 comments on commit 945377f

Please sign in to comment.