Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a migration helper for reducing HistoryDepth config value in staking pallet #452

Open
Ank4n opened this issue Sep 21, 2022 · 3 comments

Comments

@Ank4n
Copy link
Contributor

Ank4n commented Sep 21, 2022

PR paritytech/substrate#12230 introduced HistoryDepth as a config value in staking pallet which bounds several storage item in the pallet. In order to lower this value for a runtime once it has been already set, we need to handle migration of the storage items affected.

Following storage items are affected by History Depth
ErasStakers, ErasStakersClipped, ErasValidatorPrefs, ErasValidatorReward, ErasRewardPoints, ErasTotalStake, ErasStartSessionIndex, StakingLedger.claimed_rewards

We should add a generic migration function fn migrate_history_depth(from: u32, to: u32) -> Weight and put it in staking::migrations that can be used by a future migrations to reduce the HistoryDepth gracefully.

We should also look into reducing the current HistoryDepth = 84 on Polkadot and Kusama once we have this.

cc: @kianenigma

@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 22, 2022

@kianenigma Not sure if its a nonsense idea. But I am thinking wouldn't it be great if there is a hook in the pallets like fn on_config_change(key, old_val, new_val) whenever runtime is upgraded with a new configuration value. And in this hook, we should be able to take care of these hidden/opaque migrations that are needed on config change, such as reducing HistoryDepth.

I am assuming we will have lots of cases like this elsewhere in pallets since these config values are used to bound storage items and I think its easy for a developer to not go through docs and believe a config change would be safe.

@kianenigma
Copy link
Contributor

I like the idea overall, but the reason that we have so far tried to not automate things too much about this has been that doing such things in a blockchain is really hard. For example, if we write this script, perhaps in a blockchain it takes a lot of weight to do this whole migration.

Our simplistic strategy so far has been to "let the developer deal with it", not because we couldn't do it for them, but mainly because they can probably do it better.

I think was a reasonable approach in substrate in the early adoption days. I am happy to explore more automated features like this.

@kianenigma
Copy link
Contributor

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📕 Backlog
Development

No branches or pull requests

2 participants