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.

Sign up for GitHub

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

[WIP] collation-generation: resolve mismatch between descriptor and commitments core index #7104

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] collation-generation: resolve mismatch between descriptor and commitments core index #7104

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 43 additions & 2 deletions polkadot/node/collation-generation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,8 @@ impl CollationGenerationSubsystem {
Box::pin(async move {
let transposed_claim_queue = transpose_claim_queue(claim_queue.0);

for core_index in cores_to_build_on {
let mut prev_core_selector_index = cores_to_build_on.len();
for core_index in &cores_to_build_on {
let collator_fn = match task_config.collator.as_ref() {
Some(x) => x,
None => return,
Expand All @@ -363,6 +364,46 @@ impl CollationGenerationSubsystem {
},
};

// Determine the core index for the descriptor.

// Use the core_selector method from CandidateCommitments to extract
// CoreSelector.
let mut commitments = CandidateCommitments::default();
commitments.upward_messages = collation.upward_messages.clone();

let descriptor_core_index = match commitments.core_selector() {
// If a valid CoreSelector is found, calculate the core index based on it.
Ok(Some((core_selector, _cq_offset))) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should take into account the claim queue offset, if we do have a core selector

let core_selector_index =
core_selector.0 as usize % cores_to_build_on.len();

if cores_to_build_on.len() > 1 &&
prev_core_selector_index == core_selector_index
Copy link
Contributor

Choose a reason for hiding this comment

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

we're only checking if the previous core selector is the same, but what if the core selectors look like 1,2,1?
In this case we should stop after building for 1 and 2

{
// Malicious behavior detected: The parachain is assigned to
// multiple cores but repeatedly selects the same core. This is
// unexpected and the process should stop here.
gum::error!(target: LOG_TARGET, "Parachain repeatedly selected the same core index");
Comment on lines +383 to +386
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not malicious behavior, could be legitimately only building for one core (without elastic scaling). I would also demote this log to info level

return;
}

prev_core_selector_index = core_selector_index;

let Some(commitments_core_index) =
cores_to_build_on.get(core_selector_index)
else {
// This should not happen, as the core_selector index is modulo the
// length of cores_to_build_on and we ensure that cores_to_build_on
// is not empty.
continue;
};

*commitments_core_index
},
// Fallback to the sequential core_index if no valid CoreSelector is found.
_ => *core_index,
};

let parent_head = collation.head_data.clone();
if let Err(err) = construct_and_distribute_receipt(
PreparedCollation {
Expand All @@ -372,7 +413,7 @@ impl CollationGenerationSubsystem {
validation_data: validation_data.clone(),
validation_code_hash,
n_validators,
core_index,
core_index: descriptor_core_index,
session_index,
},
task_config.key.clone(),
Expand Down
Loading