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

Jump to bottom

Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook #692

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook #692

wants to merge 3 commits into from

Conversation

ParthDesai
Copy link
Contributor

@ParthDesai ParthDesai commented Sep 25, 2024 •
edited
Loading

Description

Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook in one trait with two methods.

Both method executes same logic. Only difference being pre_assignment_* method always rollback while post_assignment_* only rollback in case it errors out.

Note

Naming and tests are still remaining. Just want to get some early feedback on overall design.

@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch from 611e197 to ac88d40 Compare September 25, 2024 08:53
@ParthDesai ParthDesai changed the title [WIP] Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook Sep 25, 2024
@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch from ac88d40 to b01541a Compare September 25, 2024 09:07
@ParthDesai ParthDesai changed the title Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook [WIP] Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook Sep 25, 2024
@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch from b01541a to b655d6b Compare September 25, 2024 09:56
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 25, 2024 •
edited
Loading

Coverage Report

(master)

@@                                   Coverage Diff                                   @@
##           master   combine-remove-paraids-and-collator-assignment-hook      +/-   ##
=======================================================================================
+ Coverage   65.47%                                                65.50%   +0.03%     
  Files         312                                                   312              
+ Lines       54503                                                 54665     +162     
=======================================================================================
+ Hits        35685                                                 35808     +123     
+ Misses      18818                                                 18857      +39
Files Changed Coverage
/pallets/collator-assignment/src/lib.rs 97.65% (+1.27%) ✅
/pallets/services-payment/src/lib.rs 89.94% (-0.33%) ❗
/primitives/traits/src/lib.rs 64.00% (-1.57%) ❗
/runtime/dancebox/src/lib.rs 87.98% (+0.21%) ✅
/runtime/dancebox/src/weights/pallet_services_payment.rs 73.91% (-13.05%) ❗
/runtime/flashbox/src/lib.rs 46.80% (+2.79%) ✅
/runtime/flashbox/src/weights/pallet_services_payment.rs 13.04% (-13.05%) ❗
/solo-chains/runtime/dancelight/src/lib.rs 68.80% (+0.91%) ✅
/solo-chains/runtime/dancelight/src/weights/pallet_services_payment.rs 13.43% (-13.44%) ❗

Coverage generated Mon Nov 25 12:39:50 UTC 2024

@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch 2 times, most recently from 916be99 to d0ad35f Compare September 30, 2024 14:46
girazoki
girazoki reviewed Oct 2, 2024
View reviewed changes
solo-chains/runtime/dancelight/src/lib.rs
fn remove_para_ids_with_no_credits(
para_ids: &mut Vec<ParaId>,
impl RemoveParaIdsWithNoCreditsImpl {
fn charge_para_ids_internal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is becoming hard to read, I would probably split it into different subfunctions that are public in the services payment pallet so that all this code is not in the runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also the naming of the function is not ideal, I would call it something like charge_services_for_para_id

girazoki
girazoki reviewed Oct 2, 2024
View reviewed changes
solo-chains/runtime/dancelight/src/lib.rs Outdated

let remaining_to_pay = remaining_block_credits_to_pay.saturating_add(remaining_session_credits_to_pay).saturating_add(max_tip);
impl<AC> RemoveParaIdsWithNoCredits<BalanceOf<Runtime>, AC> for RemoveParaIdsWithNoCreditsImpl {
fn pre_assignment_remove_para_ids_with_no_credits(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I would rename pre_assignment_remove_para_ids_with_no_credits to simply pre_assignment_hook or pre_assignment_filtering

girazoki
girazoki reviewed Oct 2, 2024
View reviewed changes
solo-chains/runtime/dancelight/src/lib.rs Outdated
// This should take into account whether we tank goes below ED
// The true refers to keepAlive
Balances::can_withdraw(&pallet_services_payment::Pallet::<Runtime>::parachain_tank(*para_id), remaining_to_pay).into_result(true).is_ok()
fn post_assignment_remove_para_ids_with_no_credits(
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here, the naming is very specific to what this is doing. I would call it something like post_assignmnet_hook

girazoki
girazoki reviewed Oct 2, 2024
View reviewed changes
runtime/dancebox/src/lib.rs
if collators.is_empty() {
return true;
}
with_storage_layer(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is with_storage_layer¿?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, with_storage_layer is similar to with_transaction but it automatically commits the storage changes into parent overlay if the closure returns Ok and do the rollback otherwise.

Parity added this function since it is more convenient to use. (i.e you don't have to manually return Ok or Err for commit and rollback respectively.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any performance concerns when running that function inside a loop? I guess not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fairly low cost. See here: paritytech/substrate#10809 (comment)

@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch from d0ad35f to 7ccaa48 Compare October 3, 2024 07:45
tmpolaczyk
tmpolaczyk reviewed Oct 4, 2024
View reviewed changes
runtime/dancebox/src/lib.rs Outdated
// This should take into account whether we tank goes below ED
// The true refers to keepAlive
Balances::can_withdraw(&pallet_services_payment::Pallet::<Runtime>::parachain_tank(*para_id), remaining_to_pay).into_result(true).is_ok()
fn post_assignment_remove_para_ids_with_no_credits(
Copy link
Contributor

Choose a reason for hiding this comment

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

So can we remove this method? pre_assignment already uses with_transaction to remove para_ids that would have failed if Self::charge_para_ids_internal returns Err, so if I'm not wrong, calling post_assignment will never remove any additional para id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In perfect world yes. But, since we are changing state in between calling pre and post we need to run it again to eliminate even a tiniest possibility that some para_id escapes the restrictions.

@ParthDesai ParthDesai force-pushed the combine-remove-paraids-and-collator-assignment-hook branch from 7ccaa48 to b47f66e Compare November 21, 2024 10:56
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

WASM runtime size check:

Compared to target branch

dancebox runtime: 1416 KB (no changes) ✅

flashbox runtime: 828 KB (no changes) ✅

dancelight runtime: 2012 KB (no changes) ✅

container chain template simple runtime: 1088 KB (no changes) ✅

container chain template frontier runtime: 1388 KB (no changes) ✅

@ParthDesai ParthDesai added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Nov 21, 2024
@ParthDesai ParthDesai marked this pull request as ready for review November 21, 2024 14:31
@ParthDesai ParthDesai changed the title [WIP] Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmpolaczyk tmpolaczyk tmpolaczyk left review comments

@girazoki girazoki girazoki left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ParthDesai @girazoki @tmpolaczyk