Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@alindima
alindima committed Feb 23, 2024
1 parent cb41758 commit bffa4e9
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 34 deletions.
49 changes: 22 additions & 27 deletions polkadot/runtime/parachains/src/paras_inherent/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ use frame_support::{
pallet_prelude::*,
traits::Randomness,
};

use frame_system::pallet_prelude::*;
use pallet_babe::{self, ParentBlockRandomness};
use primitives::{
Expand Down Expand Up @@ -145,9 +144,8 @@ pub mod pallet {
DisputeInvalid,
/// A candidate was backed by a disabled validator
BackedByDisabled,
/// A candidate was backed even though the paraid had multiple cores assigned and no
/// injected core index.
BackedByElasticScalingWithNoCoreIndex,
/// A candidate was backed even though the paraid was not scheduled.
BackedOnUnscheduledCore,
/// Too many candidates supplied.
UnscheduledCandidate,
}
Expand Down Expand Up @@ -610,7 +608,7 @@ impl<T: Config> Pallet<T> {
let SanitizedBackedCandidates {
backed_candidates_with_core,
votes_from_disabled_were_dropped,
dropped_elastic_scaling_candidates,
dropped_unscheduled_candidates,
} = sanitize_backed_candidates::<T, _>(
backed_candidates,
&allowed_relay_parents,
Expand Down Expand Up @@ -652,14 +650,10 @@ impl<T: Config> Pallet<T> {
}

// In `Enter` context (invoked during execution) we shouldn't have filtered any candidates
// due to a para having multiple cores assigned and no injected core index. They have been
// filtered during inherent data preparation (`ProvideInherent` context). Abort in such
// cases.
// due to a para not being scheduled. They have been filtered during inherent data
// preparation (`ProvideInherent` context). Abort in such cases.
if context == ProcessInherentDataContext::Enter {
ensure!(
!dropped_elastic_scaling_candidates,
Error::<T>::BackedByElasticScalingWithNoCoreIndex
);
ensure!(!dropped_unscheduled_candidates, Error::<T>::BackedOnUnscheduledCore);
}

// Process backed candidates according to scheduled cores.
Expand Down Expand Up @@ -966,7 +960,7 @@ struct SanitizedBackedCandidates<Hash> {
votes_from_disabled_were_dropped: bool,
// Set to true if any candidates were dropped due to filtering done in
// `map_candidates_to_cores`
dropped_elastic_scaling_candidates: bool,
dropped_unscheduled_candidates: bool,
}

/// Filter out:
Expand Down Expand Up @@ -1002,14 +996,17 @@ fn sanitize_backed_candidates<
!candidate_has_concluded_invalid_dispute_or_is_invalid(candidate_idx, backed_candidate)
});

let initial_candidate_count = backed_candidates.len();
// Map candidates to scheduled cores. Filter out any unscheduled candidates.
let (mut backed_candidates_with_core, dropped_elastic_scaling_candidates) =
map_candidates_to_cores::<T>(
&allowed_relay_parents,
scheduled,
core_index_enabled,
backed_candidates,
);
let mut backed_candidates_with_core = map_candidates_to_cores::<T>(
&allowed_relay_parents,
scheduled,
core_index_enabled,
backed_candidates,
);

let dropped_unscheduled_candidates =
initial_candidate_count != backed_candidates_with_core.len();

// Filter out backing statements from disabled validators
let votes_from_disabled_were_dropped = filter_backed_statements_from_disabled_validators::<T>(
Expand All @@ -1026,7 +1023,7 @@ fn sanitize_backed_candidates<
backed_candidates_with_core.sort_by(|(_x, core_x), (_y, core_y)| core_x.cmp(&core_y));

SanitizedBackedCandidates {
dropped_elastic_scaling_candidates,
dropped_unscheduled_candidates,
votes_from_disabled_were_dropped,
backed_candidates_with_core,
}
Expand Down Expand Up @@ -1218,13 +1215,13 @@ fn filter_backed_statements_from_disabled_validators<T: shared::Config + schedul
/// If the para only has one scheduled core and no `CoreIndex` is injected, map the candidate to the
/// single core. If the para has multiple cores scheduled, only map the candidates which have a
/// proper core injected. Filter out the rest.
/// Also returns whether or not we dropped any candidates.
fn map_candidates_to_cores<T: configuration::Config + scheduler::Config + inclusion::Config>(
allowed_relay_parents: &AllowedRelayParentsTracker<T::Hash, BlockNumberFor<T>>,
mut scheduled: BTreeMap<ParaId, BTreeSet<CoreIndex>>,
core_index_enabled: bool,
candidates: Vec<BackedCandidate<T::Hash>>,
) -> (Vec<(BackedCandidate<T::Hash>, CoreIndex)>, bool) {
let mut dropped_elastic_scaling_candidates = false;
) -> Vec<(BackedCandidate<T::Hash>, CoreIndex)> {
let mut backed_candidates_with_core = Vec::with_capacity(candidates.len());

// We keep a candidate if the parachain has only one core assigned or if
Expand All @@ -1237,6 +1234,7 @@ fn map_candidates_to_cores<T: configuration::Config + scheduler::Config + inclus
);

let scheduled_cores = scheduled.get_mut(&backed_candidate.descriptor().para_id);
// Candidates without scheduled cores are silently filtered out.
if let Some(scheduled_cores) = scheduled_cores {
if let Some(core_idx) = maybe_injected_core_index {
if scheduled_cores.contains(&core_idx) {
Expand All @@ -1246,16 +1244,13 @@ fn map_candidates_to_cores<T: configuration::Config + scheduler::Config + inclus
} else if scheduled_cores.len() == 1 {
backed_candidates_with_core
.push((backed_candidate, scheduled_cores.pop_first().expect("Length is 1")));
} else {
dropped_elastic_scaling_candidates = true;
}
}
}

(backed_candidates_with_core, dropped_elastic_scaling_candidates)
backed_candidates_with_core
}

// Returns `true` if the candidate contains a valid injected `CoreIndex`.
fn get_injected_core_index<T: configuration::Config + scheduler::Config + inclusion::Config>(
allowed_relay_parents: &AllowedRelayParentsTracker<T::Hash, BlockNumberFor<T>>,
candidate: &BackedCandidate<T::Hash>,
Expand Down
36 changes: 29 additions & 7 deletions polkadot/runtime/parachains/src/paras_inherent/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ mod enter {
use crate::{
builder::{Bench, BenchBuilder},
mock::{mock_assigner, new_test_ext, BlockLength, BlockWeights, MockGenesisConfig, Test},
scheduler::common::Assignment,
scheduler::{
common::{Assignment, AssignmentProvider, AssignmentProviderConfig},
ParasEntry,
},
};
use assert_matches::assert_matches;
use frame_support::assert_ok;
Expand Down Expand Up @@ -697,6 +700,25 @@ mod enter {
2
);

// One core was scheduled. We should put the assignment back, before calling enter().
let now = <frame_system::Pallet<Test>>::block_number() + 1;
let used_cores = 5;
let cores = (0..used_cores)
.into_iter()
.map(|i| {
let AssignmentProviderConfig { ttl, .. } =
scheduler::Pallet::<Test>::assignment_provider_config(CoreIndex(i));
// Load an assignment into provider so that one is present to pop
let assignment =
<Test as scheduler::Config>::AssignmentProvider::get_mock_assignment(
CoreIndex(i),
ParaId::from(i),
);
(CoreIndex(i), [ParasEntry::new(assignment, now + ttl)].into())
})
.collect();
scheduler::ClaimQueue::<Test>::set(cores);

assert_ok!(Pallet::<Test>::enter(
frame_system::RawOrigin::None.into(),
limit_inherent_data,
Expand Down Expand Up @@ -1759,7 +1781,7 @@ mod sanitizers {
SanitizedBackedCandidates {
backed_candidates_with_core: all_backed_candidates_with_core,
votes_from_disabled_were_dropped: false,
dropped_elastic_scaling_candidates: false
dropped_unscheduled_candidates: false
}
);
});
Expand Down Expand Up @@ -1790,7 +1812,7 @@ mod sanitizers {
SanitizedBackedCandidates {
backed_candidates_with_core: expected_all_backed_candidates_with_core,
votes_from_disabled_were_dropped: false,
dropped_elastic_scaling_candidates: !core_index_enabled
dropped_unscheduled_candidates: !core_index_enabled
}
);
});
Expand Down Expand Up @@ -1819,7 +1841,7 @@ mod sanitizers {
let SanitizedBackedCandidates {
backed_candidates_with_core: sanitized_backed_candidates,
votes_from_disabled_were_dropped,
dropped_elastic_scaling_candidates,
dropped_unscheduled_candidates,
} = sanitize_backed_candidates::<Test, _>(
backed_candidates.clone(),
&<shared::Pallet<Test>>::allowed_relay_parents(),
Expand All @@ -1830,7 +1852,7 @@ mod sanitizers {

assert!(sanitized_backed_candidates.is_empty());
assert!(!votes_from_disabled_were_dropped);
assert!(!dropped_elastic_scaling_candidates);
assert!(!dropped_unscheduled_candidates);
});
}

Expand Down Expand Up @@ -1858,7 +1880,7 @@ mod sanitizers {
let SanitizedBackedCandidates {
backed_candidates_with_core: sanitized_backed_candidates,
votes_from_disabled_were_dropped,
dropped_elastic_scaling_candidates,
dropped_unscheduled_candidates,
} = sanitize_backed_candidates::<Test, _>(
backed_candidates.clone(),
&<shared::Pallet<Test>>::allowed_relay_parents(),
Expand All @@ -1869,7 +1891,7 @@ mod sanitizers {

assert_eq!(sanitized_backed_candidates.len(), backed_candidates.len() / 2);
assert!(!votes_from_disabled_were_dropped);
assert!(!dropped_elastic_scaling_candidates);
assert!(!dropped_unscheduled_candidates);
});
}

Expand Down

0 comments on commit bffa4e9

Please sign in to comment.