Skip to content
Merged
Changes from 2 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
259cb4b
Filter backing votes in `paras_inherent` - initial work
tdimitrov Oct 10, 2023
d451886
Error handling
tdimitrov Oct 12, 2023
a3bafba
Merge branch 'master' into tsv-disabling-runtime
tdimitrov Oct 16, 2023
75d707a
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Oct 16, 2023
cb0d8fc
Add `fn disabled_validators` to `trait DisabledValidators`. Use the t…
tdimitrov Oct 16, 2023
8a8893e
Extract the implementation of `disabled_validators` from rutnime api …
tdimitrov Oct 17, 2023
7b868b0
Error handling
tdimitrov Oct 17, 2023
a41d21c
Remove `DisabledValidators` from paras_inherent
tdimitrov Oct 17, 2023
34a3bf0
Fix a warning
tdimitrov Oct 17, 2023
88eb018
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Oct 23, 2023
305d5b8
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Oct 24, 2023
e241312
Add setters used for unit tests
tdimitrov Oct 26, 2023
2aa1323
Mock for `DisabledValidators` in Test runtime
tdimitrov Oct 26, 2023
a8efd15
Fix test state and add an optimisation for `filter_backed_statements`
tdimitrov Oct 26, 2023
3567a93
Make disabled validators mock mutable
tdimitrov Oct 26, 2023
04dc32a
Extract common code in `get_test_data`
tdimitrov Oct 26, 2023
cce71ee
Additional cases
tdimitrov Oct 26, 2023
7a19f55
Fixes in `filter_backed_statements`
tdimitrov Oct 26, 2023
3260187
Refactor `filter_backed_statements`
tdimitrov Oct 26, 2023
ff577a5
Merge branch 'tsv-disabling' into tsv-disabling-runtime
ordian Oct 26, 2023
876655d
Apply suggestions from code review
tdimitrov Oct 28, 2023
ed2d430
wip: Filtering backed statements from disabled validators is performe…
tdimitrov Oct 31, 2023
9a8985a
para_id_to_core_idx -> scheduled
tdimitrov Oct 31, 2023
f7544d7
Fix code duplication in tests
tdimitrov Oct 31, 2023
59fc176
Fix a compilation error
tdimitrov Oct 31, 2023
cb2fe60
Fix backing votes filtering - if more than `effective_minimum_backing…
tdimitrov Nov 2, 2023
be85b5d
Ensure inherent data contains no backing votes from disabled validato…
tdimitrov Nov 2, 2023
f472bb1
Comments and small fixes
tdimitrov Nov 2, 2023
53aaffa
Fix disabled statements filtering
tdimitrov Nov 3, 2023
16823f4
Updated comments
tdimitrov Nov 3, 2023
452b202
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Nov 6, 2023
cb60cf0
Apply suggestions from code review
tdimitrov Nov 15, 2023
750f2db
Code review feedback
tdimitrov Nov 15, 2023
7022003
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Nov 16, 2023
8b5e5a8
Apply suggestions from code review: remove duplicated assert
tdimitrov Nov 28, 2023
a50800f
Code review feedback - filter_backed_statements_from_disabled -> filt…
tdimitrov Nov 28, 2023
7a308cd
Update parainherent.md with some details about data sanitization
tdimitrov Nov 28, 2023
7ccb942
Add newline to parainherent.md
tdimitrov Nov 28, 2023
1834e20
Apply suggestions from code review
tdimitrov Nov 28, 2023
610b45d
Apply suggestions from code review
tdimitrov Nov 29, 2023
83adc93
Fixes in parainherent.md
tdimitrov Nov 29, 2023
158557d
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Nov 29, 2023
79f4c40
Merge branch 'tsv-disabling' into tsv-disabling-runtime
tdimitrov Nov 30, 2023
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
129 changes: 128 additions & 1 deletion polkadot/runtime/parachains/src/paras_inherent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,11 @@ pub mod pallet {
#[pallet::config]
#[pallet::disable_frame_system_supertrait_check]
pub trait Config:
inclusion::Config + scheduler::Config + initializer::Config + pallet_babe::Config
inclusion::Config
+ scheduler::Config
+ initializer::Config
+ pallet_babe::Config
+ pallet_session::Config
{
/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
Expand Down Expand Up @@ -430,6 +434,9 @@ impl<T: Config> Pallet<T> {
T::DisputesHandler::filter_dispute_data(set, post_conclusion_acceptance_period)
};

// Filter out backing statements from disabled validators
let statements_were_dropped = filter_backed_statements::<T>(&mut backed_candidates)?;

// Limit the disputes first, since the following statements depend on the votes include
// here.
let (checked_disputes_sets, checked_disputes_sets_consumed_weight) =
Expand Down Expand Up @@ -480,6 +487,7 @@ impl<T: Config> Pallet<T> {
}

ensure!(all_weight_before.all_lte(max_block_weight), Error::<T>::InherentOverweight);
ensure!(!statements_were_dropped, Error::<T>::InherentOverweight);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to reject blocks where at least one statement is dropped? what do we do with disputes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afair the else statement is executed during block execution only (right?) and this is a sanity check that we are not importing votes from disabled validators. We discussed this here.

all_weight_before
};

Expand Down Expand Up @@ -1029,3 +1037,122 @@ fn limit_and_sanitize_disputes<
(checked, checked_disputes_weight)
}
}

// Filters statements from disabled validators in `BackedCandidate` and `MultiDisputeStatementSet`.
// Returns `true` if at least one statement is removed and `false` otherwise.
fn filter_backed_statements<T: Config>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ordian can you have a look at this? Does the index arithmetic here make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just merged #1257 that has a disabled_validators implementation, which could be extracted into a common module. There's a small caveat though, it doesn't work at session boundaries. But maybe that's not really an issue since availability cores are cleared out on session boundaries anyway.

Copy link
Contributor Author

@tdimitrov tdimitrov Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved your implementation in pallet_shared so that we can call it from the api impl and paras_inherent. Quite a lot of boilerplate code for such small change but it is what it is :)

If you have got an idea for a better place - please share.

backed_candidates: &mut Vec<BackedCandidate<<T as frame_system::Config>::Hash>>,
) -> Result<bool, DispatchErrorWithPostInfo> {
// `active_validator_indices` contain 'raw indexes' aka indexes in the broad validator set.
// We want mapping from `ValidatorIndex` (which is an index within `active_validator_indices`)
// to 'raw indexes' to process disabled validators.
let raw_idx_to_val_index = shared::Pallet::<T>::active_validator_indices()
.iter()
.enumerate()
.map(|(i, v)| (v.0, ValidatorIndex(i as u32)))
.collect::<BTreeMap<_, _>>();
// `disabled_validators` in pallet session contains 'raw indexes' (see the previous comment). We
// want them converted to `ValidatorIndex` so that we can look them up easily when processing
// backed candidates. We skip disabled validators which are not found in the active set.
let disabled_validators = <pallet_session::Pallet<T>>::disabled_validators()
.iter()
.filter_map(|i| raw_idx_to_val_index.get(i))
.collect::<BTreeSet<_>>();

// `BackedCandidate` contains `validator_indices` which are indecies within the validator group.
// To obtain `ValidatorIndex` from them we do the following steps:
// 1. Get `para_id` from `CandidateDescriptor`
// 2. Get the `core_idx`` for the corresponding `para_id`
// 3. Get the validator group assigned to the corresponding core idx

// Map `para_id` to `core_idx` for step 2 from the above list
let para_id_to_core_idx = <scheduler::Pallet<T>>::scheduled_paras()
.map(|(core_idx, para_id)| (para_id, core_idx))
.collect::<BTreeMap<_, _>>();

// Flag which will be returned. Set to `true` if at least one vote is filtered.
let mut filtered = false;

// Process all backed candidates.
backed_candidates.retain_mut(|bc| {
// Get `core_idx` assigned to the `para_id` of the candidate (step 2)
let core_idx = match para_id_to_core_idx.get(&bc.descriptor().para_id) {
Some(core_idx) => *core_idx,
None => {
log::debug!(target: LOG_TARGET, "Can't get core idx got a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id);
return false
}
};

// Get relay parent block number of the candidate. We need this to get the group index
// assigned to this core at this block number
let relay_parent_block_number = match <shared::Pallet<T>>::allowed_relay_parents()
.acquire_info(bc.descriptor().relay_parent, None) {
Some((_, block_num)) => block_num,
None => {
log::debug!(target: LOG_TARGET, "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", bc.descriptor().relay_parent);
return false
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we're duplicating some checks with verify_backed_candidate here and I am not sure why

I've also noticed verify_backed_candidate is called the second time in process_candidates, cc @eskimor



// Get the group index for the core
let group_idx = <scheduler::Pallet<T>>::group_assigned_to_core(
core_idx,
relay_parent_block_number + One::one(),
)
.unwrap();

// And finally get the validator group for this group index
let validator_group = match <scheduler::Pallet<T>>::group_validators(group_idx) {
Some(validator_group) => validator_group,
None => {
// TODO: this should be an assert?
log::debug!(target: LOG_TARGET, "Can't get the validators from group {:?}. Dropping the candidate.", group_idx);
return false
}
};

// `validator_indices` in `BackedCandidate` are indecies within the validator group. Convert
// them to `ValidatorId`.
let voted_validator_ids = bc
.validator_indices
.iter()
.enumerate()
.filter_map(|(idx, bitval)| match *bitval {
true => validator_group.get(idx), // drop indecies not found in the validator group. This will lead to filtering the vote
false => None,
})
.collect::<Vec<_>>();

{
// `validity_votes` should match `validator_indices`
let mut idx = 0;
bc.validity_votes.retain(|_| {
let voted_validator_index = match voted_validator_ids.get(idx) {
Some(validator_index) => validator_index,
None => {
log::debug!(target: LOG_TARGET, "Can't find the voted validator index {:?} in the validator group. Dropping the vote.", group_idx);
idx += 1;
return false
}
};
idx += 1;

filtered = disabled_validators.contains(voted_validator_index);

// If we are removing a validity vote - modify `validator_indices` too
if filtered {
bc.validator_indices.set(idx, false);
}

!filtered
});
}

// Remove the candidate if all entries are filtered out
!bc.validity_votes.is_empty()
});

Ok(filtered)
}