Extrinsic to restore corrupt staking ledgers (#3706)

This PR adds a new extrinsic `Call::restore_ledger ` gated by
`StakingAdmin` origin that restores a corrupted staking ledger. This
extrinsic will be used to recover ledgers that were affected by the
issue discussed in
https://github.com/paritytech/polkadot-sdk/issues/3245.

The extrinsic will re-write the storage items associated with a stash
account provided as input parameter. The data used to reset the ledger
can be either i) fetched on-chain or ii) partially/totally set by the
input parameters of the call.

In order to use on-chain data to restore the staking locks, we need a
way to read the current lock in the balances pallet. This PR adds a
`InspectLockableCurrency` trait and implements it in the pallet
balances. An alternative would be to tightly couple staking with the
pallet balances but that's inelegant (an example of how it would look
like in [this
branch](https://github.com/paritytech/polkadot-sdk/tree/gpestana/ledger-badstate-clean_tightly)).

More details on the type of corruptions and corresponding fixes
https://hackmd.io/DLb5jEYWSmmvqXC9ae4yRg?view#/

We verified that the `Call::restore_ledger` does fix all current
corrupted ledgers in Polkadot and Kusama. You can verify it here
https://hackmd.io/v-XNrEoGRpe7APR-EZGhOA.

**Changes introduced**
- Adds `Call::restore_ledger ` extrinsic to recover a corrupted ledger;
- Adds trait `frame_support::traits::currency::InspectLockableCurrency`
to allow external pallets to read current locks given an account and
lock ID;
- Implements the `InspectLockableCurrency` in the pallet-balances.
- Adds staking locks try-runtime checks
(https://github.com/paritytech/polkadot-sdk/issues/3751)

**Todo**
- [x] benchmark `Call::restore_ledger`
- [x] throughout testing of all ledger recovering cases
- [x] consider adding the staking locks try-runtime checks to this PR
(https://github.com/paritytech/polkadot-sdk/issues/3751)
- [x] simulate restoring all ledgers
(https://hackmd.io/Dsa2tvhISNSs7zcqriTaxQ?view) in Polkadot and Kusama
using chopsticks -- https://hackmd.io/v-XNrEoGRpe7APR-EZGhOA

Related to https://github.com/paritytech/polkadot-sdk/issues/3245
Closes https://github.com/paritytech/polkadot-sdk/issues/3751

---------

Co-authored-by: command-bot <>
This commit is contained in:
Gonçalo Pestana
2024-03-27 18:20:24 +01:00
committed by GitHub
parent 374aefa4f2
commit bbdbeb7ec6
15 changed files with 1201 additions and 486 deletions
+416 -30
View File
@@ -6933,40 +6933,43 @@ mod ledger {
setup_double_bonded_ledgers();
// Case 1: double bonded but not corrupted:
// stash 2 has controller 3:
assert_eq!(Bonded::<Test>::get(2), Some(3));
assert_eq!(Ledger::<Test>::get(3).unwrap().stash, 2);
// stash 444 has controller 555:
assert_eq!(Bonded::<Test>::get(444), Some(555));
assert_eq!(Ledger::<Test>::get(555).unwrap().stash, 444);
// stash 2 is also a controller of 1:
assert_eq!(Bonded::<Test>::get(1), Some(2));
assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(1)), Some(2));
assert_eq!(Ledger::<Test>::get(2).unwrap().stash, 1);
// stash 444 is also a controller of 333:
assert_eq!(Bonded::<Test>::get(333), Some(444));
assert_eq!(
StakingLedger::<Test>::paired_account(StakingAccount::Stash(333)),
Some(444)
);
assert_eq!(Ledger::<Test>::get(444).unwrap().stash, 333);
// although 2 is double bonded (it is a controller and a stash of different ledgers),
// although 444 is double bonded (it is a controller and a stash of different ledgers),
// we can safely retrieve the ledger and mutate it since the correct ledger is
// returned.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Stash(2));
assert_eq!(ledger_result.unwrap().stash, 2); // correct ledger.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Stash(444));
assert_eq!(ledger_result.unwrap().stash, 444); // correct ledger.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Controller(2));
assert_eq!(ledger_result.unwrap().stash, 1); // correct ledger.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Controller(444));
assert_eq!(ledger_result.unwrap().stash, 333); // correct ledger.
// fetching ledger 1 by its stash works.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Stash(1));
assert_eq!(ledger_result.unwrap().stash, 1);
// fetching ledger 333 by its stash works.
let ledger_result = StakingLedger::<Test>::get(StakingAccount::Stash(333));
assert_eq!(ledger_result.unwrap().stash, 333);
// Case 2: corrupted ledger bonding.
// in this case, we simulate what happens when fetching a ledger by stash returns a
// ledger with a different stash. when this happens, we return an error instead of the
// ledger to prevent ledger mutations.
let mut ledger = Ledger::<Test>::get(2).unwrap();
assert_eq!(ledger.stash, 1);
ledger.stash = 2;
Ledger::<Test>::insert(2, ledger);
let mut ledger = Ledger::<Test>::get(444).unwrap();
assert_eq!(ledger.stash, 333);
ledger.stash = 444;
Ledger::<Test>::insert(444, ledger);
// now, we are prevented from fetching the ledger by stash from 1. It's associated
// controller (2) is now bonding a ledger with a different stash (2, not 1).
assert!(StakingLedger::<Test>::get(StakingAccount::Stash(1)).is_err());
assert!(StakingLedger::<Test>::get(StakingAccount::Stash(333)).is_err());
})
}
@@ -7069,7 +7072,7 @@ mod ledger {
#[test]
fn deprecate_controller_batch_works_full_weight() {
ExtBuilder::default().build_and_execute(|| {
ExtBuilder::default().try_state(false).build_and_execute(|| {
// Given:
let start = 1001;
@@ -7253,7 +7256,7 @@ mod ledger {
let bounded_controllers: BoundedVec<
_,
<Test as Config>::MaxControllersInDeprecationBatch,
> = BoundedVec::try_from(vec![1, 2, 3, 4]).unwrap();
> = BoundedVec::try_from(vec![333, 444, 555, 777]).unwrap();
assert_ok!(Staking::deprecate_controller_batch(
RuntimeOrigin::root(),
@@ -7276,7 +7279,7 @@ mod ledger {
let bounded_controllers: BoundedVec<
_,
<Test as Config>::MaxControllersInDeprecationBatch,
> = BoundedVec::try_from(vec![4, 3, 2, 1]).unwrap();
> = BoundedVec::try_from(vec![777, 555, 444, 333]).unwrap();
assert_ok!(Staking::deprecate_controller_batch(
RuntimeOrigin::root(),
@@ -7296,9 +7299,9 @@ mod ledger {
setup_double_bonded_ledgers();
// in this case, setting controller works due to the ordering of the calls.
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(2)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(3)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(333)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(444)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(555)));
})
}
@@ -7307,17 +7310,400 @@ mod ledger {
ExtBuilder::default().has_stakers(false).try_state(false).build_and_execute(|| {
setup_double_bonded_ledgers();
// setting the controller of ledger associated with stash 3 fails since its stash is a
// setting the controller of ledger associated with stash 555 fails since its stash is a
// controller of another ledger.
assert_noop!(
Staking::set_controller(RuntimeOrigin::signed(3)),
Staking::set_controller(RuntimeOrigin::signed(555)),
Error::<Test>::BadState
);
assert_noop!(
Staking::set_controller(RuntimeOrigin::signed(2)),
Staking::set_controller(RuntimeOrigin::signed(444)),
Error::<Test>::BadState
);
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
assert_ok!(Staking::set_controller(RuntimeOrigin::signed(333)));
})
}
}
mod ledger_recovery {
use super::*;
use frame_support::traits::InspectLockableCurrency;
#[test]
fn inspect_recovery_ledger_simple_works() {
ExtBuilder::default().has_stakers(true).try_state(false).build_and_execute(|| {
setup_double_bonded_ledgers();
// non corrupted ledger.
assert_eq!(Staking::inspect_bond_state(&11).unwrap(), LedgerIntegrityState::Ok);
// non bonded stash.
assert!(Bonded::<Test>::get(&1111).is_none());
assert!(Staking::inspect_bond_state(&1111).is_err());
// double bonded but not corrupted.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
})
}
#[test]
fn inspect_recovery_ledger_corupted_killed_works() {
ExtBuilder::default().has_stakers(true).try_state(false).build_and_execute(|| {
setup_double_bonded_ledgers();
let lock_333_before = Balances::balance_locked(crate::STAKING_ID, &333);
// get into corrupted and killed ledger state by killing a corrupted ledger:
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// kill(333)
// (444, 444) -> corrupted and None.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// 333 is corrupted since it's controller is linking 444 ledger.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Corrupted);
// 444 however is OK.
assert_eq!(Staking::inspect_bond_state(&444).unwrap(), LedgerIntegrityState::Ok);
// kill the corrupted ledger that is associated with stash 333.
assert_ok!(StakingLedger::<Test>::kill(&333));
// 333 bond is no more but it returns `BadState` because the lock on this stash is
// still set (see checks below).
assert_eq!(Staking::inspect_bond_state(&333), Err(Error::<Test>::BadState));
// now the *other* ledger associated with 444 has been corrupted and killed (None).
assert_eq!(
Staking::inspect_bond_state(&444),
Ok(LedgerIntegrityState::CorruptedKilled)
);
// side effects on 333 - ledger, bonded, payee, lock should be completely empty.
// however, 333 lock remains.
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), lock_333_before); // NOK
assert!(Bonded::<Test>::get(&333).is_none()); // OK
assert!(Payee::<Test>::get(&333).is_none()); // OK
assert!(Ledger::<Test>::get(&444).is_none()); // OK
// side effects on 444 - ledger, bonded, payee, lock should remain be intact.
// however, 444 lock was removed.
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &444), 0); // NOK
assert!(Bonded::<Test>::get(&444).is_some()); // OK
assert!(Payee::<Test>::get(&444).is_some()); // OK
assert!(Ledger::<Test>::get(&555).is_none()); // NOK
assert!(Staking::do_try_state(System::block_number()).is_err());
})
}
#[test]
fn inspect_recovery_ledger_corupted_killed_other_works() {
ExtBuilder::default().has_stakers(true).try_state(false).build_and_execute(|| {
setup_double_bonded_ledgers();
let lock_333_before = Balances::balance_locked(crate::STAKING_ID, &333);
// get into corrupted and killed ledger state by killing a corrupted ledger:
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// kill(444)
// (333, 444) -> corrupted and None
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// 333 is corrupted since it's controller is linking 444 ledger.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Corrupted);
// 444 however is OK.
assert_eq!(Staking::inspect_bond_state(&444).unwrap(), LedgerIntegrityState::Ok);
// kill the *other* ledger that is double bonded but not corrupted.
assert_ok!(StakingLedger::<Test>::kill(&444));
// now 333 is corrupted and None through the *other* ledger being killed.
assert_eq!(
Staking::inspect_bond_state(&333).unwrap(),
LedgerIntegrityState::CorruptedKilled,
);
// 444 is cleaned and not a stash anymore; no lock left behind.
assert_eq!(Ledger::<Test>::get(&444), None);
assert_eq!(Staking::inspect_bond_state(&444), Err(Error::<Test>::NotStash));
// side effects on 333 - ledger, bonded, payee, lock should be intact.
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), lock_333_before); // OK
assert_eq!(Bonded::<Test>::get(&333), Some(444)); // OK
assert!(Payee::<Test>::get(&333).is_some()); // OK
// however, ledger associated with its controller was killed.
assert!(Ledger::<Test>::get(&444).is_none()); // NOK
// side effects on 444 - ledger, bonded, payee, lock should be completely removed.
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &444), 0); // OK
assert!(Bonded::<Test>::get(&444).is_none()); // OK
assert!(Payee::<Test>::get(&444).is_none()); // OK
assert!(Ledger::<Test>::get(&555).is_none()); // OK
assert!(Staking::do_try_state(System::block_number()).is_err());
})
}
#[test]
fn inspect_recovery_ledger_lock_corrupted_works() {
ExtBuilder::default().has_stakers(true).try_state(false).build_and_execute(|| {
setup_double_bonded_ledgers();
// get into lock corrupted ledger state by bond_extra on a ledger that is double bonded
// with a corrupted ledger.
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// bond_extra(333, 10) -> lock corrupted on 444
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
bond_extra_no_checks(&333, 10);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// 333 is corrupted since it's controller is linking 444 ledger.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Corrupted);
// 444 ledger is not corrupted but locks got out of sync.
assert_eq!(
Staking::inspect_bond_state(&444).unwrap(),
LedgerIntegrityState::LockCorrupted
);
})
}
// Corrupted ledger restore.
//
// * Double bonded and corrupted ledger.
#[test]
fn restore_ledger_corrupted_works() {
ExtBuilder::default().has_stakers(true).build_and_execute(|| {
setup_double_bonded_ledgers();
// get into corrupted and killed ledger state.
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Corrupted);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// recover the ledger bonded by 333 stash.
assert_ok!(Staking::restore_ledger(RuntimeOrigin::root(), 333, None, None, None));
// try-state checks are ok now.
assert_ok!(Staking::do_try_state(System::block_number()));
})
}
// Corrupted and killed ledger restore.
//
// * Double bonded and corrupted ledger.
// * Ledger killed by own controller.
#[test]
fn restore_ledger_corrupted_killed_works() {
ExtBuilder::default().has_stakers(true).build_and_execute(|| {
setup_double_bonded_ledgers();
// ledger.total == lock
let total_444_before_corruption = Balances::balance_locked(crate::STAKING_ID, &444);
// get into corrupted and killed ledger state by killing a corrupted ledger:
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// kill(333)
// (444, 444) -> corrupted and None.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
// kill the corrupted ledger that is associated with stash 333.
assert_ok!(StakingLedger::<Test>::kill(&333));
// 333 bond is no more but it returns `BadState` because the lock on this stash is
// still set (see checks below).
assert_eq!(Staking::inspect_bond_state(&333), Err(Error::<Test>::BadState));
// now the *other* ledger associated with 444 has been corrupted and killed (None).
assert!(Staking::ledger(StakingAccount::Stash(444)).is_err());
// try-state should fail.
assert!(Staking::do_try_state(System::block_number()).is_err());
// recover the ledger bonded by 333 stash.
assert_ok!(Staking::restore_ledger(RuntimeOrigin::root(), 333, None, None, None));
// for the try-state checks to pass, we also need to recover the stash 444 which is
// corrupted too by proxy of kill(333). Currently, both the lock and the ledger of 444
// have been cleared so we need to provide the new amount to restore the ledger.
assert_noop!(
Staking::restore_ledger(RuntimeOrigin::root(), 444, None, None, None),
Error::<Test>::CannotRestoreLedger
);
assert_ok!(Staking::restore_ledger(
RuntimeOrigin::root(),
444,
None,
Some(total_444_before_corruption),
None,
));
// try-state checks are ok now.
assert_ok!(Staking::do_try_state(System::block_number()));
})
}
// Corrupted and killed by *other* ledger restore.
//
// * Double bonded and corrupted ledger.
// * Ledger killed by own controller.
#[test]
fn restore_ledger_corrupted_killed_other_works() {
ExtBuilder::default().has_stakers(true).build_and_execute(|| {
setup_double_bonded_ledgers();
// get into corrupted and killed ledger state by killing a corrupted ledger:
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// kill(444)
// (333, 444) -> corrupted and None
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// 333 is corrupted since it's controller is linking 444 ledger.
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Corrupted);
// 444 however is OK.
assert_eq!(Staking::inspect_bond_state(&444).unwrap(), LedgerIntegrityState::Ok);
// kill the *other* ledger that is double bonded but not corrupted.
assert_ok!(StakingLedger::<Test>::kill(&444));
// recover the ledger bonded by 333 stash.
assert_ok!(Staking::restore_ledger(RuntimeOrigin::root(), 333, None, None, None));
// 444 does not need recover in this case since it's been killed successfully.
assert_eq!(Staking::inspect_bond_state(&444), Err(Error::<Test>::NotStash));
// try-state checks are ok now.
assert_ok!(Staking::do_try_state(System::block_number()));
})
}
// Corrupted with bond_extra.
//
// * Double bonded and corrupted ledger.
// * Corrupted ledger calls `bond_extra`
#[test]
fn restore_ledger_corrupted_bond_extra_works() {
ExtBuilder::default().has_stakers(true).build_and_execute(|| {
setup_double_bonded_ledgers();
let lock_333_before = Balances::balance_locked(crate::STAKING_ID, &333);
let lock_444_before = Balances::balance_locked(crate::STAKING_ID, &444);
// get into corrupted and killed ledger state by killing a corrupted ledger:
// init state:
// (333, 444)
// (444, 555)
// set_controller(444) to 444
// (333, 444) -> corrupted
// (444, 444)
// bond_extra(444, 40) -> OK
// bond_extra(333, 30) -> locks out of sync
assert_eq!(Staking::inspect_bond_state(&333).unwrap(), LedgerIntegrityState::Ok);
set_controller_no_checks(&444);
// now try-state fails.
assert!(Staking::do_try_state(System::block_number()).is_err());
// if 444 bonds extra, the locks remain in sync.
bond_extra_no_checks(&444, 40);
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), lock_333_before);
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &444), lock_444_before + 40);
// however if 333 bonds extra, the wrong lock is updated.
bond_extra_no_checks(&333, 30);
assert_eq!(
Balances::balance_locked(crate::STAKING_ID, &333),
lock_444_before + 40 + 30
); //not OK
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &444), lock_444_before + 40); // OK
// recover the ledger bonded by 333 stash. Note that the total/lock needs to be
// re-written since on-chain data lock has become out of sync.
assert_ok!(Staking::restore_ledger(
RuntimeOrigin::root(),
333,
None,
Some(lock_333_before + 30),
None
));
// now recover 444 that although it's not corrupted, its lock and ledger.total are out
// of sync. in which case, we need to explicitly set the ledger's lock and amount,
// otherwise the ledger recover will fail.
assert_noop!(
Staking::restore_ledger(RuntimeOrigin::root(), 444, None, None, None),
Error::<Test>::CannotRestoreLedger
);
//and enforcing a new ledger lock/total on this non-corrupted ledger will work.
assert_ok!(Staking::restore_ledger(
RuntimeOrigin::root(),
444,
None,
Some(lock_444_before + 40),
None
));
// double-check that ledgers got to expected state and bond_extra done during the
// corrupted state is part of the recovered ledgers.
let ledger_333 = Bonded::<Test>::get(&333).and_then(Ledger::<Test>::get).unwrap();
let ledger_444 = Bonded::<Test>::get(&444).and_then(Ledger::<Test>::get).unwrap();
assert_eq!(ledger_333.total, lock_333_before + 30);
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), ledger_333.total);
assert_eq!(ledger_444.total, lock_444_before + 40);
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &444), ledger_444.total);
// try-state checks are ok now.
assert_ok!(Staking::do_try_state(System::block_number()));
})
}
}