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

collator protocol changes for elastic scaling (validator side) #3302

Merged
merged 14 commits into from
Mar 15, 2024
Merged
4 changes: 3 additions & 1 deletion cumulus/client/collator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ pub mod relay_chain_driven {
this_rx.await.ok().flatten()
})
})),
with_elastic_scaling: false,
};

overseer_handle
Expand All @@ -243,8 +244,9 @@ pub async fn initialize_collator_subsystems(
key: CollatorPair,
para_id: ParaId,
reinitialize: bool,
with_elastic_scaling: bool,
) {
let config = CollationGenerationConfig { key, para_id, collator: None };
let config = CollationGenerationConfig { key, para_id, collator: None, with_elastic_scaling };

if reinitialize {
overseer_handle
Expand Down
4 changes: 4 additions & 0 deletions cumulus/client/consensus/aura/src/collators/lookahead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ pub struct Params<BI, CIDP, Client, Backend, RClient, CHP, SO, Proposer, CS> {
pub authoring_duration: Duration,
/// Whether we should reinitialize the collator config (i.e. we are transitioning to aura).
pub reinitialize: bool,
/// Whether elastic scaling is enabled for this collation.
/// If it is, the collator will send the parent-head data along with the collation.
pub with_elastic_scaling: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Can we please get rid of this with_elastic_scaling everywhere in this pr? This is just "dirty". The collation generation logic internally can just decide based on the fact if the head is passed or not if it wants to use the new message.

From all the collator implementations we want to set the head to None for now and then later do this properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, pushed 94ba49b

The collation generation logic internally can just decide based on the fact if the head is passed or not if it wants to use the new message

could you elaborate on this? passing the head from where?
i mean we could also detect elastic scaling by checking all the assignments for the para_id in collation-generation (or somewhere else)
we could also set to Some unconditionally (which seems wasteful)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

And then give maybe_parent_head better docs to explain on when to set this.

Copy link
Member Author

@ordian ordian Feb 17, 2024

Choose a reason for hiding this comment

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

currently, in this PR, maybe_parent_head_data is being set by collation-generation - it's hardcoded to None: https://github.com/paritytech/polkadot-sdk/pull/3302/files#diff-d7298fb050b10e14df59080885ebee73861d85b2c4722cc00089fc91a703d63eR543. I've tried to make it configurable with with_elastic_scaling param, but you seem to think it's a hack. So my question is, do you have a suggestion on how it should be configured properly?

Copy link
Member Author

@ordian ordian Feb 18, 2024

Choose a reason for hiding this comment

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

I'll make maybe_parent_head_data part of

pub struct Collation<BlockNumber = polkadot_primitives::BlockNumber> {
struct produced as part of CollatorFn if that the right way.

Copy link
Member

Choose a reason for hiding this comment

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

I'll make maybe_parent_head_data part of

Maybe we don't need this, because CollatorFn is sort of deprecated I would say. I mean the lookahead collator isn't using this anymore and any feature work will also not use CollatorFn anymore. SubmitCollationParams is the struct that should be updated to take maybe_parent_head_data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I think I misunderstood you a bit and after chatting with @skunert, settled on approach where collators don't configure anything and elastic scaling is detected automatically (not done in this PR).
Updated the PR and description. PTAL 🙏

}

/// Run async-backing-friendly Aura.
Expand Down Expand Up @@ -152,6 +155,7 @@ where
params.collator_key,
params.para_id,
params.reinitialize,
params.with_elastic_scaling,
)
.await;

Expand Down
5 changes: 5 additions & 0 deletions cumulus/polkadot-parachain/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ pub async fn start_rococo_parachain_node(
collator_service,
authoring_duration: Duration::from_millis(1500),
reinitialize: false,
with_elastic_scaling: false,
};

let fut = aura::run::<
Expand Down Expand Up @@ -1473,6 +1474,7 @@ where
collator_service,
authoring_duration: Duration::from_millis(1500),
reinitialize: false,
with_elastic_scaling: false,
};

let fut =
Expand Down Expand Up @@ -1768,6 +1770,7 @@ where
authoring_duration: Duration::from_millis(1500),
reinitialize: true, /* we need to always re-initialize for asset-hub moving
* to aura */
with_elastic_scaling: false,
};

aura::run::<Block, <AuraId as AppCrypto>::Pair, _, _, _, _, _, _, _, _, _>(params)
Expand Down Expand Up @@ -1870,6 +1873,7 @@ where
collator_service,
authoring_duration: Duration::from_millis(1500),
reinitialize: false,
with_elastic_scaling: false,
};

let fut =
Expand Down Expand Up @@ -2180,6 +2184,7 @@ pub async fn start_contracts_rococo_node(
// Very limited proposal time.
authoring_duration: Duration::from_millis(1500),
reinitialize: false,
with_elastic_scaling: false,
};

let fut = aura::run::<
Expand Down
18 changes: 15 additions & 3 deletions polkadot/node/collation-generation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,8 @@ async fn handle_new_activations<Context>(
},
};

let with_elastic_scaling = task_config.with_elastic_scaling;

construct_and_distribute_receipt(
PreparedCollation {
collation,
Expand All @@ -358,6 +360,7 @@ async fn handle_new_activations<Context>(
validation_data,
validation_code_hash,
n_validators,
with_elastic_scaling,
},
task_config.key.clone(),
&mut task_sender,
Expand Down Expand Up @@ -392,6 +395,7 @@ async fn handle_submit_collation<Context>(

let validators = request_validators(relay_parent, ctx.sender()).await.await??;
let n_validators = validators.len();
let with_elastic_scaling = config.with_elastic_scaling;

// We need to swap the parent-head data, but all other fields here will be correct.
let mut validation_data = match request_persisted_validation_data(
Expand Down Expand Up @@ -424,6 +428,7 @@ async fn handle_submit_collation<Context>(
validation_data,
validation_code_hash,
n_validators,
with_elastic_scaling,
};

construct_and_distribute_receipt(
Expand All @@ -445,6 +450,7 @@ struct PreparedCollation {
validation_data: PersistedValidationData,
validation_code_hash: ValidationCodeHash,
n_validators: usize,
with_elastic_scaling: bool,
}

/// Takes a prepared collation, along with its context, and produces a candidate receipt
Expand All @@ -463,6 +469,7 @@ async fn construct_and_distribute_receipt(
validation_data,
validation_code_hash,
n_validators,
with_elastic_scaling,
} = collation;

let persisted_validation_data_hash = validation_data.hash();
Expand Down Expand Up @@ -540,23 +547,28 @@ async fn construct_and_distribute_receipt(
},
};

let maybe_parent_head_data =
if with_elastic_scaling { Some(commitments.head_data.clone()) } else { None };

gum::debug!(
target: LOG_TARGET,
candidate_hash = ?ccr.hash(),
?pov_hash,
?relay_parent,
para_id = %para_id,
?with_elastic_scaling,
"candidate is generated",
);
metrics.on_collation_generated();

sender
.send_message(CollatorProtocolMessage::DistributeCollation(
ccr,
.send_message(CollatorProtocolMessage::DistributeCollation {
candidate_receipt: ccr,
parent_head_data_hash,
pov,
maybe_parent_head_data,
result_sender,
))
})
.await;
}

Expand Down
35 changes: 19 additions & 16 deletions polkadot/node/collation-generation/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ fn test_config<Id: Into<ParaId>>(para_id: Id) -> CollationGenerationConfig {
key: CollatorPair::generate().0,
collator: Some(Box::new(|_: Hash, _vd: &PersistedValidationData| TestCollator.boxed())),
para_id: para_id.into(),
with_elastic_scaling: false,
}
}

Expand All @@ -125,6 +126,7 @@ fn test_config_no_collator<Id: Into<ParaId>>(para_id: Id) -> CollationGeneration
key: CollatorPair::generate().0,
collator: None,
para_id: para_id.into(),
with_elastic_scaling: false,
}
}

Expand Down Expand Up @@ -390,11 +392,11 @@ fn sends_distribute_collation_message() {

assert_eq!(to_collator_protocol.len(), 1);
match AllMessages::from(to_collator_protocol.pop().unwrap()) {
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation(
CandidateReceipt { descriptor, .. },
_pov,
..,
)) => {
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation {
candidate_receipt,
..
}) => {
let CandidateReceipt { descriptor, .. } = candidate_receipt;
// signature generation is non-deterministic, so we can't just assert that the
// expected descriptor is correct. What we can do is validate that the produced
// descriptor has a valid signature, then just copy in the generated signature
Expand Down Expand Up @@ -529,11 +531,11 @@ fn fallback_when_no_validation_code_hash_api() {

assert_eq!(to_collator_protocol.len(), 1);
match &to_collator_protocol[0] {
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation(
CandidateReceipt { descriptor, .. },
_pov,
..,
)) => {
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation {
candidate_receipt,
..
}) => {
let CandidateReceipt { descriptor, .. } = candidate_receipt;
assert_eq!(expect_validation_code_hash, descriptor.validation_code_hash);
},
_ => panic!("received wrong message type"),
Expand Down Expand Up @@ -619,15 +621,16 @@ fn submit_collation_leads_to_distribution() {

assert_matches!(
overseer_recv(&mut virtual_overseer).await,
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation(
ccr,
AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation {
candidate_receipt,
parent_head_data_hash,
..
)) => {
}) => {
let CandidateReceipt { descriptor, .. } = candidate_receipt;
assert_eq!(parent_head_data_hash, parent_head.hash());
assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash());
assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash());
assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash);
assert_eq!(descriptor.persisted_validation_data_hash, expected_pvd.hash());
assert_eq!(descriptor.para_head, dummy_head_data().hash());
assert_eq!(descriptor.validation_code_hash, validation_code_hash);
}
);

Expand Down
5 changes: 3 additions & 2 deletions polkadot/node/core/prospective-parachains/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -771,8 +771,9 @@ fn answer_prospective_validation_data_request(
Some(s) => s,
};

let mut head_data =
storage.head_data_by_hash(&request.parent_head_data_hash).map(|x| x.clone());
let mut head_data = request
.maybe_parent_head_data
.or_else(|| storage.head_data_by_hash(&request.parent_head_data_hash).map(|x| x.clone()));
let mut relay_parent_info = None;
let mut max_pov_size = None;

Expand Down
1 change: 1 addition & 0 deletions polkadot/node/core/prospective-parachains/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ async fn get_pvd(
para_id,
candidate_relay_parent,
parent_head_data_hash: parent_head_data.hash(),
maybe_parent_head_data: None,
};
let (tx, rx) = oneshot::channel();
virtual_overseer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use polkadot_node_network_protocol::{
PeerId,
};
use polkadot_node_primitives::PoV;
use polkadot_primitives::{CandidateHash, CandidateReceipt, Hash, Id as ParaId};
use polkadot_primitives::{CandidateHash, CandidateReceipt, Hash, HeadData, Id as ParaId};

/// The status of a collation as seen from the collator.
pub enum CollationStatus {
Expand Down Expand Up @@ -63,6 +63,8 @@ pub struct Collation {
pub parent_head_data_hash: Hash,
/// Proof to verify the state transition of the parachain.
pub pov: PoV,
/// Optional parent head-data needed for elastic scaling.
pub maybe_parent_head_data: Option<HeadData>,
/// Collation status.
pub status: CollationStatus,
}
Expand Down
Loading
Loading