Adds temporary pallet to fellowship runtimes to fix Staking corrupted ledgers #2
Conversation
gpestana
changed the title
| /// restored, the ledger locks are higher or equal than the stash's free balance. If not, it | ||
| /// forces the unstake of the ledger. | ||
| /// | ||
| /// Safety note: Only ledgers associated `stash` that are corrupted will be mutated. Thus it |
There was a problem hiding this comment.
So you are saying that the original extrinsic was needlessly restrictive, and it is generally okay to let anyone call these?
There was a problem hiding this comment.
The fn restore_ledger will fail with CannotRestoreLedger if the ledger state does not fall into one of the corrupted ledger cases (basically, gated by fn inspect_bond_state. The idea of requiring staking admin origin was to make it explicit through a referendum which ledgers were being restored (and for an extra layer of safety).
There was a problem hiding this comment.
To add more safety (i.e. not relying solely on fn inspect_bond_state to ensure that the ledger should be mutated and recovered), we add also a whitelisting mechanism so we can control which stashes need to be recovered in the configs based on the current on-chain state.
|
|
||
| // check if stash's free balance covers the current ledger's total amount. If not, | ||
| // force unstake the ledger. | ||
| let weight = if ledger.total > T::Currency::free_balance(&stash) { |
There was a problem hiding this comment.
How many staking ledgers fall into this category? if it is a handful that have to be unstaked, I would rather we hardcode their accounts, rather than 'codify' their conditions, so as to not risk this code path being accidentally accessibly to any future state.
There was a problem hiding this comment.
The problem with hardcoding stashes for this check is that the condition may change with time from now until we call the extrinsic.
However, we do hardcode the stash accounts that need to be restored now. Through that, we can ensure that only the ledgers that 1. are corrupted and 2. are whitelisted can be recovered, which adds another layer of safety. The whitelisted accounts are added in the runtime configs.
| @@ -818,6 +818,22 @@ impl pallet_staking::Config for Runtime { | |||
| type WeightInfo = weights::pallet_staking::WeightInfo<Runtime>; | |||
| } | |||
|
|
|||
| parameter_types! { | |||
| pub WhitelistedStashes: Vec<AccountId> = vec). The reason to restore the corrupted ledgers as migrations implemented in the fellowship runtimes is twofold: 1. The call to `pallet_staking::Pallet::<T>::restore_ledger` and check + `force_unstake` must be done atomically (thus a ledger can't be safely restored with a set of two distinct extrinsic calls, so it's not possible to use referenda to this fx). 2. To speed up the whole process and avoid having to wait for 1. merge and releases of paritytech/polkadot-sdk#5066 and 2. referenda to call into `Call::restore_ledger` for both Polkadot and Kusama. Alternatively, we could add a new temporary pallet directly in the fellowship runtime which would expose an extrinsic to restore the ledgers and perform the extra missing check. See this [PR as an example](gpestana#2). --- - [x] on-runtime-upgrade tests against Polkadot and Kusama - [x] staking try-state checks passing after all migrations.
https://hackmd.io/m_h9DRutSZaUqCwM9tqZ3g?view
Temporary pallet that exposes a new extrinsic
restore_ledgerwhich:Staking.restore_ledger;Staking.restore_ledger);relay/polkadotto be exposed to Kusama runtime too