From 8511c9b03c63eb0b658675a0f22794a2ffa11f2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Feb 2023 11:00:55 +0100 Subject: [PATCH] pallet-scheduler: Ensure we request a preimage (#13340) * pallet-scheduler: Ensure we request a preimage The scheduler was not requesting a preimage. When a preimage is requested, a user can deposit it without paying any fees. * Review changes --- frame/scheduler/src/lib.rs | 23 ++++++++++++++++-- frame/scheduler/src/tests.rs | 23 +++++++++++++----- frame/support/src/traits/preimages.rs | 35 ++++++++++++++++++--------- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index afc4fd66e76cb..dfec98d4e443a 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -777,6 +777,8 @@ impl Pallet { ) -> Result, DispatchError> { let when = Self::resolve_time(when)?; + let lookup_hash = call.lookup_hash(); + // sanitize maybe_periodic let maybe_periodic = maybe_periodic .filter(|p| p.1 > 1 && !p.0.is_zero()) @@ -790,7 +792,14 @@ impl Pallet { origin, _phantom: PhantomData, }; - Self::place_task(when, task).map_err(|x| x.0) + let res = Self::place_task(when, task).map_err(|x| x.0)?; + + if let Some(hash) = lookup_hash { + // Request the call to be made available. + T::Preimages::request(&hash); + } + + Ok(res) } fn do_cancel( @@ -862,6 +871,8 @@ impl Pallet { let when = Self::resolve_time(when)?; + let lookup_hash = call.lookup_hash(); + // sanitize maybe_periodic let maybe_periodic = maybe_periodic .filter(|p| p.1 > 1 && !p.0.is_zero()) @@ -876,7 +887,14 @@ impl Pallet { origin, _phantom: Default::default(), }; - Self::place_task(when, task).map_err(|x| x.0) + let res = Self::place_task(when, task).map_err(|x| x.0)?; + + if let Some(hash) = lookup_hash { + // Request the call to be made available. + T::Preimages::request(&hash); + } + + Ok(res) } fn do_cancel_named(origin: Option, id: TaskName) -> DispatchResult { @@ -1027,6 +1045,7 @@ impl Pallet { } else { Agenda::::remove(when); } + postponed == 0 } diff --git a/frame/scheduler/src/tests.rs b/frame/scheduler/src/tests.rs index e5467fc8db55f..a261c6718616d 100644 --- a/frame/scheduler/src/tests.rs +++ b/frame/scheduler/src/tests.rs @@ -58,9 +58,10 @@ fn scheduling_with_preimages_works() { RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); let hash = ::Hashing::hash_of(&call); let len = call.using_encoded(|x| x.len()) as u32; - let hashed = Preimage::pick(hash, len); - assert_ok!(Preimage::note_preimage(RuntimeOrigin::signed(0), call.encode())); + // Important to use here `Bounded::Lookup` to ensure that we request the hash. + let hashed = Bounded::Lookup { hash, len }; assert_ok!(Scheduler::do_schedule(DispatchTime::At(4), None, 127, root(), hashed)); + assert_ok!(Preimage::note_preimage(RuntimeOrigin::signed(0), call.encode())); assert!(Preimage::is_requested(&hash)); run_to_block(3); assert!(logger::log().is_empty()); @@ -479,8 +480,10 @@ fn scheduler_handles_periodic_unavailable_preimage() { let call = RuntimeCall::Logger(LoggerCall::log { i: 42, weight: (max_weight / 3) * 2 }); let hash = ::Hashing::hash_of(&call); let len = call.using_encoded(|x| x.len()) as u32; - let bound = Preimage::pick(hash, len); - assert_ok!(Preimage::note(call.encode().into())); + // Important to use here `Bounded::Lookup` to ensure that we request the hash. + let bound = Bounded::Lookup { hash, len }; + // The preimage isn't requested yet. + assert!(!Preimage::is_requested(&hash)); assert_ok!(Scheduler::do_schedule( DispatchTime::At(4), @@ -489,11 +492,18 @@ fn scheduler_handles_periodic_unavailable_preimage() { root(), bound.clone(), )); + + // The preimage is requested. + assert!(Preimage::is_requested(&hash)); + + // Note the preimage. + assert_ok!(Preimage::note_preimage(RuntimeOrigin::signed(1), call.encode())); + // Executes 1 times till block 4. run_to_block(4); assert_eq!(logger::log().len(), 1); - // Unnote the preimage. + // Unnote the preimage Preimage::unnote(&hash); // Does not ever execute again. @@ -1129,7 +1139,8 @@ fn postponed_named_task_cannot_be_rescheduled() { RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(1000) }); let hash = ::Hashing::hash_of(&call); let len = call.using_encoded(|x| x.len()) as u32; - let hashed = Preimage::pick(hash, len); + // Important to use here `Bounded::Lookup` to ensure that we request the hash. + let hashed = Bounded::Lookup { hash, len }; let name: [u8; 32] = hash.as_ref().try_into().unwrap(); let address = Scheduler::do_schedule_named( diff --git a/frame/support/src/traits/preimages.rs b/frame/support/src/traits/preimages.rs index 594532ba96903..ce3537c792c08 100644 --- a/frame/support/src/traits/preimages.rs +++ b/frame/support/src/traits/preimages.rs @@ -26,6 +26,9 @@ use sp_std::borrow::Cow; pub type Hash = H256; pub type BoundedInline = crate::BoundedVec>; +/// The maximum we expect a single legacy hash lookup to be. +const MAX_LEGACY_LEN: u32 = 1_000_000; + #[derive( Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, scale_info::TypeInfo, RuntimeDebug, )] @@ -67,20 +70,25 @@ impl Bounded { /// Returns the hash of the preimage. /// /// The hash is re-calculated every time if the preimage is inlined. - pub fn hash(&self) -> H256 { + pub fn hash(&self) -> Hash { use Bounded::*; match self { - Legacy { hash, .. } => *hash, + Lookup { hash, .. } | Legacy { hash, .. } => *hash, Inline(x) => blake2_256(x.as_ref()).into(), - Lookup { hash, .. } => *hash, } } -} -// The maximum we expect a single legacy hash lookup to be. -const MAX_LEGACY_LEN: u32 = 1_000_000; + /// Returns the hash to lookup the preimage. + /// + /// If this is a `Bounded::Inline`, `None` is returned as no lookup is required. + pub fn lookup_hash(&self) -> Option { + use Bounded::*; + match self { + Lookup { hash, .. } | Legacy { hash, .. } => Some(*hash), + Inline(_) => None, + } + } -impl Bounded { /// Returns the length of the preimage or `None` if the length is unknown. pub fn len(&self) -> Option { match self { @@ -168,8 +176,11 @@ pub trait QueryPreimage { } } - /// Create a `Bounded` instance based on the `hash` and `len` of the encoded value. This may not - /// be `peek`-able or `realize`-able. + /// Create a `Bounded` instance based on the `hash` and `len` of the encoded value. + /// + /// It also directly requests the given `hash` using [`Self::request`]. + /// + /// This may not be `peek`-able or `realize`-able. fn pick(hash: Hash, len: u32) -> Bounded { Self::request(&hash); Bounded::Lookup { hash, len } @@ -228,10 +239,12 @@ pub trait StorePreimage: QueryPreimage { Self::unrequest(hash) } - /// Convert an otherwise unbounded or large value into a type ready for placing in storage. The - /// result is a type whose `MaxEncodedLen` is 131 bytes. + /// Convert an otherwise unbounded or large value into a type ready for placing in storage. + /// + /// The result is a type whose `MaxEncodedLen` is 131 bytes. /// /// NOTE: Once this API is used, you should use either `drop` or `realize`. + /// The value is also noted using [`Self::note`]. fn bound(t: T) -> Result, DispatchError> { let data = t.encode(); let len = data.len() as u32;