[Staking] Allow nominators to be non-slashable and fast unbondable#10502
[Staking] Allow nominators to be non-slashable and fast unbondable#10502
Conversation
|
We do need minimum comission and maybe minimum self stake before that. |
Yes, this is the plan: deliver both min self stake + min commission (and support for external self-stake) together with changes for nominators (no slashing, 1-day unbonding) for March, together with the new issuance curve. |
…are not slashable
| let unbond_duration = if is_nominator { | ||
| <Self as sp_staking::StakingInterface>::nominator_bonding_duration() | ||
| } else { | ||
| T::BondingDuration::get() |
There was a problem hiding this comment.
rename the bonding_duration in StakingInterface to validator_bonding_duration.
There was a problem hiding this comment.
I wouldn't mind renaming BondingDuration to ValidatorBondingDuration, as we discussed, but it's not a hill I'm willing to die on 😅
sigurpol
left a comment
There was a problem hiding this comment.
Whole logic looks solid 🚀
Just some questions / suggestions.
I still have to go through the tests
| ErasNominatorsSlashable::<T>::insert(era, AreNominatorsSlashable::<T>::get()); | ||
|
|
||
| // Clean up old era entry that's now outside the bonding window | ||
| if let Some(old_era) = era.checked_sub(T::BondingDuration::get() + 1) { |
There was a problem hiding this comment.
we discussed already but I believe it's a bit cleaner if this method just set the value for the given era and no other side effects. I believe cleaning up fits very well in the lazy era pruning so I would just add something like
pub(crate) fn clear_era_information(era_index: EraIndex) {
// ... existing clears ...
ErasNominatorsSlashable::<T>::remove(era_index);
}Ok, we might end up keeping information for 84 eras and not 28 - but it's 1 bool per era so very neglibile
There was a problem hiding this comment.
But now we use the lazy pruning right? I looked at it and it seems we are having individual steps for each storage, but I think its a bit overkill to break down single storage kills into different steps. To align with current approach, I can do the same, but wondering if you think we should refactor and combine single storage kills in one step?
There was a problem hiding this comment.
yes I believe it would definitely make sense to combine all the cheap operation like single storage kills in a single step!!!!
There was a problem hiding this comment.
Some findings
- Lazy pruning works only with history depth (84 eras) based era cleanups.
- There is an existing era based storage (ValidatorsSlashInEra cleaned up every 28 eras) that is not lazily cleaned, and is unbounded (though implicitly bounded by max validators, or if we assume 67% honest validators then 33% of validator set size).
I have few options:
- Keep bonded era (28 eras) based cleanups together (ValidatorSlashInEra + ErasNominatorsSlashable + BondedEras) and eager.
- Move bonded era cleanups also along with lazy pruning that cleans it up 84 eras (I am leaning towards this).
- Create another separate pruning mechanism for bonded era based storages.
@sigurpol any thoughts?
There was a problem hiding this comment.
Move bonded era cleanups also along with lazy pruning that cleans it up 84 eras (I am leaning towards this).
this seems the cleanest solution imho
| let unbond_duration = if is_nominator { | ||
| <Self as sp_staking::StakingInterface>::nominator_bonding_duration() | ||
| } else { | ||
| T::BondingDuration::get() |
There was a problem hiding this comment.
I wouldn't mind renaming BondingDuration to ValidatorBondingDuration, as we discussed, but it's not a hill I'm willing to die on 😅
|
feedback as outcome of the discussion with @Ank4n , @michalisFr , Jonas: let's say we are at era 100 and planning for 101. Snapshot for 101 is taken at session 1 of era 100. If you (nominator) unbond at Era 100 no matter when, then you are exposed for Era 101 and therefore unlock should happen at era 102 so at most 2d after the unbond. Basically you are always unlocked at the end of next era (so at era 102 in the example). |
kianenigma
left a comment
There was a problem hiding this comment.
Perfectly done! thanks for bearing with my questions, in the main one I was hallucinating :)
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10502-to-stable2512
git worktree add --checkout .worktree/backport-10502-to-stable2512 backport-10502-to-stable2512
cd .worktree/backport-10502-to-stable2512
git reset --hard HEAD^
git cherry-pick -x b3bfba618e98f2aa10ee8d4233a15c1e09fef50f
git push --force-with-lease |
Backport #10502 into `stable2512` from Ank4n. NOTE: this PR introduces a breaking change in the signature of the extrinsic `set_staking_configs` that now has a new extra input parameter: `are_nominators_slashable`. This is needed to be able to deliver the feature of having non slashable nominator past March 14th. Because of that, we have put `validate:false` in the related section of the prdoc around pallet-staking-async. Please note that only Root origin can call `set_staking_configs` so the practical impact of the breaking change is basically zero. We need a Governance referendum to tune the parameters and set nominators as non slashable around March 14th so we need OpenGov to actually use the new version to be able to deliver the requested feature, we can't really avoid that. Adding an extra extrinsic just to avoid the breaking change seems an unnecessary overkill. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> Co-authored-by: Ankan <ankan.anurag@gmail.com> Co-authored-by: Egor_P <egor@parity.io> Co-authored-by: Paolo La Camera <paolo@parity.io>
Context
We want to make nominators unslashable (configurable via a storage set), and once they are unslashable, they can also unbond and withdraw in 2 era instead of the full BondingDuration (28 eras).
Storage Changes
AreNominatorsSlashable: StorageValue<bool>(default: true): Runtime-configurable flag. Made this a storage value (not a config constant) so it can be enabled together with MinValidatorBond and MinCommission viaset_staking_configs.ErasNominatorsSlashable: StorageMap<EraIndex, bool>(default: true): Per-era snapshot of slashability setting. This ensures offences are processed with the rules that were in effect at the time of the offence, not the current rules. Cleaned up automatically for eras outside bonding window.LastValidatorErato track if a staker was a validator in a recent era and hence needs to follow full unbonding time. Does not need migration as long as we disable nominator slash (in other words: reduce their unbond time) at least one era after these changes are applied.Slashing logic
process_offence_validator_onlyas a separate code path instead of overloading the same function. Seeprocess_offence_for_erainsubstrate/frame/staking-async/src/slashing.rs.Unbonding logic:
NominatorFastUnbondDurationthat determines the fast unbond duration (recommended value: 2 eras) when nominators are not slashable.nominator_bonding_duration()toStakingInterfacetrait (returnsNominatorFastUnbondDurationera when not slashable, full unbond duration otherwise).nominator_bonding_duration(), so pool members also benefit from fast unbonding.InsufficientBonderrors.Era pruning:
ValidatorSlashInEraas well asErasNominatorsSlashablein lazy pruning. This has a minor (I believe acceptable) side effect that they will be cleaned up in 84 eras instead of 28 eras.TODO
MinCommission,MinValidatorBond, andAreNominatorsSlashable.slash_nominatorcompletely.