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))); + }) + } }