Skip to content

edge cases fix as per review #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
n13 wants to merge 1 commit into from
Closed

edge cases fix as per review #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 41 additions & 35 deletions pallets/scheduler/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,15 @@
//! * Could lead to undefined behavior, such as executing another runtime call with the same
//! index.
//!
//! ## Known Limitation: Stranded tasks when preimage is unavailable
//!
//! If a task's preimage is not available at execution time, the task remains in the `Agenda`
//! and its `Lookup` entry is preserved, allowing cancellation or rescheduling by name. However,
//! the scheduler will not automatically retry the task in future blocks -- it is only
//! re-attempted when the original block's agenda is serviced again (which does not happen
//! once the block has passed). Callers should ensure preimages are available before the
//! scheduled block, or cancel/reschedule stranded tasks explicitly.
//!
//! [`on_initialize`]: frame_support::traits::Hooks::on_initialize

// Ensure we're `no_std` when compiling for Wasm.
Expand Down Expand Up @@ -767,24 +776,28 @@ impl<T: Config> Pallet<T> {
Lookup::<T>::try_mutate_exists(id, |lookup| -> DispatchResult {
if let Some((when, index)) = lookup.take() {
let i = index as usize;
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()) {
Self::ensure_privilege(o, &s.origin)?;
}
if let Some(ref s) = s {
Retries::<T>::remove((when, index));
T::Preimages::drop(&s.call);
}
*s = None;
}
Ok(())
let task = Agenda::<T>::try_mutate(when, |agenda| {
agenda.get_mut(i).map_or(
Ok(None),
|s| -> Result<Option<Scheduled<_, _, _, _, _, _>>, DispatchError> {
if let (Some(ref o), Some(ref s)) = (origin, s.borrow()) {
Self::ensure_privilege(o, &s.origin)?;
}
Ok(s.take())
},
)
})?;
Self::cleanup_agenda(when);
Self::deposit_event(Event::Canceled { when, index });
Ok(())
if let Some(s) = task {
Retries::<T>::remove((when, index));
T::Preimages::drop(&s.call);
Self::cleanup_agenda(when);
Self::deposit_event(Event::Canceled { when, index });
Ok(())
} else {
Err(Error::<T>::NotFound.into())
}
} else {
return Err(Error::<T>::NotFound.into());
Err(Error::<T>::NotFound.into())
}
})
}
Expand Down Expand Up @@ -1023,13 +1036,12 @@ impl<T: Config> Pallet<T> {
postponed == 0
}

/// Service (i.e. execute) the given task, being careful not to overflow the `weight` counter.
///
/// This involves:
/// - removing and potentially replacing the `Lookup` entry for the task.
/// - realizing the task's call which can include a preimage lookup.
/// Execute a single scheduled task. Returns Ok(()) on successful dispatch, or Err with
/// the task back (Some) for retry / postponement, or None if permanently removed.
///
/// Lookup removal is deferred until the outcome is known: tasks that return to the
/// agenda (preimage unavailable, temporarily overweight) keep their Lookup entry so
/// they can still be cancelled or rescheduled by name.
fn service_task(
weight: &mut WeightMeter,
now: BlockNumberOrTimestampOf<T>,
Expand All @@ -1038,33 +1050,21 @@ impl<T: Config> Pallet<T> {
is_first: bool,
task: ScheduledOf<T>,
) -> Result<(), (ServiceTaskError, Option<ScheduledOf<T>>)> {
// Eagerly remove the name->address lookup. If the task succeeds or gets rescheduled
// via a retry, the new address will be re-inserted by place_task. If it fails,
// the name is freed.
if let Some(ref id) = task.maybe_id {
Lookup::<T>::remove(id);
}

// Try to retrieve the actual call data. For inline calls this is a no-op decode.
// For lookup calls this fetches from the preimage store.
let (call, lookup_len) = match T::Preimages::peek(&task.call) {
Ok(c) => c,
Err(_) => {
// Preimage not available (not yet submitted or previously dropped).
// Drop our reference, emit an event, and return the task back to the
// agenda so it can be retried if the preimage appears later.
Self::deposit_event(Event::CallUnavailable {
task: (when, agenda_index),
id: task.maybe_id,
});

T::Preimages::drop(&task.call);

let _ = weight.try_consume(T::WeightInfo::service_task(
task.call.lookup_len().map(|x| x as usize),
task.maybe_id.is_some(),
));

// Keep preimage requested -- the task returns to the agenda and
// whoever resolves it (cancel/reschedule) will drop the reference.
return Err((Unavailable, Some(task)));
},
};
Expand All @@ -1076,6 +1076,9 @@ impl<T: Config> Pallet<T> {

match Self::execute_dispatch(weight, task.origin.clone(), call) {
Err(()) if is_first => {
if let Some(ref id) = task.maybe_id {
Lookup::<T>::remove(id);
}
T::Preimages::drop(&task.call);
Self::deposit_event(Event::PermanentlyOverweight {
task: (when, agenda_index),
Expand All @@ -1085,6 +1088,9 @@ impl<T: Config> Pallet<T> {
},
Err(()) => Err((Overweight, Some(task))),
Ok(result) => {
if let Some(ref id) = task.maybe_id {
Lookup::<T>::remove(id);
}
let failed = result.is_err();
let maybe_retry_config = Retries::<T>::take((when, agenda_index));

Expand Down
47 changes: 14 additions & 33 deletions pallets/scheduler/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,29 +1449,24 @@ fn cancel_retries_works() {
}

#[test]
fn postponed_named_task_cannot_be_rescheduled() {
fn unavailable_preimage_preserves_lookup_for_cancellation() {
new_test_ext().execute_with(|| {
let call =
RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_parts(1000, 0) });
let hash = <Test as frame_system::Config>::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 hashed = Bounded::Lookup { hash, len };
let name: [u8; 32] = hash.as_ref().try_into().unwrap();

let address =
Scheduler::do_schedule_named(name, DispatchTime::At(4), 127, root(), hashed.clone())
.unwrap();
Scheduler::do_schedule_named(name, DispatchTime::At(4), 127, root(), hashed.clone())
.unwrap();
assert!(Preimage::is_requested(&hash));
assert!(Lookup::<Test>::contains_key(name));

// Run to a very large block.
run_to_block(10);

// It was not executed.
assert!(logger::log().is_empty());

// Preimage was not available
assert_eq!(
System::events().last().unwrap().event,
crate::Event::CallUnavailable {
Expand All @@ -1481,12 +1476,11 @@ fn postponed_named_task_cannot_be_rescheduled() {
.into()
);

// So it should not be requested.
assert!(!Preimage::is_requested(&hash));
// Postponing removes the lookup.
assert!(!Lookup::<Test>::contains_key(name));
// Preimage stays requested -- the task is still in the agenda.
assert!(Preimage::is_requested(&hash));
// Lookup is preserved so the task can still be cancelled/rescheduled by name.
assert!(Lookup::<Test>::contains_key(name));

// The agenda still contains the call.
let agenda = Agenda::<Test>::iter().collect::<Vec<_>>();
assert_eq!(agenda.len(), 1);
assert_eq!(
Expand All @@ -1500,24 +1494,10 @@ fn postponed_named_task_cannot_be_rescheduled() {
})]
);

// Finally add the preimage.
assert_ok!(Preimage::note_preimage(RuntimeOrigin::signed(0), call.encode()));

run_to_block(1000);
// It did not execute.
assert!(logger::log().is_empty());
assert!(!Preimage::is_requested(&hash));

// Manually re-schedule the call by name does not work.
assert_err!(
Scheduler::do_reschedule_named(name, DispatchTime::At(1001)),
Error::<Test>::NotFound
);
// Manually re-scheduling the call by address errors.
assert_err!(
Scheduler::do_reschedule(address, DispatchTime::At(1001)),
Error::<Test>::Named
);
// The stranded task can still be cancelled by name.
assert_ok!(Scheduler::do_cancel_named(None, name));
assert!(!Lookup::<Test>::contains_key(name));
assert!(Agenda::<Test>::iter().collect::<Vec<_>>().is_empty());
});
}

Expand Down Expand Up @@ -2228,8 +2208,9 @@ fn unavailable_call_is_detected() {
}
.into()
);
// It should not be requested anymore.
assert!(!Preimage::is_requested(&hash));
// Preimage stays requested -- the task is still in the agenda and can be
// cancelled or rescheduled by name.
assert!(Preimage::is_requested(&hash));
});
}
#[test]
Expand Down
Loading