From a93caaca537d8b89946d112da279e2bdf332f748 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 21 Nov 2022 21:09:50 +1300 Subject: [PATCH 1/6] prevent nested DelayedOrigin --- authority/src/lib.rs | 86 +++++++++++++++++++++++++++++++++++++++++- authority/src/tests.rs | 21 +++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/authority/src/lib.rs b/authority/src/lib.rs index cc49a9074..82c9e8054 100644 --- a/authority/src/lib.rs +++ b/authority/src/lib.rs @@ -32,6 +32,7 @@ use frame_support::{ }; use frame_system::{pallet_prelude::*, EnsureRoot, EnsureSigned}; use scale_info::TypeInfo; +use sp_core::defer; use sp_runtime::{ traits::{CheckedSub, Dispatchable, Hash, Saturating}, ArithmeticError, DispatchError, DispatchResult, Either, RuntimeDebug, @@ -45,7 +46,7 @@ mod weights; pub use weights::WeightInfo; /// A delayed origin. Can only be dispatched via `dispatch_as` with a delay. -#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen)] +#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, TypeInfo)] pub struct DelayedOrigin { /// Number of blocks that this call have been delayed. pub delay: BlockNumber, @@ -53,6 +54,89 @@ pub struct DelayedOrigin { pub origin: Box, } +#[cfg(feature = "std")] +mod helper { + use std::cell::RefCell; + + thread_local! { + static NESTED_DECODE: RefCell = RefCell::new(false); + static NESTED_MAX_ENCODED_LEN: RefCell = RefCell::new(false); + } + + pub fn set_nested_decode(val: bool) { + NESTED_DECODE.with(|v| *v.borrow_mut() = val); + } + + pub fn nested_decode() -> bool { + NESTED_DECODE.with(|v| *v.borrow()) + } + + pub fn set_nested_max_encoded_len(val: bool) { + NESTED_MAX_ENCODED_LEN.with(|v| *v.borrow_mut() = val); + } + + pub fn nested_max_encoded_len() -> bool { + NESTED_MAX_ENCODED_LEN.with(|v| *v.borrow()) + } +} + +#[cfg(not(feature = "std"))] +mod helper { + static mut NESTED_DECODE: bool = false; + static mut NESTED_MAX_ENCODED_LEN: bool = false; + + pub fn set_nested_decode(val: bool) { + unsafe { + NESTED_DECODE = val; + } + } + + pub fn nested_decode() -> bool { + unsafe { NESTED_DECODE } + } + + pub fn set_nested_max_encoded_len(val: bool) { + unsafe { + NESTED_MAX_ENCODED_LEN = val; + } + } + + pub fn nested_max_encoded_len() -> bool { + unsafe { NESTED_MAX_ENCODED_LEN } + } +} + +impl Decode for DelayedOrigin { + fn decode(input: &mut I) -> Result { + if helper::nested_decode() { + return Err("Nested DelayedOrigin::decode is not allowed".into()); + } + + helper::set_nested_decode(true); + defer!(helper::set_nested_decode(false)); + + Ok(DelayedOrigin { + delay: Decode::decode(input)?, + origin: Decode::decode(input)?, + }) + } +} + +impl MaxEncodedLen + for DelayedOrigin +{ + fn max_encoded_len() -> usize { + if helper::nested_max_encoded_len() { + return 0; + } + + helper::set_nested_max_encoded_len(true); + defer!(helper::set_nested_max_encoded_len(false)); + + BlockNumber::max_encoded_len() + PalletsOrigin::max_encoded_len() + } +} + /// Ensure the origin have a minimum amount of delay. pub struct EnsureDelayed( sp_std::marker::PhantomData<(Delay, Inner, BlockNumber, PalletsOrigin)>, diff --git a/authority/src/tests.rs b/authority/src/tests.rs index f44c2a840..222bb1d93 100644 --- a/authority/src/tests.rs +++ b/authority/src/tests.rs @@ -3,6 +3,7 @@ #![cfg(test)] use super::*; +use codec::MaxEncodedLen; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchErrorWithPostInfo, @@ -691,3 +692,23 @@ fn trigger_old_call_should_be_free_and_operational() { ); }); } + +#[test] +fn origin_max_encoded_len_works() { + assert_eq!(DelayedOrigin::::max_encoded_len(), 22); + assert_eq!(OriginCaller::max_encoded_len(), 27); +} + +#[test] +fn nested_delayed_origin_is_invalid() { + let orgin = DelayedOrigin:: { + delay: 1u64, + origin: Box::new(OriginCaller::Authority(DelayedOrigin:: { + delay: 1u64, + origin: Box::new(OriginCaller::system(frame_system::RawOrigin::Root)), + })), + }; + + let encoded = orgin.encode(); + assert!(OriginCaller::decode(&mut &encoded[..]).is_err()); +} From 33e7003878363d0c8b5b0624027e10670a5194a2 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 21 Nov 2022 21:27:51 +1300 Subject: [PATCH 2/6] fix deps --- authority/Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/Cargo.toml b/authority/Cargo.toml index e0468a06b..03535affb 100644 --- a/authority/Cargo.toml +++ b/authority/Cargo.toml @@ -12,15 +12,15 @@ scale-info = { version = "2.1.2", default-features = false, features = ["derive" serde = { version = "1.0.145", optional = true } codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false } -sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } +sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } sp-io = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } +sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } sp-std = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } frame-support = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } frame-system = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" } [dev-dependencies] sp-io = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" } -sp-core = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" } pallet-scheduler = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" } pallet-preimage = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" } From 6b8f5a4b5817f66e825b6be523259da6b3992aa8 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Tue, 22 Nov 2022 00:05:07 +1300 Subject: [PATCH 3/6] Update authority/src/tests.rs Co-authored-by: zjb0807 --- authority/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/src/tests.rs b/authority/src/tests.rs index 222bb1d93..6e7ae220d 100644 --- a/authority/src/tests.rs +++ b/authority/src/tests.rs @@ -710,5 +710,5 @@ fn nested_delayed_origin_is_invalid() { }; let encoded = orgin.encode(); - assert!(OriginCaller::decode(&mut &encoded[..]).is_err()); + assert!(DelayedOrigin::::decode(&mut &encoded[..]).is_err()); } From fe56f90fa8b7a8d28ca693342a33748fb1f0d448 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 22 Nov 2022 10:38:54 +1300 Subject: [PATCH 4/6] allow nested DelayedOrigin --- authority/src/lib.rs | 47 +++++++----------------------------------- authority/src/tests.rs | 14 ------------- 2 files changed, 8 insertions(+), 53 deletions(-) diff --git a/authority/src/lib.rs b/authority/src/lib.rs index 82c9e8054..030f41e2b 100644 --- a/authority/src/lib.rs +++ b/authority/src/lib.rs @@ -46,12 +46,12 @@ mod weights; pub use weights::WeightInfo; /// A delayed origin. Can only be dispatched via `dispatch_as` with a delay. -#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, TypeInfo)] +#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode, TypeInfo)] pub struct DelayedOrigin { /// Number of blocks that this call have been delayed. - pub delay: BlockNumber, + pub(crate) delay: BlockNumber, /// The initial origin. - pub origin: Box, + pub(crate) origin: Box, } #[cfg(feature = "std")] @@ -59,18 +59,9 @@ mod helper { use std::cell::RefCell; thread_local! { - static NESTED_DECODE: RefCell = RefCell::new(false); static NESTED_MAX_ENCODED_LEN: RefCell = RefCell::new(false); } - pub fn set_nested_decode(val: bool) { - NESTED_DECODE.with(|v| *v.borrow_mut() = val); - } - - pub fn nested_decode() -> bool { - NESTED_DECODE.with(|v| *v.borrow()) - } - pub fn set_nested_max_encoded_len(val: bool) { NESTED_MAX_ENCODED_LEN.with(|v| *v.borrow_mut() = val); } @@ -82,19 +73,8 @@ mod helper { #[cfg(not(feature = "std"))] mod helper { - static mut NESTED_DECODE: bool = false; static mut NESTED_MAX_ENCODED_LEN: bool = false; - pub fn set_nested_decode(val: bool) { - unsafe { - NESTED_DECODE = val; - } - } - - pub fn nested_decode() -> bool { - unsafe { NESTED_DECODE } - } - pub fn set_nested_max_encoded_len(val: bool) { unsafe { NESTED_MAX_ENCODED_LEN = val; @@ -106,22 +86,11 @@ mod helper { } } -impl Decode for DelayedOrigin { - fn decode(input: &mut I) -> Result { - if helper::nested_decode() { - return Err("Nested DelayedOrigin::decode is not allowed".into()); - } - - helper::set_nested_decode(true); - defer!(helper::set_nested_decode(false)); - - Ok(DelayedOrigin { - delay: Decode::decode(input)?, - origin: Decode::decode(input)?, - }) - } -} - +// Manual implementation to break recursive calls of `MaxEncodedLen` as the +// implementation of `PalletsOrigin::max_encoded_len` will also call +// `MaxEncodedLen` on `DelayedOrigin`. This is only safe if there are no nested +// `DelayedOrigin`. It is only possible to construct a `DelayedOrigin` via +// `schedule_dispatch` which is a protected call only accessible via governance. impl MaxEncodedLen for DelayedOrigin { diff --git a/authority/src/tests.rs b/authority/src/tests.rs index 6e7ae220d..c4bd784d6 100644 --- a/authority/src/tests.rs +++ b/authority/src/tests.rs @@ -698,17 +698,3 @@ fn origin_max_encoded_len_works() { assert_eq!(DelayedOrigin::::max_encoded_len(), 22); assert_eq!(OriginCaller::max_encoded_len(), 27); } - -#[test] -fn nested_delayed_origin_is_invalid() { - let orgin = DelayedOrigin:: { - delay: 1u64, - origin: Box::new(OriginCaller::Authority(DelayedOrigin:: { - delay: 1u64, - origin: Box::new(OriginCaller::system(frame_system::RawOrigin::Root)), - })), - }; - - let encoded = orgin.encode(); - assert!(DelayedOrigin::::decode(&mut &encoded[..]).is_err()); -} From 22629221aca52e56f0ad42d6ed80f4f6bfced255 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 22 Nov 2022 12:03:17 +1300 Subject: [PATCH 5/6] update docstring --- authority/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/authority/src/lib.rs b/authority/src/lib.rs index 030f41e2b..e5e993375 100644 --- a/authority/src/lib.rs +++ b/authority/src/lib.rs @@ -11,6 +11,13 @@ //! Two functionalities are provided by this module: //! - schedule a dispatchable //! - dispatch method with on behalf of other origins +//! +//! NOTE: +//! +//! In order to derive a feasible max encoded len for `DelayedOrigin`, it is +//! assumed that there are no nested `DelayedOrigin` in `OriginCaller`. +//! In practice, this means there should not be nested `schedule_dispatch`. +//! Otherwise the proof size estimation may not be accurate. #![cfg_attr(not(feature = "std"), no_std)] // Disable the following three lints since they originate from an external macro From 1141aefbf18e7747295704131a2c4b1d9b387fba Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 22 Nov 2022 12:04:44 +1300 Subject: [PATCH 6/6] typo fix --- authority/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/authority/src/lib.rs b/authority/src/lib.rs index e5e993375..93a30dac6 100644 --- a/authority/src/lib.rs +++ b/authority/src/lib.rs @@ -159,10 +159,10 @@ pub trait AuthorityConfig { new_delay: BlockNumber, ) -> DispatchResult; /// Check if the `origin` is allow to delay a scheduled task that - /// initially created by `inital_origin`. + /// initially created by `initial_origin`. fn check_delay_schedule(origin: Origin, initial_origin: &PalletsOrigin) -> DispatchResult; /// Check if the `origin` is allow to cancel a scheduled task that - /// initially created by `inital_origin`. + /// initially created by `initial_origin`. fn check_cancel_schedule(origin: Origin, initial_origin: &PalletsOrigin) -> DispatchResult; } @@ -456,12 +456,12 @@ pub mod module { #[pallet::weight(T::WeightInfo::remove_authorized_call())] pub fn remove_authorized_call(origin: OriginFor, hash: T::Hash) -> DispatchResult { - let root_or_sigend = + let root_or_signed = EitherOfDiverse::, EnsureSigned>::ensure_origin(origin)?; SavedCalls::::try_mutate_exists(hash, |maybe_call| { let (_, maybe_caller) = maybe_call.take().ok_or(Error::::CallNotAuthorized)?; - match root_or_sigend { + match root_or_signed { Either::Left(_) => {} // root, do nothing Either::Right(who) => { // signed, ensure it's the caller