-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feat/1205528919110616 slashing liveness fault #323
base: dev
Are you sure you want to change the base?
Changes from all commits
65157e0
5392a56
5f19c09
b808709
68ff1e3
7228c77
102753f
4abf945
6b5427b
6471650
c251d02
fb478bc
eedc4e2
ae7c784
fce5808
ca5c952
81d5d4f
1ed0367
7f301e3
7e0b410
c30e3f5
72c9145
39fddaa
b09b3db
32b56d7
69710ce
fdb0fda
1f580f3
ec1c859
25fb45b
29b2ed7
cd00420
17c7963
688c5f5
e313236
f98a2f6
abb8596
4987d7d
47eb117
5213294
ebdefc0
99dd4a5
7621acd
9b68d4a
68d8112
c140150
ad2e928
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 |
---|---|---|
|
@@ -519,6 +519,15 @@ pub mod pallet { | |
/// The commission for a collator has been changed. | ||
/// \[collator's account, new commission\] | ||
CollatorCommissionChanged(T::AccountId, Permill), | ||
/// A collator have been slashed | ||
/// \[collator's account, amount slashed\] | ||
CollatorSlashed(T::AccountId, BalanceOf<T>), | ||
/// Slashing has been enabled/disabled | ||
/// \[new slashing status\] | ||
SlashingEnabledChanged(bool), | ||
/// A collator was kicked out of the candidate pool because of malicious behavior | ||
/// \[collator's account] | ||
CollatorKicked(T::AccountId), | ||
} | ||
|
||
#[pallet::hooks] | ||
|
@@ -534,7 +543,13 @@ pub mod pallet { | |
crate::migrations::on_runtime_upgrade::<T>() | ||
} | ||
|
||
fn on_finalize(_n: BlockNumberFor<T>) { | ||
fn on_finalize(n: BlockNumberFor<T>) { | ||
// Check if it's the first block of the round | ||
let current_round = Self::round(); | ||
if SlashingEnabled::<T>::get() && current_round.first == n { | ||
// Slash any collators that didn't author blocks in previous round | ||
Self::get_collators_without_blocks(current_round.current - 1); | ||
} | ||
Self::payout_collator(); | ||
} | ||
} | ||
|
@@ -677,22 +692,33 @@ pub mod pallet { | |
pub(crate) type DelayedPayoutInfo<T: Config> = | ||
StorageValue<_, DelayedPayoutInfoT<SessionIndex, BalanceOf<T>>, OptionQuery>; | ||
|
||
// Slashing enabled/disabled option | ||
#[pallet::storage] | ||
#[pallet::getter(fn slashing_enabled)] | ||
pub(crate) type SlashingEnabled<T> = StorageValue<_, bool, ValueQuery>; | ||
|
||
#[pallet::genesis_config] | ||
pub struct GenesisConfig<T: Config> { | ||
pub stakers: GenesisStaker<T>, | ||
pub max_candidate_stake: BalanceOf<T>, | ||
pub slashing_enabled: bool, | ||
} | ||
|
||
impl<T: Config> Default for GenesisConfig<T> { | ||
fn default() -> Self { | ||
Self { stakers: Default::default(), max_candidate_stake: Default::default() } | ||
Self { | ||
stakers: Default::default(), | ||
max_candidate_stake: Default::default(), | ||
slashing_enabled: true, | ||
} | ||
} | ||
} | ||
|
||
#[pallet::genesis_build] | ||
impl<T: Config> BuildGenesisConfig for GenesisConfig<T> { | ||
fn build(&self) { | ||
MaxCollatorCandidateStake::<T>::put(self.max_candidate_stake); | ||
SlashingEnabled::<T>::put(self.slashing_enabled); | ||
|
||
// Setup delegate & collators | ||
for &(ref actor, ref opt_val, balance) in &self.stakers { | ||
|
@@ -1986,6 +2012,15 @@ pub mod pallet { | |
)); | ||
Ok(()) | ||
} | ||
|
||
#[pallet::call_index(21)] | ||
#[pallet::weight(<T as crate::pallet::Config>::WeightInfo::set_slashing_enabled())] | ||
pub fn set_slashing_enabled(origin: OriginFor<T>, enabled: bool) -> DispatchResult { | ||
ensure_root(origin)?; | ||
SlashingEnabled::<T>::put(enabled); | ||
Self::deposit_event(Event::SlashingEnabledChanged(enabled)); | ||
Ok(()) | ||
} | ||
} | ||
|
||
impl<T: Config> Pallet<T> { | ||
|
@@ -2797,6 +2832,40 @@ pub mod pallet { | |
T::PotId::get().into_account_truncating() | ||
} | ||
|
||
// Get collators that didn't author blocks in previous round | ||
fn get_collators_without_blocks(round: SessionIndex) { | ||
let selected_candidates = Self::selected_candidates(); | ||
selected_candidates.into_iter().for_each(|collator| { | ||
if !CollatorBlocks::<T>::contains_key(round, &collator) { | ||
Self::kickout_faulty_collator(collator); | ||
} | ||
}); | ||
} | ||
|
||
fn kickout_faulty_collator(collator: T::AccountId) { | ||
let state = CandidatePool::<T>::get(&collator).expect("Collator must exist"); | ||
let mut candidates = TopCandidates::<T>::get(); | ||
if (candidates.len() as u32) <= T::MinRequiredCollators::get() { | ||
return; | ||
} | ||
|
||
if Self::remove_candidate(&collator, &state).is_err() { | ||
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. Does that mean this collator will lose all delegator's staking? If that is the case, this punishment is also severe. Let us check with our business team about the spec 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. Okay, let’s check with them. Another workaround would be to remove the candidate from the pool and add a new field to the Candidate object that indicates whether the candidate can be selected. After calling an extrinsic, this candidate could change this value to allow it to be selected again. |
||
log::error!("Failed to remove collator {:?}", collator); | ||
} | ||
|
||
if candidates | ||
.remove(&Stake { owner: collator.clone(), amount: state.total }) | ||
.is_some() | ||
{ | ||
// update top candidates | ||
TopCandidates::<T>::put(candidates); | ||
// update total amount at stake from scratch | ||
Self::update_total_stake(); | ||
}; | ||
|
||
Self::deposit_event(Event::CollatorKicked(collator)); | ||
} | ||
|
||
/// Handles staking reward payout for previous session for one collator and their delegators | ||
/// At Worst: 5 DB Reads and 'MaxSelectedCandidate + 1' DB Writes | ||
/// Complexity: O(n) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,15 @@ | |
|
||
use crate::{ | ||
pallet::{Config, Pallet, OLD_STAKING_ID, STAKING_ID}, | ||
types::{Candidate, OldCandidate}, | ||
types::{AccountIdOf, Candidate, OldCandidate}, | ||
CandidatePool, ForceNewRound, Round, | ||
}; | ||
use frame_support::{ | ||
pallet_prelude::{GetStorageVersion, StorageVersion}, | ||
pallet_prelude::{GetStorageVersion, StorageVersion, ValueQuery}, | ||
storage_alias, | ||
traits::{Get, LockableCurrency, WithdrawReasons}, | ||
weights::Weight, | ||
Twox64Concat, | ||
}; | ||
use pallet_balances::Locks; | ||
use sp_runtime::Permill; | ||
|
@@ -20,8 +22,9 @@ pub enum Versions { | |
_V8 = 8, | ||
V9 = 9, | ||
V10 = 10, | ||
#[default] | ||
V11 = 11, | ||
#[default] | ||
V12 = 12, | ||
} | ||
|
||
pub(crate) fn on_runtime_upgrade<T: Config>() -> Weight { | ||
|
@@ -31,7 +34,11 @@ pub(crate) fn on_runtime_upgrade<T: Config>() -> Weight { | |
mod upgrade { | ||
|
||
use super::*; | ||
use crate::pallet::SlashingEnabled; | ||
|
||
#[storage_alias] | ||
type CollatorBlock<T: Config> = | ||
StorageMap<Pallet<T>, Twox64Concat, AccountIdOf<T>, u32, ValueQuery>; | ||
/// Migration implementation that deletes the old reward rate config and changes the staking ID. | ||
pub struct Migrate<T>(sp_std::marker::PhantomData<T>); | ||
|
||
|
@@ -102,6 +109,21 @@ mod upgrade { | |
|
||
log::info!("V11 Migrating Done."); | ||
} | ||
|
||
if onchain_storage_version < StorageVersion::new(Versions::V12 as u16) { | ||
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. Do you think whether we should remove the above migration logic? Or is keeping them better? 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. Are you implying that since the migration has already occurred and the round is now mandatory, it might be reasonable to eliminate it? I think that would make sense but let’s discuss this with @talhadaar. 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. Yep, you are right. I'm thinking because only Krest has the old version; however, we shouldn't upgrade it to the lastest version directly. Therefore, it's possible to remove it. Let us discuss it 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'm not sure what this entails just yet, i'll have a closer look at this PR. |
||
log::info!( | ||
"Running storage migration from version {:?} to {:?}", | ||
onchain_storage_version, | ||
Versions::default() as u16 | ||
); | ||
|
||
// enable slashing | ||
SlashingEnabled::<T>::put(true); | ||
weight_writes += 1; | ||
|
||
log::info!("V12 Migrating Done."); | ||
} | ||
|
||
// update onchain storage version | ||
StorageVersion::new(Versions::default() as u16).put::<Pallet<T>>(); | ||
weight_writes += 1; | ||
|
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.
One question about the spec but not about the implementation. If the sudo user triggers the force_new_round, should we punish the collators who are not generating the block? Or should we waive it?
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.
For now, I kept it like this because it was better for testing purposes. That’s the only way I found to test it in an environment like Chopsticks. Another reason I kept it is that forcing a new round shouldn’t happen frequently. The only scenario I can think of is when we change the token economy. If we really want to force a new round in production we can always deactivate slashing. We can discuss that with the product team.
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.
Yep, let us discuss with them
That's a good workaround!