statement-distribution: fix filtering of statements for elastic parachains #3879
Conversation
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
sandreim
added
the
R0-silent
|
The CI pipeline was cancelled due to failure one of the required jobs. |
…reim/elastic_statement_dist
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
…reim/elastic_statement_dist Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
polkadot/node/network/statement-distribution/src/v2/tests/cluster.rs
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
I'll craft a new PR with the test. |
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
…reim/elastic_statement_dist
Signed-off-by: Andrei Sandu <[email protected]>
| claim_queue | ||
| .keys() | ||
| .filter_map(|core_index| { | ||
| let Some(scheduled_core) = fetch_next_scheduled_on_core(&claim_queue, *core_index) |
There was a problem hiding this comment.
Ok, first (not introduced here): I find the name of this function vastly misleading as it does not fetch anything.
Second, why are we iterating only keys of the claim queue to fetch the actual element for each key, instead of iterating key/values to begin with?
There was a problem hiding this comment.
I wanted to reuse the fn, but you are right, it is actually useless in this case. A wrapper for this BTreeMap with a method like iter_claims(depth: BlockNumber) returning an iterator over (core_index, para) so we can lookup as far into the future as the lookahead param/ async backing settings allow.
| @@ -693,15 +695,24 @@ pub(crate) async fn handle_active_leaves_update<Context>( | |||
| } | |||
| }); | |||
|
|
|||
| let maybe_claim_queue = fetch_claim_queue(ctx.sender(), new_relay_parent) | |||
There was a problem hiding this comment.
Given that we pass per value (can't re-use the result), might make sense to move that call into determine_groups_per_para.
The result of request_availability_cores is used in find_active_validator_state, but wrongly. It uses para_id a function we should remove, as it makes no sense to either return the ParaId of the occupying para or the scheduled one.*)
Regardless, I would move the fetching code into the function that need the data. handle_active_leaves_update is already pages of code.
*) Note: This is obviously unrelated, so can be a separate PR, but it should be fixed for Coretime asap. @tdimitrov
| @@ -312,6 +312,66 @@ fn useful_cluster_statement_from_non_cluster_peer_rejected() { | |||
| }); | |||
| } | |||
|
|
|||
| // Both validators in the test are part of backing groups assigned to same parachain | |||
| #[test] | |||
| fn elastic_scaling_useful_cluster_statement_from_non_cluster_peer_rejected() { | |||
|
Looks good, nevertheless had to leave a few nits. |
Signed-off-by: Andrei Sandu <[email protected]>
| max_candidate_depth: usize, | ||
| ) -> HashMap<ParaId, Vec<GroupIndex>> { | ||
| let maybe_claim_queue = fetch_claim_queue(sender, relay_parent) |
There was a problem hiding this comment.
Ok, my thinking was to move all fetches here, including availability_cores for consistency. It's weird if we pass in one thing, but fetch the other our selves. I know this is used in one other place as well (wrongly), but given that we pass per value already, there is not much gain in avoiding the double fetch.
There was a problem hiding this comment.
LocalValidatorState is built outside this fn and requires availability_cores
| .iter() | ||
| .any(|g| g == &manifest_summary.claimed_group_index) | ||
| { | ||
| if !expected_groups.iter().any(|g| g == &manifest_summary.claimed_group_index) { |
On top of #3879 I've also moved the previous test where we ensure multiple cores per para doesn't break non elastic parachains. --------- Signed-off-by: Andrei Sandu <[email protected]> Co-authored-by: Javier Viola <[email protected]>
…hains (#3879) fixes #3775 Additionally moves the claim queue fetch utilities into `subsystem-util`. TODO: - [x] fix tests - [x] add elastic scaling tests --------- Signed-off-by: Andrei Sandu <[email protected]>
On top of #3879 I've also moved the previous test where we ensure multiple cores per para doesn't break non elastic parachains. --------- Signed-off-by: Andrei Sandu <[email protected]> Co-authored-by: Javier Viola <[email protected]>
…hains (paritytech#3879) fixes paritytech#3775 Additionally moves the claim queue fetch utilities into `subsystem-util`. TODO: - [x] fix tests - [x] add elastic scaling tests --------- Signed-off-by: Andrei Sandu <[email protected]>
On top of paritytech#3879 I've also moved the previous test where we ensure multiple cores per para doesn't break non elastic parachains. --------- Signed-off-by: Andrei Sandu <[email protected]> Co-authored-by: Javier Viola <[email protected]>
fixes #3775
Additionally moves the claim queue fetch utilities into
subsystem-util.TODO: