Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

min collator check #498

Merged
merged 9 commits into from
Jun 22, 2021
Merged

min collator check #498

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e4c266
min collator check
JesseAbram Jun 17, 2021
9321045
change statemint/mine min candidates
JesseAbram Jun 17, 2021
d4ffcfc
Ci pass
JesseAbram Jun 17, 2021
abc663e
Update pallets/collator-selection/src/lib.rs
JesseAbram Jun 17, 2021
e409ca8
Update pallets/collator-selection/src/lib.rs
JesseAbram Jun 17, 2021
325cf3b
Apply suggestions from code review
shawntabrizi Jun 17, 2021
bfa603a
build fixes
JesseAbram Jun 17, 2021
b4c8405
add error messages to errors
JesseAbram Jun 17, 2021
f3f66b3
added validator register check
JesseAbram Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pallets/collator-selection/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,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.
Copy link
Member

@shawntabrizi shawntabrizi Jun 17, 2021 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Contributor

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

type MinCandidates: Get<usize>;
JesseAbram marked this conversation as resolved.
Show resolved Hide resolved


/// Maximum number of invulnerables.
///
/// Used only for benchmarking.
Expand Down Expand Up @@ -239,6 +245,7 @@ pub mod pallet {
#[pallet::error]
pub enum Error<T> {
TooManyCandidates,
TooFewCandidates,
Unknown,
Permission,
AlreadyCandidate,
JesseAbram marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -323,7 +330,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() > T::MinCandidates::get(), Error::<T>::TooFewCandidates);
JesseAbram marked this conversation as resolved.
Show resolved Hide resolved
let current_count = Self::try_remove_candidate(&who)?;

Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into())
Expand Down Expand Up @@ -365,7 +372,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() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() {
if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() {

Why are you reading candidates here but also getting candidates from the input params of this function?

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 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);
Expand Down
2 changes: 2 additions & 0 deletions pallets/collator-selection/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ parameter_types! {
pub const PotId: PalletId = PalletId(*b"PotStake");
pub const MaxCandidates: u32 = 20;
pub const MaxInvulnerables: u32 = 20;
pub const MinCandidates: usize = 1;
}

impl Config for Test {
Expand All @@ -196,6 +197,7 @@ impl Config for Test {
type UpdateOrigin = EnsureSignedBy<RootAccount, u64>;
type PotId = PotId;
type MaxCandidates = MaxCandidates;
type MinCandidates = MinCandidates;
type MaxInvulnerables = MaxInvulnerables;
type KickThreshold = Period;
type WeightInfo = ();
Expand Down
47 changes: 47 additions & 0 deletions pallets/collator-selection/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,21 @@ fn cannot_register_candidate_if_too_many() {
})
}

#[test]
fn cannot_unregister_candidate_if_too_few() {
new_test_ext().execute_with(|| {
// reset desired candidates:
<crate::DesiredCandidates<Test>>::put(1);
assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(4)));

// can not remove too few
assert_noop!(
CollatorSelection::leave_intent(Origin::signed(4)),
Error::<Test>::TooFewCandidates,
);
})
}

#[test]
fn cannot_register_as_candidate_if_invulnerable() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -184,6 +199,10 @@ fn leave_intent() {
assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3)));
assert_eq!(Balances::free_balance(3), 90);

// register too so can leave above min candidates
assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(5)));
assert_eq!(Balances::free_balance(5), 90);

// cannot leave if not candidate.
assert_noop!(
CollatorSelection::leave_intent(Origin::signed(4)),
Expand Down Expand Up @@ -318,6 +337,34 @@ fn kick_mechanism() {
});
}

#[test]
fn should_not_kick_mechanism_too_few() {
new_test_ext().execute_with(|| {
// add a new collator
assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(5)));
initialize_to_block(10);
assert_eq!(CollatorSelection::candidates().len(), 2);
initialize_to_block(20);
assert_eq!(SessionChangeBlock::get(), 20);
// 4 authored this block, 5 gets to stay too few 3 was kicked
assert_eq!(CollatorSelection::candidates().len(), 1);
// 3 will be kicked after 1 session delay
assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 5]);
let collator = CandidateInfo {
who: 5,
deposit: 10,
};
assert_eq!(CollatorSelection::candidates(), vec![collator]);
assert_eq!(CollatorSelection::last_authored_block(4), 20);
initialize_to_block(30);
// 3 gets kicked after 1 session delay
assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5]);
// kicked collator gets funds back
assert_eq!(Balances::free_balance(3), 100);
});
}


#[test]
#[should_panic = "duplicate invulnerables in genesis."]
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/statemine-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,7 @@ impl pallet_aura::Config for Runtime {
parameter_types! {
pub const PotId: PalletId = PalletId(*b"PotStake");
pub const MaxCandidates: u32 = 1000;
pub const MinCandidates: usize = 1;
JesseAbram marked this conversation as resolved.
Show resolved Hide resolved
pub const SessionLength: BlockNumber = 6 * HOURS;
pub const MaxInvulnerables: u32 = 100;
}
Expand All @@ -655,6 +656,7 @@ impl pallet_collator_selection::Config for Runtime {
type UpdateOrigin = CollatorSelectionUpdateOrigin;
type PotId = PotId;
type MaxCandidates = MaxCandidates;
type MinCandidates = MinCandidates;
type MaxInvulnerables = MaxInvulnerables;
// should be a multiple of session or things will get inconsistent
type KickThreshold = Period;
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/statemint-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ impl pallet_aura::Config for Runtime {
parameter_types! {
pub const PotId: PalletId = PalletId(*b"PotStake");
pub const MaxCandidates: u32 = 1000;
pub const MinCandidates: usize = 1;
pub const SessionLength: BlockNumber = 6 * HOURS;
pub const MaxInvulnerables: u32 = 100;
}
Expand All @@ -596,6 +597,7 @@ impl pallet_collator_selection::Config for Runtime {
type UpdateOrigin = CollatorSelectionUpdateOrigin;
type PotId = PotId;
type MaxCandidates = MaxCandidates;
type MinCandidates = MinCandidates;
type MaxInvulnerables = MaxInvulnerables;
// should be a multiple of session or things will get inconsistent
type KickThreshold = Period;
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/westmint-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,7 @@ impl pallet_aura::Config for Runtime {
parameter_types! {
pub const PotId: PalletId = PalletId(*b"PotStake");
pub const MaxCandidates: u32 = 1000;
pub const MinCandidates: usize = 1;
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
pub const SessionLength: BlockNumber = 6 * HOURS;
pub const MaxInvulnerables: u32 = 100;
}
Expand All @@ -586,6 +587,7 @@ impl pallet_collator_selection::Config for Runtime {
type UpdateOrigin = EnsureRoot<AccountId>;
type PotId = PotId;
type MaxCandidates = MaxCandidates;
type MinCandidates = MinCandidates;
type MaxInvulnerables = MaxInvulnerables;
// should be a multiple of session or things will get inconsistent
type KickThreshold = Period;
Expand Down