diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index b115087d51..9fac468519 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -459,10 +459,6 @@ impl_opaque_keys! { } } -parameter_types! { - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(17); -} - impl pallet_session::Config for Runtime { type Event = Event; type ValidatorId = ::AccountId; @@ -472,7 +468,6 @@ impl pallet_session::Config for Runtime { type SessionManager = pallet_session::historical::NoteHistoricalRoot; type SessionHandler = ::KeyTypeIdProviders; type Keys = SessionKeys; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type WeightInfo = pallet_session::weights::SubstrateWeight; } @@ -498,6 +493,7 @@ parameter_types! { pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration. pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; pub const MaxNominatorRewardedPerValidator: u32 = 256; + pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17); pub OffchainRepeat: BlockNumber = 5; } @@ -529,6 +525,7 @@ impl pallet_staking::Config for Runtime { type EraPayout = pallet_staking::ConvertCurve; type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = ElectionProviderMultiPhase; type GenesisElectionProvider = onchain::OnChainSequentialPhragmen; // Alternatively, use pallet_staking::UseNominatorsMap to just use the nominators map. diff --git a/substrate/frame/aura/src/lib.rs b/substrate/frame/aura/src/lib.rs index e8b68f928e..4b52948354 100644 --- a/substrate/frame/aura/src/lib.rs +++ b/substrate/frame/aura/src/lib.rs @@ -221,7 +221,7 @@ impl OneSessionHandler for Pallet { } } - fn on_disabled(i: usize) { + fn on_disabled(i: u32) { let log: DigestItem = DigestItem::Consensus( AURA_ENGINE_ID, ConsensusLog::::OnDisabled(i as AuthorityIndex).encode(), diff --git a/substrate/frame/authority-discovery/src/lib.rs b/substrate/frame/authority-discovery/src/lib.rs index d093b1533c..8fced0d18c 100644 --- a/substrate/frame/authority-discovery/src/lib.rs +++ b/substrate/frame/authority-discovery/src/lib.rs @@ -166,7 +166,7 @@ impl OneSessionHandler for Pallet { } } - fn on_disabled(_i: usize) { + fn on_disabled(_i: u32) { // ignore } } @@ -218,7 +218,6 @@ mod tests { type Event = Event; type ValidatorId = AuthorityId; type ValidatorIdOf = ConvertInto; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type NextSessionRotation = pallet_session::PeriodicSessions; type WeightInfo = (); } @@ -276,7 +275,7 @@ mod tests { ) { } - fn on_disabled(_validator_index: usize) {} + fn on_disabled(_validator_index: u32) {} fn on_genesis_session(_validators: &[(AuthorityId, Ks)]) {} } diff --git a/substrate/frame/babe/src/lib.rs b/substrate/frame/babe/src/lib.rs index 4ccfdf6c13..9c755eea6c 100644 --- a/substrate/frame/babe/src/lib.rs +++ b/substrate/frame/babe/src/lib.rs @@ -926,8 +926,8 @@ impl OneSessionHandler for Pallet { Self::enact_epoch_change(bounded_authorities, next_bounded_authorities) } - fn on_disabled(i: usize) { - Self::deposit_consensus(ConsensusLog::OnDisabled(i as u32)) + fn on_disabled(i: u32) { + Self::deposit_consensus(ConsensusLog::OnDisabled(i)) } } diff --git a/substrate/frame/babe/src/mock.rs b/substrate/frame/babe/src/mock.rs index b504a26f60..e7ec692689 100644 --- a/substrate/frame/babe/src/mock.rs +++ b/substrate/frame/babe/src/mock.rs @@ -68,7 +68,6 @@ frame_support::construct_runtime!( parameter_types! { pub const BlockHashCount: u64 = 250; - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(16); pub BlockWeights: frame_system::limits::BlockWeights = frame_system::limits::BlockWeights::simple_max(1024); } @@ -122,7 +121,6 @@ impl pallet_session::Config for Test { type SessionManager = pallet_session::historical::NoteHistoricalRoot; type SessionHandler = ::KeyTypeIdProviders; type Keys = MockSessionKeys; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type WeightInfo = (); } @@ -189,6 +187,7 @@ parameter_types! { pub const MaxNominatorRewardedPerValidator: u32 = 64; pub const ElectionLookahead: u64 = 0; pub const StakingUnsignedPriority: u64 = u64::MAX / 2; + pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(16); } impl onchain::Config for Test { @@ -212,6 +211,7 @@ impl pallet_staking::Config for Test { type UnixTime = pallet_timestamp::Pallet; type EraPayout = pallet_staking::ConvertCurve; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; diff --git a/substrate/frame/beefy-mmr/src/mock.rs b/substrate/frame/beefy-mmr/src/mock.rs index 4c9e103eb7..95b87c3605 100644 --- a/substrate/frame/beefy-mmr/src/mock.rs +++ b/substrate/frame/beefy-mmr/src/mock.rs @@ -28,7 +28,6 @@ use sp_runtime::{ impl_opaque_keys, testing::Header, traits::{BlakeTwo256, ConvertInto, IdentityLookup, Keccak256, OpaqueKeys}, - Perbill, }; use crate as pallet_beefy_mmr; @@ -92,7 +91,6 @@ impl frame_system::Config for Test { parameter_types! { pub const Period: u64 = 1; pub const Offset: u64 = 0; - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(33); } impl pallet_session::Config for Test { @@ -104,7 +102,6 @@ impl pallet_session::Config for Test { type SessionManager = MockSessionManager; type SessionHandler = ::KeyTypeIdProviders; type Keys = MockSessionKeys; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type WeightInfo = (); } diff --git a/substrate/frame/beefy/src/lib.rs b/substrate/frame/beefy/src/lib.rs index 32f3133373..3b28d45484 100644 --- a/substrate/frame/beefy/src/lib.rs +++ b/substrate/frame/beefy/src/lib.rs @@ -162,7 +162,7 @@ impl OneSessionHandler for Pallet { } } - fn on_disabled(i: usize) { + fn on_disabled(i: u32) { let log: DigestItem = DigestItem::Consensus( BEEFY_ENGINE_ID, ConsensusLog::::OnDisabled(i as AuthorityIndex).encode(), diff --git a/substrate/frame/beefy/src/mock.rs b/substrate/frame/beefy/src/mock.rs index baa2fae746..a1fbeda4ab 100644 --- a/substrate/frame/beefy/src/mock.rs +++ b/substrate/frame/beefy/src/mock.rs @@ -105,7 +105,6 @@ impl pallet_session::Config for Test { type SessionManager = MockSessionManager; type SessionHandler = ::KeyTypeIdProviders; type Keys = MockSessionKeys; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type WeightInfo = (); } diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index 687207151f..9f6967a7d3 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -674,7 +674,7 @@ where SetIdSession::::insert(current_set_id, &session_index); } - fn on_disabled(i: usize) { + fn on_disabled(i: u32) { Self::deposit_log(ConsensusLog::OnDisabled(i as u64)) } } diff --git a/substrate/frame/grandpa/src/mock.rs b/substrate/frame/grandpa/src/mock.rs index 4e5e44ce36..f1996553f0 100644 --- a/substrate/frame/grandpa/src/mock.rs +++ b/substrate/frame/grandpa/src/mock.rs @@ -111,7 +111,6 @@ where parameter_types! { pub const Period: u64 = 1; pub const Offset: u64 = 0; - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(17); } /// Custom `SessionHandler` since we use `TestSessionKeys` as `Keys`. @@ -124,7 +123,6 @@ impl pallet_session::Config for Test { type SessionManager = pallet_session::historical::NoteHistoricalRoot; type SessionHandler = ::KeyTypeIdProviders; type Keys = TestSessionKeys; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type WeightInfo = (); } @@ -191,6 +189,7 @@ parameter_types! { pub const MaxNominatorRewardedPerValidator: u32 = 64; pub const ElectionLookahead: u64 = 0; pub const StakingUnsignedPriority: u64 = u64::MAX / 2; + pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17); } impl onchain::Config for Test { @@ -214,6 +213,7 @@ impl pallet_staking::Config for Test { type UnixTime = pallet_timestamp::Pallet; type EraPayout = pallet_staking::ConvertCurve; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; diff --git a/substrate/frame/im-online/src/lib.rs b/substrate/frame/im-online/src/lib.rs index 559c5edcf4..d76bbaaa2f 100644 --- a/substrate/frame/im-online/src/lib.rs +++ b/substrate/frame/im-online/src/lib.rs @@ -919,7 +919,7 @@ impl OneSessionHandler for Pallet { } } - fn on_disabled(_i: usize) { + fn on_disabled(_i: u32) { // ignore } } diff --git a/substrate/frame/im-online/src/mock.rs b/substrate/frame/im-online/src/mock.rs index 92d1fe8e3f..1e4d4b43d5 100644 --- a/substrate/frame/im-online/src/mock.rs +++ b/substrate/frame/im-online/src/mock.rs @@ -27,7 +27,7 @@ use sp_core::H256; use sp_runtime::{ testing::{Header, TestXt, UintAuthorityId}, traits::{BlakeTwo256, ConvertInto, IdentityLookup}, - Perbill, Permill, + Permill, }; use sp_staking::{ offence::{OffenceError, ReportOffence}, @@ -146,10 +146,6 @@ parameter_types! { pub const Offset: u64 = 0; } -parameter_types! { - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(33); -} - impl pallet_session::Config for Runtime { type ShouldEndSession = pallet_session::PeriodicSessions; type SessionManager = @@ -159,7 +155,6 @@ impl pallet_session::Config for Runtime { type ValidatorIdOf = ConvertInto; type Keys = UintAuthorityId; type Event = Event; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type NextSessionRotation = pallet_session::PeriodicSessions; type WeightInfo = (); } diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index 6973e25371..3097f9b95b 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/substrate/frame/offences/benchmarking/src/mock.rs @@ -112,7 +112,7 @@ impl pallet_session::SessionHandler for TestSessionHandler { ) { } - fn on_disabled(_: usize) {} + fn on_disabled(_: u32) {} } parameter_types! { @@ -129,7 +129,6 @@ impl pallet_session::Config for Test { type Event = Event; type ValidatorId = AccountId; type ValidatorIdOf = pallet_staking::StashOf; - type DisabledValidatorsThreshold = (); type WeightInfo = (); } @@ -175,6 +174,7 @@ impl pallet_staking::Config for Test { type EraPayout = pallet_staking::ConvertCurve; type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = (); type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; type SortedListProvider = pallet_staking::UseNominatorsMap; diff --git a/substrate/frame/session/benchmarking/src/mock.rs b/substrate/frame/session/benchmarking/src/mock.rs index 4d3a1a2d86..f534cc097e 100644 --- a/substrate/frame/session/benchmarking/src/mock.rs +++ b/substrate/frame/session/benchmarking/src/mock.rs @@ -117,7 +117,7 @@ impl pallet_session::SessionHandler for TestSessionHandler { ) { } - fn on_disabled(_: usize) {} + fn on_disabled(_: u32) {} } impl pallet_session::Config for Test { @@ -129,7 +129,6 @@ impl pallet_session::Config for Test { type Event = Event; type ValidatorId = AccountId; type ValidatorIdOf = pallet_staking::StashOf; - type DisabledValidatorsThreshold = (); type WeightInfo = (); } pallet_staking_reward_curve::build! { @@ -180,6 +179,7 @@ impl pallet_staking::Config for Test { type EraPayout = pallet_staking::ConvertCurve; type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = (); type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; type SortedListProvider = pallet_staking::UseNominatorsMap; diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 2742d302ce..10c7ea42b3 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -116,7 +116,7 @@ pub mod weights; use sp_runtime::{ traits::{AtLeast32BitUnsigned, Convert, Member, One, OpaqueKeys, Zero}, - ConsensusEngineId, KeyTypeId, Perbill, Permill, RuntimeAppPublic, + ConsensusEngineId, KeyTypeId, Permill, RuntimeAppPublic, }; use sp_staking::SessionIndex; use sp_std::{ @@ -298,7 +298,7 @@ pub trait SessionHandler { fn on_before_session_ending() {} /// A validator got disabled. Act accordingly until a new session begins. - fn on_disabled(validator_index: usize); + fn on_disabled(validator_index: u32); } #[impl_trait_for_tuples::impl_for_tuples(1, 30)] @@ -342,7 +342,7 @@ impl SessionHandler for Tuple { for_tuples!( #( Tuple::on_before_session_ending(); )* ) } - fn on_disabled(i: usize) { + fn on_disabled(i: u32) { for_tuples!( #( Tuple::on_disabled(i); )* ) } } @@ -354,7 +354,7 @@ impl SessionHandler for TestSessionHandler { fn on_genesis_session(_: &[(AId, Ks)]) {} fn on_new_session(_: bool, _: &[(AId, Ks)], _: &[(AId, Ks)]) {} fn on_before_session_ending() {} - fn on_disabled(_: usize) {} + fn on_disabled(_: u32) {} } #[frame_support::pallet] @@ -401,12 +401,6 @@ pub mod pallet { /// The keys. type Keys: OpaqueKeys + Member + Parameter + Default + MaybeSerializeDeserialize; - /// The fraction of validators set that is safe to be disabled. - /// - /// After the threshold is reached `disabled` method starts to return true, - /// which in combination with `pallet_staking` forces a new era. - type DisabledValidatorsThreshold: Get; - /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; } @@ -514,7 +508,9 @@ pub mod pallet { /// Indices of disabled validators. /// - /// The set is cleared when `on_session_ending` returns a new set of identities. + /// The vec is always kept sorted so that we can find whether a given validator is + /// disabled using binary search. It gets cleared when `on_session_ending` returns + /// a new set of identities. #[pallet::storage] #[pallet::getter(fn disabled_validators)] pub type DisabledValidators = StorageValue<_, Vec, ValueQuery>; @@ -705,42 +701,34 @@ impl Pallet { T::SessionHandler::on_new_session::(changed, &session_keys, &queued_amalgamated); } - /// Disable the validator of index `i`. - /// - /// Returns `true` if this causes a `DisabledValidatorsThreshold` of validators - /// to be already disabled. - pub fn disable_index(i: usize) -> bool { - let (fire_event, threshold_reached) = >::mutate(|disabled| { - let i = i as u32; - if let Err(index) = disabled.binary_search(&i) { - let count = >::decode_len().unwrap_or(0) as u32; - let threshold = T::DisabledValidatorsThreshold::get() * count; - disabled.insert(index, i); - (true, disabled.len() as u32 > threshold) - } else { - (false, false) - } - }); - - if fire_event { - T::SessionHandler::on_disabled(i); + /// Disable the validator of index `i`, returns `false` if the validator was already disabled. + pub fn disable_index(i: u32) -> bool { + if i >= Validators::::decode_len().unwrap_or(0) as u32 { + return false } - threshold_reached + >::mutate(|disabled| { + if let Err(index) = disabled.binary_search(&i) { + disabled.insert(index, i); + T::SessionHandler::on_disabled(i); + return true + } + + false + }) } /// Disable the validator identified by `c`. (If using with the staking pallet, /// this would be their *stash* account.) /// - /// Returns `Ok(true)` if more than `DisabledValidatorsThreshold` validators in current - /// session is already disabled. - /// If used with the staking pallet it allows to force a new era in such case. - pub fn disable(c: &T::ValidatorId) -> sp_std::result::Result { + /// Returns `false` either if the validator could not be found or it was already + /// disabled. + pub fn disable(c: &T::ValidatorId) -> bool { Self::validators() .iter() .position(|i| i == c) - .map(Self::disable_index) - .ok_or(()) + .map(|i| Self::disable_index(i as u32)) + .unwrap_or(false) } /// Upgrade the key type from some old type to a new type. Supports adding diff --git a/substrate/frame/session/src/mock.rs b/substrate/frame/session/src/mock.rs index c6b5f64448..277dec6106 100644 --- a/substrate/frame/session/src/mock.rs +++ b/substrate/frame/session/src/mock.rs @@ -29,7 +29,6 @@ use sp_runtime::{ impl_opaque_keys, testing::{Header, UintAuthorityId}, traits::{BlakeTwo256, ConvertInto, IdentityLookup}, - Perbill, }; use sp_staking::SessionIndex; @@ -144,7 +143,7 @@ impl SessionHandler for TestSessionHandler { .collect() }); } - fn on_disabled(_validator_index: usize) { + fn on_disabled(_validator_index: u32) { DISABLED.with(|l| *l.borrow_mut() = true) } fn on_before_session_ending() { @@ -269,10 +268,6 @@ impl pallet_timestamp::Config for Test { type WeightInfo = (); } -parameter_types! { - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(33); -} - impl Config for Test { type ShouldEndSession = TestShouldEndSession; #[cfg(feature = "historical")] @@ -284,7 +279,6 @@ impl Config for Test { type ValidatorIdOf = ConvertInto; type Keys = MockSessionKeys; type Event = Event; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type NextSessionRotation = (); type WeightInfo = (); } diff --git a/substrate/frame/session/src/tests.rs b/substrate/frame/session/src/tests.rs index 47152042d2..42a2dd74fd 100644 --- a/substrate/frame/session/src/tests.rs +++ b/substrate/frame/session/src/tests.rs @@ -338,7 +338,7 @@ fn session_keys_generate_output_works_as_set_keys_input() { } #[test] -fn return_true_if_more_than_third_is_disabled() { +fn disable_index_returns_false_if_already_disabled() { new_test_ext().execute_with(|| { set_next_validators(vec![1, 2, 3, 4, 5, 6, 7]); force_new_session(); @@ -347,10 +347,9 @@ fn return_true_if_more_than_third_is_disabled() { force_new_session(); initialize_block(2); + assert_eq!(Session::disable_index(0), true); assert_eq!(Session::disable_index(0), false); - assert_eq!(Session::disable_index(1), false); - assert_eq!(Session::disable_index(2), true); - assert_eq!(Session::disable_index(3), true); + assert_eq!(Session::disable_index(1), true); }); } diff --git a/substrate/frame/staking/fuzzer/src/mock.rs b/substrate/frame/staking/fuzzer/src/mock.rs index 921e0d3b48..d5ca78193b 100644 --- a/substrate/frame/staking/fuzzer/src/mock.rs +++ b/substrate/frame/staking/fuzzer/src/mock.rs @@ -119,7 +119,7 @@ impl pallet_session::SessionHandler for TestSessionHandler { _: &[(AccountId, Ks)], ) {} - fn on_disabled(_: usize) {} + fn on_disabled(_: u32) {} } impl pallet_session::Config for Test { diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 582e9e49bd..be02e8d91d 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -619,12 +619,9 @@ pub struct UnappliedSlash { /// /// This is needed because `Staking` sets the `ValidatorIdOf` of the `pallet_session::Config` pub trait SessionInterface: frame_system::Config { - /// Disable a given validator by stash ID. - /// - /// Returns `true` if new era should be forced at the end of this session. - /// This allows preventing a situation where there is too many validators - /// disabled and block production stalls. - fn disable_validator(validator: &AccountId) -> Result; + /// Disable the validator at the given index, returns `false` if the validator was already + /// disabled or the index is out of bounds. + fn disable_validator(validator_index: u32) -> bool; /// Get the validators from session. fn validators() -> Vec; /// Prune historical session tries up to but not including the given index. @@ -645,8 +642,8 @@ where Option<::AccountId>, >, { - fn disable_validator(validator: &::AccountId) -> Result { - >::disable(validator) + fn disable_validator(validator_index: u32) -> bool { + >::disable_index(validator_index) } fn validators() -> Vec<::AccountId> { diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 06c9be9c01..95d397359f 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -34,7 +34,7 @@ use sp_runtime::{ traits::{IdentityLookup, Zero}, }; use sp_staking::offence::{OffenceDetails, OnOffenceHandler}; -use std::{cell::RefCell, collections::HashSet}; +use std::cell::RefCell; pub const INIT_TIMESTAMP: u64 = 30_000; pub const BLOCK_TIME: u64 = 1000; @@ -45,10 +45,6 @@ pub(crate) type AccountIndex = u64; pub(crate) type BlockNumber = u64; pub(crate) type Balance = u128; -thread_local! { - static SESSION: RefCell<(Vec, HashSet)> = RefCell::new(Default::default()); -} - /// Another session handler struct to test on_disabled. pub struct OtherSessionHandler; impl OneSessionHandler for OtherSessionHandler { @@ -61,23 +57,14 @@ impl OneSessionHandler for OtherSessionHandler { { } - fn on_new_session<'a, I: 'a>(_: bool, validators: I, _: I) + fn on_new_session<'a, I: 'a>(_: bool, _: I, _: I) where I: Iterator, AccountId: 'a, { - SESSION.with(|x| { - *x.borrow_mut() = (validators.map(|x| x.0.clone()).collect(), HashSet::new()) - }); } - fn on_disabled(validator_index: usize) { - SESSION.with(|d| { - let mut d = d.borrow_mut(); - let value = d.0[validator_index]; - d.1.insert(value); - }) - } + fn on_disabled(_validator_index: u32) {} } impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler { @@ -86,7 +73,12 @@ impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler { pub fn is_disabled(controller: AccountId) -> bool { let stash = Staking::ledger(&controller).unwrap().stash; - SESSION.with(|d| d.borrow().1.contains(&stash)) + let validator_index = match Session::validators().iter().position(|v| *v == stash) { + Some(index) => index as u32, + None => return false, + }; + + Session::disabled_validators().contains(&validator_index) } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -171,7 +163,6 @@ impl pallet_balances::Config for Test { } parameter_types! { pub const UncleGenerations: u64 = 0; - pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(25); } sp_runtime::impl_opaque_keys! { pub struct SessionKeys { @@ -186,7 +177,6 @@ impl pallet_session::Config for Test { type Event = Event; type ValidatorId = AccountId; type ValidatorIdOf = crate::StashOf; - type DisabledValidatorsThreshold = DisabledValidatorsThreshold; type NextSessionRotation = pallet_session::PeriodicSessions; type WeightInfo = (); } @@ -224,6 +214,7 @@ parameter_types! { pub const BondingDuration: EraIndex = 3; pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS; pub const MaxNominatorRewardedPerValidator: u32 = 64; + pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(75); } thread_local! { @@ -277,6 +268,7 @@ impl crate::pallet::pallet::Config for Test { type EraPayout = ConvertCurve; type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; + type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); @@ -510,10 +502,6 @@ impl ExtBuilder { .assimilate_storage(&mut storage); let mut ext = sp_io::TestExternalities::from(storage); - ext.execute_with(|| { - let validators = Session::validators(); - SESSION.with(|x| *x.borrow_mut() = (validators.clone(), HashSet::new())); - }); if self.initialize_first_session { // We consider all test to start after timestamp is initialized This must be ensured by diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 3ae520872f..02099d8543 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -302,6 +302,13 @@ impl Pallet { Self::start_era(start_session); } } + + // disable all offending validators that have been disabled for the whole era + for (index, disabled) in >::get() { + if disabled { + T::SessionInterface::disable_validator(index); + } + } } /// End a session potentially ending an era. @@ -374,6 +381,9 @@ impl Pallet { // Set ending era reward. >::insert(&active_era.index, validator_payout); T::RewardRemainder::on_unbalanced(T::Currency::issue(rest)); + + // Clear offending validators. + >::kill(); } } diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index dad958ccae..8e97a90e07 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -141,6 +141,10 @@ pub mod pallet { #[pallet::constant] type MaxNominatorRewardedPerValidator: Get; + /// The fraction of the validator set that is safe to be offending. + /// After the threshold is reached a new era will be forced. + type OffendingValidatorsThreshold: Get; + /// Something that can provide a sorted list of voters in a somewhat sorted way. The /// original use case for this was designed with [`pallet_bags_list::Pallet`] in mind. If /// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option. @@ -437,6 +441,19 @@ pub mod pallet { #[pallet::getter(fn current_planned_session)] pub type CurrentPlannedSession = StorageValue<_, SessionIndex, ValueQuery>; + /// Indices of validators that have offended in the active era and whether they are currently + /// disabled. + /// + /// This value should be a superset of disabled validators since not all offences lead to the + /// validator being disabled (if there was no slash). This is needed to track the percentage of + /// validators that have offended in the current era, ensuring a new era is forced if + /// `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. + #[pallet::storage] + #[pallet::getter(fn offending_validators)] + pub type OffendingValidators = StorageValue<_, Vec<(u32, bool)>, ValueQuery>; + /// True if network has been upgraded to this version. /// Storage version of the pallet. /// diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 15ca85b4d0..68088d0e0d 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -56,7 +56,7 @@ use crate::{ use codec::{Decode, Encode}; use frame_support::{ ensure, - traits::{Currency, Imbalance, OnUnbalanced}, + traits::{Currency, Get, Imbalance, OnUnbalanced}, }; use scale_info::TypeInfo; use sp_runtime::{ @@ -278,15 +278,13 @@ pub(crate) fn compute_slash( // not continue in the next election. also end the slashing span. spans.end_span(now); >::chill_stash(stash); - - // make sure to disable validator till the end of this session - if T::SessionInterface::disable_validator(stash).unwrap_or(false) { - // force a new era, to select a new validator set - >::ensure_new_era() - } } } + // add the validator to the offenders list and make sure it is disabled for + // the duration of the era + add_offending_validator::(params.stash, true); + let mut nominators_slashed = Vec::new(); reward_payout += slash_nominators::(params, prior_slash_p, &mut nominators_slashed); @@ -316,13 +314,53 @@ fn kick_out_if_recent(params: SlashParams) { if spans.era_span(params.slash_era).map(|s| s.index) == Some(spans.span_index()) { spans.end_span(params.now); >::chill_stash(params.stash); - - // make sure to disable validator till the end of this session - if T::SessionInterface::disable_validator(params.stash).unwrap_or(false) { - // force a new era, to select a new validator set - >::ensure_new_era() - } } + + // add the validator to the offenders list but since there's no slash being + // applied there's no need to disable the validator + add_offending_validator::(params.stash, false); +} + +/// Add the given validator to the offenders list and optionally disable it. +/// If after adding the validator `OffendingValidatorsThreshold` is reached +/// a new era will be forced. +fn add_offending_validator(stash: &T::AccountId, disable: bool) { + as Store>::OffendingValidators::mutate(|offending| { + let validators = T::SessionInterface::validators(); + let validator_index = match validators.iter().position(|i| i == stash) { + Some(index) => index, + None => return, + }; + + let validator_index_u32 = validator_index as u32; + + match offending.binary_search_by_key(&validator_index_u32, |(index, _)| *index) { + // this is a new offending validator + Err(index) => { + offending.insert(index, (validator_index_u32, disable)); + + let offending_threshold = + T::OffendingValidatorsThreshold::get() * validators.len() as u32; + + if offending.len() >= offending_threshold as usize { + // force a new era, to select a new validator set + >::ensure_new_era() + } + + if disable { + T::SessionInterface::disable_validator(validator_index_u32); + } + }, + Ok(index) => { + if disable && !offending[index].1 { + // the validator had previously offended without being disabled, + // let's make sure we disable it now + offending[index].1 = true; + T::SessionInterface::disable_validator(validator_index_u32); + } + }, + } + }); } /// Slash nominators. Accepts general parameters and the prior slash percentage of the validator. diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 6f024eb1e6..d6d92d5bd5 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -2318,10 +2318,11 @@ fn slash_in_old_span_does_not_deselect() { 1, ); - // not forcing for zero-slash and previous span. - assert_eq!(Staking::force_era(), Forcing::NotForcing); - assert!(>::contains_key(11)); - assert!(Session::validators().contains(&11)); + // the validator doesn't get chilled again + assert!(::Validators::iter().find(|(stash, _)| *stash == 11).is_some()); + + // but we are still forcing a new era + assert_eq!(Staking::force_era(), Forcing::ForceNew); on_offence_in_era( &[OffenceDetails { @@ -2333,10 +2334,13 @@ fn slash_in_old_span_does_not_deselect() { 1, ); - // or non-zero. - assert_eq!(Staking::force_era(), Forcing::NotForcing); - assert!(>::contains_key(11)); - assert!(Session::validators().contains(&11)); + // the validator doesn't get chilled again + assert!(::Validators::iter().find(|(stash, _)| *stash == 11).is_some()); + + // but it's disabled + assert!(is_disabled(10)); + // and we are still forcing a new era + assert_eq!(Staking::force_era(), Forcing::ForceNew); }); } @@ -2967,6 +2971,132 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid }); } +#[test] +fn non_slashable_offence_doesnt_disable_validator() { + ExtBuilder::default().build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21]); + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + + // offence with no slash associated + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::zero()], + ); + + // offence that slashes 25% of the bond + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(25)], + ); + + // the offence for validator 10 wasn't slashable so it wasn't disabled + assert!(!is_disabled(10)); + // whereas validator 20 gets disabled + assert!(is_disabled(20)); + }); +} + +#[test] +fn offence_threshold_triggers_new_era() { + ExtBuilder::default() + .validator_count(4) + .set_status(41, StakerStatus::Validator) + .build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41]); + + assert_eq!( + ::OffendingValidatorsThreshold::get(), + Perbill::from_percent(75), + ); + + // we have 4 validators and an offending validator threshold of 75%, + // once the third validator commits an offence a new era should be forced + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + let exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31); + + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::zero()], + ); + + assert_eq!(ForceEra::::get(), Forcing::NotForcing); + + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::zero()], + ); + + assert_eq!(ForceEra::::get(), Forcing::NotForcing); + + on_offence_now( + &[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }], + &[Perbill::zero()], + ); + + assert_eq!(ForceEra::::get(), Forcing::ForceNew); + }); +} + +#[test] +fn disabled_validators_are_kept_disabled_for_whole_era() { + ExtBuilder::default() + .validator_count(4) + .set_status(41, StakerStatus::Validator) + .build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41]); + assert_eq!(::SessionsPerEra::get(), 3); + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::zero()], + ); + + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(25)], + ); + + // validator 10 should not be disabled since the offence wasn't slashable + assert!(!is_disabled(10)); + // validator 20 gets disabled since it got slashed + assert!(is_disabled(20)); + + advance_session(); + + // disabled validators should carry-on through all sessions in the era + assert!(!is_disabled(10)); + assert!(is_disabled(20)); + + // validator 10 should now get disabled + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::from_percent(25)], + ); + + advance_session(); + + // and both are disabled in the last session of the era + assert!(is_disabled(10)); + assert!(is_disabled(20)); + + mock::start_active_era(2); + + // when a new era starts disabled validators get cleared + assert!(!is_disabled(10)); + assert!(!is_disabled(20)); + }); +} + #[test] fn claim_reward_at_the_last_era_and_no_double_claim_and_invalid_claim() { // should check that: diff --git a/substrate/frame/support/src/traits/validation.rs b/substrate/frame/support/src/traits/validation.rs index 11ea5a79f6..674f2d718f 100644 --- a/substrate/frame/support/src/traits/validation.rs +++ b/substrate/frame/support/src/traits/validation.rs @@ -109,7 +109,7 @@ pub trait OneSessionHandler: BoundToRuntimeAppPublic { fn on_before_session_ending() {} /// A validator got disabled. Act accordingly until a new session begins. - fn on_disabled(_validator_index: usize); + fn on_disabled(_validator_index: u32); } /// Something that can estimate at which block the next session rotation will happen (i.e. a new