From 11392992b95aef6cfcbd9d6b5ce693ef37329f21 Mon Sep 17 00:00:00 2001 From: alindima Date: Fri, 25 Oct 2024 18:14:33 +0300 Subject: [PATCH 1/5] fix claim queue size --- polkadot/runtime/parachains/src/scheduler.rs | 32 ++++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index 329df3a8a9deb..89a4205bc7198 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -46,6 +46,7 @@ use frame_system::pallet_prelude::BlockNumberFor; pub use polkadot_core_primitives::v2::BlockNumber; use polkadot_primitives::{ CoreIndex, GroupIndex, GroupRotationInfo, Id as ParaId, ScheduledCore, ValidatorIndex, + SchedulerParams, }; use sp_runtime::traits::One; @@ -131,7 +132,7 @@ impl Pallet { pub(crate) fn initializer_on_new_session( notification: &SessionChangeNotification>, ) { - let SessionChangeNotification { validators, new_config, prev_config, .. } = notification; + let SessionChangeNotification { validators, new_config, .. } = notification; let config = new_config; let assigner_cores = config.scheduler_params.num_cores; @@ -186,7 +187,7 @@ impl Pallet { } // Resize and populate claim queue. - Self::maybe_resize_claim_queue(prev_config.scheduler_params.num_cores, assigner_cores); + Self::maybe_resize_claim_queue(); Self::populate_claim_queue_after_session_change(); let now = frame_system::Pallet::::block_number() + One::one(); @@ -203,6 +204,14 @@ impl Pallet { ValidatorGroups::::decode_len().unwrap_or(0) } + /// Claim queue len + fn expected_claim_queue_len(config: &SchedulerParams>) -> u32 { + core::cmp::min( + config.num_cores, + Self::num_availability_cores() as u32 + ) + } + /// Get the group assigned to a specific core by index at the current block number. Result /// undefined if the core index is unknown or the block number is less than the session start /// index. @@ -325,11 +334,11 @@ impl Pallet { /// and fill the queue from the assignment provider. pub(crate) fn advance_claim_queue(except_for: &BTreeSet) { let config = configuration::ActiveConfig::::get(); - let num_assigner_cores = config.scheduler_params.num_cores; + let expected_claim_queue_len = Self::expected_claim_queue_len(&config.scheduler_params); // Extra sanity, config should already never be smaller than 1: let n_lookahead = config.scheduler_params.lookahead.max(1); - for core_idx in 0..num_assigner_cores { + for core_idx in 0..expected_claim_queue_len { let core_idx = CoreIndex::from(core_idx); if !except_for.contains(&core_idx) { @@ -345,11 +354,16 @@ impl Pallet { } // on new session - fn maybe_resize_claim_queue(old_core_count: u32, new_core_count: u32) { - if new_core_count < old_core_count { + fn maybe_resize_claim_queue() { + let cq = ClaimQueue::::get(); + let Some((old_max_core, _)) = cq.last_key_value() else {return}; + let config = configuration::ActiveConfig::::get(); + let new_core_count = Self::expected_claim_queue_len(&config.scheduler_params); + + if new_core_count < (old_max_core.0 + 1) { ClaimQueue::::mutate(|cq| { let to_remove: Vec<_> = cq - .range(CoreIndex(new_core_count)..CoreIndex(old_core_count)) + .range(CoreIndex(new_core_count)..=*old_max_core) .map(|(k, _)| *k) .collect(); for key in to_remove { @@ -367,9 +381,9 @@ impl Pallet { let config = configuration::ActiveConfig::::get(); // Extra sanity, config should already never be smaller than 1: let n_lookahead = config.scheduler_params.lookahead.max(1); - let new_core_count = config.scheduler_params.num_cores; + let expected_claim_queue_len = Self::expected_claim_queue_len(&config.scheduler_params); - for core_idx in 0..new_core_count { + for core_idx in 0..expected_claim_queue_len { let core_idx = CoreIndex::from(core_idx); Self::fill_claim_queue(core_idx, n_lookahead); } From b6bcec02c53f3ddd0ce8ff42083e72f19234df53 Mon Sep 17 00:00:00 2001 From: alindima Date: Thu, 31 Oct 2024 16:41:35 +0200 Subject: [PATCH 2/5] fmt --- polkadot/runtime/parachains/src/scheduler.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index 89a4205bc7198..9c111c2d28e79 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -45,8 +45,8 @@ use frame_support::{pallet_prelude::*, traits::Defensive}; use frame_system::pallet_prelude::BlockNumberFor; pub use polkadot_core_primitives::v2::BlockNumber; use polkadot_primitives::{ - CoreIndex, GroupIndex, GroupRotationInfo, Id as ParaId, ScheduledCore, ValidatorIndex, - SchedulerParams, + CoreIndex, GroupIndex, GroupRotationInfo, Id as ParaId, ScheduledCore, SchedulerParams, + ValidatorIndex, }; use sp_runtime::traits::One; @@ -204,12 +204,10 @@ impl Pallet { ValidatorGroups::::decode_len().unwrap_or(0) } - /// Claim queue len + /// Expected claim queue len. Can be different than the real length if for example we don't have + /// assignments for a core. fn expected_claim_queue_len(config: &SchedulerParams>) -> u32 { - core::cmp::min( - config.num_cores, - Self::num_availability_cores() as u32 - ) + core::cmp::min(config.num_cores, Self::num_availability_cores() as u32) } /// Get the group assigned to a specific core by index at the current block number. Result @@ -356,16 +354,14 @@ impl Pallet { // on new session fn maybe_resize_claim_queue() { let cq = ClaimQueue::::get(); - let Some((old_max_core, _)) = cq.last_key_value() else {return}; + let Some((old_max_core, _)) = cq.last_key_value() else { return }; let config = configuration::ActiveConfig::::get(); let new_core_count = Self::expected_claim_queue_len(&config.scheduler_params); if new_core_count < (old_max_core.0 + 1) { ClaimQueue::::mutate(|cq| { - let to_remove: Vec<_> = cq - .range(CoreIndex(new_core_count)..=*old_max_core) - .map(|(k, _)| *k) - .collect(); + let to_remove: Vec<_> = + cq.range(CoreIndex(new_core_count)..=*old_max_core).map(|(k, _)| *k).collect(); for key in to_remove { if let Some(dropped_assignments) = cq.remove(&key) { Self::push_back_to_assignment_provider(dropped_assignments.into_iter()); From fcd34c4cc7c26c98ba38393f012dc424e07eeb0d Mon Sep 17 00:00:00 2001 From: alindima Date: Thu, 31 Oct 2024 16:45:41 +0200 Subject: [PATCH 3/5] test --- .../runtime/parachains/src/scheduler/tests.rs | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/scheduler/tests.rs b/polkadot/runtime/parachains/src/scheduler/tests.rs index 5be7e084f3bca..431562c6e6fb7 100644 --- a/polkadot/runtime/parachains/src/scheduler/tests.rs +++ b/polkadot/runtime/parachains/src/scheduler/tests.rs @@ -726,8 +726,26 @@ fn session_change_decreasing_number_of_cores() { [assignment_b.clone()].into_iter().collect::>() ); - // No more assignments now. Scheduler::advance_claim_queue(&Default::default()); + // No more assignments now. + assert_eq!(Scheduler::claim_queue_len(), 0); + + // Retain number of cores to 1 but remove all validator groups. The claim queue length + // should be the minimum of these two. + + // Add an assignment. + MockAssigner::add_test_assignment(assignment_b.clone()); + + run_to_block(4, |number| match number { + 4 => Some(SessionChangeNotification { + new_config: new_config.clone(), + prev_config: new_config.clone(), + validators: vec![], + ..Default::default() + }), + _ => None, + }); + assert_eq!(Scheduler::claim_queue_len(), 0); }); } From 0f4c65a8a50e97c54deaef5dc0f181367690983f Mon Sep 17 00:00:00 2001 From: alindima Date: Thu, 31 Oct 2024 16:50:15 +0200 Subject: [PATCH 4/5] add prdoc --- prdoc/pr_6257.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_6257.prdoc diff --git a/prdoc/pr_6257.prdoc b/prdoc/pr_6257.prdoc new file mode 100644 index 0000000000000..f7e7df5cf8bbd --- /dev/null +++ b/prdoc/pr_6257.prdoc @@ -0,0 +1,10 @@ +title: 'fix claim queue size when validator groups count is smaller' +doc: +- audience: Runtime Dev + description: Fixes a bug introduced in https://github.com/paritytech/polkadot-sdk/pull/5461, where the claim queue + would contain entries even if the validator groups storage is empty (which happens during the first session). + This PR sets the claim queue core count to be the minimum between the num_cores param and the number of validator groups. + +crates: +- name: polkadot-runtime-parachains + bump: patch From 12df471872d17135ff19213db97db9ad9d903b5f Mon Sep 17 00:00:00 2001 From: alindima Date: Thu, 31 Oct 2024 16:54:07 +0200 Subject: [PATCH 5/5] fix prdoc? --- prdoc/pr_6257.prdoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_6257.prdoc b/prdoc/pr_6257.prdoc index f7e7df5cf8bbd..45f9810108ef7 100644 --- a/prdoc/pr_6257.prdoc +++ b/prdoc/pr_6257.prdoc @@ -1,9 +1,9 @@ title: 'fix claim queue size when validator groups count is smaller' doc: - audience: Runtime Dev - description: Fixes a bug introduced in https://github.com/paritytech/polkadot-sdk/pull/5461, where the claim queue - would contain entries even if the validator groups storage is empty (which happens during the first session). - This PR sets the claim queue core count to be the minimum between the num_cores param and the number of validator groups. + description: 'Fixes a bug introduced in https://github.com/paritytech/polkadot-sdk/pull/5461, where the claim queue + would contain entries even if the validator groups storage is empty (which happens during the first session). + This PR sets the claim queue core count to be the minimum between the num_cores param and the number of validator groups.' crates: - name: polkadot-runtime-parachains