From 80a6c14f562f85e0b2c50c68e3b952def7f0ba38 Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 11:15:38 +0100 Subject: [PATCH 1/7] Relax requirements for `assign_core`. --- .../parachains/src/assigner_coretime/mod.rs | 90 ++++++++----------- .../parachains/src/assigner_coretime/tests.rs | 85 ++++++------------ 2 files changed, 64 insertions(+), 111 deletions(-) diff --git a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs index 33a36a1bb2ea..50be72fb4960 100644 --- a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs +++ b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs @@ -35,6 +35,7 @@ use crate::{ }; use alloc::{vec, vec::Vec}; +use core::mem; use frame_support::{defensive, pallet_prelude::*}; use frame_system::pallet_prelude::*; use pallet_broker::CoreAssignment; @@ -236,17 +237,9 @@ pub mod pallet { #[pallet::error] pub enum Error { AssignmentsEmpty, - /// Assignments together exceeded 57600. - OverScheduled, - /// Assignments together less than 57600 - UnderScheduled, /// assign_core is only allowed to append new assignments at the end of already existing - /// ones. + /// ones or update the last entry. DisallowedInsert, - /// Tried to insert a schedule for the same core and block number as an existing schedule - DuplicateInsert, - /// Tried to add an unsorted set of assignments - AssignmentsNotSorted, } } @@ -387,67 +380,56 @@ impl Pallet { /// Append another assignment for a core. /// - /// Important only appending is allowed. Meaning, all already existing assignments must have a - /// begin smaller than the one passed here. This restriction exists, because it makes the - /// insertion O(1) and the author could not think of a reason, why this restriction should be - /// causing any problems. Inserting arbitrarily causes a `DispatchError::DisallowedInsert` - /// error. This restriction could easily be lifted if need be and in fact an implementation is - /// available + /// Important only appending is allowed or insertion into the last item is possible. Meaning, + /// all already existing assignments must have a begin smaller or equal than the one passed + /// here. This restriction exists, because it makes the insertion O(1) and the author could not + /// think of a reason, why this restriction should be causing any problems. Inserting + /// arbitrarily causes a `DispatchError::DisallowedInsert` error. This restriction could easily + /// be lifted if need be and in fact an implementation is available /// [here](https://github.com/paritytech/polkadot-sdk/pull/1694/commits/c0c23b01fd2830910cde92c11960dad12cdff398#diff-0c85a46e448de79a5452395829986ee8747e17a857c27ab624304987d2dde8baR386). /// The problem is that insertion complexity then depends on the size of the existing queue, /// which makes determining weights hard and could lead to issues like overweight blocks (at /// least in theory). + /// + /// Updating the last entry is supported to allow for making a core assignment multiple calls to + /// assign_core. Thus if you have too much interlacing for e.g. a single UMP message you can + /// split that up into multiple messages, each triggering a call to `assign_core`, together + /// forming the total assignment. pub fn assign_core( core_idx: CoreIndex, begin: BlockNumberFor, - assignments: Vec<(CoreAssignment, PartsOf57600)>, + mut assignments: Vec<(CoreAssignment, PartsOf57600)>, end_hint: Option>, ) -> Result<(), DispatchError> { // There should be at least one assignment. ensure!(!assignments.is_empty(), Error::::AssignmentsEmpty); - // Checking for sort and unique manually, since we don't have access to iterator tools. - // This way of checking uniqueness only works since we also check sortedness. - assignments.iter().map(|x| &x.0).try_fold(None, |prev, cur| { - if prev.map_or(false, |p| p >= cur) { - Err(Error::::AssignmentsNotSorted) - } else { - Ok(Some(cur)) - } - })?; - - // Check that the total parts between all assignments are equal to 57600 - let parts_sum = assignments - .iter() - .map(|assignment| assignment.1) - .try_fold(PartsOf57600::ZERO, |sum, parts| { - sum.checked_add(parts).ok_or(Error::::OverScheduled) - })?; - ensure!(parts_sum.is_full(), Error::::UnderScheduled); - CoreDescriptors::::mutate(core_idx, |core_descriptor| { let new_queue = match core_descriptor.queue { Some(queue) => { - ensure!(begin > queue.last, Error::::DisallowedInsert); - - CoreSchedules::::try_mutate((queue.last, core_idx), |schedule| { - if let Some(schedule) = schedule.as_mut() { - debug_assert!(schedule.next_schedule.is_none(), "queue.end was supposed to be the end, so the next item must be `None`!"); - schedule.next_schedule = Some(begin); + ensure!(begin >= queue.last, Error::::DisallowedInsert); + + // Update queue if we are appending: + if begin > queue.last { + CoreSchedules::::mutate((queue.last, core_idx), |schedule| { + if let Some(schedule) = schedule.as_mut() { + debug_assert!(schedule.next_schedule.is_none(), "queue.end was supposed to be the end, so the next item must be `None`!"); + schedule.next_schedule = Some(begin); + } else { + defensive!("Queue end entry does not exist?"); + } + }); + } + + CoreSchedules::::mutate((begin, core_idx), |schedule| { + let assignments = if let Some(mut old_schedule) = mem::take(schedule) { + old_schedule.assignments.append(&mut assignments); + mem::take(&mut old_schedule.assignments) } else { - defensive!("Queue end entry does not exist?"); - } - CoreSchedules::::try_mutate((begin, core_idx), |schedule| { - // It should already be impossible to overwrite an existing schedule due - // to strictly increasing block number. But we check here for safety and - // in case the design changes. - ensure!(schedule.is_none(), Error::::DuplicateInsert); - *schedule = - Some(Schedule { assignments, end_hint, next_schedule: None }); - Ok::<(), DispatchError>(()) - })?; - Ok::<(), DispatchError>(()) - })?; + assignments + }; + *schedule = Some(Schedule { assignments, end_hint, next_schedule: None }); + }); QueueDescriptor { first: queue.first, last: begin } }, diff --git a/polkadot/runtime/parachains/src/assigner_coretime/tests.rs b/polkadot/runtime/parachains/src/assigner_coretime/tests.rs index 25007f0eed6a..dff53de7cdb4 100644 --- a/polkadot/runtime/parachains/src/assigner_coretime/tests.rs +++ b/polkadot/runtime/parachains/src/assigner_coretime/tests.rs @@ -235,10 +235,7 @@ fn assign_core_works_with_prior_schedule() { } #[test] -// Invariants: We assume that CoreSchedules is append only and consumed. In other words new -// schedules inserted for a core must have a higher block number than all of the already existing -// schedules. -fn assign_core_enforces_higher_block_number() { +fn assign_core_enforces_higher_or_equal_block_number() { let core_idx = CoreIndex(0); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { @@ -255,7 +252,7 @@ fn assign_core_enforces_higher_block_number() { assert_ok!(CoretimeAssigner::assign_core( core_idx, BlockNumberFor::::from(15u32), - default_test_assignments(), + vec![(CoreAssignment::Idle, PartsOf57600(28800))], None, )); @@ -281,6 +278,16 @@ fn assign_core_enforces_higher_block_number() { ), Error::::DisallowedInsert ); + // Call assign core again on last entry should work: + assert_eq!( + CoretimeAssigner::assign_core( + core_idx, + BlockNumberFor::::from(15u32), + vec![(CoreAssignment::Pool, PartsOf57600(28800))], + None, + ), + Ok(()) + ); }); } @@ -293,20 +300,6 @@ fn assign_core_enforces_well_formed_schedule() { run_to_block(1, |n| if n == 1 { Some(Default::default()) } else { None }); let empty_assignments: Vec<(CoreAssignment, PartsOf57600)> = vec![]; - let overscheduled = vec![ - (CoreAssignment::Pool, PartsOf57600::FULL), - (CoreAssignment::Task(para_id.into()), PartsOf57600::FULL), - ]; - let underscheduled = vec![(CoreAssignment::Pool, PartsOf57600(30000))]; - let not_unique = vec![ - (CoreAssignment::Pool, PartsOf57600::FULL / 2), - (CoreAssignment::Pool, PartsOf57600::FULL / 2), - ]; - let not_sorted = vec![ - (CoreAssignment::Task(para_id.into()), PartsOf57600(19200)), - (CoreAssignment::Pool, PartsOf57600(19200)), - (CoreAssignment::Idle, PartsOf57600(19200)), - ]; // Attempting assign_core with malformed assignments such that all error cases // are tested @@ -319,42 +312,6 @@ fn assign_core_enforces_well_formed_schedule() { ), Error::::AssignmentsEmpty ); - assert_noop!( - CoretimeAssigner::assign_core( - core_idx, - BlockNumberFor::::from(11u32), - overscheduled, - None, - ), - Error::::OverScheduled - ); - assert_noop!( - CoretimeAssigner::assign_core( - core_idx, - BlockNumberFor::::from(11u32), - underscheduled, - None, - ), - Error::::UnderScheduled - ); - assert_noop!( - CoretimeAssigner::assign_core( - core_idx, - BlockNumberFor::::from(11u32), - not_unique, - None, - ), - Error::::AssignmentsNotSorted - ); - assert_noop!( - CoretimeAssigner::assign_core( - core_idx, - BlockNumberFor::::from(11u32), - not_sorted, - None, - ), - Error::::AssignmentsNotSorted - ); }); } @@ -374,7 +331,14 @@ fn next_schedule_always_points_to_next_work_plan_item() { Schedule { next_schedule: Some(start_4), ..default_test_schedule() }; let expected_schedule_4 = Schedule { next_schedule: Some(start_5), ..default_test_schedule() }; - let expected_schedule_5 = default_test_schedule(); + let expected_schedule_5 = Schedule { + next_schedule: None, + end_hint: None, + assignments: vec![ + (CoreAssignment::Pool, PartsOf57600(28800)), + (CoreAssignment::Idle, PartsOf57600(28800)), + ], + }; // Call assign_core for each of five schedules assert_ok!(CoretimeAssigner::assign_core( @@ -408,7 +372,14 @@ fn next_schedule_always_points_to_next_work_plan_item() { assert_ok!(CoretimeAssigner::assign_core( core_idx, BlockNumberFor::::from(start_5), - default_test_assignments(), + vec![(CoreAssignment::Pool, PartsOf57600(28800))], + None, + )); + // Test updating last entry once more: + assert_ok!(CoretimeAssigner::assign_core( + core_idx, + BlockNumberFor::::from(start_5), + vec![(CoreAssignment::Idle, PartsOf57600(28800))], None, )); From e9b0b14c92352d66e77c904564229b7ef906123a Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Wed, 6 Nov 2024 10:27:50 +0000 Subject: [PATCH 2/7] Update from eskimor running command 'prdoc' --- prdoc/pr_6384.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_6384.prdoc diff --git a/prdoc/pr_6384.prdoc b/prdoc/pr_6384.prdoc new file mode 100644 index 000000000000..f3843421d5a1 --- /dev/null +++ b/prdoc/pr_6384.prdoc @@ -0,0 +1,10 @@ +title: 'Fix #6102' +doc: +- audience: Todo + description: |- + Relax requirements for `assign_core` so that it accepts updates for the last scheduled entry. + + Fixes #6102 +crates: +- name: polkadot-runtime-parachains + bump: major From 1a0309f9e00e3d39c9132b139f001d56c681a14e Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 11:31:04 +0100 Subject: [PATCH 3/7] Add prdoc --- prdoc/pr_6384.prdoc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/prdoc/pr_6384.prdoc b/prdoc/pr_6384.prdoc index f3843421d5a1..6dbe9d43dd5b 100644 --- a/prdoc/pr_6384.prdoc +++ b/prdoc/pr_6384.prdoc @@ -1,10 +1,12 @@ -title: 'Fix #6102' +title: Relax requirements on `assign_core`. doc: -- audience: Todo +- audience: Runtime Dev description: |- Relax requirements for `assign_core` so that it accepts updates for the last scheduled entry. + This will allow the coretime chain to split up assignments into multiple + messages, which allows for interlacing down to single block granularity. - Fixes #6102 + Fixes: https://github.com/paritytech/polkadot-sdk/issues/6102 crates: - name: polkadot-runtime-parachains - bump: major + bump: minor From a3c323332796a69e1d10f7b4f31649b611b00e39 Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 11:31:58 +0100 Subject: [PATCH 4/7] Bump is actually major. --- prdoc/pr_6384.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_6384.prdoc b/prdoc/pr_6384.prdoc index 6dbe9d43dd5b..2ea0bc1043c3 100644 --- a/prdoc/pr_6384.prdoc +++ b/prdoc/pr_6384.prdoc @@ -9,4 +9,4 @@ doc: Fixes: https://github.com/paritytech/polkadot-sdk/issues/6102 crates: - name: polkadot-runtime-parachains - bump: minor + bump: major From 3d9f4189877617506c97bc59661bdae8d36846d9 Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 11:51:16 +0100 Subject: [PATCH 5/7] Fix warning --- polkadot/runtime/parachains/src/assigner_coretime/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/assigner_coretime/tests.rs b/polkadot/runtime/parachains/src/assigner_coretime/tests.rs index dff53de7cdb4..ab011bfc4ae1 100644 --- a/polkadot/runtime/parachains/src/assigner_coretime/tests.rs +++ b/polkadot/runtime/parachains/src/assigner_coretime/tests.rs @@ -293,7 +293,6 @@ fn assign_core_enforces_higher_or_equal_block_number() { #[test] fn assign_core_enforces_well_formed_schedule() { - let para_id = ParaId::from(1u32); let core_idx = CoreIndex(0); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { From 94ecabcd7071a6c7c78dba9f2e7a8486f7e5924d Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 12:49:50 +0100 Subject: [PATCH 6/7] Review remarks. --- .../parachains/src/assigner_coretime/mod.rs | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs index 50be72fb4960..64d449712d09 100644 --- a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs +++ b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs @@ -35,7 +35,6 @@ use crate::{ }; use alloc::{vec, vec::Vec}; -use core::mem; use frame_support::{defensive, pallet_prelude::*}; use frame_system::pallet_prelude::*; use pallet_broker::CoreAssignment; @@ -380,21 +379,21 @@ impl Pallet { /// Append another assignment for a core. /// - /// Important only appending is allowed or insertion into the last item is possible. Meaning, - /// all already existing assignments must have a begin smaller or equal than the one passed - /// here. This restriction exists, because it makes the insertion O(1) and the author could not - /// think of a reason, why this restriction should be causing any problems. Inserting - /// arbitrarily causes a `DispatchError::DisallowedInsert` error. This restriction could easily - /// be lifted if need be and in fact an implementation is available - /// [here](https://github.com/paritytech/polkadot-sdk/pull/1694/commits/c0c23b01fd2830910cde92c11960dad12cdff398#diff-0c85a46e448de79a5452395829986ee8747e17a857c27ab624304987d2dde8baR386). - /// The problem is that insertion complexity then depends on the size of the existing queue, - /// which makes determining weights hard and could lead to issues like overweight blocks (at - /// least in theory). - /// + /// Important: Only appending is allowed or insertion into the last item. Meaning, + /// all already existing assignments must have a `begin` smaller or equal than the one passed + /// here. /// Updating the last entry is supported to allow for making a core assignment multiple calls to /// assign_core. Thus if you have too much interlacing for e.g. a single UMP message you can /// split that up into multiple messages, each triggering a call to `assign_core`, together /// forming the total assignment. + //With this restriction this function allows for O(1) complexity.This restriction exists, + // because it makes the insertion O(1) and the author could not think of a reason, why this + // restriction should be causing any problems. Inserting arbitrarily causes a + // `DispatchError::DisallowedInsert` error. This restriction could easily be lifted if need be + // and in fact an implementation is available [here](https://github.com/paritytech/polkadot-sdk/pull/1694/commits/c0c23b01fd2830910cde92c11960dad12cdff398#diff-0c85a46e448de79a5452395829986ee8747e17a857c27ab624304987d2dde8baR386). + // The problem is that insertion complexity then depends on the size of the existing queue, + // which makes determining weights hard and could lead to issues like overweight blocks (at + // least in theory). pub fn assign_core( core_idx: CoreIndex, begin: BlockNumberFor, @@ -422,9 +421,9 @@ impl Pallet { } CoreSchedules::::mutate((begin, core_idx), |schedule| { - let assignments = if let Some(mut old_schedule) = mem::take(schedule) { + let assignments = if let Some(mut old_schedule) = schedule.take() { old_schedule.assignments.append(&mut assignments); - mem::take(&mut old_schedule.assignments) + old_schedule.assignments } else { assignments }; From 192301cee33a792a7b549ad34e20fc9fda46eabd Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 6 Nov 2024 12:53:22 +0100 Subject: [PATCH 7/7] Fix doc --- .../runtime/parachains/src/assigner_coretime/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs index 64d449712d09..866d52dc9848 100644 --- a/polkadot/runtime/parachains/src/assigner_coretime/mod.rs +++ b/polkadot/runtime/parachains/src/assigner_coretime/mod.rs @@ -386,11 +386,11 @@ impl Pallet { /// assign_core. Thus if you have too much interlacing for e.g. a single UMP message you can /// split that up into multiple messages, each triggering a call to `assign_core`, together /// forming the total assignment. - //With this restriction this function allows for O(1) complexity.This restriction exists, - // because it makes the insertion O(1) and the author could not think of a reason, why this - // restriction should be causing any problems. Inserting arbitrarily causes a - // `DispatchError::DisallowedInsert` error. This restriction could easily be lifted if need be - // and in fact an implementation is available [here](https://github.com/paritytech/polkadot-sdk/pull/1694/commits/c0c23b01fd2830910cde92c11960dad12cdff398#diff-0c85a46e448de79a5452395829986ee8747e17a857c27ab624304987d2dde8baR386). + /// + /// Inserting arbitrarily causes a `DispatchError::DisallowedInsert` error. + // With this restriction this function allows for O(1) complexity. It could easily be lifted, if + // need be and in fact an implementation is available + // [here](https://github.com/paritytech/polkadot-sdk/pull/1694/commits/c0c23b01fd2830910cde92c11960dad12cdff398#diff-0c85a46e448de79a5452395829986ee8747e17a857c27ab624304987d2dde8baR386). // The problem is that insertion complexity then depends on the size of the existing queue, // which makes determining weights hard and could lead to issues like overweight blocks (at // least in theory).