From 30b00e378363bb4f00645e0dd0b30513ffb4e219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 28 Feb 2024 18:58:48 +0100 Subject: [PATCH 01/21] Add more try runtime checks to staking --- substrate/frame/staking/src/pallet/impls.rs | 37 ++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 757c46f4faf9..bf7cca81905c 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1847,6 +1847,7 @@ impl Pallet { /// 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> { for (stash, _) in Bonded::::iter() { ensure!(Payee::::get(&stash).is_some(), "bonded ledger does not have payee set"); @@ -1861,6 +1862,11 @@ impl Pallet { 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() == @@ -1879,6 +1885,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(|(stash, ctrl)| { @@ -1896,8 +1907,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| { @@ -1915,6 +1928,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; @@ -1979,6 +1996,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. @@ -2049,6 +2068,22 @@ impl Pallet { ledger.unlocking.iter().fold(ledger.active, |a, c| a + c.value); ensure!(real_total == ledger.total, "ledger.total corrupt"); + // bonded stash matches the (generated) controller in the staking ledger. + if let Some(controller) = Bonded::::get(ledger.stash.clone()) { + if ledger.controller != Some(controller.clone()) { + log!( + warn, + "๐Ÿงช controller in existing ledger != diff bonded controller:\nbonded_ctrl: {:?}\nctrl: {:?}", + controller, + ledger.controller, + ); + } + //ensure!(ledger.stash == bonded_stash, "bonded stash != stash in the ledger"); + } else { + log!(warn, "๐Ÿงจ ledger controller not bonded for stash {:?}", ledger.stash); + } + //.ok_or("ledger does not have a bonded (controller, stash) tuple")?; + Ok(()) } } From 8b49b43ac0b13c0f22f611e9742ce8fe295245e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Fri, 1 Mar 2024 15:55:17 +0100 Subject: [PATCH 02/21] more changes --- substrate/frame/staking/src/pallet/impls.rs | 41 ++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index bf7cca81905c..b64dc5933c0c 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1836,7 +1836,9 @@ impl Pallet { "VoterList contains non-staker" ); - Self::check_payees()?; + Self::check_storage_consistency()?; + + //Self::check_payees()?; Self::check_nominators()?; Self::check_exposures()?; Self::check_paged_exposures()?; @@ -1844,6 +1846,43 @@ impl Pallet { Self::check_count() } + fn check_storage_consistency() -> Result<(), TryRuntimeError> { + // from the Bonded POV + for (stash, controller) in Bonded::::iter() { + if let Some(ledger) = Ledger::::get(controller.clone()) { + if ledger.stash != stash { + log!( + error, + "โŒ bonded stash {:?}, ledger stash: {:?} - bonded stash != ledger stash", + stash, + ledger.stash + ); + } + } else { + log!( + error, + "โŒ controller {:?} - bonded controller doesn't have a ledger", + controller + ); + } + } + + // from the ledger POV. + for (controller, ledger) in Ledger::::iter() { + let stash = ledger.stash; + if let Some(bonded_controller) = Bonded::::get(&stash) { + if controller != bonded_controller { + log!(error, "โŒ stash {:?}, controller: {:?} - bonded controller different than ledger's controller, problem!", stash, controller); + } + } else { + // after deprecate, this happens. + log!(error, "โŒ stash {:?} - ledger's stash no bonded, problem!", stash); + } + } + + Ok(()) + } + /// Invariants: /// * A bonded ledger should always have an assigned `Payee`. /// * The number of entries in `Payee` and of bonded staking ledgers *must* match. From 20a87e2e7a9a9ef720bc85db1c0fa35410abf400 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 6 Mar 2024 09:43:59 +0100 Subject: [PATCH 03/21] clean up --- substrate/frame/staking/src/pallet/impls.rs | 67 ++++++++++----------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index b64dc5933c0c..6980000527d7 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1836,9 +1836,8 @@ impl Pallet { "VoterList contains non-staker" ); - Self::check_storage_consistency()?; - - //Self::check_payees()?; + Self::check_bonded_consistency()?; + Self::check_payees()?; Self::check_nominators()?; Self::check_exposures()?; Self::check_paged_exposures()?; @@ -1846,39 +1845,39 @@ impl Pallet { Self::check_count() } - fn check_storage_consistency() -> Result<(), TryRuntimeError> { - // from the Bonded POV - for (stash, controller) in Bonded::::iter() { - if let Some(ledger) = Ledger::::get(controller.clone()) { - if ledger.stash != stash { - log!( - error, - "โŒ bonded stash {:?}, ledger stash: {:?} - bonded stash != ledger stash", - stash, - ledger.stash - ); - } - } else { - log!( - error, - "โŒ controller {:?} - bonded controller doesn't have a ledger", - controller - ); - } + /// Invariants: + /// * 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. + fn check_bonded_consistency() -> Result<(), TryRuntimeError> { + let mut count_double = 0; + let mut count_none = 0; + + for (stash, controller) in >::iter() { + 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; + }, + _ => {}, + }; } - // from the ledger POV. - for (controller, ledger) in Ledger::::iter() { - let stash = ledger.stash; - if let Some(bonded_controller) = Bonded::::get(&stash) { - if controller != bonded_controller { - log!(error, "โŒ stash {:?}, controller: {:?} - bonded controller different than ledger's controller, problem!", stash, controller); - } - } else { - // after deprecate, this happens. - log!(error, "โŒ stash {:?} - ledger's stash no bonded, problem!", stash); - } - } + ensure!( + count_double == 0, + "inconsistent bonded state: (stash, controller) pair bond more than one ledger", + ); + + ensure!( + count_none == 0, + "inconsistent bonded state: (stash, controller) pair missing associated ledger", + ); Ok(()) } From 31cbd74463e24a226516847b324ae392781c25ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Mon, 11 Mar 2024 11:57:29 +0100 Subject: [PATCH 04/21] fixes set controller and batch deprecation when controller is double bonded --- substrate/frame/staking/src/ledger.rs | 30 ++++++++- substrate/frame/staking/src/mock.rs | 52 +++++++++++++++ substrate/frame/staking/src/pallet/impls.rs | 21 ++++-- substrate/frame/staking/src/pallet/mod.rs | 25 +++---- substrate/frame/staking/src/tests.rs | 74 +++++++++++++++++++++ 5 files changed, 183 insertions(+), 19 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 5947adb9028b..7e0e189c1676 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::*; @@ -201,6 +201,32 @@ 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); + + match Ledger::::get(&self.stash) { + Some(other_ledger) => { + if other_ledger.stash != self.stash { + // stash is a controller of another ledger, fail with `Error::::DoubleBonded` + // to avoid data inconsistencies. + return Err(Error::::DoubleBonded); + } + }, + None => (), // proceed. + } + + >::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 24311cb9e782..98f14dc3889b 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -786,6 +786,58 @@ 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. + let _ = [1, 2, 3] + .iter() + .map(|s| Payee::::insert(s, RewardDestination::Staked)) + .collect::>(); + + // 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 6980000527d7..b9b2ae1924f4 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1847,13 +1847,25 @@ impl Pallet { /// Invariants: /// * 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. + /// ledger is bonded by stash, the controller account must not bond a different ledger. (note: + /// only warn, do not fail this check). /// * A bonded (stash, controller) pair must have an associated ledger. fn check_bonded_consistency() -> Result<(), TryRuntimeError> { + use sp_std::collections::btree_map::BTreeMap; + 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: BTreeMap = BTreeMap::new(); for (stash, controller) in >::iter() { + ensure!( + !controllers.contains_key(&controller), + "controller associated with more than 1 ledger" + ); + controllers.insert(controller.clone(), ()); + match (>::get(&stash), >::get(&controller)) { (Some(_), Some(_)) => // if stash == controller, it means that the ledger has migrated to @@ -1869,10 +1881,9 @@ impl Pallet { }; } - ensure!( - count_double == 0, - "inconsistent bonded state: (stash, controller) pair bond more than one ledger", - ); + if count_double == 0 { + log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger"); + }; ensure!( count_none == 0, diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 992a7fe2c9c6..17259cc5b312 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -791,6 +791,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] @@ -853,6 +855,9 @@ pub mod pallet { BoundNotMet, /// Used when attempting to use deprecated controller account logic. ControllerDeprecated, + /// Account is double bonded, i.e. a stash is also a controller for another ledger or a + /// controller is a stash for another ledger. + DoubleBonded, } #[pallet::hooks] @@ -1332,8 +1337,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.") @@ -1343,9 +1346,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(()) })? } @@ -1960,7 +1962,7 @@ pub mod pallet { }; if ledger.stash != *controller && !payee_deprecated { - Some((controller.clone(), ledger)) + Some(ledger) } else { None } @@ -1969,13 +1971,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 3f4e28b1f6af..a91d51f2f441 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -7161,4 +7161,78 @@ mod ledger { assert_eq!(ledger_updated.stash, stash); }) } + + #[test] + fn deprecate_controller_batch_with_double_bonded_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_double_bonded_failures() { + 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![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_double_bonded_ok() { + ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + 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_double_bonded_fail() { + ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + setup_double_bonded_ledgers(); + + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(3)), + Error::::DoubleBonded + ); + assert_noop!( + Staking::set_controller(RuntimeOrigin::signed(2)), + Error::::DoubleBonded + ); + assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1))); + }) + } } From 6bc85bca017b7c9fa95647625bf103906840dfb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Mon, 11 Mar 2024 14:31:30 +0100 Subject: [PATCH 05/21] removes unecessary warns --- substrate/frame/staking/src/pallet/impls.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index b9b2ae1924f4..2ad1e07dc243 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2117,22 +2117,6 @@ impl Pallet { ledger.unlocking.iter().fold(ledger.active, |a, c| a + c.value); ensure!(real_total == ledger.total, "ledger.total corrupt"); - // bonded stash matches the (generated) controller in the staking ledger. - if let Some(controller) = Bonded::::get(ledger.stash.clone()) { - if ledger.controller != Some(controller.clone()) { - log!( - warn, - "๐Ÿงช controller in existing ledger != diff bonded controller:\nbonded_ctrl: {:?}\nctrl: {:?}", - controller, - ledger.controller, - ); - } - //ensure!(ledger.stash == bonded_stash, "bonded stash != stash in the ledger"); - } else { - log!(warn, "๐Ÿงจ ledger controller not bonded for stash {:?}", ledger.stash); - } - //.ok_or("ledger does not have a bonded (controller, stash) tuple")?; - Ok(()) } } From 16a692e229d9270a0180332fb88c9a7143cf634b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 10:54:37 +0100 Subject: [PATCH 06/21] prevents controller to become a stash --- substrate/frame/staking/src/ledger.rs | 4 +++- substrate/frame/staking/src/pallet/impls.rs | 26 +++++++++++++++------ substrate/frame/staking/src/pallet/mod.rs | 5 ++++ substrate/frame/staking/src/tests.rs | 22 +++++++++++++++++ 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 7e0e189c1676..053fdbb0d9c6 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -209,11 +209,13 @@ impl StakingLedger { ensure!(self.stash != *controller, Error::::AlreadyPaired); + // check if the ledger's stash is a controller of another ledger. match Ledger::::get(&self.stash) { Some(other_ledger) => { if other_ledger.stash != self.stash { // stash is a controller of another ledger, fail with `Error::::DoubleBonded` - // to avoid data inconsistencies. + // to avoid data inconsistencies. See + // for more details. return Err(Error::::DoubleBonded); } }, diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 2ad1e07dc243..7582e1f7421d 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1846,13 +1846,18 @@ impl Pallet { } /// Invariants: + /// * A controller should not be associated with more than one ledger (note: only warn for now, + /// do not fail this check until is + /// resolved). /// * 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. (note: - /// only warn, do not fail this check). + /// only warn, do not fail this check until + /// is resolved). /// * A bonded (stash, controller) pair must have an associated ledger. fn check_bonded_consistency() -> Result<(), TryRuntimeError> { use sp_std::collections::btree_map::BTreeMap; + 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 @@ -1860,10 +1865,9 @@ impl Pallet { let mut controllers: BTreeMap = BTreeMap::new(); for (stash, controller) in >::iter() { - ensure!( - !controllers.contains_key(&controller), - "controller associated with more than 1 ledger" - ); + if controllers.contains_key(&controller) { + count_controller_double += 1; + } controllers.insert(controller.clone(), ()); match (>::get(&stash), >::get(&controller)) { @@ -1881,8 +1885,16 @@ impl Pallet { }; } - if count_double == 0 { - log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger"); + 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); }; ensure!( diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 17259cc5b312..ccfc58a71b68 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -940,6 +940,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()) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index a91d51f2f441..27cc8d52ae2e 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6933,6 +6933,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(|| { From 47f9e250ce1f329f84966acd448150533bdf3859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 11:04:29 +0100 Subject: [PATCH 07/21] warn only try-runtime checks --- substrate/frame/staking/src/pallet/impls.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 7582e1f7421d..e302d822b127 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1846,14 +1846,13 @@ impl Pallet { } /// Invariants: - /// * A controller should not be associated with more than one ledger (note: only warn for now, - /// do not fail this check until is - /// resolved). + /// * 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. (note: - /// only warn, do not fail this check until - /// is resolved). + /// 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_map::BTreeMap; @@ -1895,12 +1894,11 @@ impl Pallet { if count_double != 0 { log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger ({} occurrences)", count_double); - }; + } - ensure!( - count_none == 0, - "inconsistent bonded state: (stash, controller) pair missing associated ledger", - ); + if count_none != 0 { + log!(warn, "inconsistent bonded state: (stash, controller) pair missing associated ledger ({} occurrences)", count_none); + } Ok(()) } From 8ed57de1689fa46b11a66d55ce5ab73811e26f69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 12:15:33 +0100 Subject: [PATCH 08/21] prevents calling bond_extra in inconsistent ledgers --- substrate/frame/staking/src/ledger.rs | 16 ++++------- substrate/frame/staking/src/pallet/mod.rs | 7 ++--- substrate/frame/staking/src/tests.rs | 35 +++++++++++++++++------ 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 053fdbb0d9c6..9dc83ab1e151 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -210,16 +210,12 @@ impl StakingLedger { ensure!(self.stash != *controller, Error::::AlreadyPaired); // check if the ledger's stash is a controller of another ledger. - match Ledger::::get(&self.stash) { - Some(other_ledger) => { - if other_ledger.stash != self.stash { - // stash is a controller of another ledger, fail with `Error::::DoubleBonded` - // to avoid data inconsistencies. See - // for more details. - return Err(Error::::DoubleBonded); - } - }, - None => (), // proceed. + 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); diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index ccfc58a71b68..4a6ce2bf1844 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -855,9 +855,6 @@ pub mod pallet { BoundNotMet, /// Used when attempting to use deprecated controller account logic. ControllerDeprecated, - /// Account is double bonded, i.e. a stash is also a controller for another ledger or a - /// controller is a stash for another ledger. - DoubleBonded, } #[pallet::hooks] @@ -985,9 +982,11 @@ pub mod pallet { #[pallet::compact] max_additional: BalanceOf, ) -> DispatchResult { let stash = ensure_signed(origin)?; - let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; + // return early if ledger is in a bad state (ledger's stash != expected). + ensure!(ledger.stash == stash, Error::::BadState); + let stash_balance = T::Currency::free_balance(&stash); if let Some(extra) = stash_balance.checked_sub(&ledger.total) { let extra = extra.min(max_additional); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 27cc8d52ae2e..53045ae94e44 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -1248,6 +1248,22 @@ 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); + + // which means that when fetching the ledger of stash 31 we get 41's ledger instead. + assert_eq!(StakingLedger::::get(StakingAccount::Stash(31)).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() { // @@ -7185,7 +7201,7 @@ mod ledger { } #[test] - fn deprecate_controller_batch_with_double_bonded_ok() { + fn deprecate_controller_batch_with_bad_state_ok() { ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { setup_double_bonded_ledgers(); @@ -7208,8 +7224,8 @@ mod ledger { } #[test] - fn deprecate_controller_batch_with_double_bonded_failures() { - ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + 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. @@ -7231,10 +7247,11 @@ mod ledger { } #[test] - fn set_controller_with_double_bonded_ok() { + 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))); @@ -7242,17 +7259,19 @@ mod ledger { } #[test] - fn set_controller_with_double_bonded_fail() { - ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| { + 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::::DoubleBonded + Error::::BadState ); assert_noop!( Staking::set_controller(RuntimeOrigin::signed(2)), - Error::::DoubleBonded + Error::::BadState ); assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1))); }) From 410d784b73421a71af23591ada9b81eeaa81be7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 12:25:35 +0100 Subject: [PATCH 09/21] Update substrate/frame/staking/src/mock.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/frame/staking/src/mock.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 98f14dc3889b..65e660f248dd 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -791,10 +791,9 @@ pub(crate) fn setup_double_bonded_ledgers() { 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. - let _ = [1, 2, 3] + [1, 2, 3] .iter() - .map(|s| Payee::::insert(s, RewardDestination::Staked)) - .collect::>(); + .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: From 86b0ad70e5d515e6b24c9a70d48fe5b7b5462af5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 15:05:12 +0100 Subject: [PATCH 10/21] adds bad state checks at the staking ledger level; ensures all accesses to leger are made through the staking ledger API --- substrate/frame/staking/src/ledger.rs | 28 ++++++++++++++++++--- substrate/frame/staking/src/pallet/impls.rs | 5 ++-- substrate/frame/staking/src/pallet/mod.rs | 3 --- substrate/frame/staking/src/tests.rs | 5 ++-- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 9dc83ab1e151..e9b483b05919 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -106,11 +106,31 @@ 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, + ), + }; + + // if ledger bond is in a bad state, return error to prevent applying operations that may + // affect the ledger's state. A bond is in bad state when a stash has a controller which is + // bonding a ledger of another stash. + // + // See for more details. + if Bonded::::get(&stash).is_some() && + Ledger::::get(controller.clone()).map(|l| l.stash != stash).unwrap_or(false) + { + return Err(Error::::BadState); + } >::get(&controller) .map(|mut ledger| { diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index e302d822b127..529eb8002695 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -165,7 +165,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 @@ -1728,7 +1729,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) } diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 4a6ce2bf1844..1bf8bd8b09cb 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -984,9 +984,6 @@ pub mod pallet { let stash = ensure_signed(origin)?; let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; - // return early if ledger is in a bad state (ledger's stash != expected). - ensure!(ledger.stash == stash, Error::::BadState); - let stash_balance = T::Currency::free_balance(&stash); if let Some(extra) = stash_balance.checked_sub(&ledger.total) { let extra = extra.min(max_additional); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 53045ae94e44..f48541579ee9 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -1256,8 +1256,9 @@ fn bond_extra_controller_bad_state_works() { // simulate ledger in bad state: the controller 41 is associated to the stash 31 and 41. Bonded::::insert(31, 41); - // which means that when fetching the ledger of stash 31 we get 41's ledger instead. - assert_eq!(StakingLedger::::get(StakingAccount::Stash(31)).unwrap().stash, 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); From 6e16fa491a6f823283f2e4157d7575e81b506a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 15:57:44 +0100 Subject: [PATCH 11/21] nit --- substrate/frame/staking/src/ledger.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index e9b483b05919..269da220faa0 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -126,11 +126,11 @@ impl StakingLedger { // bonding a ledger of another stash. // // See for more details. - if Bonded::::get(&stash).is_some() && - Ledger::::get(controller.clone()).map(|l| l.stash != stash).unwrap_or(false) - { - return Err(Error::::BadState); - } + ensure!( + Bonded::::get(&stash).is_some() && + Ledger::::get(&controller).map(|l| l.stash == stash).unwrap_or(false), + Error::::BadState + ); >::get(&controller) .map(|mut ledger| { From 5986a4ee522b77d01c25a7cad386c04313301e86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 16:09:09 +0100 Subject: [PATCH 12/21] comments nits --- substrate/frame/staking/src/ledger.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 269da220faa0..7a691b0f5d2c 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -122,8 +122,8 @@ impl StakingLedger { }; // if ledger bond is in a bad state, return error to prevent applying operations that may - // affect the ledger's state. A bond is in bad state when a stash has a controller which is - // bonding a ledger of another stash. + // 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!( From 578a4070b02b3a4af1157e56a508f213d3005e8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 17:05:53 +0100 Subject: [PATCH 13/21] add tests for staking ledger fetch in bad state --- substrate/frame/staking/src/tests.rs | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index f48541579ee9..3725c9e3c2c5 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6927,6 +6927,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(|| { From 8dd4818534117b240ac9e566558cc1492061dd46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 17:22:48 +0100 Subject: [PATCH 14/21] adds prdoc --- prdoc/pr_3639.prdoc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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..e636b2b6317b --- /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 From 52c648e8acfd52fbedb58fb4743a312a889930ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 17:27:17 +0100 Subject: [PATCH 15/21] fixes prdoc --- prdoc/pr_3639.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_3639.prdoc b/prdoc/pr_3639.prdoc index e636b2b6317b..46e3f6f4d763 100644 --- a/prdoc/pr_3639.prdoc +++ b/prdoc/pr_3639.prdoc @@ -4,8 +4,8 @@ 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. + 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; From e3e9672d2f4fa13f5553d29f8107a39f24785e5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 18:26:35 +0100 Subject: [PATCH 16/21] Update substrate/frame/staking/src/ledger.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/frame/staking/src/ledger.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 7a691b0f5d2c..0a1bd1deb4ff 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -112,7 +112,7 @@ impl StakingLedger { pub(crate) fn get(account: StakingAccount) -> Result, Error> { let (stash, controller) = match account.clone() { StakingAccount::Stash(stash) => - (stash.clone(), >::get(&stash).ok_or(Error::::NotStash)?), + (stash.clone(), >::get(&stash).defensive_ok_or(Error::::NotStash)?), StakingAccount::Controller(controller) => ( Ledger::::get(&controller) .map(|l| l.stash) From 541037fd890934d0b0950560a677c890770e2e2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 18:26:43 +0100 Subject: [PATCH 17/21] Update substrate/frame/staking/src/ledger.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/frame/staking/src/ledger.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 0a1bd1deb4ff..22d27331c26f 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -116,7 +116,7 @@ impl StakingLedger { StakingAccount::Controller(controller) => ( Ledger::::get(&controller) .map(|l| l.stash) - .ok_or(Error::::NotController)?, + .defensive_ok_or(Error::::NotController)?, controller, ), }; From 0c1bff6e77ca29bcdf8fc561d0f4d8ece91c5ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 18:48:13 +0100 Subject: [PATCH 18/21] replaces btreemap with btreeset in try runtime checks --- substrate/frame/staking/src/ledger.rs | 4 ++-- substrate/frame/staking/src/pallet/impls.rs | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 22d27331c26f..7a691b0f5d2c 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -112,11 +112,11 @@ impl StakingLedger { pub(crate) fn get(account: StakingAccount) -> Result, Error> { let (stash, controller) = match account.clone() { StakingAccount::Stash(stash) => - (stash.clone(), >::get(&stash).defensive_ok_or(Error::::NotStash)?), + (stash.clone(), >::get(&stash).ok_or(Error::::NotStash)?), StakingAccount::Controller(controller) => ( Ledger::::get(&controller) .map(|l| l.stash) - .defensive_ok_or(Error::::NotController)?, + .ok_or(Error::::NotController)?, controller, ), }; diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 529eb8002695..d42456e53b13 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1855,20 +1855,19 @@ impl Pallet { /// is resolved, turn warns into check /// failures. fn check_bonded_consistency() -> Result<(), TryRuntimeError> { - use sp_std::collections::btree_map::BTreeMap; + 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: BTreeMap = BTreeMap::new(); + let mut controllers = BTreeSet::new(); for (stash, controller) in >::iter() { - if controllers.contains_key(&controller) { + if !controllers.insert(controller.clone()) { count_controller_double += 1; } - controllers.insert(controller.clone(), ()); match (>::get(&stash), >::get(&controller)) { (Some(_), Some(_)) => From 371b0cfeccc3681170702d8a0e563e7950eaddb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 19:06:53 +0100 Subject: [PATCH 19/21] simplifies getter check --- substrate/frame/staking/src/ledger.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 7a691b0f5d2c..5bd46f85a160 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -121,23 +121,21 @@ impl StakingLedger { ), }; + let ledger = >::get(&controller) + .map(|mut ledger| { + ledger.controller = Some(controller.clone()); + ledger + }) + .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).is_some() && - Ledger::::get(&controller).map(|l| l.stash == stash).unwrap_or(false), - Error::::BadState - ); + ensure!(Bonded::::get(&stash).is_some() && ledger.stash == stash, Error::::BadState); - >::get(&controller) - .map(|mut ledger| { - ledger.controller = Some(controller.clone()); - ledger - }) - .ok_or(Error::::NotController) + Ok(ledger) } /// Returns the reward destination of a staking ledger, stored in [`Payee`]. From 467065936f811d1df6db8c26dbeeebb660841a4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Wed, 13 Mar 2024 21:56:43 +0100 Subject: [PATCH 20/21] check if bonded controller is the expected --- substrate/frame/staking/src/ledger.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 5bd46f85a160..67be1e15bc6e 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -133,7 +133,10 @@ impl StakingLedger { // associted with a different ledger (i.e. a ledger with a different stash). // // See for more details. - ensure!(Bonded::::get(&stash).is_some() && ledger.stash == stash, Error::::BadState); + ensure!( + Bonded::::get(&stash) == Some(controller) && ledger.stash == stash, + Error::::BadState + ); Ok(ledger) } From 0dde68ac56e3aee6289f00e5b6c73111e8a4df74 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Mar 2024 09:26:45 +0000 Subject: [PATCH 21/21] Empty-Commit