Release crates io v1.6.0 staking backport

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs

@@ -48,14 +48,14 @@ use xcm_builder::CurrencyAdapter;
 use xcm_builder::{
 	AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
 	AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, DenyReserveTransferToRelayChain,
-	DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FungiblesAdapter,
-	GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint,
-	NetworkExportTableItem, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative,
-	SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
-	SignedToAccountId32, SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith,
-	StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents,
-	WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents,
-	XcmFeeToAccount, FrameTransactionalProcessor,
+	DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor,
+	FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete,
+	LocalMint, NetworkExportTableItem, NoChecking, ParentAsSuperuser, ParentIsPreset,
+	RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia,
+	SignedAccountId32AsNative, SignedToAccountId32, SovereignPaidRemoteExporter,
+	SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit,
+	TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic,
+	XcmFeeManagerFromComponents, XcmFeeToAccount,
 };
 use xcm_executor::{traits::WithOriginFilter, XcmExecutor};
 

cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs

@@ -46,14 +46,14 @@ use xcm_builder::CurrencyAdapter;
 use xcm_builder::{
 	AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
 	AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, DenyReserveTransferToRelayChain,
-	DenyThenTry, DescribeFamily, DescribePalletTerminal, EnsureXcmOrigin, FungiblesAdapter,
-	GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint,
-	NetworkExportTableItem, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative,
-	FrameTransactionalProcessor,
-	SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
-	SignedToAccountId32, SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus,
-	TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin,
-	WithUniqueTopic, XcmFeeManagerFromComponents, XcmFeeToAccount,
+	DenyThenTry, DescribeFamily, DescribePalletTerminal, EnsureXcmOrigin,
+	FrameTransactionalProcessor, FungiblesAdapter, GlobalConsensusParachainConvertsFor,
+	HashedDescription, IsConcrete, LocalMint, NetworkExportTableItem, NoChecking,
+	ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative,
+	SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32,
+	SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit,
+	TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic,
+	XcmFeeManagerFromComponents, XcmFeeToAccount,
 };
 use xcm_executor::{traits::WithOriginFilter, XcmExecutor};
 

cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs

@@ -50,11 +50,11 @@ use xcm_builder::{
 	AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
 	AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AsPrefixedGeneralIndex,
 	ConvertedConcreteId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds,
-	FrameTransactionalProcessor, FungiblesAdapter, IsConcrete, LocalMint,
-	NativeAsset, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative,
-	SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative,
-	SignedToAccountId32, SovereignSignedViaLocation, StartsWith, TakeWeightCredit,
-	TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic,
+	FrameTransactionalProcessor, FungiblesAdapter, IsConcrete, LocalMint, NativeAsset, NoChecking,
+	ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative,
+	SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32,
+	SovereignSignedViaLocation, StartsWith, TakeWeightCredit, TrailingSetTopicAsId,
+	UsingComponents, WithComputedOrigin, WithUniqueTopic,
 };
 use xcm_executor::{traits::JustTry, XcmExecutor};
 
@@ -310,8 +310,7 @@ pub type Reserves = (
 	AssetPrefixFrom<EthereumLocation, SystemAssetHubLocation>,
 );
 
-pub type TrustedTeleporters =
-	AssetFromChain<LocalTeleportableToAssetHub, SystemAssetHubLocation>;
+pub type TrustedTeleporters = AssetFromChain<LocalTeleportableToAssetHub, SystemAssetHubLocation>;
 
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {

polkadot/xcm/xcm-executor/src/lib.rs

@@ -829,8 +829,8 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			DepositReserveAsset { assets, dest, xcm } => {
 				let old_holding = self.holding.clone();
 				let result = Config::TransactionalProcessor::process(|| {
-					// we need to do this take/put cycle to solve wildcards and get exact assets to be
-					// weighed
+					// we need to do this take/put cycle to solve wildcards and get exact assets to
+					// be weighed
 					let to_weigh = self.holding.saturating_take(assets.clone());
 					self.holding.subsume_assets(to_weigh.clone());
 
@@ -847,8 +847,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
 					for asset in deposited.assets_iter() {
 						Config::AssetTransactor::deposit_asset(&asset, &dest, Some(&self.context))?;
 					}
-					// Note that we pass `None` as `maybe_failed_bin` and drop any assets which cannot
-					// be reanchored  because we have already called `deposit_asset` on all assets.
+					// Note that we pass `None` as `maybe_failed_bin` and drop any assets which
+					// cannot be reanchored  because we have already called `deposit_asset` on all
+					// assets.
 					let assets = Self::reanchored(deposited, &dest, None);
 					let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
 					message.extend(xcm.0.into_iter());

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

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<T: Config> StakingLedger<T> {
 	/// 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<T::AccountId>) -> Result<StakingLedger<T>, Error<T>> {
-		let controller = match account {
-			StakingAccount::Stash(stash) => <Bonded<T>>::get(stash).ok_or(Error::<T>::NotStash),
-			StakingAccount::Controller(controller) => Ok(controller),
-		}?;
+		let (stash, controller) = match account.clone() {
+			StakingAccount::Stash(stash) =>
+				(stash.clone(), <Bonded<T>>::get(&stash).ok_or(Error::<T>::NotStash)?),
+			StakingAccount::Controller(controller) => (
+				Ledger::<T>::get(&controller)
+					.map(|l| l.stash)
+					.ok_or(Error::<T>::NotController)?,
+				controller,
+			),
+		};
 
-		<Ledger<T>>::get(&controller)
+		let ledger = <Ledger<T>>::get(&controller)
 			.map(|mut ledger| {
 				ledger.controller = Some(controller.clone());
 				ledger
 			})
-			.ok_or(Error::<T>::NotController)
+			.ok_or(Error::<T>::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 <https://github.com/paritytech/polkadot-sdk/issues/3245> for more details.
+		ensure!(
+			Bonded::<T>::get(&stash) == Some(controller) && ledger.stash == stash,
+			Error::<T>::BadState
+		);
+
+		Ok(ledger)
 	}
 
 	/// Returns the reward destination of a staking ledger, stored in [`Payee`].
@@ -201,6 +222,30 @@ impl<T: Config> StakingLedger<T> {
 		}
 	}
 
+	/// Sets the ledger controller to its stash.
+	pub(crate) fn set_controller_to_stash(self) -> Result<(), Error<T>> {
+		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::<T>::NotController)?;
+
+		ensure!(self.stash != *controller, Error::<T>::AlreadyPaired);
+
+		// check if the ledger's stash is a controller of another ledger.
+		if let Some(bonded_ledger) = Ledger::<T>::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 <https://github.com/paritytech/polkadot-sdk/pull/3639> for more
+			// details.
+			ensure!(bonded_ledger.stash == self.stash, Error::<T>::BadState);
+		}
+
+		<Ledger<T>>::remove(&controller);
+		<Ledger<T>>::insert(&self.stash, &self);
+		<Bonded<T>>::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<T>> {

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::<Test>::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::<Test>::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::<Test>::get(1).unwrap();
+	let ledger_2 = Ledger::<Test>::get(2).unwrap();
+	let ledger_3 = Ledger::<Test>::get(3).unwrap();
+
+	// 4 becomes controller of 3.
+	Bonded::<Test>::mutate(3, |controller| *controller = Some(4));
+	Ledger::<Test>::insert(4, ledger_3);
+
+	// 3 becomes controller of 2.
+	Bonded::<Test>::mutate(2, |controller| *controller = Some(3));
+	Ledger::<Test>::insert(3, ledger_2);
+
+	// 2 becomes controller of 1
+	Bonded::<Test>::mutate(1, |controller| *controller = Some(2));
+	Ledger::<Test>::insert(2, ledger_1);
+	// 1 is not controller anymore.
+	Ledger::<Test>::remove(1);
+
+	// checks. now we have:
+	// * 3 ledgers
+	assert_eq!(Ledger::<Test>::iter().count(), 3);
+	// * stash 1 has controller 2.
+	assert_eq!(Bonded::<Test>::get(1), Some(2));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(1)), Some(2));
+	assert_eq!(Ledger::<Test>::get(2).unwrap().stash, 1);
+
+	// * stash 2 has controller 3.
+	assert_eq!(Bonded::<Test>::get(2), Some(3));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(2)), Some(3));
+	assert_eq!(Ledger::<Test>::get(3).unwrap().stash, 2);
+
+	// * stash 3 has controller 4.
+	assert_eq!(Bonded::<Test>::get(3), Some(4));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(3)), Some(4));
+	assert_eq!(Ledger::<Test>::get(4).unwrap().stash, 3);
+}
+
 #[macro_export]
 macro_rules! assert_session_era {
 	($session:expr, $era:expr) => {