Skip to content

Commit

Permalink
stake-tracker feedback PR (#4411)
Browse files Browse the repository at this point in the history
- Changes the target list score type from balance to `u128`
(`ExtendedBalance`)
  - simplifies `balance/ vote_weight/ extended_balance` conversions
  - relies less on the currency total issuance call
  - ensures approval stake calculations/storage is safe 
- Simplifies stake tracker logic
  - removes a few redundant checks
  - split `on_stake_update` code
- Ensures `VoterUpdateMode` is taken into consideration in try-state
checks
  - use voter mode lazy in benchmarks
  - update weights + check the diff with voter lazy 
- Docs improvements
- All tests passing after merging validator disabling and virtual
stakers

**new**
- ensures duplicate nominations are dedup before calculating the
approvals stake
- chills nominator after total slash to ensure the target can be
reaped/kill without leaving nominations behind.
- ensures that switching from validator to nominator and back is correct
- a nominator may have an entry in the targetlist (if it was a validator
+ has nominations)
  - ensure target node is removed if balance is 0 and it is a nominator.
- addresses
paritytech-secops/srlabs_findings#383

---
**Other experiments:** (not included)
- #4402 (passing the
status of the staker with who in the OnStakingUpdate interface is less
clean, it turns out)
- #4361

---------

Co-authored-by: command-bot <>
  • Loading branch information
gpestana authored May 10, 2024
1 parent e9218b6 commit 628bceb
Show file tree
Hide file tree
Showing 15 changed files with 934 additions and 704 deletions.
2 changes: 1 addition & 1 deletion polkadot/runtime/westend/src/bag_thresholds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ pub const VOTER_THRESHOLDS: [u64; 200] = [
];

/// Upper thresholds delimiting the targets bag list.
pub const TARGET_THRESHOLDS: [crate::Balance; 200] = [
pub const TARGET_THRESHOLDS: [u128; 200] = [
10_000_000_000,
11_131_723_507,
12_391_526_824,
Expand Down
12 changes: 7 additions & 5 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,9 @@ impl pallet_election_provider_multi_phase::Config for Runtime {

parameter_types! {
pub const VoterBagThresholds: &'static [u64] = &bag_thresholds::VOTER_THRESHOLDS;
pub const TargetBagThresholds: &'static [Balance] = &bag_thresholds::TARGET_THRESHOLDS;
pub const TargetBagThresholds: &'static [u128] = &bag_thresholds::TARGET_THRESHOLDS;

pub const VoterUpdateMode: pallet_stake_tracker::VoterUpdateMode = pallet_stake_tracker::VoterUpdateMode::Strict;
}

type VoterBagsListInstance = pallet_bags_list::Instance1;
Expand All @@ -596,16 +598,15 @@ impl pallet_bags_list::Config<TargetBagsListInstance> for Runtime {
type ScoreProvider = pallet_bags_list::Pallet<Runtime, TargetBagsListInstance>;
type WeightInfo = weights::pallet_bags_list::WeightInfo<Runtime>;
type BagThresholds = TargetBagThresholds;
type Score = Balance;
type Score = u128;
}

impl pallet_stake_tracker::Config for Runtime {
type Currency = Balances;
type RuntimeEvent = RuntimeEvent;
type Staking = Staking;
type VoterList = VoterList;
type TargetList = TargetList;
type WeightInfo = (); // TODO
type VoterUpdateMode = VoterUpdateMode;
}

pallet_staking_reward_curve::build! {
Expand Down Expand Up @@ -1680,7 +1681,8 @@ pub mod migrations {
}

/// Unreleased migrations. Add new ones here:
pub type Unreleased = (pallet_staking::migrations::v15::MigrateV14ToV15<Runtime>,);
pub type Unreleased =
(pallet_staking::migrations::single_block::v15::MigrateV14ToV15<Runtime>,);
}

/// Unchecked extrinsic type as expected by this runtime.
Expand Down
9 changes: 9 additions & 0 deletions polkadot/runtime/westend/src/weights/pallet_staking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -826,4 +826,13 @@ impl<T: frame_system::Config> pallet_staking::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(5))
.saturating_add(T::DbWeight::get().writes(4))
}

// TODO: run CI bot benchmarks
fn drop_dangling_nomination() -> Weight {
Weight::default()
}

fn v13_mmb_step() -> Weight {
Weight::default()
}
}
6 changes: 5 additions & 1 deletion substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -703,12 +703,16 @@ impl pallet_staking::Config for Runtime {
type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
}

parameter_types! {
pub const VoterUpdateMode: pallet_stake_tracker::VoterUpdateMode = pallet_stake_tracker::VoterUpdateMode::Strict;
}

impl pallet_stake_tracker::Config for Runtime {
type Currency = Balances;
type RuntimeEvent = RuntimeEvent;
type Staking = Staking;
type VoterList = VoterList;
type TargetList = TargetList;
type VoterUpdateMode = VoterUpdateMode;
}

impl pallet_fast_unstake::Config for Runtime {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1212,7 +1212,7 @@ mod benchmarks {

impl_benchmark_test_suite!(
Staking,
crate::mock::ExtBuilder::default().has_stakers(true),
crate::mock::ExtBuilder::default().has_stakers(true).set_voter_list_lazy(),
crate::mock::Test,
exec_name = build_and_execute
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,12 @@ fn mb_migration_target_list_dangling_validators_works() {
}

#[test]
fn mb_migration_target_list_bench_works() {
fn mb_migration_target_list_single_step_bench_works() {
ExtBuilder::default()
.has_stakers(false)
.max_winners(1000)
// skip checks as not all the steps are applied.
.stake_tracker_try_state(false)
.build_and_execute(|| {
// setup:
// 1000 validators;
Expand Down
17 changes: 16 additions & 1 deletion substrate/frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,16 @@ impl OnStakingUpdate<AccountId, Balance> for EventTracker {
}
}

parameter_types! {
pub static VoterUpdateMode: pallet_stake_tracker::VoterUpdateMode = pallet_stake_tracker::VoterUpdateMode::Strict;
}

impl pallet_stake_tracker::Config for Test {
type Currency = Balances;
type RuntimeEvent = RuntimeEvent;
type Staking = Staking;
type VoterList = VoterBagsList;
type TargetList = TargetBagsList;
type VoterUpdateMode = VoterUpdateMode;
}

// Disabling threshold for `UpToLimitDisablingStrategy`
Expand Down Expand Up @@ -568,6 +572,10 @@ impl ExtBuilder {
SkipStakeTrackerTryStateCheck::set(!enable);
self
}
pub fn set_voter_list_lazy(self) -> Self {
VoterUpdateMode::set(pallet_stake_tracker::VoterUpdateMode::Lazy);
self
}
pub fn max_winners(self, max: u32) -> Self {
MaxWinners::set(max);
self
Expand Down Expand Up @@ -1100,6 +1108,13 @@ macro_rules! assert_session_era {
};
}

pub(crate) fn nominators_of(t: &AccountId) -> Vec<AccountId> {
Nominators::<Test>::iter()
.filter(|(_, n)| n.targets.contains(&t))
.map(|(v, _)| v)
.collect::<Vec<_>>()
}

pub(crate) fn staking_events() -> Vec<crate::Event<Test>> {
System::events()
.into_iter()
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1080,8 +1080,8 @@ impl<T: Config> Pallet<T> {
let mut all_targets = Vec::<T::AccountId>::with_capacity(final_predicted_len as usize);
let mut targets_seen = 0;

// target list may contain chilled validators and dangling (i.e. unbonded) targets, filter
// those.
// target list may contain chilled validators, dangling (i.e. unbonded) targets or even
// nominators that have been validators - filter those out.
let mut targets_iter = T::TargetList::iter().filter(|t| match Self::status(&t) {
Ok(StakerStatus::Idle) | Ok(StakerStatus::Nominator(_)) | Err(_) => false,
Ok(StakerStatus::Validator) => true,
Expand Down
2 changes: 0 additions & 2 deletions substrate/frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1094,8 +1094,6 @@ pub mod pallet {
.try_push(UnlockChunk { value, era })
.map_err(|_| Error::<T>::NoMoreChunks)?;
};
// NOTE: ledger must be updated prior to calling `Self::weight_of`.

ledger.update()?;

Self::deposit_event(Event::<T>::Unbonded { stash, amount: value });
Expand Down
Loading

0 comments on commit 628bceb

Please sign in to comment.