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

Remove implicit approval chilling upon slash. #12420

Merged
merged 25 commits into from
Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e067a17
don't read slashing spans when taking election snapshot
kianenigma Oct 4, 2022
6ba277b
update cargo.toml
kianenigma Oct 4, 2022
df5cbe2
bring back remote test
kianenigma Oct 4, 2022
6a9ad83
undo doc changes
kianenigma Oct 6, 2022
88f8470
fix merge stuff
kianenigma Oct 6, 2022
cc184dc
fix npos-voters function sig
kianenigma Oct 6, 2022
f86e368
remove as much redundant diff as you can
kianenigma Oct 6, 2022
c67961d
Update frame/staking/src/pallet/mod.rs
kianenigma Nov 1, 2022
d23326f
update'
kianenigma Nov 8, 2022
87b488c
Merge branch 'kiz-slashing-spans-snapshot-fix' of github.com:parityte…
kianenigma Nov 8, 2022
d644f8f
fix
kianenigma Nov 8, 2022
1f64829
Update frame/staking/src/pallet/impls.rs
kianenigma Nov 8, 2022
67ef21d
update lock
kianenigma Nov 10, 2022
ceccee2
Merge branch 'kiz-slashing-spans-snapshot-fix' of github.com:parityte…
kianenigma Nov 10, 2022
32abede
Master.into()
kianenigma Nov 10, 2022
a89dce4
Merge branch 'master' into kiz-slashing-spans-snapshot-fix
Ank4n Dec 9, 2022
4871296
Master.into()
kianenigma Dec 10, 2022
d045394
Merge branch 'kiz-slashing-spans-snapshot-fix' of github.com:parityte…
kianenigma Dec 10, 2022
fd03b10
fix all tests
kianenigma Dec 10, 2022
1525744
review comments
kianenigma Dec 10, 2022
a254b23
fmt
kianenigma Dec 10, 2022
99396f3
fix offence bench
kianenigma Dec 10, 2022
df93a94
clippy
kianenigma Dec 12, 2022
9354b6b
Merge branch 'master' of https://github.com/paritytech/substrate into…
Dec 12, 2022
6b61c24
".git/.scripts/bench-bot.sh" pallet dev pallet_staking
Dec 12, 2022
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
17 changes: 13 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2174,6 +2174,8 @@ mod tests {
assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback."));
// phase is now emergency.
assert_eq!(MultiPhase::current_phase(), Phase::Emergency);
// snapshot is still there until election finalizes.
assert!(MultiPhase::snapshot().is_some());
})
}

Expand All @@ -2190,6 +2192,7 @@ mod tests {
// phase is now emergency.
assert_eq!(MultiPhase::current_phase(), Phase::Emergency);
assert!(MultiPhase::queued_solution().is_none());
assert!(MultiPhase::snapshot().is_some());

// no single account can trigger this
assert_noop!(
Expand Down
15 changes: 6 additions & 9 deletions frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,12 +791,10 @@ benchmarks! {
}

get_npos_voters {
// number of validator intention.
let v in (MaxValidators::<T>::get() / 2) .. MaxValidators::<T>::get();
// number of nominator intention.
let n in (MaxNominators::<T>::get() / 2) .. MaxNominators::<T>::get();
// total number of slashing spans. Assigned to validators randomly.
let s in 1 .. 20;
// number of validator intention. we will iterate all of them.
let v in (MaxValidators::<T>::get() / 4) .. MaxValidators::<T>::get();
Copy link
Member

Choose a reason for hiding this comment

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

Ok. This has changed from 2 to 4. I don't see any justification for either number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda puzzling to me too, I think this is set to a 100, so this iterates from 25 to a 100. Why this magic number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We know that this code path's complexity is a function MaxNominators and MaxValidaotrs. That's called a component. So you chose a subset of the entire values that the component has, and let the benchmark run over that range.

In some cases, you can let it run over the entire range of the component, but in this case that would lead to some test failures because e.g. MaxNominators is not allowed anyways.

with that in mind, why /2 and /4? it is entirely arbitrary. If you want to chose a "better" magic number, please suggest. I recall /4 worked better with tests, although I ran them again with /2 and /4 and they all, work, and without a surprise, the benchmark values are the same.

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 will revert it back to /2.

// number of nominator intention. we will iterate all of them.
let n in (MaxNominators::<T>::get() / 4) .. MaxNominators::<T>::get();
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

let validators = create_validators_with_nominators_for_era::<T>(
v, n, T::MaxNominations::get() as usize, false, None
Expand All @@ -805,9 +803,8 @@ benchmarks! {
.map(|v| T::Lookup::lookup(v).unwrap())
.collect::<Vec<_>>();

(0..s).for_each(|index| {
add_slashing_spans::<T>(&validators[index as usize], 10);
});
assert_eq!(Validators::<T>::count(), v);
assert_eq!(Nominators::<T>::count(), n);

let num_voters = (v + n) as usize;
}: {
Expand Down
28 changes: 14 additions & 14 deletions frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use sp_staking::{
offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
EraIndex, SessionIndex, StakingInterface,
};
use sp_std::{collections::btree_map::BTreeMap, prelude::*};
use sp_std::prelude::*;

use crate::{
log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, Exposure, ExposureOf,
Expand Down Expand Up @@ -348,6 +348,7 @@ impl<T: Config> Pallet<T> {
}
}

/// Start a new era. It does:
///
/// * Increment `active_era.index`,
/// * reset `active_era.start`,
Expand Down Expand Up @@ -696,7 +697,6 @@ impl<T: Config> Pallet<T> {

kianenigma marked this conversation as resolved.
Show resolved Hide resolved
// cache a few things.
let weight_of = Self::weight_of_fn();
let slashing_spans = <SlashingSpans<T>>::iter().collect::<BTreeMap<_, _>>();

let mut voters_seen = 0u32;
let mut validators_taken = 0u32;
Expand All @@ -714,18 +714,12 @@ impl<T: Config> Pallet<T> {
None => break,
};

if let Some(Nominations { submitted_in, mut targets, suppressed: _ }) =
<Nominators<T>>::get(&voter)
{
// if this voter is a nominator:
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
targets.retain(|stash| {
slashing_spans
.get(stash)
.map_or(true, |spans| submitted_in >= spans.last_nonzero_slash())
});
if !targets.len().is_zero() {
if let Some(Nominations { targets, .. }) = <Nominators<T>>::get(&voter) {
if !targets.is_empty() {
all_voters.push((voter.clone(), weight_of(&voter), targets));
nominators_taken.saturating_inc();
} else {
// A nominator's nominations might become empty due to slashing.
ordian marked this conversation as resolved.
Show resolved Hide resolved
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
}
} else if Validators::<T>::contains_key(&voter) {
// if this voter is a validator:
Expand All @@ -748,7 +742,7 @@ impl<T: Config> Pallet<T> {
warn,
"DEFENSIVE: invalid item in `VoterList`: {:?}, this nominator probably has too many nominations now",
voter
)
);
}
}

Expand All @@ -758,7 +752,7 @@ impl<T: Config> Pallet<T> {
Self::register_weight(T::WeightInfo::get_npos_voters(
validators_taken,
nominators_taken,
slashing_spans.len() as u32,
0,
));

log!(
Expand Down Expand Up @@ -1262,6 +1256,12 @@ where
disable_strategy,
});

Self::deposit_event(Event::<T>::SlashReported {
validator: stash.clone(),
fraction: slash_fraction.clone(),
slash_era,
});

if let Some(mut unapplied) = unapplied {
let nominators_len = unapplied.others.len() as u64;
let reporters_len = details.reporters.len() as u64;
Expand Down
8 changes: 6 additions & 2 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ pub mod pallet {
#[pallet::storage]
#[pallet::getter(fn slashing_spans)]
#[pallet::unbounded]
pub(crate) type SlashingSpans<T: Config> =
pub type SlashingSpans<T: Config> =
StorageMap<_, Twox64Concat, T::AccountId, slashing::SlashingSpans>;

/// Records information about the maximum slash of a stash within a slashing span,
Expand Down Expand Up @@ -545,6 +545,7 @@ pub mod pallet {
/// `OffendingValidatorsThreshold` is reached. The vec is always kept sorted so that we can find
/// whether a given validator has previously offended using binary search. It gets cleared when
/// the era ends.
// TODO: invariant test for the above.
#[pallet::storage]
#[pallet::unbounded]
#[pallet::getter(fn offending_validators)]
Expand Down Expand Up @@ -665,8 +666,11 @@ pub mod pallet {
EraPaid { era_index: EraIndex, validator_payout: BalanceOf<T>, remainder: BalanceOf<T> },
/// The nominator has been rewarded by this amount.
Rewarded { stash: T::AccountId, amount: BalanceOf<T> },
/// One staker (and potentially its nominators) has been slashed by the given amount.
/// A staker (validator or nominator) has been slashed by the given amount.
Slashed { staker: T::AccountId, amount: BalanceOf<T> },
/// A slash for the given validator, for the given percentage of their stake, at the given
/// era as been reporter
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
SlashReported { validator: T::AccountId, fraction: Perbill, slash_era: EraIndex },
/// An old slashing report from a prior era was discarded because it could
/// not be processed.
OldSlashingReportDiscarded { session_index: SessionIndex },
Expand Down
Loading