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

Allow rotating a subset of collators instead of all of them #770

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

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Nov 28, 2024

Adds new field to pallet_configuration: full_rotation_mode. Allows to specify how to perform the rotation, allowing a different value for each kind of chain (orchestrator, parachain, parathread):

  • Default: RotateAll, will rotate all collators (as before this PR)
  • KeepAll (same as disabling rotation, but now can be set only for parathreads or only for orchestrator for example)
  • KeepCollators: will keep N collators assigned on every rotation. If the collators are still eligible, this guarantees there will be N collators that don't rotate.
  • KeepPerbill: same but allows to easily set it to 50%, allowing to change the number of collators per chain without having to also update this value. This refers to the % respect to the max number of collators, so if max_orchestrator = 5 but there are only 2 collators assigned to the orchestrator chain, and we try to keep 50%, then it will try to keep 2.5 collators, so all of them.

Contains migration for configuration pallet. I had to fix the migration because it used a hardcoded key that depends on pallet name, thus failing in dancelight.

Copy link
Contributor

github-actions bot commented Nov 28, 2024

WASM runtime size check:

Compared to target branch

dancebox runtime: 1416 KB (no changes) ✅

flashbox runtime: 824 KB (no changes) ✅

dancelight runtime: 2144 KB (no changes) ✅

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

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

@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Nov 28, 2024
Copy link
Contributor

github-actions bot commented Nov 28, 2024

Coverage Report

(master)

@@                   Coverage Diff                    @@
##           master   tomasz-rotate-subset      +/-   ##
========================================================
+ Coverage   65.15%                 65.27%   +0.12%     
+ Files         327                    329       +2     
+ Lines       57299                  57620     +321     
========================================================
+ Hits        37330                  37608     +278     
+ Misses      19969                  20012      +43     
Files Changed Coverage
/node/src/chain_spec/dancebox.rs 94.92% (+0.04%)
/node/src/chain_spec/flashbox.rs 70.66% (-0.16%)
/node/src/rpc.rs 96.43% (-1.44%)
/node/src/service.rs 15.54% (+2.93%)
/pallets/collator-assignment/src/assignment.rs 99.33% (+0.04%)
/pallets/collator-assignment/src/lib.rs 98.69% (+1.04%)
/pallets/configuration/src/lib.rs 86.43% (+2.78%)
/primitives/traits/src/lib.rs 62.94% (+1.34%)
/runtime/common/src/migrations.rs 86.23% (-4.09%)
/runtime/dancebox/src/tests/integration_test.rs 99.70% (-0.01%)

Coverage generated Fri Dec 20 14:55:15 UTC 2024

Copy link
Contributor

@chexware chexware left a comment

Choose a reason for hiding this comment

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

Besides the unresolved conversations all looks good to me

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

in general I think it looks good, just a few nitpicks from open conversations!

@tmpolaczyk tmpolaczyk requested a review from girazoki December 20, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants