From ebf4f8d2d590f41817d5d38b2d9b5812a46f2342 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Thu, 15 Aug 2024 02:10:31 +0200 Subject: [PATCH] [Pools] fix derivation of pool account (#4999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/paritytech-secops/srlabs_findings/issues/408. This fixes how ProxyDelegator accounts are derived but may cause issues in Westend since it would use the old derivative accounts. Does not affect Polkadot/Kusama as this pallet is not deployed to them yet. --------- Co-authored-by: Gonçalo Pestana --- Cargo.lock | 1 + polkadot/runtime/westend/src/lib.rs | 15 +-- prdoc/pr_4999.prdoc | 18 +++ substrate/frame/delegated-staking/Cargo.toml | 3 + substrate/frame/delegated-staking/src/lib.rs | 20 +++- .../frame/delegated-staking/src/migration.rs | 107 ++++++++++++++++++ 6 files changed, 152 insertions(+), 12 deletions(-) create mode 100644 prdoc/pr_4999.prdoc create mode 100644 substrate/frame/delegated-staking/src/migration.rs diff --git a/Cargo.lock b/Cargo.lock index cc8b64457a79..76973e3397b3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10489,6 +10489,7 @@ dependencies = [ "frame-election-provider-support", "frame-support", "frame-system", + "log", "pallet-balances", "pallet-nomination-pools", "pallet-staking", diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index a9fbbb4f33da..5f8d5d424938 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1760,11 +1760,8 @@ pub type SignedExtra = ( ); parameter_types! { - // This is the max pools that will be migrated in the runtime upgrade. Westend has more pools - // than this, but we want to emulate some non migrated pools. In prod runtimes, if weight is not - // a concern, it is recommended to set to (existing pools + 10) to also account for any new - // pools getting created before the migration is actually executed. - pub const MaxPoolsToMigrate: u32 = 250; + /// Bounding number of agent pot accounts to be migrated in a single block. + pub const MaxAgentsToMigrate: u32 = 300; } /// All migrations that will run on the next runtime upgrade. @@ -1798,13 +1795,11 @@ pub mod migrations { /// Unreleased migrations. Add new ones here: pub type Unreleased = ( - // Migrate NominationPools to `DelegateStake` adapter. This is unversioned upgrade and - // should not be applied yet in Kusama/Polkadot. - pallet_nomination_pools::migration::unversioned::DelegationStakeMigration< + // This is only needed for Westend. + pallet_delegated_staking::migration::unversioned::ProxyDelegatorMigration< Runtime, - MaxPoolsToMigrate, + MaxAgentsToMigrate, >, - pallet_staking::migrations::v15::MigrateV14ToV15, ); } diff --git a/prdoc/pr_4999.prdoc b/prdoc/pr_4999.prdoc new file mode 100644 index 000000000000..d396fcdbe8b3 --- /dev/null +++ b/prdoc/pr_4999.prdoc @@ -0,0 +1,18 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fixes entropy for derivation of proxy delegator account. + +doc: + - audience: Runtime Dev + description: | + This fixes how ProxyDelegator accounts are derived but may cause issues in Westend since it would use the old + derivative accounts. Does not affect Polkadot/Kusama as this pallet is not deployed to them yet. + +crates: + - name: westend-runtime + bump: patch + - name: pallet-delegated-staking + bump: patch + - name: pallet-nomination-pools + bump: patch \ No newline at end of file diff --git a/substrate/frame/delegated-staking/Cargo.toml b/substrate/frame/delegated-staking/Cargo.toml index 1bd17c59f1e7..8d5ccd342b6b 100644 --- a/substrate/frame/delegated-staking/Cargo.toml +++ b/substrate/frame/delegated-staking/Cargo.toml @@ -18,6 +18,8 @@ frame-system = { workspace = true } scale-info = { features = ["derive"], workspace = true } sp-runtime = { workspace = true } sp-staking = { workspace = true } +sp-io = { workspace = true } +log = { workspace = true } [dev-dependencies] sp-core = { workspace = true, default-features = true } @@ -38,6 +40,7 @@ std = [ "frame-election-provider-support/std", "frame-support/std", "frame-system/std", + "log/std", "pallet-balances/std", "pallet-nomination-pools/std", "pallet-staking/std", diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 08ce747ec0c0..7b8d14b0a611 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -126,6 +126,7 @@ #![deny(rustdoc::broken_intra_doc_links)] mod impls; +pub mod migration; #[cfg(test)] mod mock; #[cfg(test)] @@ -152,12 +153,25 @@ use frame_support::{ Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; +use sp_io::hashing::blake2_256; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero}, + traits::{CheckedAdd, CheckedSub, TrailingZeroInput, Zero}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, }; use sp_staking::{Agent, Delegator, EraIndex, StakingInterface, StakingUnchecked}; +/// The log target of this pallet. +pub const LOG_TARGET: &str = "runtime::delegated-staking"; +// syntactic sugar for logging. +#[macro_export] +macro_rules! log { + ($level:tt, $patter:expr $(, $values:expr)* $(,)?) => { + log::$level!( + target: $crate::LOG_TARGET, + concat!("[{:?}] 🏊‍♂️ ", $patter), >::block_number() $(, $values)* + ) + }; +} pub type BalanceOf = <::Currency as FunInspect<::AccountId>>::Balance; @@ -463,7 +477,9 @@ impl Pallet { /// Derive a (keyless) pot account from the given agent account and account type. fn sub_account(account_type: AccountType, acc: T::AccountId) -> T::AccountId { - T::PalletId::get().into_sub_account_truncating((account_type, acc.clone())) + let entropy = (T::PalletId::get(), acc, account_type).using_encoded(blake2_256); + Decode::decode(&mut TrailingZeroInput::new(entropy.as_ref())) + .expect("infinite length input; no invalid inputs for type; qed") } /// Held balance of a delegator. diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs new file mode 100644 index 000000000000..8bc7312c4eab --- /dev/null +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -0,0 +1,107 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::*; +use frame_support::traits::OnRuntimeUpgrade; + +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + +pub mod unversioned { + use super::*; + #[cfg(feature = "try-runtime")] + use alloc::vec::Vec; + use sp_runtime::traits::AccountIdConversion; + + /// Migrates `ProxyDelegator` accounts with better entropy than the old logic which didn't take + /// into account all the bytes of the agent account ID. + pub struct ProxyDelegatorMigration(PhantomData<(T, MaxAgents)>); + + impl> OnRuntimeUpgrade for ProxyDelegatorMigration { + fn on_runtime_upgrade() -> Weight { + let mut weight = Weight::zero(); + let old_proxy_delegator = |agent: T::AccountId| { + T::PalletId::get() + .into_sub_account_truncating((AccountType::ProxyDelegator, agent.clone())) + }; + + Agents::::iter_keys().take(MaxAgents::get() as usize).for_each(|agent| { + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); + let old_proxy = old_proxy_delegator(agent.clone()); + + // if delegation does not exist, it does not need to be migrated. + if let Some(delegation) = Delegation::::get(&old_proxy) { + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); + + let new_proxy = + Pallet::::generate_proxy_delegator(Agent::from(agent.clone())); + + // accrue read writes for `do_migrate_delegation` + weight.saturating_accrue(T::DbWeight::get().reads_writes(8, 8)); + let _ = Pallet::::do_migrate_delegation( + Delegator::from(old_proxy.clone()), + new_proxy.clone(), + delegation.amount, + ) + .map_err(|e| { + log!( + error, + "Failed to migrate old proxy delegator {:?} to new proxy {:?} for agent {:?} with error: {:?}", + old_proxy, + new_proxy, + agent, + e, + ); + }); + }; + }); + + log!(info, "Finished migrating old proxy delegator accounts to new ones"); + weight + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + let mut unmigrated_count = 0; + let old_proxy_delegator = |agent: T::AccountId| { + T::PalletId::get() + .into_sub_account_truncating((AccountType::ProxyDelegator, agent.clone())) + }; + + Agents::::iter_keys().take(MaxAgents::get() as usize).for_each(|agent| { + let old_proxy: T::AccountId = old_proxy_delegator(agent.clone()); + let held_balance = Pallet::::held_balance_of(Delegator::from(old_proxy.clone())); + let delegation = Delegation::::get(&old_proxy); + if delegation.is_some() || !held_balance.is_zero() { + log!( + error, + "Old proxy delegator {:?} for agent {:?} is not migrated.", + old_proxy, + agent, + ); + unmigrated_count += 1; + } + }); + + if unmigrated_count > 0 { + Err(TryRuntimeError::Other("Some old proxy delegator accounts are not migrated.")) + } else { + Ok(()) + } + } + } +}