-
Notifications
You must be signed in to change notification settings - Fork 365
min collator check #498
min collator check #498
Changes from 6 commits
8e4c266
9321045
d4ffcfc
abc663e
e409ca8
325cf3b
bfa603a
b4c8405
f3f66b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,9 @@ | |||||
| //! The current implementation resolves congestion of [`Candidates`] in a first-come-first-serve | ||||||
| //! manner. | ||||||
| //! | ||||||
| //! Candidates will not be allowed to get kicked or leave_intent if the total number of candidates | ||||||
| //! fall below MinCandidates. This is for potential disaster recovery scenarios. | ||||||
| //! | ||||||
| //! ### Rewards | ||||||
| //! | ||||||
| //! The Collator Selection pallet maintains an on-chain account (the "Pot"). In each block, the | ||||||
|
|
@@ -127,6 +130,12 @@ pub mod pallet { | |||||
| /// This does not take into account the invulnerables. | ||||||
| type MaxCandidates: Get<u32>; | ||||||
|
|
||||||
| /// Minimum number of candidates that we should have. This is used for disaster recovery. | ||||||
| /// | ||||||
| /// This does not take into account the invulnerables. | ||||||
| type MinCandidates: Get<u32>; | ||||||
|
|
||||||
|
|
||||||
| /// Maximum number of invulnerables. | ||||||
| /// | ||||||
| /// Used only for benchmarking. | ||||||
|
|
@@ -239,6 +248,7 @@ pub mod pallet { | |||||
| #[pallet::error] | ||||||
| pub enum Error<T> { | ||||||
| TooManyCandidates, | ||||||
| TooFewCandidates, | ||||||
| Unknown, | ||||||
| Permission, | ||||||
| AlreadyCandidate, | ||||||
|
JesseAbram marked this conversation as resolved.
|
||||||
|
|
@@ -323,7 +333,7 @@ pub mod pallet { | |||||
| #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] | ||||||
| pub fn leave_intent(origin: OriginFor<T>) -> DispatchResultWithPostInfo { | ||||||
| let who = ensure_signed(origin)?; | ||||||
|
|
||||||
| ensure!(Self::candidates().len() as u32 > T::MinCandidates::get(), Error::<T>::TooFewCandidates); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this logic be in Else it may not be checked everywhere that this is happening correctly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had originally looked at that, but try_remove_candidates is used in kick_candidates and that would track two items, the candidates array and the current candidates to be accumulated as collators. So if one failed while the other passed they would not track each other correctly. So it was either rewrite the function, or add this here, this was less code and I don't see how this would not cause it to track properly. |
||||||
| let current_count = Self::try_remove_candidate(&who)?; | ||||||
|
|
||||||
| Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) | ||||||
|
|
@@ -365,7 +375,7 @@ pub mod pallet { | |||||
| let new_candidates = candidates.into_iter().filter_map(|c| { | ||||||
| let last_block = <LastAuthoredBlock<T>>::get(c.who.clone()); | ||||||
| let since_last = now.saturating_sub(last_block); | ||||||
| if since_last < kick_threshold { | ||||||
| if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why are you reading candidates here but also getting candidates from the input params of this function?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the candidates input does not get changed, it gets at the end collected into new_candidates, so if 5 people get removed, it does not know that, however the global candidates is tracking this, so it would for each candidate have the current knowledge of how many are left after the previous have been kicked |
||||||
| Some(c.who) | ||||||
| } else { | ||||||
| let outcome = Self::try_remove_candidate(&c.who); | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are saying this count is the number of people required not including any existing invulnerables?
I think that is over doing it. Invulnerables existing should satisfy the requirement for a minimum number of collators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in theory but invulnerables can be removed all at once (through a permissioned call) I was thinking this would be more of a assume invulnerables would be removed soon thing would be an easier transition, but open to changing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invulnerables should eventually be removed and by then minimum number of collators will be super useful