Adds migrations to restore currupted staking ledgers in Polkadot and Kusama #447
Conversation
|
This could just be an on chain referendum that calls the functions? |
We only can tell if a ledger needs force unstake at the time of recovery. We need to do the restore, check and potentially unstake atomically. In the time between the referenda are open and executed, new stashes can get in the situation that need to be force unstaked. |
|
@bkchr'ss comment still holds: the flaw you are pointing out, the ledgers changing between the time of proposal and enactment, also applies to the hardcoded list of ledgers here. From my understanding in previous talks, the goal of bringing this code here was that the original |
So there's two things at play here:
|
|
I see, you are right! I got a bit confused because I didn't look at the code and just checked the comments here. Then I would rephrase the asnwer to @bkchr as: The logic here is a bit more than just what is available in a Indeed a migration seems cleaner than my original suggestion of a temp pallet, good choice! |
Bkchr says that he didn't know that the calls need to happen atomically. Then it makes sense to have them as a migration. |
|
Hi @gpestana. Yesterday, we tried to stake DOT via a multisig account using Do you think this issue is related to the one this PR fixes, or is it a separate one? Note: our account isn't part of the list from this PR. |
|
@mrshiposha a |
|
Just for the record, the issue that @mrshiposha has raised is not related to corrupted ledgers. The BadState error was raised due to trying to bond with a unfunded account as stash. related to: paritytech/polkadot-sdk#5627 |
| T::AccountId::decode(&mut &Into::<[u8; 32]>::into(stash.clone())[..]) | ||
| .expect("32 bytes fits, qed."); |
There was a problem hiding this comment.
another way to build AccountId
AccountId::from_str("ESGsxFePah1qb96ooTU4QJMxMKUG7NZvgTig3eJxP9f3wLa").unwrap();
| // force unstake the ledger. | ||
| if ledger.total > T::Currency::free_balance(&stash_account) { | ||
| let slashing_spans = 10; // default slashing spans for migration. | ||
| let _ = pallet_staking::Pallet::<T>::force_unstake( |
There was a problem hiding this comment.
Would it be better to force unstake only ledger.total - free_balance? Or the rationale is these ledgers are the overwritten ones and should be removed?
| total_weight += weight; | ||
| } | ||
|
|
||
| log::info!(target: LOG_TARGET, "migrations::corrupted_ledgers: done"); |
There was a problem hiding this comment.
nit: useful generally to print a summary such as success: x, errored: y
Note: for more details on the corrupted ledgers issue and recovery steps check https://hackmd.io/m_h9DRutSZaUqCwM9tqZ3g?view.
This PR adds a migration in Polkadot and Kusama runtimes to recover the current corrupted ledgers in Polkadot and Kusama. A migration consists of:
pallet_staking::Pallet::<T>::restore_ledgerfor each of the "whitelisted" stashes asRootorigin.The reason to restore the corrupted ledgers as migrations implemented in the fellowship runtimes is twofold:
pallet_staking::Pallet::<T>::restore_ledgerand check +force_unstakemust 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).Call::Staking.restore_ledgerto ensure a restored ledger has enough free balance to cover staking locks paritytech/polkadot-sdk#5066 and 2. referenda to call intoCall::restore_ledgerfor 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.