From eb882f30e66300ce1e0b3df40e1a93bded9ba9c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 8 Feb 2023 14:47:40 +0100 Subject: [PATCH 1/2] 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. --- frame/scheduler/src/lib.rs | 23 ++++++++++++++++-- frame/scheduler/src/tests.rs | 25 ++++++++++++------- frame/support/src/traits/preimages.rs | 35 ++++++++++++++++++--------- 3 files changed, 61 insertions(+), 22 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..dab318ab412b4 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,9 @@ 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())); + 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,19 +491,23 @@ 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. run_to_block(100); assert_eq!(logger::log().len(), 1); - - // The preimage is not requested anymore. - assert!(!Preimage::is_requested(&hash)); }); } @@ -1129,7 +1135,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; From 7b6d9ee10b7fb40d852333be068f6112c4adbb95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Feb 2023 09:55:26 +0100 Subject: [PATCH 2/2] Review changes --- frame/scheduler/src/tests.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frame/scheduler/src/tests.rs b/frame/scheduler/src/tests.rs index dab318ab412b4..a261c6718616d 100644 --- a/frame/scheduler/src/tests.rs +++ b/frame/scheduler/src/tests.rs @@ -480,6 +480,7 @@ 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; + // 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)); @@ -508,6 +509,9 @@ fn scheduler_handles_periodic_unavailable_preimage() { // Does not ever execute again. run_to_block(100); assert_eq!(logger::log().len(), 1); + + // The preimage is not requested anymore. + assert!(!Preimage::is_requested(&hash)); }); }