From 6d4d85eddac4d4def1ccca8cb220e68c2af2e9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 5 Nov 2020 16:29:47 +0100 Subject: [PATCH] Make sure we clear the freed the availability cores (#1921) If a core is freed, we need to make sure it is inserted as free into availability cores, because we can not be sure that the core is occupied directly again. --- polkadot/runtime/parachains/src/inclusion.rs | 2 +- .../parachains/src/inclusion_inherent.rs | 2 +- polkadot/runtime/parachains/src/scheduler.rs | 97 ++++++++++++++++++- 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/polkadot/runtime/parachains/src/inclusion.rs b/polkadot/runtime/parachains/src/inclusion.rs index d5a6e92674..61bdd58bf2 100644 --- a/polkadot/runtime/parachains/src/inclusion.rs +++ b/polkadot/runtime/parachains/src/inclusion.rs @@ -312,7 +312,7 @@ impl Module { { if pending_availability.availability_votes.count_ones() >= threshold { >::remove(¶_id); - let commitments = match ::take(¶_id) { + let commitments = match PendingAvailabilityCommitments::take(¶_id) { Some(commitments) => commitments, None => { debug::warn!(r#" diff --git a/polkadot/runtime/parachains/src/inclusion_inherent.rs b/polkadot/runtime/parachains/src/inclusion_inherent.rs index d827b359c8..14f63c9dbb 100644 --- a/polkadot/runtime/parachains/src/inclusion_inherent.rs +++ b/polkadot/runtime/parachains/src/inclusion_inherent.rs @@ -104,7 +104,7 @@ decl_module! { let freed = freed_concluded.into_iter().map(|c| (c, FreedReason::Concluded)) .chain(freed_timeout.into_iter().map(|c| (c, FreedReason::TimedOut))); - >::schedule(freed.collect()); + >::schedule(freed); // Process backed candidates according to scheduled cores. let occupied = >::process_candidates( diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index 455a07f94e..a40526aa09 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -367,7 +367,7 @@ impl Module { /// Schedule all unassigned cores, where possible. Provide a list of cores that should be considered /// newly-freed along with the reason for them being freed. The list is assumed to be sorted in /// ascending order by core index. - pub(crate) fn schedule(just_freed_cores: Vec<(CoreIndex, FreedReason)>) { + pub(crate) fn schedule(just_freed_cores: impl IntoIterator) { let mut cores = AvailabilityCores::get(); let config = >::config(); @@ -495,6 +495,7 @@ impl Module { Scheduled::set(scheduled); ParathreadQueue::set(parathread_queue); + AvailabilityCores::set(cores); } /// Note that the given cores have become occupied. Behavior undefined if any of the given cores were not scheduled @@ -1328,6 +1329,100 @@ mod tests { }); } + #[test] + fn schedule_clears_availability_cores() { + let genesis_config = MockGenesisConfig { + configuration: crate::configuration::GenesisConfig { + config: default_config(), + ..Default::default() + }, + ..Default::default() + }; + + let chain_a = ParaId::from(1); + let chain_b = ParaId::from(2); + let chain_c = ParaId::from(3); + + let schedule_blank_para = |id, is_chain| Paras::schedule_para_initialize(id, ParaGenesisArgs { + genesis_head: Vec::new().into(), + validation_code: Vec::new().into(), + parachain: is_chain, + }); + + new_test_ext(genesis_config).execute_with(|| { + assert_eq!(default_config().parathread_cores, 3); + + // register 3 parachains + schedule_blank_para(chain_a, true); + schedule_blank_para(chain_b, true); + schedule_blank_para(chain_c, true); + + // start a new session to activate, 5 validators for 5 cores. + run_to_block(1, |number| match number { + 1 => Some(SessionChangeNotification { + new_config: default_config(), + validators: vec![ + ValidatorId::from(Sr25519Keyring::Alice.public()), + ValidatorId::from(Sr25519Keyring::Bob.public()), + ValidatorId::from(Sr25519Keyring::Charlie.public()), + ValidatorId::from(Sr25519Keyring::Dave.public()), + ValidatorId::from(Sr25519Keyring::Eve.public()), + ], + ..Default::default() + }), + _ => None, + }); + + run_to_block(2, |_| None); + + assert_eq!(Scheduler::scheduled().len(), 3); + + // cores 0, 1, and 2 should be occupied. mark them as such. + Scheduler::occupied(&[CoreIndex(0), CoreIndex(1), CoreIndex(2)]); + + { + let cores = AvailabilityCores::get(); + + assert!(cores[0].is_some()); + assert!(cores[1].is_some()); + assert!(cores[2].is_some()); + + assert!(Scheduler::scheduled().is_empty()); + } + + run_to_block(3, |_| None); + + // now note that cores 0 and 2 were freed. + Scheduler::schedule(vec![ + (CoreIndex(0), FreedReason::Concluded), + (CoreIndex(2), FreedReason::Concluded), + ]); + + { + let scheduled = Scheduler::scheduled(); + + assert_eq!(scheduled.len(), 2); + assert_eq!(scheduled[0], CoreAssignment { + core: CoreIndex(0), + para_id: chain_a, + kind: AssignmentKind::Parachain, + group_idx: GroupIndex(0), + }); + assert_eq!(scheduled[1], CoreAssignment { + core: CoreIndex(2), + para_id: chain_c, + kind: AssignmentKind::Parachain, + group_idx: GroupIndex(2), + }); + + // The freed cores should be `None` in `AvailabilityCores`. + let cores = AvailabilityCores::get(); + assert!(cores[0].is_none()); + assert!(cores[2].is_none()); + } + }); + } + #[test] fn schedule_rotates_groups() { let config = {