staking: only disable slashed validators and keep them disabled for whole era #9448
Conversation
andresilva
added
A0-please_review
andresilva
changed the title
andresilva
removed
the
A3-in_progress
frame/staking/src/tests.rs
Outdated
| // the validator doesn't get chilled again | ||
| assert!(<Staking as Store>::Validators::iter().find(|(stash, _)| *stash == 11).is_some()); | ||
|
|
||
| // but we are still forcing a new era | ||
| assert_eq!(Staking::force_era(), Forcing::ForceNew); |
There was a problem hiding this comment.
Please check that this makes sense, I changed the logic so that a validator still gets disabled when there's an offence but it has a new slashing span. It will not get chilled though which was the original behavior and only the slash value gets updated (if higher).
There was a problem hiding this comment.
it makes sense to me to still disable him. And so trigger a new era because threshold is reached. But @kianenigma should know better
andresilva
force-pushed
the
andre/staking-offences-era
branch
from
2cae9b9
to
5c6ee50
Compare
| /// `OffendingValidatorsThreshold` is reached. The set is cleared when the era ends. | ||
| #[pallet::storage] | ||
| #[pallet::getter(fn offending_validators)] | ||
| pub type OffendingValidators<T: Config> = StorageValue<_, Vec<(u32, bool)>, ValueQuery>; |
There was a problem hiding this comment.
| pub type OffendingValidators<T: Config> = StorageValue<_, Vec<(u32, bool)>, ValueQuery>; | |
| pub type OffendingValidators<T: Config> = StorageValue<_, BoundedVec<(u32, bool)>, ValueQuery>; |
Can you already make this a bounded vec? It will be something we will need to update and break in the future otherwise.
There was a problem hiding this comment.
note you will also need to add a bounding condition here
There was a problem hiding this comment.
The bounding condition would be the maximum number of validators but that's dynamic (MaxValidatorsCount storage item), so I'm not sure how I could go about using it here.
There was a problem hiding this comment.
We should have a static upper limit which even bounds the dynamic limit, and then if we need to go higher, we need to do a runtime upgrade.
Unfortunately, I think this is the only way...
There was a problem hiding this comment.
yeah probably using a weak bonded vec is the easiest. I see we are going to have MaxAuthorities in pallet-authority-discovery, pallet-aura. I guess we can have same in pallet-staking. And use this max to bound this storage.
Anyway we can do in a follow-up
|
|
||
| if offending.len() >= offending_threshold as usize { | ||
| // force a new era, to select a new validator set | ||
| <Pallet<T>>::ensure_new_era() |
There was a problem hiding this comment.
note to follow up here to understand if committing the changes to offending is needed in this scenario (as it is doing right now)
There was a problem hiding this comment.
This might have implications for the election provider. The test case estimate_next_election_works should be enough to ensure forcing a new era will not cause any issues.
There was a problem hiding this comment.
I think it is needed.
forcing a new era doesn't mean that next active session is the first session of a new era.
pallet_session is "delaying" session by one. So the when staking is configured with ForceNewEra then when pallet_session rotate session it will end the session (let's say) 3 (with end_session(3)), start the session 4 (with start_session(4)) and plan the session 5 (with new_session(5)) the session 5 will be the first session of the new era.
So we still need to disable the validators in the start_session(4)
|
@andresilva what's the plan for this? |
|
@kianenigma Sorry, I was out for the past 2 weeks 😊 I've updated the PR with current master and added comments regarding the sorted vecs. I think I've addressed all comments from review except using |
|
After merging this we need to make sure the polkadot companion isn't merged right away. I needed to make the companion use a temporary beefy branch. |
andresilva
added
D9-needsaudit 👮
|
After talking to @kianenigma we decided to upgrade this to |
* master: (125 commits) Update multiple dependencies (#9936) Speed up timestamp generation when logging (#9933) First word should be Substrate not Polkadot (#9935) Improved file not found error message (#9931) don't read events in elections anymore. (#9898) Remove incorrect sanity check (#9924) Require crypto scheme for `insert-key` (#9909) chore: refresh of the substrate_builder image (#9808) Introduce block authorship soft deadline (#9663) Rework Transaction Priority calculation (#9834) Do not propagate host RUSTFLAGS when checking for WASM toolchain (#9926) Small quoting comment fix (#9927) add clippy to CI (#9694) Ensure BeforeBestBlockBy voting rule accounts for base (#9920) rm `.maintain` lock (#9919) Downstream `node-template` pull (#9915) Implement core::fmt::Debug for BoundedVec (#9914) Quickly skip invalid transactions during block authorship. (#9789) Add SS58 prefix for Automata (#9805) Clean up sc-peerset (#9806) ...
redzsina
added
D1-audited 👍
|
bot merge |
|
Waiting for commit status. |
|
@ordian Thanks for updating the PR (and the companion)! |
ghost
merged commit 
This PR changes the logic for disabling validators and trigger a new era. Whenever a validator commits an offence it is now tracked in the staking pallet and whenever a threshold is reached a new era is started. Only offences that led to a slash lead to the validator being disabled (e.g. if the validator is offline just by itself it is not disabled). Additionally the staking pallet makes sure that disabled validators stay disabled for the whole era.
The reason to not disable validators when there's no slash is #9414 which will make sure that disabled validators can't author new blocks. If a validator is not slashed we can assume that the offence was not critical enough to block the validator from participating in consensus (it will be kicked from the set whenever the next era starts).
Fixes paritytech/polkadot#2088.
polkadot companion: paritytech/polkadot#3527