Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coretime chain] Add high assignment count mitigation to testnets #6022

Merged
merged 10 commits into from
Oct 21, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn transact_hardcoded_weights_are_sane() {

// Create and populate schedule with the worst case assignment on this core.
let mut schedule = Vec::new();
for i in 0..27 {
for i in 0..80 {
schedule.push(ScheduleItem {
mask: CoreMask::void().set(i),
assignment: CoreAssignment::Task(2000 + i),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn transact_hardcoded_weights_are_sane() {

// Create and populate schedule with the worst case assignment on this core.
let mut schedule = Vec::new();
for i in 0..27 {
for i in 0..80 {
schedule.push(ScheduleItem {
mask: CoreMask::void().set(i),
assignment: CoreAssignment::Task(2000 + i),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,36 @@ impl CoretimeInterface for CoretimeAllocator {
end_hint: Option<RCBlockNumberOf<Self>>,
) {
use crate::coretime::CoretimeProviderCalls::AssignCore;

// The relay chain currently only allows `assign_core` to be called with a complete mask
// and only ever with increasing `begin`. The assignments must be truncated to avoid
// dropping that core's assignment completely.

// This shadowing of `assignment` is temporary and can be removed when the relay can accept
// multiple messages to assign a single core.
let assignment = if assignment.len() > 28 {
let mut total_parts = 0u16;
// Account for missing parts with a new `Idle` assignment at the start as
// `assign_core` on the relay assumes this is sorted. We'll add the rest of the
// assignments and sum the parts in one pass, so this is just initialized to 0.
let mut assignment_truncated = vec![(CoreAssignment::Idle, 0)];
// Truncate to first 27 non-idle assignments.
assignment_truncated.extend(
assignment
.into_iter()
.filter(|(a, _)| *a != CoreAssignment::Idle)
.take(27)
.inspect(|(_, parts)| total_parts += *parts)
.collect::<Vec<_>>(),
);

// Set the parts of the `Idle` assignment we injected at the start of the vec above.
assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
assignment_truncated[0].1 = PartsOf57600::FULL.saturating_sub(total_parts);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realised that this is just a u16 in the broker pallet, rather than the type that the relay chain uses. This means we don't have FULL etc here. It will introduce another dependency and require an into anyway. I think longer term we should change the type in the broker pallet, but this will be a breaking change

Copy link
Member

Choose a reason for hiding this comment

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

I think longer term we should change the type in the broker pallet, but this will be a breaking change

In the relay chain it is also a u16, but just a transparent wrapper around u16. While this is a "breaking change" on the code level, it is not changing the storage layout or anything. Thus, changing it can be done very easily.

assignment_truncated
} else {
assignment
};

let assign_core_call =
RelayRuntimePallets::Coretime(AssignCore(core, begin, assignment, end_hint));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,45 @@ impl CoretimeInterface for CoretimeAllocator {
end_hint: Option<RCBlockNumberOf<Self>>,
) {
use crate::coretime::CoretimeProviderCalls::AssignCore;
let assign_core_call =
RelayRuntimePallets::Coretime(AssignCore(core, begin, assignment, end_hint));

// Weight for `assign_core` from westend benchmarks:
// `ref_time` = 10177115 + (1 * 25000000) + (2 * 100000000) + (57600 * 13932) = 937660315
// `proof_size` = 3612
// Add 5% to each component and round to 2 significant figures.
let call_weight = Weight::from_parts(980_000_000, 3800);

// The relay chain currently only allows `assign_core` to be called with a complete mask
// and only ever with increasing `begin`. The assignments must be truncated to avoid
// dropping that core's assignment completely.

// This shadowing of `assignment` is temporary and can be removed when the relay can accept
// multiple messages to assign a single core.
let assignment = if assignment.len() > 28 {
let mut total_parts = 0u16;
// Account for missing parts with a new `Idle` assignment at the start as
// `assign_core` on the relay assumes this is sorted. We'll add the rest of the
// assignments and sum the parts in one pass, so this is just initialized to 0.
let mut assignment_truncated = vec![(CoreAssignment::Idle, 0)];
// Truncate to first 27 non-idle assignments.
assignment_truncated.extend(
assignment
.into_iter()
.filter(|(a, _)| *a != CoreAssignment::Idle)
.take(27)
.inspect(|(_, parts)| total_parts += *parts)
.collect::<Vec<_>>(),
);

// Set the parts of the `Idle` assignment we injected at the start of the vec above.
assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
seadanda marked this conversation as resolved.
Show resolved Hide resolved
assignment_truncated
} else {
assignment
};

let assign_core_call =
RelayRuntimePallets::Coretime(AssignCore(core, begin, assignment, end_hint));

let message = Xcm(vec![
Instruction::UnpaidExecution {
weight_limit: WeightLimit::Unlimited,
Expand Down
14 changes: 14 additions & 0 deletions prdoc/pr_6022.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
title: '[Coretime chain] Add high assignment count mitigation to testnets'
doc:
- audience: Runtime User
description: |
We can handle a maximum of 28 assignments inside one XCM, while it's possible to have 80 (if a
region is interlaced 79 times). This can be chunked on the coretime chain side but currently the
relay does not support this. This PR truncates the additional assignments on Rococo and Westend
to mitigate this until the relay is fixed. The first 27 assignments are taken, the final 28th is
used to pad with idle to complete the mask. Any other assignments are dropped.
crates:
- name: coretime-rococo-runtime
bump: patch
- name: coretime-westend-runtime
bump: patch
Loading