From 6524144cfa1cead63c99f4860e933110e4be90f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Thu, 14 Mar 2024 10:49:46 +0100 Subject: [PATCH] Staking ledger bonding fixes (#3639) Currently, the staking logic does not prevent a controller from becoming a stash of *another* ledger (introduced by [removing this check](https://github.com/paritytech/polkadot-sdk/pull/1484/files#diff-3aa6ceab5aa4e0ab2ed73a7245e0f5b42e0832d8ca5b1ed85d7b2a52fb196524L850)). Given that the remaining of the code expects that never happens, bonding a ledger with a stash that is a controller of another ledger may lead to data inconsistencies and data losses in bonded ledgers. For more detailed explanation of this issue: https://hackmd.io/@gpestana/HJoBm2tqo/%2FTPdi28H7Qc2mNUqLSMn15w In a nutshell, when fetching a ledger with a given controller, we may be end up getting the wrong ledger which can lead to unexpected ledger states. This PR also ensures that `set_controller` does not lead to data inconsistencies in the staking ledger and bonded storage in the case when a controller of a stash is a stash of *another* ledger. and improves the staking `try-runtime` checks to catch potential issues with the storage preemptively. In summary, there are two important cases here: 1. **"Sane" double bonded ledger** When a controller of a ledger is a stash of *another* ledger. In this case, we have: ``` > Bonded(stash, controller) (A, B) // stash A with controller B (B, C) // B is also a stash of another ledger (C, D) > Ledger(controller) Ledger(B) = L_a (stash = A) Ledger(C) = L_b (stash = B) Ledger(D) = L_c (stash = C) ``` In this case, the ledgers can be mutated and all operations are OK. However, we should not allow `set_controller` to be called if it means it results in a "corrupt" double bonded ledger (see below). 3. **"Corrupt" double bonded ledger** ``` > Bonded(stash, controller) (A, B) // stash A with controller B (B, B) (C, D) ``` In this case, B is a stash and controller AND is corrupted, since B is responsible for 2 ledgers which is not correct and will lead to inconsistent states. Thus, in this case, in this PR we are preventing these ledgers from mutating (i.e. operations like bonding extra etc) until the ledger is brought back to a consistent state. --- **Changes**: - Checks if stash is already a controller when calling `Call::bond` (fixes the regression introduced by [removing this check](https://github.com/paritytech/polkadot-sdk/pull/1484/files#diff-3aa6ceab5aa4e0ab2ed73a7245e0f5b42e0832d8ca5b1ed85d7b2a52fb196524L850)); - Ensures that all fetching ledgers from storage are done through the `StakingLedger` API; - Ensures that -- when fetching a ledger from storage using the `StakingLedger` API --, a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g. `bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies. - Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state. - Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata. Related to https://github.com/paritytech/polkadot-sdk/issues/3245 --------- Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma --- prdoc/pr_3639.prdoc | 19 +++ substrate/frame/staking/src/ledger.rs | 61 +++++++- substrate/frame/staking/src/mock.rs | 64 +++++++- substrate/frame/staking/src/pallet/impls.rs | 98 +++++++++++- substrate/frame/staking/src/pallet/mod.rs | 28 ++-- substrate/frame/staking/src/tests.rs | 164 +++++++++++++++++++- 6 files changed, 407 insertions(+), 27 deletions(-) create mode 100644 prdoc/pr_3639.prdoc diff --git a/prdoc/pr_3639.prdoc b/prdoc/pr_3639.prdoc new file mode 100644 index 000000000000..46e3f6f4d763 --- /dev/null +++ b/prdoc/pr_3639.prdoc @@ -0,0 +1,19 @@ +title: Prevents staking controllers from becoming stashes of different ledgers; Ensures that no ledger in bad state is mutated. + +doc: + - audience: Runtime User + description: | + This PR introduces a fix to the staking logic which prevents an existing controller from bonding as a stash of another ledger, which + lead to staking ledger inconsistencies down the line. In addition, it adds a few (temporary) gates to prevent ledgers that are already + in a bad state from mutating its state. + + In summary: + * Checks if stash is already a controller when calling `Call::bond` and fails if that's the case; + * Ensures that all fetching ledgers from storage are done through the `StakingLedger` API; + * Ensures that a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g. + `bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies. + * Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state. + * Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata. + +crates: +- name: pallet-staking diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 84bb4d167dcb..5f74d36b0407 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -32,8 +32,8 @@ //! state consistency. use frame_support::{ - defensive, - traits::{LockableCurrency, WithdrawReasons}, + defensive, ensure, + traits::{Defensive, LockableCurrency, WithdrawReasons}, }; use sp_staking::StakingAccount; use sp_std::prelude::*; @@ -106,18 +106,39 @@ impl StakingLedger { /// This getter can be called with either a controller or stash account, provided that the /// account is properly wrapped in the respective [`StakingAccount`] variant. This is meant to /// abstract the concept of controller/stash accounts from the caller. + /// + /// Returns [`Error::BadState`] when a bond is in "bad state". A bond is in a bad state when a + /// stash has a controller which is bonding a ledger associated with another stash. pub(crate) fn get(account: StakingAccount) -> Result, Error> { - let controller = match account { - StakingAccount::Stash(stash) => >::get(stash).ok_or(Error::::NotStash), - StakingAccount::Controller(controller) => Ok(controller), - }?; + let (stash, controller) = match account.clone() { + StakingAccount::Stash(stash) => + (stash.clone(), >::get(&stash).ok_or(Error::::NotStash)?), + StakingAccount::Controller(controller) => ( + Ledger::::get(&controller) + .map(|l| l.stash) + .ok_or(Error::::NotController)?, + controller, + ), + }; - >::get(&controller) + let ledger = >::get(&controller) .map(|mut ledger| { ledger.controller = Some(controller.clone()); ledger }) - .ok_or(Error::::NotController) + .ok_or(Error::::NotController)?; + + // if ledger bond is in a bad state, return error to prevent applying operations that may + // further spoil the ledger's state. A bond is in bad state when the bonded controller is + // associted with a different ledger (i.e. a ledger with a different stash). + // + // See for more details. + ensure!( + Bonded::::get(&stash) == Some(controller) && ledger.stash == stash, + Error::::BadState + ); + + Ok(ledger) } /// Returns the reward destination of a staking ledger, stored in [`Payee`]. @@ -201,6 +222,30 @@ impl StakingLedger { } } + /// Sets the ledger controller to its stash. + pub(crate) fn set_controller_to_stash(self) -> Result<(), Error> { + let controller = self.controller.as_ref() + .defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.") + .ok_or(Error::::NotController)?; + + ensure!(self.stash != *controller, Error::::AlreadyPaired); + + // check if the ledger's stash is a controller of another ledger. + if let Some(bonded_ledger) = Ledger::::get(&self.stash) { + // there is a ledger bonded by the stash. In this case, the stash of the bonded ledger + // should be the same as the ledger's stash. Otherwise fail to prevent data + // inconsistencies. See for more + // details. + ensure!(bonded_ledger.stash == self.stash, Error::::BadState); + } + + >::remove(&controller); + >::insert(&self.stash, &self); + >::insert(&self.stash, &self.stash); + + Ok(()) + } + /// Clears all data related to a staking ledger and its bond in both [`Ledger`] and [`Bonded`] /// storage items and updates the stash staking lock. pub(crate) fn kill(stash: &T::AccountId) -> Result<(), Error> { diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 5332dbfdd5b2..f0bc483c882c 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -379,6 +379,11 @@ impl Default for ExtBuilder { } } +parameter_types! { + // if true, skips the try-state for the test running. + pub static SkipTryStateCheck: bool = false; +} + impl ExtBuilder { pub fn existential_deposit(self, existential_deposit: Balance) -> Self { EXISTENTIAL_DEPOSIT.with(|v| *v.borrow_mut() = existential_deposit); @@ -454,6 +459,10 @@ impl ExtBuilder { self.balance_factor = factor; self } + pub fn try_state(self, enable: bool) -> Self { + SkipTryStateCheck::set(!enable); + self + } fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -582,7 +591,9 @@ impl ExtBuilder { let mut ext = self.build(); ext.execute_with(test); ext.execute_with(|| { - Staking::do_try_state(System::block_number()).unwrap(); + if !SkipTryStateCheck::get() { + Staking::do_try_state(System::block_number()).unwrap(); + } }); } } @@ -803,6 +814,57 @@ pub(crate) fn bond_controller_stash(controller: AccountId, stash: AccountId) -> Ok(()) } +pub(crate) fn setup_double_bonded_ledgers() { + assert_ok!(Staking::bond(RuntimeOrigin::signed(1), 10, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(2), 20, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(3), 20, RewardDestination::Staked)); + // not relevant to the test case, but ensures try-runtime checks pass. + [1, 2, 3] + .iter() + .for_each(|s| Payee::::insert(s, RewardDestination::Staked)); + + // we want to test the case where a controller can also be a stash of another ledger. + // for that, we change the controller/stash bonding so that: + // * 2 becomes controller of 1. + // * 3 becomes controller of 2. + // * 4 becomes controller of 3. + let ledger_1 = Ledger::::get(1).unwrap(); + let ledger_2 = Ledger::::get(2).unwrap(); + let ledger_3 = Ledger::::get(3).unwrap(); + + // 4 becomes controller of 3. + Bonded::::mutate(3, |controller| *controller = Some(4)); + Ledger::::insert(4, ledger_3); + + // 3 becomes controller of 2. + Bonded::::mutate(2, |controller| *controller = Some(3)); + Ledger::::insert(3, ledger_2); + + // 2 becomes controller of 1 + Bonded::::mutate(1, |controller| *controller = Some(2)); + Ledger::::insert(2, ledger_1); + // 1 is not controller anymore. + Ledger::::remove(1); + + // checks. now we have: + // * 3 ledgers + assert_eq!(Ledger::::iter().count(), 3); + // * stash 1 has controller 2. + assert_eq!(Bonded::::get(1), Some(2)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(1)), Some(2)); + assert_eq!(Ledger::::get(2).unwrap().stash, 1); + + // * stash 2 has controller 3. + assert_eq!(Bonded::::get(2), Some(3)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(2)), Some(3)); + assert_eq!(Ledger::::get(3).unwrap().stash, 2); + + // * stash 3 has controller 4. + assert_eq!(Bonded::::get(3), Some(4)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(3)), Some(4)); + assert_eq!(Ledger::::get(4).unwrap().stash, 3); +} + #[macro_export] macro_rules! assert_session_era { ($session:expr, $era:expr) => { diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 093cdfdb9cb9..b012b8bf7974 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -162,7 +162,8 @@ impl Pallet { let controller = Self::bonded(&validator_stash).ok_or_else(|| { Error::::NotStash.with_weight(T::WeightInfo::payout_stakers_alive_staked(0)) })?; - let ledger = >::get(&controller).ok_or(Error::::NotController)?; + + let ledger = Self::ledger(StakingAccount::Controller(controller))?; let page = EraInfo::::get_next_claimable_page(era, &validator_stash, &ledger) .ok_or_else(|| { Error::::AlreadyClaimed @@ -1718,7 +1719,7 @@ impl StakingInterface for Pallet { ) -> Result { let ctrl = Self::bonded(&who).ok_or(Error::::NotStash)?; Self::withdraw_unbonded(RawOrigin::Signed(ctrl.clone()).into(), num_slashing_spans) - .map(|_| !Ledger::::contains_key(&ctrl)) + .map(|_| !StakingLedger::::is_bonded(StakingAccount::Controller(ctrl))) .map_err(|with_post| with_post.error) } @@ -1826,6 +1827,8 @@ impl Pallet { "VoterList contains non-staker" ); + Self::check_bonded_consistency()?; + Self::check_payees()?; Self::check_nominators()?; Self::check_exposures()?; Self::check_paged_exposures()?; @@ -1833,6 +1836,82 @@ impl Pallet { Self::check_count() } + /// Invariants: + /// * A controller should not be associated with more than one ledger. + /// * A bonded (stash, controller) pair should have only one associated ledger. I.e. if the + /// ledger is bonded by stash, the controller account must not bond a different ledger. + /// * A bonded (stash, controller) pair must have an associated ledger. + /// NOTE: these checks result in warnings only. Once + /// is resolved, turn warns into check + /// failures. + fn check_bonded_consistency() -> Result<(), TryRuntimeError> { + use sp_std::collections::btree_set::BTreeSet; + + let mut count_controller_double = 0; + let mut count_double = 0; + let mut count_none = 0; + // sanity check to ensure that each controller in Bonded storage is associated with only one + // ledger. + let mut controllers = BTreeSet::new(); + + for (stash, controller) in >::iter() { + if !controllers.insert(controller.clone()) { + count_controller_double += 1; + } + + match (>::get(&stash), >::get(&controller)) { + (Some(_), Some(_)) => + // if stash == controller, it means that the ledger has migrated to + // post-controller. If no migration happened, we expect that the (stash, + // controller) pair has only one associated ledger. + if stash != controller { + count_double += 1; + }, + (None, None) => { + count_none += 1; + }, + _ => {}, + }; + } + + if count_controller_double != 0 { + log!( + warn, + "a controller is associated with more than one ledger ({} occurrences)", + count_controller_double + ); + }; + + if count_double != 0 { + log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger ({} occurrences)", count_double); + } + + if count_none != 0 { + log!(warn, "inconsistent bonded state: (stash, controller) pair missing associated ledger ({} occurrences)", count_none); + } + + Ok(()) + } + + /// Invariants: + /// * A bonded ledger should always have an assigned `Payee`. + /// * The number of entries in `Payee` and of bonded staking ledgers *must* match. + /// * The stash account in the ledger must match that of the bonded acount. + fn check_payees() -> Result<(), TryRuntimeError> { + ensure!( + (Ledger::::iter().count() == Payee::::iter().count()) && + (Ledger::::iter().count() == Bonded::::iter().count()), + "number of entries in payee storage items does not match the number of bonded ledgers", + ); + + Ok(()) + } + + /// Invariants: + /// * Number of voters in `VoterList` match that of the number of Nominators and Validators in + /// the system (validator is both voter and target). + /// * Number of targets in `TargetList` matches the number of validators in the system. + /// * Current validator count is bounded by the election provider's max winners. fn check_count() -> Result<(), TryRuntimeError> { ensure!( ::VoterList::count() == @@ -1851,6 +1930,11 @@ impl Pallet { Ok(()) } + /// Invariants: + /// * `ledger.controller` is not stored in the storage (but populated at retrieval). + /// * Stake consistency: ledger.total == ledger.active + sum(ledger.unlocking). + /// * The controller keyeing the ledger and the ledger stash matches the state of the `Bonded` + /// storage. fn check_ledgers() -> Result<(), TryRuntimeError> { Bonded::::iter() .map(|(_, ctrl)| Self::ensure_ledger_consistent(ctrl)) @@ -1858,8 +1942,10 @@ impl Pallet { Ok(()) } + /// Invariants: + /// * For each era exposed validator, check if the exposure total is sane (exposure.total = + /// exposure.own + exposure.own). fn check_exposures() -> Result<(), TryRuntimeError> { - // a check per validator to ensure the exposure struct is always sane. let era = Self::active_era().unwrap().index; ErasStakers::::iter_prefix_values(era) .map(|expo| { @@ -1877,6 +1963,10 @@ impl Pallet { .collect::>() } + /// Invariants: + /// * For each paged era exposed validator, check if the exposure total is sane (exposure.total + /// = exposure.own + exposure.own). + /// * Paged exposures metadata (`ErasStakersOverview`) matches the paged exposures state. fn check_paged_exposures() -> Result<(), TryRuntimeError> { use sp_staking::PagedExposureMetadata; use sp_std::collections::btree_map::BTreeMap; @@ -1941,6 +2031,8 @@ impl Pallet { .collect::>() } + /// Invariants: + /// * Checks that each nominator has its entire stake correctly distributed. fn check_nominators() -> Result<(), TryRuntimeError> { // a check per nominator to ensure their entire stake is correctly distributed. Will only // kick-in if the nomination was submitted before the current era. diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index b914545a76b9..5d92b1271393 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -785,6 +785,8 @@ pub mod pallet { SnapshotTargetsSizeExceeded { size: u32 }, /// A new force era mode was set. ForceEra { mode: Forcing }, + /// Report of a controller batch deprecation. + ControllerBatchDeprecated { failures: u32 }, } #[pallet::error] @@ -929,6 +931,11 @@ pub mod pallet { return Err(Error::::AlreadyBonded.into()) } + // An existing controller cannot become a stash. + if StakingLedger::::is_bonded(StakingAccount::Controller(stash.clone())) { + return Err(Error::::AlreadyPaired.into()) + } + // Reject a bond which is considered to be _dust_. if value < T::Currency::minimum_balance() { return Err(Error::::InsufficientBond.into()) @@ -969,7 +976,6 @@ pub mod pallet { #[pallet::compact] max_additional: BalanceOf, ) -> DispatchResult { let stash = ensure_signed(origin)?; - let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; let stash_balance = T::Currency::free_balance(&stash); @@ -1326,8 +1332,6 @@ pub mod pallet { pub fn set_controller(origin: OriginFor) -> DispatchResult { let stash = ensure_signed(origin)?; - // The bonded map and ledger are mutated directly as this extrinsic is related to a - // (temporary) passive migration. Self::ledger(StakingAccount::Stash(stash.clone())).map(|ledger| { let controller = ledger.controller() .defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.") @@ -1337,9 +1341,8 @@ pub mod pallet { // Stash is already its own controller. return Err(Error::::AlreadyPaired.into()) } - >::remove(controller); - >::insert(&stash, &stash); - >::insert(&stash, ledger); + + let _ = ledger.set_controller_to_stash()?; Ok(()) })? } @@ -1952,7 +1955,7 @@ pub mod pallet { }; if ledger.stash != *controller && !payee_deprecated { - Some((controller.clone(), ledger)) + Some(ledger) } else { None } @@ -1961,13 +1964,12 @@ pub mod pallet { .collect(); // Update unique pairs. - for (controller, ledger) in filtered_batch_with_ledger { - let stash = ledger.stash.clone(); - - >::insert(&stash, &stash); - >::remove(controller); - >::insert(stash, ledger); + let mut failures = 0; + for ledger in filtered_batch_with_ledger { + let _ = ledger.clone().set_controller_to_stash().map_err(|_| failures += 1); } + Self::deposit_event(Event::::ControllerBatchDeprecated { failures }); + Ok(Some(T::WeightInfo::deprecate_controller_batch(controllers.len() as u32)).into()) } } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 0e9be70ee7d2..4803e4325a46 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -1229,6 +1229,23 @@ fn bond_extra_works() { }); } +#[test] +fn bond_extra_controller_bad_state_works() { + ExtBuilder::default().try_state(false).build_and_execute(|| { + assert_eq!(StakingLedger::::get(StakingAccount::Stash(31)).unwrap().stash, 31); + + // simulate ledger in bad state: the controller 41 is associated to the stash 31 and 41. + Bonded::::insert(31, 41); + + // we confirm that the ledger is in bad state: 31 has 41 as controller and when fetching + // the ledger associated with the controler 41, its stash is 41 (and not 31). + assert_eq!(Ledger::::get(41).unwrap().stash, 41); + + // if the ledger is in this bad state, the `bond_extra` should fail. + assert_noop!(Staking::bond_extra(RuntimeOrigin::signed(31), 10), Error::::BadState); + }) +} + #[test] fn bond_extra_and_withdraw_unbonded_works() { // @@ -1728,6 +1745,7 @@ fn rebond_emits_right_value_in_event() { #[test] fn reward_to_stake_works() { ExtBuilder::default() + .try_state(false) .nominate(false) .set_status(31, StakerStatus::Idle) .set_status(41, StakerStatus::Idle) @@ -6720,7 +6738,7 @@ mod ledger { #[test] fn paired_account_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { assert_ok!(Staking::bond( RuntimeOrigin::signed(10), 100, @@ -6755,7 +6773,7 @@ mod ledger { #[test] fn get_ledger_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { // stash does not exist assert!(StakingLedger::::get(StakingAccount::Stash(42)).is_err()); @@ -6793,6 +6811,49 @@ mod ledger { }) } + #[test] + fn get_ledger_bad_state_fails() { + ExtBuilder::default().has_stakers(false).try_state(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // Case 1: double bonded but not corrupted: + // stash 2 has controller 3: + assert_eq!(Bonded::::get(2), Some(3)); + assert_eq!(Ledger::::get(3).unwrap().stash, 2); + + // stash 2 is also a controller of 1: + assert_eq!(Bonded::::get(1), Some(2)); + assert_eq!(StakingLedger::::paired_account(StakingAccount::Stash(1)), Some(2)); + assert_eq!(Ledger::::get(2).unwrap().stash, 1); + + // although 2 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::::get(StakingAccount::Stash(2)); + assert_eq!(ledger_result.unwrap().stash, 2); // correct ledger. + + let ledger_result = StakingLedger::::get(StakingAccount::Controller(2)); + assert_eq!(ledger_result.unwrap().stash, 1); // correct ledger. + + // fetching ledger 1 by its stash works. + let ledger_result = StakingLedger::::get(StakingAccount::Stash(1)); + assert_eq!(ledger_result.unwrap().stash, 1); + + // 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::::get(2).unwrap(); + assert_eq!(ledger.stash, 1); + ledger.stash = 2; + Ledger::::insert(2, 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::::get(StakingAccount::Stash(1)).is_err()); + }) + } + #[test] fn bond_works() { ExtBuilder::default().build_and_execute(|| { @@ -6816,6 +6877,28 @@ mod ledger { }) } + #[test] + fn bond_controller_cannot_be_stash_works() { + ExtBuilder::default().build_and_execute(|| { + let (stash, controller) = testing_utils::create_unique_stash_controller::( + 0, + 10, + RewardDestination::Staked, + false, + ) + .unwrap(); + + assert_eq!(Bonded::::get(stash), Some(controller)); + assert_eq!(Ledger::::get(controller).map(|l| l.stash), Some(stash)); + + // existing controller should not be able become a stash. + assert_noop!( + Staking::bond(RuntimeOrigin::signed(controller), 10, RewardDestination::Staked), + Error::::AlreadyPaired, + ); + }) + } + #[test] fn is_bonded_works() { ExtBuilder::default().build_and_execute(|| { @@ -7044,4 +7127,81 @@ mod ledger { assert_eq!(ledger_updated.stash, stash); }) } + + #[test] + fn deprecate_controller_batch_with_bad_state_ok() { + ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // now let's deprecate all the controllers for all the existing ledgers. + let bounded_controllers: BoundedVec< + _, + ::MaxControllersInDeprecationBatch, + > = BoundedVec::try_from(vec![1, 2, 3, 4]).unwrap(); + + assert_ok!(Staking::deprecate_controller_batch( + RuntimeOrigin::root(), + bounded_controllers + )); + + assert_eq!( + *staking_events().last().unwrap(), + Event::ControllerBatchDeprecated { failures: 0 } + ); + }) + } + + #[test] + fn deprecate_controller_batch_with_bad_state_failures() { + ExtBuilder::default().has_stakers(false).try_state(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + // now let's deprecate all the controllers for all the existing ledgers. + let bounded_controllers: BoundedVec< + _, + ::MaxControllersInDeprecationBatch, + > = BoundedVec::try_from(vec![4, 3, 2, 1]).unwrap(); + + assert_ok!(Staking::deprecate_controller_batch( + RuntimeOrigin::root(), + bounded_controllers + )); + + assert_eq!( + *staking_events().last().unwrap(), + Event::ControllerBatchDeprecated { failures: 2 } + ); + }) + } + + #[test] + fn set_controller_with_bad_state_ok() { + ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + 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))); + }) + } + + #[test] + fn set_controller_with_bad_state_fails() { + 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 + // controller of another ledger. + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(3)), + Error::::BadState + ); + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(2)), + Error::::BadState + ); + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1))); + }) + } }