Better Handling of Candidates Who Become Invulnerable#2801
Better Handling of Candidates Who Become Invulnerable#2801paritytech-processbot[bot] merged 19 commits intomasterfrom
Conversation
joepetrowski
added
A0-please_review
|
@gpestana @Ank4n a bit of context here: The end goal of collator selection in asset hub (and possibly other parachains) is using approval stakes tracked in an instance of the bags-list as the election provider. This is being made for the relay chain as well, but in relay chain we use this merely as the pre-sorting step. We might take over this task from SP in the future. |
| origin: OriginFor<T>, | ||
| new: Vec<T::AccountId>, | ||
| ) -> DispatchResultWithPostInfo { | ||
| pub fn set_invulnerables(origin: OriginFor<T>, new: Vec<T::AccountId>) -> DispatchResult { |
There was a problem hiding this comment.
From my PoV, I'd expect set_invulnerables to be atomic, so I lean towards removing the non-atomic group set dispatchable and expose an individual set_invulnerable that can be batched at the extrinsic level. Perhaps the non-atomic set is OK in this case, not sure what are the user's expectations when using this. However, perhaps it would be good to emit an event when at least one candidate was not set as invulnerable (or an event per candidate not set)
There was a problem hiding this comment.
an individual set_invulnerable that can be batched at the extrinsic level
This is pretty much add_invulnerable.
I waivered about the atomicity of this, but I think non-atomic is better (with the recommendation in the docs that add/remove invulnerable is better for many reasons). The reason is that you could set 20 Invulnerables, and one guy either decides not to set his keys or just forgets (probably makes him a bad choice for invulnerable...), but then this ruins the entire call.
There was a problem hiding this comment.
However, perhaps it would be good to emit an event when at least one candidate was not set as invulnerable (or an event per candidate not set)
Done
| } | ||
|
|
||
| // should never fail since `new_with_keys` must be equal to or shorter than `new` | ||
| let mut bounded_invulnerables = |
There was a problem hiding this comment.
slightly more performant to check the length of new as early as possible.
|
|
||
| /// Set the candidacy bond amount. | ||
| /// | ||
| /// The origin for this call must be the `UpdateOrigin`. |
|
bot merge |
ghost
mentioned this pull request
3 participants
Closes #2782
Key points:
MinCandidatesand replaces it withMinEligibleCollators. Rather than enforcing a minimum number ofCandidates(i.e. non-invulnerable collators), it treats all potential collators equally.add_invulnerable, it will remove an account fromCandidatesif it is there.set_invulnerables, it does not remove an account fromCandidates. This is due to likely massive overestimates of weight, since it will be based onMaxCandidates, it loops through each account innew, and there's a good chance that many members are not candidates. Added docs that this call is a bit blunt and{add, remove}_invulnerableis the preferred method to manipulate the Invulnerable set. But just in case, ...Candidateson session change.parameter_types!toConstU32. Also adjusted values to more reasonable numbers.set_invulnerablesnon-atomic. This call previously failed entirely if even one collator innewdid not have session keys registered. This happened e.g. in Kusama referendum 224. The change here just removes a collator fromnewif they are not prepared to be a collator.UpdateOriginis accepting qualified collators as "individuals". On the other hand, it prevents one unprepared collator from ruining the whole batch. An alternative would be to revert this change (177b307) and just recommend always using a batch of{add, remove}_invulnerableto manage the invulnerable collators.Question:
I think there is probably a more graceful way to account for the weight of (maybe) removing accounts that are being set as invulnerable.<- Addressed