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

Feat/1205528919110616 slashing liveness fault #323

Open
wants to merge 47 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
65157e0
taking staking snapshot at start_session
talhadaar Aug 28, 2024
5392a56
implemented delayed payouts
talhadaar Aug 28, 2024
5f19c09
Merge branch 'dev' into feat/1207513338125664_delated-staking-rewards
talhadaar Aug 28, 2024
b808709
use take instead of get for AtStake
talhadaar Aug 28, 2024
68ff1e3
prepare to refresh staking snapshot even if selected candidates dont …
talhadaar Sep 3, 2024
7228c77
staking snapshot refreshes if collator set doesn't change
talhadaar Sep 4, 2024
102753f
setup storage migration for moving to delayed rewards distribution
talhadaar Sep 4, 2024
4abf945
fix build issue with parachain staking unit tests
talhadaar Sep 11, 2024
6b5427b
(bugfix) payout_collator at on_finalize and skip delayed reward calc …
talhadaar Sep 11, 2024
6471650
updated some unit tests, improved distribution mechanism to consider …
talhadaar Sep 11, 2024
c251d02
(chore): cargo fmt
talhadaar Sep 11, 2024
fb478bc
Fixued up most unit tests on parachain staking, ensure correct round …
talhadaar Sep 12, 2024
eedc4e2
(chore) cargo fmt
talhadaar Sep 12, 2024
ae7c784
fix coinbase_rewards_many_blocks_simple_check unit test in parachain …
talhadaar Sep 12, 2024
fce5808
fixed up further unit tests in parachain staking
talhadaar Sep 12, 2024
ca5c952
Some enw and some updated unit tests on parachain staking pallet, mit…
talhadaar Sep 13, 2024
81d5d4f
fixed unit tests on parachain staking, incorporate delayed staking re…
talhadaar Sep 13, 2024
1ed0367
Merge branch 'dev' into feat/1207513338125664_delated-staking-rewards
talhadaar Sep 16, 2024
7f301e3
use BlockNumberFor<T> instead of T::BlockNumber for parachain staking
talhadaar Sep 16, 2024
7e0b410
(chore): cargo fmt and clippy
talhadaar Sep 16, 2024
c30e3f5
(chore) minor cargo clippy fix
talhadaar Sep 16, 2024
72c9145
use end_session and new_session hooks in parachain staking
talhadaar Sep 25, 2024
39fddaa
minor fix to parachain-staking tests
talhadaar Sep 25, 2024
b09b3db
fixed prepare_delayed_rewards to use updated RoundInfo
talhadaar Sep 26, 2024
32b56d7
(chore) cargo clippy
talhadaar Sep 26, 2024
69710ce
Merge branch 'dev' into feat/1207513338125664_delated-staking-rewards
talhadaar Sep 26, 2024
fdb0fda
restrict sudo from forcing new round before payouts are finished
talhadaar Oct 10, 2024
1f580f3
(chore) cargo fmt and clippy
talhadaar Oct 10, 2024
ec1c859
remove unused code
talhadaar Oct 10, 2024
25fb45b
feat(slashing): add liveness fault slashing basic. This commit makes …
DocteurPing Oct 22, 2024
29b2ed7
feat(slashing): add extrinsic to change slashing factor
DocteurPing Oct 25, 2024
cd00420
feat(slashing): add extrinsic to enable/disable slashing. this commit…
DocteurPing Oct 25, 2024
17c7963
feat(slashing): update weights
DocteurPing Oct 28, 2024
688c5f5
feat(slashing): apply slashing to delegators
DocteurPing Oct 30, 2024
e313236
feat(slashing): update top candidates after slashing
DocteurPing Nov 19, 2024
f98a2f6
sync with dev
talhadaar Nov 20, 2024
abb8596
added weights to parachain staking hooks
talhadaar Nov 20, 2024
4987d7d
Merge branch 'feat/1207513338125664_delated-staking-rewards' into fea…
DocteurPing Nov 21, 2024
47eb117
fix(slashing): make sure collators not in the pool are not being slashed
DocteurPing Nov 26, 2024
5213294
Feat(slashing): remove collator from the pool instead of slashing it
DocteurPing Dec 18, 2024
ebdefc0
fix(slashing): remove slashing factor for the moment
DocteurPing Jan 6, 2025
99dd4a5
feat(slashing): add more test for slashing
DocteurPing Jan 6, 2025
7621acd
Merge branch 'dev' into feat/1205528919110616_slashing-liveness-fault
DocteurPing Jan 6, 2025
9b68d4a
fix cargo fmt
DocteurPing Jan 6, 2025
68d8112
fix(slashing): remove unused TreasuryPalletId
DocteurPing Jan 6, 2025
c140150
fix(slashing): remove dead code
DocteurPing Jan 6, 2025
ad2e928
fix(slashing): add back the tests that got deleted
DocteurPing Jan 6, 2025
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
1 change: 1 addition & 0 deletions node/src/parachain/dev_chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ fn configure_genesis(
parachain_staking: ParachainStakingConfig {
stakers,
max_candidate_stake: staking::MAX_COLLATOR_STAKE,
slashing_enabled: true,
},
inflation_manager: Default::default(),
block_reward: BlockRewardConfig {
Expand Down
1 change: 1 addition & 0 deletions node/src/parachain/krest_chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ fn configure_genesis(
parachain_staking: ParachainStakingConfig {
stakers,
max_candidate_stake: staking::MAX_COLLATOR_STAKE,
slashing_enabled: true,
},
inflation_manager: Default::default(),
block_reward: BlockRewardConfig {
Expand Down
1 change: 1 addition & 0 deletions node/src/parachain/peaq_chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ fn configure_genesis(
parachain_staking: ParachainStakingConfig {
stakers,
max_candidate_stake: staking::MAX_COLLATOR_STAKE,
slashing_enabled: true,
},
inflation_manager: Default::default(),
block_reward: BlockRewardConfig {
Expand Down
73 changes: 71 additions & 2 deletions pallets/parachain-staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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>) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@sfffaaa sfffaaa Jan 7, 2025

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

If we really want to force a new round in production we can always deactivate slashing

That's a good workaround!

// 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();
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Expand Down
28 changes: 25 additions & 3 deletions pallets/parachain-staking/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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>);

Expand Down Expand Up @@ -102,6 +109,21 @@ mod upgrade {

log::info!("V11 Migrating Done.");
}

if onchain_storage_version < StorageVersion::new(Versions::V12 as u16) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down
10 changes: 7 additions & 3 deletions pallets/parachain-staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,13 @@ impl ExtBuilder {
for delegator in self.delegators.clone() {
stakers.push((delegator.0, Some(delegator.1), delegator.2));
}
stake::GenesisConfig::<Test> { stakers, max_candidate_stake: 160_000_000 * DECIMALS }
.assimilate_storage(&mut t)
.expect("Parachain Staking's storage can be assimilated");
stake::GenesisConfig::<Test> {
stakers,
max_candidate_stake: 160_000_000 * DECIMALS,
slashing_enabled: true,
}
.assimilate_storage(&mut t)
.expect("Parachain Staking's storage can be assimilated");

// stashes are the AccountId
let session_keys: Vec<_> = self
Expand Down
Loading