From dda12d499db3da8a4acd3bc84472a77a7d2a3f2a Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 29 Mar 2024 10:13:36 +0100 Subject: [PATCH 01/29] add ability to super bond to staking --- substrate/frame/staking/src/ledger.rs | 32 ++++-- substrate/frame/staking/src/pallet/impls.rs | 119 +++++++++++++++++++- substrate/frame/staking/src/pallet/mod.rs | 44 +++----- substrate/frame/staking/src/slashing.rs | 22 ++-- substrate/primitives/staking/src/lib.rs | 95 +++++++++++++++- 5 files changed, 259 insertions(+), 53 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 9461daefed65..641d7c81ae41 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -33,13 +33,14 @@ use frame_support::{ defensive, ensure, - traits::{Defensive, LockableCurrency, WithdrawReasons}, + traits::{Defensive, LockableCurrency}, }; use sp_staking::StakingAccount; use sp_std::prelude::*; use crate::{ - BalanceOf, Bonded, Config, Error, Ledger, Payee, RewardDestination, StakingLedger, STAKING_ID, + BalanceOf, Bonded, Config, Error, Ledger, Pallet, Payee, RewardDestination, StakingLedger, + STAKING_ID, }; #[cfg(any(feature = "runtime-benchmarks", test))] @@ -187,7 +188,7 @@ impl StakingLedger { return Err(Error::::NotStash) } - T::Currency::set_lock(STAKING_ID, &self.stash, self.total, WithdrawReasons::all()); + Pallet::::update_lock(&self.stash, self.total).map_err(|_| Error::::BadState)?; Ledger::::insert( &self.controller().ok_or_else(|| { defensive!("update called on a ledger that is not bonded."); @@ -204,22 +205,29 @@ impl StakingLedger { /// It sets the reward preferences for the bonded stash. pub(crate) fn bond(self, payee: RewardDestination) -> Result<(), Error> { if >::contains_key(&self.stash) { - Err(Error::::AlreadyBonded) - } else { - >::insert(&self.stash, payee); - >::insert(&self.stash, &self.stash); - self.update() + return Err(Error::::AlreadyBonded) + } + if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { + return Err(Error::::RewardDestinationRestricted); } + + >::insert(&self.stash, payee); + >::insert(&self.stash, &self.stash); + self.update() } /// Sets the ledger Payee. pub(crate) fn set_payee(self, payee: RewardDestination) -> Result<(), Error> { if !>::contains_key(&self.stash) { - Err(Error::::NotStash) - } else { - >::insert(&self.stash, payee); - Ok(()) + return Err(Error::::NotStash) + } + + if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { + return Err(Error::::RewardDestinationRestricted); } + + >::insert(&self.stash, payee); + Ok(()) } /// Sets the ledger controller to its stash. diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 2f43e4847e45..a7428d9ad905 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -28,14 +28,16 @@ use frame_support::{ pallet_prelude::*, traits::{ Currency, Defensive, DefensiveSaturating, EstimateNextNewSession, Get, Imbalance, - InspectLockableCurrency, Len, OnUnbalanced, TryCollect, UnixTime, + InspectLockableCurrency, Len, LockableCurrency, OnUnbalanced, TryCollect, UnixTime, }, weights::Weight, }; use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; use pallet_session::historical; use sp_runtime::{ - traits::{Bounded, Convert, One, SaturatedConversion, Saturating, StaticLookup, Zero}, + traits::{ + Bounded, CheckedSub, Convert, One, SaturatedConversion, Saturating, StaticLookup, Zero, + }, Perbill, Percent, }; use sp_staking::{ @@ -149,6 +151,37 @@ impl Pallet { Self::slashable_balance_of_vote_weight(who, issuance) } + pub(super) fn do_bond_extra(stash: &T::AccountId, additional: BalanceOf) -> DispatchResult { + let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; + + let extra = if Self::is_virtual_nominator(stash) { + additional + } else { + // additional amount or actual balance of stash whichever is lower. + additional.min( + T::Currency::free_balance(stash) + .checked_sub(&ledger.total) + .ok_or(sp_runtime::ArithmeticError::Overflow)?, + ) + }; + + ledger.total += extra; + ledger.active += extra; + // Last check: the new active amount of ledger must be more than ED. + ensure!(ledger.active >= T::Currency::minimum_balance(), Error::::InsufficientBond); + + // NOTE: ledger must be updated prior to calling `Self::weight_of`. + ledger.update()?; + // update this staker in the sorted list, if they exist in it. + if T::VoterList::contains(stash) { + let _ = T::VoterList::on_update(&stash, Self::weight_of(stash)).defensive(); + } + + Self::deposit_event(Event::::Bonded { stash: stash.clone(), amount: extra }); + + Ok(()) + } + pub(super) fn do_withdraw_unbonded( controller: &T::AccountId, num_slashing_spans: u32, @@ -1132,6 +1165,45 @@ impl Pallet { ) -> Exposure> { EraInfo::::get_full_exposure(era, account) } + + /// Whether the passed reward destination is restricted for the given account. + /// + /// Virtual nominators are not allowed to compound their rewards as this pallet does not manage + /// locks for them. For external pallets that manage the virtual bond, it is their + /// responsibility to distribute the reward and re-bond them. + /// + /// Conservatively, we expect them to always set the reward destination to a non stash account. + pub(crate) fn restrict_reward_destination( + who: &T::AccountId, + reward_destination: RewardDestination, + ) -> bool { + Self::is_virtual_nominator(who) && + match reward_destination { + RewardDestination::Account(payee) => payee == *who, + _ => true, + } + } + + pub(crate) fn is_virtual_nominator(who: &T::AccountId) -> bool { + VirtualNominators::::contains_key(who) + } + + pub(crate) fn update_lock( + who: &T::AccountId, + amount: BalanceOf, + ) -> sp_runtime::DispatchResult { + // Skip locking virtual nominators. They are handled by external pallets. + if !Self::is_virtual_nominator(who) { + T::Currency::set_lock( + crate::STAKING_ID, + who, + amount, + frame_support::traits::WithdrawReasons::all(), + ); + } + + Ok(()) + } } impl Pallet { @@ -1748,6 +1820,15 @@ impl StakingInterface for Pallet { .map(|_| ()) } + fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult { + // since controller is deprecated and this function is never used for old ledgers with + // distinct controllers, we can safely assume that stash is the controller. + Self::set_payee( + RawOrigin::Signed(stash.clone()).into(), + RewardDestination::Account(reward_acc.clone()), + ) + } + fn chill(who: &Self::AccountId) -> DispatchResult { // defensive-only: any account bonded via this interface has the stash set as the // controller, but we have to be sure. Same comment anywhere else that we read this. @@ -1832,6 +1913,10 @@ impl StakingInterface for Pallet { } } + fn slash_reward_fraction() -> Perbill { + SlashRewardFraction::::get() + } + sp_staking::runtime_benchmarks_enabled! { fn nominations(who: &Self::AccountId) -> Option> { Nominators::::get(who).map(|n| n.targets.into_inner()) @@ -1860,6 +1945,36 @@ impl StakingInterface for Pallet { } } +impl sp_staking::StakingUnsafe for Pallet { + fn force_release(who: &Self::AccountId) { + T::Currency::remove_lock(crate::STAKING_ID, who) + } + + fn virtual_bond( + who: &Self::AccountId, + value: Self::Balance, + payee: &Self::AccountId, + ) -> DispatchResult { + if StakingLedger::::is_bonded(StakingAccount::Stash(who.clone())) { + return Err(Error::::AlreadyBonded.into()) + } + + frame_system::Pallet::::inc_consumers(&who).map_err(|_| Error::::BadState)?; + + // mark who as a virtual nominator + VirtualNominators::::insert(who, ()); + + Self::deposit_event(Event::::Bonded { stash: who.clone(), amount: value }); + let ledger = StakingLedger::::new(who.clone(), value); + + // You're auto-bonded forever, here. We might improve this by only bonding when + // you actually validate/nominate and remove once you unbond __everything__. + ledger.bond(RewardDestination::Account(payee.clone()))?; + + Ok(()) + } +} + #[cfg(any(test, feature = "try-runtime"))] impl Pallet { pub(crate) fn do_try_state(_: BlockNumberFor) -> Result<(), TryRuntimeError> { diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 2e5b3aa7b873..bd5372a3e6f2 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -32,7 +32,7 @@ use frame_support::{ }; use frame_system::{ensure_root, ensure_signed, pallet_prelude::*}; use sp_runtime::{ - traits::{CheckedSub, SaturatedConversion, StaticLookup, Zero}, + traits::{SaturatedConversion, StaticLookup, Zero}, ArithmeticError, Perbill, Percent, }; @@ -379,6 +379,16 @@ pub mod pallet { pub type Nominators = CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations>; + /// Nominators whose funds are managed by other pallets. + /// + /// This pallet does not apply any locks on them, therefore they are only virtually bonded. They + /// are expected to be keyless accounts and hence should not be allowed to mutate their ledger + /// directly via this pallet. Instead, these accounts are managed by other pallets and accessed + /// via low level apis. We keep track of them to do minimal integrity checks. + // TODO(ank4n): Can we keep this entry in `Ledger`? Worth a migration? + #[pallet::storage] + pub type VirtualNominators = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>; + /// The maximum nominator count before we stop allowing new validators to join. /// /// When this value is not set, no limits are enforced. @@ -858,6 +868,10 @@ pub mod pallet { ControllerDeprecated, /// Cannot reset a ledger. CannotRestoreLedger, + /// Provided reward destination is not allowed. + RewardDestinationRestricted, + /// Not enough funds available to withdraw + NotEnoughFunds, } #[pallet::hooks] @@ -985,29 +999,7 @@ 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); - if let Some(extra) = stash_balance.checked_sub(&ledger.total) { - let extra = extra.min(max_additional); - ledger.total += extra; - ledger.active += extra; - // Last check: the new active amount of ledger must be more than ED. - ensure!( - ledger.active >= T::Currency::minimum_balance(), - Error::::InsufficientBond - ); - - // NOTE: ledger must be updated prior to calling `Self::weight_of`. - ledger.update()?; - // update this staker in the sorted list, if they exist in it. - if T::VoterList::contains(&stash) { - let _ = T::VoterList::on_update(&stash, Self::weight_of(&stash)).defensive(); - } - - Self::deposit_event(Event::::Bonded { stash, amount: extra }); - } - Ok(()) + Self::do_bond_extra(&stash, max_additional) } /// Schedule a portion of the stash to be unlocked ready for transfer out after the bond @@ -1315,9 +1307,7 @@ pub mod pallet { Error::::ControllerDeprecated ); - let _ = ledger - .set_payee(payee) - .defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?; + ledger.set_payee(payee)?; Ok(()) } diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 709fd1441ec3..19424f57dcdb 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -609,8 +609,13 @@ pub fn do_slash( }; let value = ledger.slash(value, T::Currency::minimum_balance(), slash_era); + if value.is_zero() { + // nothing to do + return + } - if !value.is_zero() { + // Skip slashing for virtual nominators. The pallets managing them should handle the slashing. + if !Pallet::::is_virtual_nominator(stash) { let (imbalance, missing) = T::Currency::slash(stash, value); slashed_imbalance.subsume(imbalance); @@ -618,17 +623,14 @@ pub fn do_slash( // deduct overslash from the reward payout *reward_payout = reward_payout.saturating_sub(missing); } + } - let _ = ledger - .update() - .defensive_proof("ledger fetched from storage so it exists in storage; qed."); + let _ = ledger + .update() + .defensive_proof("ledger fetched from storage so it exists in storage; qed."); - // trigger the event - >::deposit_event(super::Event::::Slashed { - staker: stash.clone(), - amount: value, - }); - } + // trigger the event + >::deposit_event(super::Event::::Slashed { staker: stash.clone(), amount: value }); } /// Apply a previously-unapplied slash. diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 11b7ef41b9a7..3f2c8c9fb493 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -29,7 +29,7 @@ use core::ops::Sub; use scale_info::TypeInfo; use sp_runtime::{ traits::{AtLeast32BitUnsigned, Zero}, - DispatchError, DispatchResult, RuntimeDebug, Saturating, + DispatchError, DispatchResult, Perbill, RuntimeDebug, Saturating, }; pub mod offence; @@ -254,6 +254,9 @@ pub trait StakingInterface { /// schedules have reached their unlocking era should allow more calls to this function. fn unbond(stash: &Self::AccountId, value: Self::Balance) -> DispatchResult; + /// Update the reward destination for the ledger associated with the stash. + fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult; + /// Unlock any funds schedule to unlock before or at the current era. /// /// Returns whether the stash was killed because of this withdraw or not. @@ -274,7 +277,7 @@ pub trait StakingInterface { /// Checks whether an account `staker` has been exposed in an era. fn is_exposed_in_era(who: &Self::AccountId, era: &EraIndex) -> bool; - /// Return the status of the given staker, `None` if not staked at all. + /// Return the status of the given staker, `Err` if not staked at all. fn status(who: &Self::AccountId) -> Result, DispatchError>; /// Checks whether or not this is a validator account. @@ -290,6 +293,9 @@ pub trait StakingInterface { } } + /// Returns the fraction of the slash to be rewarded to reporter. + fn slash_reward_fraction() -> Perbill; + #[cfg(feature = "runtime-benchmarks")] fn max_exposure_page_size() -> Page; @@ -304,6 +310,27 @@ pub trait StakingInterface { fn set_current_era(era: EraIndex); } +/// Set of low level apis to manipulate staking ledger. +/// +/// These apis bypass some or all safety checks and should only be used if you know what you are +/// doing. +pub trait StakingUnsafe: StakingInterface { + /// Release all funds bonded for stake without unbonding the ledger. + /// + /// Unsafe, only used for migration of `nominator` to `virtual_nominator`. + fn force_release(who: &Self::AccountId); + + /// Book-keep a new bond for `who` without applying any locks (hence virtual). + /// + /// It is important that who is a keyless account and therefore cannot interact with staking + /// pallet directly. Caller is responsible for ensuring the passed amount is locked and valid. + fn virtual_bond( + keyless_who: &Self::AccountId, + value: Self::Balance, + payee: &Self::AccountId, + ) -> DispatchResult; +} + /// The amount of exposure for an era that an individual nominator has (susceptible to slashing). #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Encode, Decode, RuntimeDebug, TypeInfo)] pub struct IndividualExposure { @@ -422,4 +449,68 @@ pub struct PagedExposureMetadata { pub page_count: Page, } +/// Extension of [`StakingInterface`] with delegation functionality. +/// +/// Introduces two new actors: +/// - `Delegator`: An account that delegates funds to a `Delegatee`. +/// - `Delegatee`: An account that receives delegated funds from `Delegators`. It can then use these +/// funds to participate in the staking system. It can never use its own funds. +/// +/// The `Delegatee` is responsible for managing rewards and slashing for all the `Delegators` that +/// have delegated funds to it. +pub trait DelegatedStakeInterface: StakingInterface { + /// Effective balance of the `delegatee` account. + /// + /// This takes into account any pending slashes to `Delegatee`. + fn delegatee_balance(delegatee: &Self::AccountId) -> Self::Balance; + + /// Returns the total amount of funds delegated by a `delegator`. + fn delegator_balance(delegator: &Self::AccountId) -> Self::Balance; + + /// Delegate funds to `delegatee`. + /// + /// Only used for the initial delegation. Use [`Self::delegate_extra`] to add more delegation. + fn delegate( + delegator: &Self::AccountId, + delegatee: &Self::AccountId, + reward_account: &Self::AccountId, + amount: Self::Balance, + ) -> DispatchResult; + + /// Add more delegation to the `delegatee`. + /// + /// If this is the first delegation, use [`Self::delegate`] instead. + fn delegate_extra( + delegator: &Self::AccountId, + delegatee: &Self::AccountId, + amount: Self::Balance, + ) -> DispatchResult; + + /// Withdraw or revoke delegation to `delegatee`. + /// + /// If there are `delegatee` funds upto `amount` available to withdraw, then those funds would + /// be released to the `delegator` + fn withdraw_delegation( + delegator: &Self::AccountId, + delegatee: &Self::AccountId, + amount: Self::Balance, + ) -> DispatchResult; + + /// Returns true if there are pending slashes posted to the `delegatee` account. + /// + /// Slashes to `delegatee` account are not immediate and are applied lazily. Since `delegatee` + /// has an unbounded number of delegators, immediate slashing is not possible. + fn has_pending_slash(delegatee: &Self::AccountId) -> bool; + + /// Apply a pending slash to a `delegatee` by slashing `value` from `delegator`. + /// + /// If a reporter is provided, the reporter will receive a fraction of the slash as reward. + fn delegator_slash( + delegatee: &Self::AccountId, + delegator: &Self::AccountId, + value: Self::Balance, + maybe_reporter: Option, + ) -> sp_runtime::DispatchResult; +} + sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $); From 802784c44f2d3f691f708ba7ba3910f5445bf5e0 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 29 Mar 2024 10:25:28 +0100 Subject: [PATCH 02/29] some comments --- substrate/frame/staking/src/pallet/impls.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index a7428d9ad905..bd890e86468c 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -154,6 +154,8 @@ impl Pallet { pub(super) fn do_bond_extra(stash: &T::AccountId, additional: BalanceOf) -> DispatchResult { let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; + // for virtual nominators, we don't need to check the balance. Since they are only accessed + // via low level apis, we can assume that the caller has done the due diligence. let extra = if Self::is_virtual_nominator(stash) { additional } else { @@ -1184,10 +1186,15 @@ impl Pallet { } } + /// Whether `who` is a virtual nominator whose funds are managed by another pallet. pub(crate) fn is_virtual_nominator(who: &T::AccountId) -> bool { VirtualNominators::::contains_key(who) } + + /// Update the lock for a staker. + /// + /// For virtual nominators, it is no-op. pub(crate) fn update_lock( who: &T::AccountId, amount: BalanceOf, From d2b680e4b6d337fa39fca748e5a95c44dba5dc8d Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 29 Mar 2024 10:35:25 +0100 Subject: [PATCH 03/29] fix tests --- substrate/frame/nomination-pools/src/mock.rs | 8 ++++++++ substrate/frame/staking/src/pallet/impls.rs | 1 - 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 686759604c23..2e30bbeed59e 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -128,6 +128,10 @@ impl sp_staking::StakingInterface for StakingMock { Ok(()) } + fn update_payee(_stash: &Self::AccountId, _reward_acc: &Self::AccountId) -> DispatchResult { + unimplemented!("method currently not used in testing") + } + fn chill(_: &Self::AccountId) -> sp_runtime::DispatchResult { Ok(()) } @@ -220,6 +224,10 @@ impl sp_staking::StakingInterface for StakingMock { fn max_exposure_page_size() -> sp_staking::Page { unimplemented!("method currently not used in testing") } + + fn slash_reward_fraction() -> Perbill { + unimplemented!("method currently not used in testing") + } } #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index bd890e86468c..aed8eba61383 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1191,7 +1191,6 @@ impl Pallet { VirtualNominators::::contains_key(who) } - /// Update the lock for a staker. /// /// For virtual nominators, it is no-op. From 646c7f4edff8bbd9aee1e4af8cbf865cf1772744 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 31 Mar 2024 21:19:48 +0200 Subject: [PATCH 04/29] tests --- substrate/frame/staking/src/ledger.rs | 11 +-- substrate/frame/staking/src/pallet/impls.rs | 11 ++- substrate/frame/staking/src/pallet/mod.rs | 4 +- substrate/frame/staking/src/tests.rs | 74 +++++++++++++++++++++ substrate/primitives/staking/src/lib.rs | 64 ------------------ 5 files changed, 91 insertions(+), 73 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 641d7c81ae41..2c9fb65925a0 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -38,10 +38,7 @@ use frame_support::{ use sp_staking::StakingAccount; use sp_std::prelude::*; -use crate::{ - BalanceOf, Bonded, Config, Error, Ledger, Pallet, Payee, RewardDestination, StakingLedger, - STAKING_ID, -}; +use crate::{BalanceOf, Bonded, Config, Error, Ledger, Pallet, Payee, RewardDestination, StakingLedger, STAKING_ID, VirtualStakers}; #[cfg(any(feature = "runtime-benchmarks", test))] use sp_runtime::traits::Zero; @@ -188,7 +185,9 @@ impl StakingLedger { return Err(Error::::NotStash) } + // update lock on stash based on ledger. Pallet::::update_lock(&self.stash, self.total).map_err(|_| Error::::BadState)?; + Ledger::::insert( &self.controller().ok_or_else(|| { defensive!("update called on a ledger that is not bonded."); @@ -207,6 +206,8 @@ impl StakingLedger { if >::contains_key(&self.stash) { return Err(Error::::AlreadyBonded) } + + // check if the payee is ok. if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { return Err(Error::::RewardDestinationRestricted); } @@ -222,6 +223,7 @@ impl StakingLedger { return Err(Error::::NotStash) } + // check if the payee is ok. if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { return Err(Error::::RewardDestinationRestricted); } @@ -265,6 +267,7 @@ impl StakingLedger { >::remove(&stash); >::remove(&stash); + >::remove(&stash); Ok(()) })? diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index aed8eba61383..0a72222e5c5e 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1188,7 +1188,7 @@ impl Pallet { /// Whether `who` is a virtual nominator whose funds are managed by another pallet. pub(crate) fn is_virtual_nominator(who: &T::AccountId) -> bool { - VirtualNominators::::contains_key(who) + VirtualStakers::::contains_key(who) } /// Update the lock for a staker. @@ -1965,10 +1965,14 @@ impl sp_staking::StakingUnsafe for Pallet { return Err(Error::::AlreadyBonded.into()) } + // check if payee not same as who. + ensure!(who != payee, Error::::RewardDestinationRestricted); + + // mark this pallet as consumer of `who`. frame_system::Pallet::::inc_consumers(&who).map_err(|_| Error::::BadState)?; - // mark who as a virtual nominator - VirtualNominators::::insert(who, ()); + // mark who as a virtual nominator. + VirtualStakers::::insert(who, ()); Self::deposit_event(Event::::Bonded { stash: who.clone(), amount: value }); let ledger = StakingLedger::::new(who.clone(), value); @@ -2105,6 +2109,7 @@ impl Pallet { /// * Staking ledger and bond are not corrupted. fn check_ledgers() -> Result<(), TryRuntimeError> { Bonded::::iter() + .filter(|(stash, _)| !VirtualStakers::::contains_key(stash)) .map(|(stash, ctrl)| { // ensure locks consistency. ensure!( diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index bd5372a3e6f2..8f18102175c0 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -379,7 +379,7 @@ pub mod pallet { pub type Nominators = CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations>; - /// Nominators whose funds are managed by other pallets. + /// Stakers whose funds are managed by other pallets. /// /// This pallet does not apply any locks on them, therefore they are only virtually bonded. They /// are expected to be keyless accounts and hence should not be allowed to mutate their ledger @@ -387,7 +387,7 @@ pub mod pallet { /// via low level apis. We keep track of them to do minimal integrity checks. // TODO(ank4n): Can we keep this entry in `Ledger`? Worth a migration? #[pallet::storage] - pub type VirtualNominators = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>; + pub type VirtualStakers = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>; /// The maximum nominator count before we stop allowing new validators to join. /// diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index a5c9abe2f176..f79586072f1d 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6849,6 +6849,80 @@ mod staking_interface { } } +mod staking_unsafe { + use frame_support::traits::InspectLockableCurrency; + use sp_staking::{StakingUnsafe, StakingInterface, Stake}; + + use super::*; + + #[test] + fn virtual_bond_does_not_lock() { + ExtBuilder::default().build_and_execute(|| { + mock::start_active_era(1); + assert_eq!(Balances::free_balance(10), 1); + // 10 can bond more than its balance amount since we do not require lock for virtual + // bonding. + assert_ok!(::virtual_bond(&10, 100, &15)); + // nothing is locked on 10. + assert_eq!(Balances::balance_locked(STAKING_ID, &10), 0); + // adding more balance does not lock anything as well. + assert_ok!(::bond_extra(&10, 1000)); + // but ledger is updated correctly. + assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 1100 })); + + // lets try unbonding some amount. + assert_ok!(::unbond(&10, 200)); + assert_eq!( + Staking::ledger(10.into()).unwrap(), + StakingLedgerInspect { + stash: 10, + total: 1100, + active: 1100 - 200, + unlocking: bounded_vec![UnlockChunk { value: 200, era: 1 + 3 }], + legacy_claimed_rewards: bounded_vec![], + }); + + assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 900 })); + // still no locks. + assert_eq!(Balances::balance_locked(STAKING_ID, &10), 0); + + mock::start_active_era(2); + // cannot withdraw without waiting for unbonding period. + assert_ok!(::withdraw_unbonded(10, 0)); + assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 900 })); + + // in era 4, 10 can withdraw unlocking amount. + mock::start_active_era(4); + assert_ok!(::withdraw_unbonded(10, 0)); + assert_eq!(::stake(&10), Ok(Stake { total: 900, active: 900 })); + + // unbond all. + assert_ok!(::unbond(&10, 900)); + assert_eq!(::stake(&10), Ok(Stake { total: 900, active: 0 })); + mock::start_active_era(7); + assert_ok!(::withdraw_unbonded(10, 0)); + + // ensure withdrawing all amount cleans up storage. + assert_eq!(Staking::ledger(10.into()), Err(Error::::NotStash)); + assert_eq!(VirtualStakers::::contains_key(10), false); + }) + } + + #[test] + fn virtual_nominator_cannot_pay_reward_to_self_account() { + ExtBuilder::default().build_and_execute(|| { + // cannot set payee to self + assert_noop!(::virtual_bond(&10, 100, &10), Error::::RewardDestinationRestricted); + + // to another account works + assert_ok!(::virtual_bond(&10, 100, &11)); + + // cannot set via set_payee as well. + assert_noop!(::update_payee(&10, &10), Error::::RewardDestinationRestricted); + }); + } +} + mod ledger { use super::*; diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 3f2c8c9fb493..0e1812e10ab2 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -449,68 +449,4 @@ pub struct PagedExposureMetadata { pub page_count: Page, } -/// Extension of [`StakingInterface`] with delegation functionality. -/// -/// Introduces two new actors: -/// - `Delegator`: An account that delegates funds to a `Delegatee`. -/// - `Delegatee`: An account that receives delegated funds from `Delegators`. It can then use these -/// funds to participate in the staking system. It can never use its own funds. -/// -/// The `Delegatee` is responsible for managing rewards and slashing for all the `Delegators` that -/// have delegated funds to it. -pub trait DelegatedStakeInterface: StakingInterface { - /// Effective balance of the `delegatee` account. - /// - /// This takes into account any pending slashes to `Delegatee`. - fn delegatee_balance(delegatee: &Self::AccountId) -> Self::Balance; - - /// Returns the total amount of funds delegated by a `delegator`. - fn delegator_balance(delegator: &Self::AccountId) -> Self::Balance; - - /// Delegate funds to `delegatee`. - /// - /// Only used for the initial delegation. Use [`Self::delegate_extra`] to add more delegation. - fn delegate( - delegator: &Self::AccountId, - delegatee: &Self::AccountId, - reward_account: &Self::AccountId, - amount: Self::Balance, - ) -> DispatchResult; - - /// Add more delegation to the `delegatee`. - /// - /// If this is the first delegation, use [`Self::delegate`] instead. - fn delegate_extra( - delegator: &Self::AccountId, - delegatee: &Self::AccountId, - amount: Self::Balance, - ) -> DispatchResult; - - /// Withdraw or revoke delegation to `delegatee`. - /// - /// If there are `delegatee` funds upto `amount` available to withdraw, then those funds would - /// be released to the `delegator` - fn withdraw_delegation( - delegator: &Self::AccountId, - delegatee: &Self::AccountId, - amount: Self::Balance, - ) -> DispatchResult; - - /// Returns true if there are pending slashes posted to the `delegatee` account. - /// - /// Slashes to `delegatee` account are not immediate and are applied lazily. Since `delegatee` - /// has an unbounded number of delegators, immediate slashing is not possible. - fn has_pending_slash(delegatee: &Self::AccountId) -> bool; - - /// Apply a pending slash to a `delegatee` by slashing `value` from `delegator`. - /// - /// If a reporter is provided, the reporter will receive a fraction of the slash as reward. - fn delegator_slash( - delegatee: &Self::AccountId, - delegator: &Self::AccountId, - value: Self::Balance, - maybe_reporter: Option, - ) -> sp_runtime::DispatchResult; -} - sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $); From bc92a19056a1a9496c54b0be5ced3711b9394434 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 31 Mar 2024 21:24:25 +0200 Subject: [PATCH 05/29] fix naming --- substrate/frame/staking/src/pallet/impls.rs | 20 ++++++++++---------- substrate/frame/staking/src/slashing.rs | 4 ++-- substrate/frame/staking/src/tests.rs | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 0a72222e5c5e..5fda07fecf4a 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -154,9 +154,9 @@ impl Pallet { pub(super) fn do_bond_extra(stash: &T::AccountId, additional: BalanceOf) -> DispatchResult { let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?; - // for virtual nominators, we don't need to check the balance. Since they are only accessed + // for virtual stakers, we don't need to check the balance. Since they are only accessed // via low level apis, we can assume that the caller has done the due diligence. - let extra = if Self::is_virtual_nominator(stash) { + let extra = if Self::is_virtual_staker(stash) { additional } else { // additional amount or actual balance of stash whichever is lower. @@ -1170,7 +1170,7 @@ impl Pallet { /// Whether the passed reward destination is restricted for the given account. /// - /// Virtual nominators are not allowed to compound their rewards as this pallet does not manage + /// virtual stakers are not allowed to compound their rewards as this pallet does not manage /// locks for them. For external pallets that manage the virtual bond, it is their /// responsibility to distribute the reward and re-bond them. /// @@ -1179,27 +1179,27 @@ impl Pallet { who: &T::AccountId, reward_destination: RewardDestination, ) -> bool { - Self::is_virtual_nominator(who) && + Self::is_virtual_staker(who) && match reward_destination { RewardDestination::Account(payee) => payee == *who, _ => true, } } - /// Whether `who` is a virtual nominator whose funds are managed by another pallet. - pub(crate) fn is_virtual_nominator(who: &T::AccountId) -> bool { + /// Whether `who` is a virtual staker whose funds are managed by another pallet. + pub(crate) fn is_virtual_staker(who: &T::AccountId) -> bool { VirtualStakers::::contains_key(who) } /// Update the lock for a staker. /// - /// For virtual nominators, it is no-op. + /// For virtual stakers, it is no-op. pub(crate) fn update_lock( who: &T::AccountId, amount: BalanceOf, ) -> sp_runtime::DispatchResult { - // Skip locking virtual nominators. They are handled by external pallets. - if !Self::is_virtual_nominator(who) { + // Skip locking virtual stakers. They are handled by external pallets. + if !Self::is_virtual_staker(who) { T::Currency::set_lock( crate::STAKING_ID, who, @@ -1971,7 +1971,7 @@ impl sp_staking::StakingUnsafe for Pallet { // mark this pallet as consumer of `who`. frame_system::Pallet::::inc_consumers(&who).map_err(|_| Error::::BadState)?; - // mark who as a virtual nominator. + // mark who as a virtual staker. VirtualStakers::::insert(who, ()); Self::deposit_event(Event::::Bonded { stash: who.clone(), amount: value }); diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 19424f57dcdb..2011e9eb8301 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -614,8 +614,8 @@ pub fn do_slash( return } - // Skip slashing for virtual nominators. The pallets managing them should handle the slashing. - if !Pallet::::is_virtual_nominator(stash) { + // Skip slashing for virtual stakers. The pallets managing them should handle the slashing. + if !Pallet::::is_virtual_staker(stash) { let (imbalance, missing) = T::Currency::slash(stash, value); slashed_imbalance.subsume(imbalance); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index f79586072f1d..7d614366d522 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6909,7 +6909,7 @@ mod staking_unsafe { } #[test] - fn virtual_nominator_cannot_pay_reward_to_self_account() { + fn virtual_staker_cannot_pay_reward_to_self_account() { ExtBuilder::default().build_and_execute(|| { // cannot set payee to self assert_noop!(::virtual_bond(&10, 100, &10), Error::::RewardDestinationRestricted); From 2528da740db8b615d3c428cf5c03068a059dc473 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 31 Mar 2024 21:24:51 +0200 Subject: [PATCH 06/29] fmt --- substrate/frame/staking/src/ledger.rs | 5 ++- substrate/frame/staking/src/tests.rs | 54 +++++++++++++++++++-------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 2c9fb65925a0..4d3bfe980482 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -38,7 +38,10 @@ use frame_support::{ use sp_staking::StakingAccount; use sp_std::prelude::*; -use crate::{BalanceOf, Bonded, Config, Error, Ledger, Pallet, Payee, RewardDestination, StakingLedger, STAKING_ID, VirtualStakers}; +use crate::{ + BalanceOf, Bonded, Config, Error, Ledger, Pallet, Payee, RewardDestination, StakingLedger, + VirtualStakers, STAKING_ID, +}; #[cfg(any(feature = "runtime-benchmarks", test))] use sp_runtime::traits::Zero; diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 7d614366d522..bde67760cb0e 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6851,7 +6851,7 @@ mod staking_interface { mod staking_unsafe { use frame_support::traits::InspectLockableCurrency; - use sp_staking::{StakingUnsafe, StakingInterface, Stake}; + use sp_staking::{Stake, StakingInterface, StakingUnsafe}; use super::*; @@ -6868,37 +6868,53 @@ mod staking_unsafe { // adding more balance does not lock anything as well. assert_ok!(::bond_extra(&10, 1000)); // but ledger is updated correctly. - assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 1100 })); + assert_eq!( + ::stake(&10), + Ok(Stake { total: 1100, active: 1100 }) + ); // lets try unbonding some amount. assert_ok!(::unbond(&10, 200)); assert_eq!( - Staking::ledger(10.into()).unwrap(), - StakingLedgerInspect { - stash: 10, - total: 1100, - active: 1100 - 200, - unlocking: bounded_vec![UnlockChunk { value: 200, era: 1 + 3 }], - legacy_claimed_rewards: bounded_vec![], - }); + Staking::ledger(10.into()).unwrap(), + StakingLedgerInspect { + stash: 10, + total: 1100, + active: 1100 - 200, + unlocking: bounded_vec![UnlockChunk { value: 200, era: 1 + 3 }], + legacy_claimed_rewards: bounded_vec![], + } + ); - assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 900 })); + assert_eq!( + ::stake(&10), + Ok(Stake { total: 1100, active: 900 }) + ); // still no locks. assert_eq!(Balances::balance_locked(STAKING_ID, &10), 0); mock::start_active_era(2); // cannot withdraw without waiting for unbonding period. assert_ok!(::withdraw_unbonded(10, 0)); - assert_eq!(::stake(&10), Ok(Stake { total: 1100, active: 900 })); + assert_eq!( + ::stake(&10), + Ok(Stake { total: 1100, active: 900 }) + ); // in era 4, 10 can withdraw unlocking amount. mock::start_active_era(4); assert_ok!(::withdraw_unbonded(10, 0)); - assert_eq!(::stake(&10), Ok(Stake { total: 900, active: 900 })); + assert_eq!( + ::stake(&10), + Ok(Stake { total: 900, active: 900 }) + ); // unbond all. assert_ok!(::unbond(&10, 900)); - assert_eq!(::stake(&10), Ok(Stake { total: 900, active: 0 })); + assert_eq!( + ::stake(&10), + Ok(Stake { total: 900, active: 0 }) + ); mock::start_active_era(7); assert_ok!(::withdraw_unbonded(10, 0)); @@ -6912,13 +6928,19 @@ mod staking_unsafe { fn virtual_staker_cannot_pay_reward_to_self_account() { ExtBuilder::default().build_and_execute(|| { // cannot set payee to self - assert_noop!(::virtual_bond(&10, 100, &10), Error::::RewardDestinationRestricted); + assert_noop!( + ::virtual_bond(&10, 100, &10), + Error::::RewardDestinationRestricted + ); // to another account works assert_ok!(::virtual_bond(&10, 100, &11)); // cannot set via set_payee as well. - assert_noop!(::update_payee(&10, &10), Error::::RewardDestinationRestricted); + assert_noop!( + ::update_payee(&10, &10), + Error::::RewardDestinationRestricted + ); }); } } From 7104fafa86a541d09cb4bae21b0531abfef4cc84 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 07:01:43 +0200 Subject: [PATCH 07/29] move reward account check to caller --- substrate/frame/staking/src/ledger.rs | 10 -------- substrate/frame/staking/src/pallet/impls.rs | 26 +++++++-------------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 4d3bfe980482..01531703c316 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -210,11 +210,6 @@ impl StakingLedger { return Err(Error::::AlreadyBonded) } - // check if the payee is ok. - if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { - return Err(Error::::RewardDestinationRestricted); - } - >::insert(&self.stash, payee); >::insert(&self.stash, &self.stash); self.update() @@ -226,11 +221,6 @@ impl StakingLedger { return Err(Error::::NotStash) } - // check if the payee is ok. - if Pallet::::restrict_reward_destination(&self.stash, payee.clone()) { - return Err(Error::::RewardDestinationRestricted); - } - >::insert(&self.stash, payee); Ok(()) } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 5fda07fecf4a..bd27a328aa21 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1168,24 +1168,6 @@ impl Pallet { EraInfo::::get_full_exposure(era, account) } - /// Whether the passed reward destination is restricted for the given account. - /// - /// virtual stakers are not allowed to compound their rewards as this pallet does not manage - /// locks for them. For external pallets that manage the virtual bond, it is their - /// responsibility to distribute the reward and re-bond them. - /// - /// Conservatively, we expect them to always set the reward destination to a non stash account. - pub(crate) fn restrict_reward_destination( - who: &T::AccountId, - reward_destination: RewardDestination, - ) -> bool { - Self::is_virtual_staker(who) && - match reward_destination { - RewardDestination::Account(payee) => payee == *who, - _ => true, - } - } - /// Whether `who` is a virtual staker whose funds are managed by another pallet. pub(crate) fn is_virtual_staker(who: &T::AccountId) -> bool { VirtualStakers::::contains_key(who) @@ -1827,6 +1809,14 @@ impl StakingInterface for Pallet { } fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult { + // Since virtual stakers are not allowed to compound their rewards as this pallet does not + // manage their locks, we do not allow reward account to be set same as stash. For + // external pallets that manage the virtual bond, they can claim rewards and re-bond them. + ensure!( + !Self::is_virtual_staker(stash) || stash != reward_acc, + Error::::RewardDestinationRestricted + ); + // since controller is deprecated and this function is never used for old ledgers with // distinct controllers, we can safely assume that stash is the controller. Self::set_payee( From aed693c14b97d4c6f40a5d45306ed22f612f18e6 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 07:11:53 +0200 Subject: [PATCH 08/29] virtual staker cannot bond again --- substrate/frame/staking/src/tests.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index bde67760cb0e..094de96f19a5 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6943,6 +6943,32 @@ mod staking_unsafe { ); }); } + + #[test] + fn virtual_staker_cannot_bond_again() { + ExtBuilder::default().build_and_execute(|| { + // 10 virtual bonds + assert_ok!(::virtual_bond(&10, 100, &11)); + + // Tries bonding again + assert_noop!( + ::virtual_bond(&10, 20, &11), + Error::::AlreadyBonded + ); + + // And again with a different reward destination. + assert_noop!( + ::virtual_bond(&10, 20, &12), + Error::::AlreadyBonded + ); + + // Direct bond is not allowed as well. + assert_noop!( + ::bond(&10, 20, &12), + Error::::AlreadyBonded + ); + }); + } } mod ledger { From 989bc500b4ba6a6174441c02ec462d7142232a28 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 07:19:46 +0200 Subject: [PATCH 09/29] release all becomes migrate_to_virtual_staker --- substrate/frame/staking/src/pallet/impls.rs | 5 +++-- substrate/frame/staking/src/tests.rs | 5 +++++ substrate/primitives/staking/src/lib.rs | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index bd27a328aa21..029146935861 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1942,8 +1942,9 @@ impl StakingInterface for Pallet { } impl sp_staking::StakingUnsafe for Pallet { - fn force_release(who: &Self::AccountId) { - T::Currency::remove_lock(crate::STAKING_ID, who) + fn migrate_to_virtual_staker(who: &Self::AccountId) { + T::Currency::remove_lock(crate::STAKING_ID, who); + VirtualStakers::::insert(who, ()); } fn virtual_bond( diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 094de96f19a5..67cc5b54b1a8 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6969,6 +6969,11 @@ mod staking_unsafe { ); }); } + + #[test] + fn migrate_virtual_staker(){ + // TODO(ank4n) test migration integrity + } } mod ledger { diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 0e1812e10ab2..fec915587d40 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -317,8 +317,8 @@ pub trait StakingInterface { pub trait StakingUnsafe: StakingInterface { /// Release all funds bonded for stake without unbonding the ledger. /// - /// Unsafe, only used for migration of `nominator` to `virtual_nominator`. - fn force_release(who: &Self::AccountId); + /// Unsafe, should only used for migration of `nominator` to `virtual_nominator`. + fn migrate_to_virtual_staker(who: &Self::AccountId); /// Book-keep a new bond for `who` without applying any locks (hence virtual). /// From 6f05a52049aa91dd56547a435202f5c06193bf0f Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 07:54:09 +0200 Subject: [PATCH 10/29] add test --- substrate/frame/staking/src/tests.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 67cc5b54b1a8..108cfe3dd0bc 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6971,11 +6971,26 @@ mod staking_unsafe { } #[test] - fn migrate_virtual_staker(){ - // TODO(ank4n) test migration integrity + fn migrate_virtual_staker() { + ExtBuilder::default().build_and_execute(|| { + // give some balance to 200 + Balances::make_free_balance_be(&200, 2000); + + // stake + assert_ok!(Staking::bond(RuntimeOrigin::signed(200), 1000, RewardDestination::Staked)); + assert_eq!(Balances::balance_locked(crate::STAKING_ID, &200), 1000); + + // migrate them to virtual staker + ::migrate_to_virtual_staker(&200); + + // ensure the balance is not locked anymore + assert_eq!(Balances::balance_locked(crate::STAKING_ID, &200), 0); + + // and they are marked as virtual stakers + assert_eq!(Pallet::::is_virtual_staker(&200), true); + }); } } - mod ledger { use super::*; From 264f71a255972c1b35fb521a8fb95a25681f559c Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 08:24:36 +0200 Subject: [PATCH 11/29] small refactors --- substrate/frame/staking/src/mock.rs | 7 +++++++ substrate/frame/staking/src/tests.rs | 10 +++++----- substrate/primitives/staking/src/lib.rs | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 6db462c1a70f..11546e7971a1 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -598,6 +598,13 @@ pub(crate) fn bond_nominator(who: AccountId, val: Balance, target: Vec) { + // who is provided by another pallet in a real scenario. + System::inc_providers(&who); + assert_ok!(::virtual_bond(&who, val, &payee)); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(who), target)); +} + /// Progress to the given block, triggering session and era changes as we progress. /// /// This will finalize the previous block, initialize up to the given block, essentially simulating diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 108cfe3dd0bc..5be7b9c999b9 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6947,24 +6947,24 @@ mod staking_unsafe { #[test] fn virtual_staker_cannot_bond_again() { ExtBuilder::default().build_and_execute(|| { - // 10 virtual bonds - assert_ok!(::virtual_bond(&10, 100, &11)); + // 200 virtual bonds + bond_virtual_nominator(200, 201, 500, vec![11, 21]); // Tries bonding again assert_noop!( - ::virtual_bond(&10, 20, &11), + ::virtual_bond(&200, 200, &201), Error::::AlreadyBonded ); // And again with a different reward destination. assert_noop!( - ::virtual_bond(&10, 20, &12), + ::virtual_bond(&200, 200, &202), Error::::AlreadyBonded ); // Direct bond is not allowed as well. assert_noop!( - ::bond(&10, 20, &12), + ::bond(&200, 200, &202), Error::::AlreadyBonded ); }); diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index fec915587d40..9939860d5da8 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -315,9 +315,9 @@ pub trait StakingInterface { /// These apis bypass some or all safety checks and should only be used if you know what you are /// doing. pub trait StakingUnsafe: StakingInterface { - /// Release all funds bonded for stake without unbonding the ledger. + /// Migrate an existing staker to a virtual staker. /// - /// Unsafe, should only used for migration of `nominator` to `virtual_nominator`. + /// It would release all funds held by the implementation pallet. fn migrate_to_virtual_staker(who: &Self::AccountId); /// Book-keep a new bond for `who` without applying any locks (hence virtual). From f9a52f181499a738a5afced6cba1983a8264c590 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 3 Apr 2024 08:24:59 +0200 Subject: [PATCH 12/29] fmt --- substrate/frame/staking/src/mock.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 11546e7971a1..718ee697faed 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -598,7 +598,12 @@ pub(crate) fn bond_nominator(who: AccountId, val: Balance, target: Vec) { +pub(crate) fn bond_virtual_nominator( + who: AccountId, + payee: AccountId, + val: Balance, + target: Vec, +) { // who is provided by another pallet in a real scenario. System::inc_providers(&who); assert_ok!(::virtual_bond(&who, val, &payee)); From 58b50c9f4e66e2009fdf0b37504982fbe8566de8 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 4 Apr 2024 12:19:25 +0200 Subject: [PATCH 13/29] test for bonders cannot virtual bond --- substrate/frame/staking/src/pallet/mod.rs | 3 ++- substrate/frame/staking/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 8f18102175c0..6d6b993fe211 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -385,7 +385,8 @@ pub mod pallet { /// are expected to be keyless accounts and hence should not be allowed to mutate their ledger /// directly via this pallet. Instead, these accounts are managed by other pallets and accessed /// via low level apis. We keep track of them to do minimal integrity checks. - // TODO(ank4n): Can we keep this entry in `Ledger`? Worth a migration? + // TODO(ank4n): Can we keep this entry in `Ledger`? Or migrate later in conjunction with + // fungible migration? #[pallet::storage] pub type VirtualStakers = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>; diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 5be7b9c999b9..601779f4992c 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6970,6 +6970,23 @@ mod staking_unsafe { }); } + #[test] + fn normal_staker_cannot_virtual_bond() { + ExtBuilder::default().build_and_execute(|| { + // 101 is a nominator trying to virtual bond + assert_noop!( + ::virtual_bond(&101, 200, &102), + Error::::AlreadyBonded + ); + + // validator 21 tries to virtual bond + assert_noop!( + ::virtual_bond(&21, 200, &22), + Error::::AlreadyBonded + ); + }); + } + #[test] fn migrate_virtual_staker() { ExtBuilder::default().build_and_execute(|| { From b946c4a042f2152f68b4a05d91f8bfc780fe2c1a Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 4 Apr 2024 12:38:51 +0200 Subject: [PATCH 14/29] virtual nominators receive rewards --- substrate/frame/staking/src/tests.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 601779f4992c..9300f5ec79fd 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -623,12 +623,8 @@ fn nominating_and_rewards_should_work() { )); assert_ok!(Staking::nominate(RuntimeOrigin::signed(1), vec![11, 21, 31])); - assert_ok!(Staking::bond( - RuntimeOrigin::signed(3), - 1000, - RewardDestination::Account(3) - )); - assert_ok!(Staking::nominate(RuntimeOrigin::signed(3), vec![11, 21, 41])); + // the second nominator is virtual. + bond_virtual_nominator(3, 333, 1000, vec![11, 21, 41]); // the total reward for era 0 let total_payout_0 = current_total_payout_for_duration(reward_time_per_era()); @@ -694,11 +690,10 @@ fn nominating_and_rewards_should_work() { ); // Nominator 3: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 21]'s reward. ==> // 2/9 + 3/11 + assert_eq!(Balances::total_balance(&3), initial_balance); + /// 333 is the reward destination for 3. assert_eq_error_rate!( - Balances::total_balance(&3), - initial_balance + (2 * payout_for_11 / 9 + 3 * payout_for_21 / 11), - 2, - ); + Balances::total_balance(&333), 2 * payout_for_11 / 9 + 3 * payout_for_21 / 11, 2); // Validator 11: got 800 / 1800 external stake => 8/18 =? 4/9 => Validator's share = 5/9 assert_eq_error_rate!( From 0aa9006542ab02f8a5b999552daf31101cbd9dea Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 4 Apr 2024 13:30:52 +0200 Subject: [PATCH 15/29] virtual nominator slashing test --- substrate/frame/staking/src/mock.rs | 9 ++++- substrate/frame/staking/src/tests.rs | 58 +++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 718ee697faed..74762079101e 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -249,17 +249,22 @@ parameter_types! { pub static LedgerSlashPerEra: (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); + pub static SlashObserver: BTreeMap> = BTreeMap::new(); } pub struct EventListenerMock; impl OnStakingUpdate for EventListenerMock { fn on_slash( - _pool_account: &AccountId, + pool_account: &AccountId, slashed_bonded: Balance, slashed_chunks: &BTreeMap, - _total_slashed: Balance, + total_slashed: Balance, ) { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); + // update the observer. + let mut slash_observer = SlashObserver::get(); + slash_observer.insert(pool_account.clone(), total_slashed); + SlashObserver::set(slash_observer); } } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 9300f5ec79fd..b2171d87b32f 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -691,7 +691,7 @@ fn nominating_and_rewards_should_work() { // Nominator 3: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 21]'s reward. ==> // 2/9 + 3/11 assert_eq!(Balances::total_balance(&3), initial_balance); - /// 333 is the reward destination for 3. + // 333 is the reward destination for 3. assert_eq_error_rate!( Balances::total_balance(&333), 2 * payout_for_11 / 9 + 3 * payout_for_21 / 11, 2); @@ -7002,6 +7002,62 @@ mod staking_unsafe { assert_eq!(Pallet::::is_virtual_staker(&200), true); }); } + + #[test] + fn virtual_nominators_are_lazily_slashed() { + ExtBuilder::default().build_and_execute(|| { + mock::start_active_era(1); + let slash_percent = Perbill::from_percent(5); + let initial_exposure = Staking::eras_stakers(active_era(), &11); + // 101 is a nominator for 11 + assert_eq!(initial_exposure.others.first().unwrap().who, 101); + // make 101 a virtual nominator + ::migrate_to_virtual_staker(&101); + // set payee different to self. + assert_ok!(::update_payee(&101, &102)); + + // cache values + let nominator_stake = Staking::ledger(101.into()).unwrap().active; + let nominator_balance = balances(&101).0; + let validator_stake = Staking::ledger(11.into()).unwrap().active; + let validator_balance = balances(&11).0; + let exposed_stake = initial_exposure.total; + let exposed_validator = initial_exposure.own; + let exposed_nominator = initial_exposure.others.first().unwrap().value; + + // 11 goes offline + on_offence_now( + &[OffenceDetails { offender: (11, initial_exposure.clone()), reporters: vec![] }], + &[slash_percent], + ); + + let slash_amount = slash_percent * exposed_stake; + let validator_share = + Perbill::from_rational(exposed_validator, exposed_stake) * slash_amount; + let nominator_share = + Perbill::from_rational(exposed_nominator, exposed_stake) * slash_amount; + + // both slash amounts need to be positive for the test to make sense. + assert!(validator_share > 0); + assert!(nominator_share > 0); + + // both stakes must have been decreased pro-rata. + assert_eq!(Staking::ledger(101.into()).unwrap().active, nominator_stake - nominator_share); + assert_eq!(Staking::ledger(11.into()).unwrap().active, validator_stake - validator_share); + + // validator balance is slashed as usual + assert_eq!(balances(&11).0, validator_balance - validator_share); + // Because slashing happened. + assert!(is_disabled(11)); + + // but virtual nominator's balance is not slashed. + assert_eq!(Balances::free_balance(&101), nominator_balance); + // but slash is broadcasted to slash observers. + assert_eq!(SlashObserver::get().get(&101).unwrap(), &nominator_share); + + + }) + } } mod ledger { use super::*; From d242356751bbcd140b578d418a4fad67cc47ecde Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 4 Apr 2024 13:32:22 +0200 Subject: [PATCH 16/29] fmt --- substrate/frame/staking/src/tests.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index b2171d87b32f..14c740be807c 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -693,7 +693,10 @@ fn nominating_and_rewards_should_work() { assert_eq!(Balances::total_balance(&3), initial_balance); // 333 is the reward destination for 3. assert_eq_error_rate!( - Balances::total_balance(&333), 2 * payout_for_11 / 9 + 3 * payout_for_21 / 11, 2); + Balances::total_balance(&333), + 2 * payout_for_11 / 9 + 3 * payout_for_21 / 11, + 2 + ); // Validator 11: got 800 / 1800 external stake => 8/18 =? 4/9 => Validator's share = 5/9 assert_eq_error_rate!( @@ -7042,8 +7045,14 @@ mod staking_unsafe { assert!(nominator_share > 0); // both stakes must have been decreased pro-rata. - assert_eq!(Staking::ledger(101.into()).unwrap().active, nominator_stake - nominator_share); - assert_eq!(Staking::ledger(11.into()).unwrap().active, validator_stake - validator_share); + assert_eq!( + Staking::ledger(101.into()).unwrap().active, + nominator_stake - nominator_share + ); + assert_eq!( + Staking::ledger(11.into()).unwrap().active, + validator_stake - validator_share + ); // validator balance is slashed as usual assert_eq!(balances(&11).0, validator_balance - validator_share); @@ -7054,8 +7063,6 @@ mod staking_unsafe { assert_eq!(Balances::free_balance(&101), nominator_balance); // but slash is broadcasted to slash observers. assert_eq!(SlashObserver::get().get(&101).unwrap(), &nominator_share); - - }) } } From 8bd3bc3b52fd6be1e037d75d4a616d94e1f19f45 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 4 Apr 2024 15:54:37 +0200 Subject: [PATCH 17/29] fix clippy error --- substrate/frame/staking/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 74762079101e..42578b1b7a30 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -263,7 +263,7 @@ impl OnStakingUpdate for EventListenerMock { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); // update the observer. let mut slash_observer = SlashObserver::get(); - slash_observer.insert(pool_account.clone(), total_slashed); + slash_observer.insert(*pool_account, total_slashed); SlashObserver::set(slash_observer); } } From e46b01b357bd1a37f65130b29e0a1303d3383e97 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 9 Apr 2024 08:30:41 +0200 Subject: [PATCH 18/29] prdoc --- prdoc/pr_3889.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_3889.prdoc diff --git a/prdoc/pr_3889.prdoc b/prdoc/pr_3889.prdoc new file mode 100644 index 000000000000..b32ffcc214c0 --- /dev/null +++ b/prdoc/pr_3889.prdoc @@ -0,0 +1,14 @@ +# 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: Allow privileged virtual bond into pallet Staking + +doc: + - audience: Runtime Dev + description: | + Introduces a new low level API to allow privileged virtual bond into pallet Staking. This allows other pallets + to stake funds into staking pallet while managing the fund lock and unlocking process themselves. + +crates: + - name: pallet-staking + From 9079d8227d1f4f33e9f5ec377fe2fa450ef106a8 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Apr 2024 12:33:22 +0200 Subject: [PATCH 19/29] add migrate to direct staker for testing --- substrate/frame/staking/src/pallet/impls.rs | 13 +++++++++++++ substrate/primitives/staking/src/lib.rs | 3 +++ 2 files changed, 16 insertions(+) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 029146935861..b3dbb7c49322 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1974,6 +1974,19 @@ impl sp_staking::StakingUnsafe for Pallet { Ok(()) } + + #[cfg(feature = "runtime-benchmarks")] + fn migrate_to_direct_staker(who: &Self::AccountId) { + assert!(VirtualStakers::::contains_key(who)); + let ledger = StakingLedger::::get(Stash(who.clone())).unwrap(); + T::Currency::set_lock( + crate::STAKING_ID, + who, + ledger.total, + frame_support::traits::WithdrawReasons::all(), + ); + VirtualStakers::::remove(who); + } } #[cfg(any(test, feature = "try-runtime"))] diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 9939860d5da8..0da02f190489 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -329,6 +329,9 @@ pub trait StakingUnsafe: StakingInterface { value: Self::Balance, payee: &Self::AccountId, ) -> DispatchResult; + + #[cfg(feature = "runtime-benchmarks")] + fn migrate_to_direct_staker(who: &Self::AccountId); } /// The amount of exposure for an era that an individual nominator has (susceptible to slashing). From 616dfa78b6e813732f530ae9b579761604c89359 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Apr 2024 12:40:21 +0200 Subject: [PATCH 20/29] doc comment --- substrate/primitives/staking/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 0da02f190489..e482bf1b85ac 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -330,6 +330,9 @@ pub trait StakingUnsafe: StakingInterface { payee: &Self::AccountId, ) -> DispatchResult; + /// Migrate a virtual staker to a direct staker. + /// + /// Only used for testing. #[cfg(feature = "runtime-benchmarks")] fn migrate_to_direct_staker(who: &Self::AccountId); } From afe8c87b5a234af4fbf418913c2bb681fd9173c5 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Apr 2024 13:59:37 +0200 Subject: [PATCH 21/29] pr feedback --- substrate/frame/staking/src/pallet/impls.rs | 2 +- substrate/frame/staking/src/pallet/mod.rs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index b3dbb7c49322..ff7c95131506 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -169,7 +169,7 @@ impl Pallet { ledger.total += extra; ledger.active += extra; - // Last check: the new active amount of ledger must be more than ED. + // last check: the new active amount of ledger must be more than ED. ensure!(ledger.active >= T::Currency::minimum_balance(), Error::::InsufficientBond); // NOTE: ledger must be updated prior to calling `Self::weight_of`. diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 6d6b993fe211..31f4ddeb32d4 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -871,7 +871,7 @@ pub mod pallet { CannotRestoreLedger, /// Provided reward destination is not allowed. RewardDestinationRestricted, - /// Not enough funds available to withdraw + /// Not enough funds available to withdraw. NotEnoughFunds, } @@ -1308,7 +1308,9 @@ pub mod pallet { Error::::ControllerDeprecated ); - ledger.set_payee(payee)?; + let _ = ledger + .set_payee(payee) + .defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?; Ok(()) } From e7d65e2debcb233c79b8e6285bceb584d8a19152 Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Apr 2024 14:24:01 +0200 Subject: [PATCH 22/29] add try state checks for virtual ledger --- substrate/frame/staking/src/pallet/impls.rs | 25 ++++++++++++++++----- substrate/frame/staking/src/tests.rs | 2 ++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index ff7c95131506..f0c779c1bfb2 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2109,17 +2109,30 @@ impl Pallet { /// Invariants: /// * Stake consistency: ledger.total == ledger.active + sum(ledger.unlocking). /// * The ledger's controller and stash matches the associated `Bonded` tuple. - /// * Staking locked funds for every bonded stash should be the same as its ledger's total. + /// * Staking locked funds for every bonded stash (non virtual stakers) should be the same as + /// its ledger's total. + /// * For virtual stakers, locked funds should be zero and payee should be non-stash account. /// * Staking ledger and bond are not corrupted. fn check_ledgers() -> Result<(), TryRuntimeError> { Bonded::::iter() - .filter(|(stash, _)| !VirtualStakers::::contains_key(stash)) .map(|(stash, ctrl)| { // ensure locks consistency. - ensure!( - Self::inspect_bond_state(&stash) == Ok(LedgerIntegrityState::Ok), - "bond, ledger and/or staking lock inconsistent for a bonded stash." - ); + if VirtualStakers::::contains_key(stash.clone()) { + ensure!(T::Currency::balance_locked(crate::STAKING_ID, &stash) == Zero::zero(), "virtual stakers should not have any locked balance"); + ensure!(>::get(stash.clone()).unwrap() == stash.clone(), "stash and controller should be same"); + ensure!(Ledger::::get(stash.clone()).unwrap().stash == stash, "ledger corrupted for virtual staker"); + let reward_destination = >::get(stash.clone()).unwrap(); + if let RewardDestination::Account(payee) = reward_destination { + ensure!(payee != stash.clone(), "reward destination should not be same as stash for virtual staker"); + } else { + return Err(DispatchError::Other("reward destination must be of account variant for virtual staker")); + } + } else { + ensure!( + Self::inspect_bond_state(&stash) == Ok(LedgerIntegrityState::Ok), + "bond, ledger and/or staking lock inconsistent for a bonded stash." + ); + } // ensure ledger consistency. Self::ensure_ledger_consistent(ctrl) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 14c740be807c..3837aad05193 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6997,6 +6997,8 @@ mod staking_unsafe { // migrate them to virtual staker ::migrate_to_virtual_staker(&200); + // payee needs to be updated to a non-stash account. + assert_ok!(::update_payee(&200, &201)); // ensure the balance is not locked anymore assert_eq!(Balances::balance_locked(crate::STAKING_ID, &200), 0); From 474c1dee3b718366e4c302249eeb0407d1787afb Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Apr 2024 14:24:22 +0200 Subject: [PATCH 23/29] fmt --- substrate/frame/staking/src/pallet/impls.rs | 24 ++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index f0c779c1bfb2..81f4a9ead883 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2118,14 +2118,28 @@ impl Pallet { .map(|(stash, ctrl)| { // ensure locks consistency. if VirtualStakers::::contains_key(stash.clone()) { - ensure!(T::Currency::balance_locked(crate::STAKING_ID, &stash) == Zero::zero(), "virtual stakers should not have any locked balance"); - ensure!(>::get(stash.clone()).unwrap() == stash.clone(), "stash and controller should be same"); - ensure!(Ledger::::get(stash.clone()).unwrap().stash == stash, "ledger corrupted for virtual staker"); + ensure!( + T::Currency::balance_locked(crate::STAKING_ID, &stash) == Zero::zero(), + "virtual stakers should not have any locked balance" + ); + ensure!( + >::get(stash.clone()).unwrap() == stash.clone(), + "stash and controller should be same" + ); + ensure!( + Ledger::::get(stash.clone()).unwrap().stash == stash, + "ledger corrupted for virtual staker" + ); let reward_destination = >::get(stash.clone()).unwrap(); if let RewardDestination::Account(payee) = reward_destination { - ensure!(payee != stash.clone(), "reward destination should not be same as stash for virtual staker"); + ensure!( + payee != stash.clone(), + "reward destination should not be same as stash for virtual staker" + ); } else { - return Err(DispatchError::Other("reward destination must be of account variant for virtual staker")); + return Err(DispatchError::Other( + "reward destination must be of account variant for virtual staker", + )); } } else { ensure!( From a884846703b8dab4eae593beca30abf658a333ae Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 17 Apr 2024 18:29:23 +0200 Subject: [PATCH 24/29] safe maths --- substrate/frame/staking/src/pallet/impls.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 81f4a9ead883..c14849ba8ba6 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -36,9 +36,10 @@ use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; use pallet_session::historical; use sp_runtime::{ traits::{ - Bounded, CheckedSub, Convert, One, SaturatedConversion, Saturating, StaticLookup, Zero, + Bounded, CheckedAdd, CheckedSub, Convert, One, SaturatedConversion, Saturating, + StaticLookup, Zero, }, - Perbill, Percent, + ArithmeticError, Perbill, Percent, }; use sp_staking::{ currency_to_vote::CurrencyToVote, @@ -163,12 +164,12 @@ impl Pallet { additional.min( T::Currency::free_balance(stash) .checked_sub(&ledger.total) - .ok_or(sp_runtime::ArithmeticError::Overflow)?, + .ok_or(ArithmeticError::Overflow)?, ) }; - ledger.total += extra; - ledger.active += extra; + ledger.total = ledger.total.checked_add(&extra).ok_or(ArithmeticError::Overflow)?; + ledger.active = ledger.active.checked_add(&extra).ok_or(ArithmeticError::Overflow)?; // last check: the new active amount of ledger must be more than ED. ensure!(ledger.active >= T::Currency::minimum_balance(), Error::::InsufficientBond); From d4633c2c0bf1a1a45ce326cf0b24d8e668a51dd7 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 17 Apr 2024 18:39:15 +0200 Subject: [PATCH 25/29] fix docs based on feedback --- substrate/frame/staking/src/mock.rs | 5 ++++- substrate/frame/staking/src/pallet/impls.rs | 19 ++++++++++--------- substrate/primitives/staking/src/lib.rs | 7 ++++--- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 42578b1b7a30..76a4a579e0ab 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -609,8 +609,11 @@ pub(crate) fn bond_virtual_nominator( val: Balance, target: Vec, ) { - // who is provided by another pallet in a real scenario. + // In a real scenario, `who` is a keyless account managed by another pallet which provides for + // it. System::inc_providers(&who); + + // Bond who virtually. assert_ok!(::virtual_bond(&who, val, &payee)); assert_ok!(Staking::nominate(RuntimeOrigin::signed(who), target)); } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index c14849ba8ba6..24ea647d8a6b 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1948,29 +1948,30 @@ impl sp_staking::StakingUnsafe for Pallet { VirtualStakers::::insert(who, ()); } + /// Virtually bonds `keyless_who` to `payee` with `value`. + /// + /// The payee must not be the same as the `keyless_who`. fn virtual_bond( - who: &Self::AccountId, + keyless_who: &Self::AccountId, value: Self::Balance, payee: &Self::AccountId, ) -> DispatchResult { - if StakingLedger::::is_bonded(StakingAccount::Stash(who.clone())) { + if StakingLedger::::is_bonded(StakingAccount::Stash(keyless_who.clone())) { return Err(Error::::AlreadyBonded.into()) } // check if payee not same as who. - ensure!(who != payee, Error::::RewardDestinationRestricted); + ensure!(keyless_who != payee, Error::::RewardDestinationRestricted); // mark this pallet as consumer of `who`. - frame_system::Pallet::::inc_consumers(&who).map_err(|_| Error::::BadState)?; + frame_system::Pallet::::inc_consumers(&keyless_who).map_err(|_| Error::::BadState)?; // mark who as a virtual staker. - VirtualStakers::::insert(who, ()); + VirtualStakers::::insert(keyless_who, ()); - Self::deposit_event(Event::::Bonded { stash: who.clone(), amount: value }); - let ledger = StakingLedger::::new(who.clone(), value); + Self::deposit_event(Event::::Bonded { stash: keyless_who.clone(), amount: value }); + let ledger = StakingLedger::::new(keyless_who.clone(), value); - // You're auto-bonded forever, here. We might improve this by only bonding when - // you actually validate/nominate and remove once you unbond __everything__. ledger.bond(RewardDestination::Account(payee.clone()))?; Ok(()) diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index e482bf1b85ac..662d71c7e3d1 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -320,10 +320,11 @@ pub trait StakingUnsafe: StakingInterface { /// It would release all funds held by the implementation pallet. fn migrate_to_virtual_staker(who: &Self::AccountId); - /// Book-keep a new bond for `who` without applying any locks (hence virtual). + /// Book-keep a new bond for `keyless_who` without applying any locks (hence virtual). /// - /// It is important that who is a keyless account and therefore cannot interact with staking - /// pallet directly. Caller is responsible for ensuring the passed amount is locked and valid. + /// It is important that `keyless_who` is a keyless account and therefore cannot interact with + /// staking pallet directly. Caller is responsible for ensuring the passed amount is locked and + /// valid. fn virtual_bond( keyless_who: &Self::AccountId, value: Self::Balance, From c013cded09529f9a395736d421abc1a265a45f4d Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 17 Apr 2024 18:41:20 +0200 Subject: [PATCH 26/29] rename StakingUnsafe to StakingUnchecked --- substrate/frame/staking/src/mock.rs | 2 +- substrate/frame/staking/src/pallet/impls.rs | 2 +- substrate/frame/staking/src/tests.rs | 22 ++++++++++----------- substrate/primitives/staking/src/lib.rs | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 76a4a579e0ab..4df616d91370 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -614,7 +614,7 @@ pub(crate) fn bond_virtual_nominator( System::inc_providers(&who); // Bond who virtually. - assert_ok!(::virtual_bond(&who, val, &payee)); + assert_ok!(::virtual_bond(&who, val, &payee)); assert_ok!(Staking::nominate(RuntimeOrigin::signed(who), target)); } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 24ea647d8a6b..3c1925cdfac4 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1942,7 +1942,7 @@ impl StakingInterface for Pallet { } } -impl sp_staking::StakingUnsafe for Pallet { +impl sp_staking::StakingUnchecked for Pallet { fn migrate_to_virtual_staker(who: &Self::AccountId) { T::Currency::remove_lock(crate::STAKING_ID, who); VirtualStakers::::insert(who, ()); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 3837aad05193..aff7e40efc43 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6847,9 +6847,9 @@ mod staking_interface { } } -mod staking_unsafe { +mod staking_unchecked { use frame_support::traits::InspectLockableCurrency; - use sp_staking::{Stake, StakingInterface, StakingUnsafe}; + use sp_staking::{Stake, StakingInterface, StakingUnchecked}; use super::*; @@ -6860,7 +6860,7 @@ mod staking_unsafe { assert_eq!(Balances::free_balance(10), 1); // 10 can bond more than its balance amount since we do not require lock for virtual // bonding. - assert_ok!(::virtual_bond(&10, 100, &15)); + assert_ok!(::virtual_bond(&10, 100, &15)); // nothing is locked on 10. assert_eq!(Balances::balance_locked(STAKING_ID, &10), 0); // adding more balance does not lock anything as well. @@ -6927,12 +6927,12 @@ mod staking_unsafe { ExtBuilder::default().build_and_execute(|| { // cannot set payee to self assert_noop!( - ::virtual_bond(&10, 100, &10), + ::virtual_bond(&10, 100, &10), Error::::RewardDestinationRestricted ); // to another account works - assert_ok!(::virtual_bond(&10, 100, &11)); + assert_ok!(::virtual_bond(&10, 100, &11)); // cannot set via set_payee as well. assert_noop!( @@ -6950,13 +6950,13 @@ mod staking_unsafe { // Tries bonding again assert_noop!( - ::virtual_bond(&200, 200, &201), + ::virtual_bond(&200, 200, &201), Error::::AlreadyBonded ); // And again with a different reward destination. assert_noop!( - ::virtual_bond(&200, 200, &202), + ::virtual_bond(&200, 200, &202), Error::::AlreadyBonded ); @@ -6973,13 +6973,13 @@ mod staking_unsafe { ExtBuilder::default().build_and_execute(|| { // 101 is a nominator trying to virtual bond assert_noop!( - ::virtual_bond(&101, 200, &102), + ::virtual_bond(&101, 200, &102), Error::::AlreadyBonded ); // validator 21 tries to virtual bond assert_noop!( - ::virtual_bond(&21, 200, &22), + ::virtual_bond(&21, 200, &22), Error::::AlreadyBonded ); }); @@ -6996,7 +6996,7 @@ mod staking_unsafe { assert_eq!(Balances::balance_locked(crate::STAKING_ID, &200), 1000); // migrate them to virtual staker - ::migrate_to_virtual_staker(&200); + ::migrate_to_virtual_staker(&200); // payee needs to be updated to a non-stash account. assert_ok!(::update_payee(&200, &201)); @@ -7017,7 +7017,7 @@ mod staking_unsafe { // 101 is a nominator for 11 assert_eq!(initial_exposure.others.first().unwrap().who, 101); // make 101 a virtual nominator - ::migrate_to_virtual_staker(&101); + ::migrate_to_virtual_staker(&101); // set payee different to self. assert_ok!(::update_payee(&101, &102)); diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 662d71c7e3d1..ad6cc6e2f4ff 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -314,7 +314,7 @@ pub trait StakingInterface { /// /// These apis bypass some or all safety checks and should only be used if you know what you are /// doing. -pub trait StakingUnsafe: StakingInterface { +pub trait StakingUnchecked: StakingInterface { /// Migrate an existing staker to a virtual staker. /// /// It would release all funds held by the implementation pallet. From ac3cadf78bf4a13093258ea801b118a693f4e61b Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 17 Apr 2024 18:58:55 +0200 Subject: [PATCH 27/29] use static mutate --- substrate/frame/staking/src/mock.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 4df616d91370..385ce656f446 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -261,10 +261,7 @@ impl OnStakingUpdate for EventListenerMock { total_slashed: Balance, ) { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); - // update the observer. - let mut slash_observer = SlashObserver::get(); - slash_observer.insert(*pool_account, total_slashed); - SlashObserver::set(slash_observer); + SlashObserver::mutate(|map| map.insert(*pool_account, total_slashed)); } } From e31866e842e65e8713f488bc14dbcb935630bcad Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Apr 2024 15:46:50 +0200 Subject: [PATCH 28/29] inline locking --- substrate/frame/staking/src/ledger.rs | 12 ++++++++++-- substrate/frame/staking/src/pallet/impls.rs | 20 -------------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 01531703c316..dd9ce11c4d2c 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -188,8 +188,16 @@ impl StakingLedger { return Err(Error::::NotStash) } - // update lock on stash based on ledger. - Pallet::::update_lock(&self.stash, self.total).map_err(|_| Error::::BadState)?; + // We skip locking virtual stakers. + if !Pallet::::is_virtual_staker(&self.stash) { + // for direct stakers, update lock on stash based on ledger. + T::Currency::set_lock( + STAKING_ID, + &self.stash, + self.total, + frame_support::traits::WithdrawReasons::all(), + ); + } Ledger::::insert( &self.controller().ok_or_else(|| { diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 3c1925cdfac4..0c0ef0dbf463 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1173,26 +1173,6 @@ impl Pallet { pub(crate) fn is_virtual_staker(who: &T::AccountId) -> bool { VirtualStakers::::contains_key(who) } - - /// Update the lock for a staker. - /// - /// For virtual stakers, it is no-op. - pub(crate) fn update_lock( - who: &T::AccountId, - amount: BalanceOf, - ) -> sp_runtime::DispatchResult { - // Skip locking virtual stakers. They are handled by external pallets. - if !Self::is_virtual_staker(who) { - T::Currency::set_lock( - crate::STAKING_ID, - who, - amount, - frame_support::traits::WithdrawReasons::all(), - ); - } - - Ok(()) - } } impl Pallet { From 078cf5fe72079072b21b3823ab45e5deb17c2714 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Apr 2024 16:25:25 +0200 Subject: [PATCH 29/29] optimize stash kill --- substrate/frame/staking/src/ledger.rs | 9 ++++++--- substrate/frame/staking/src/mock.rs | 4 +++- substrate/frame/staking/src/pallet/mod.rs | 2 -- substrate/frame/staking/src/tests.rs | 8 ++++---- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index dd9ce11c4d2c..67a86b86226c 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -263,12 +263,15 @@ impl StakingLedger { let controller = >::get(stash).ok_or(Error::::NotStash)?; >::get(&controller).ok_or(Error::::NotController).map(|ledger| { - T::Currency::remove_lock(STAKING_ID, &ledger.stash); Ledger::::remove(controller); - >::remove(&stash); >::remove(&stash); - >::remove(&stash); + + // kill virtual staker if it exists. + if >::take(&stash).is_none() { + // if not virtual staker, clear locks. + T::Currency::remove_lock(STAKING_ID, &ledger.stash); + } Ok(()) })? diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 385ce656f446..b46b863c016e 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -261,7 +261,9 @@ impl OnStakingUpdate for EventListenerMock { total_slashed: Balance, ) { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); - SlashObserver::mutate(|map| map.insert(*pool_account, total_slashed)); + SlashObserver::mutate(|map| { + map.insert(*pool_account, map.get(pool_account).unwrap_or(&0) + total_slashed) + }); } } diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 31f4ddeb32d4..76ddad6f1359 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -385,8 +385,6 @@ pub mod pallet { /// are expected to be keyless accounts and hence should not be allowed to mutate their ledger /// directly via this pallet. Instead, these accounts are managed by other pallets and accessed /// via low level apis. We keep track of them to do minimal integrity checks. - // TODO(ank4n): Can we keep this entry in `Ledger`? Or migrate later in conjunction with - // fungible migration? #[pallet::storage] pub type VirtualStakers = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>; diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index aff7e40efc43..87f6fd424bd7 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -27,7 +27,7 @@ use frame_support::{ assert_noop, assert_ok, assert_storage_noop, dispatch::{extract_actual_weight, GetDispatchInfo, WithPostDispatchInfo}, pallet_prelude::*, - traits::{Currency, Get, ReservableCurrency}, + traits::{Currency, Get, InspectLockableCurrency, ReservableCurrency}, }; use mock::*; @@ -1891,7 +1891,7 @@ fn reap_stash_works() { .balance_factor(10) .build_and_execute(|| { // given - assert_eq!(Balances::free_balance(11), 10 * 1000); + assert_eq!(Balances::balance_locked(STAKING_ID, &11), 10 * 1000); assert_eq!(Staking::bonded(&11), Some(11)); assert!(>::contains_key(&11)); @@ -1917,6 +1917,8 @@ fn reap_stash_works() { assert!(!>::contains_key(&11)); assert!(!>::contains_key(&11)); assert!(!>::contains_key(&11)); + // lock is removed. + assert_eq!(Balances::balance_locked(STAKING_ID, &11), 0); }); } @@ -6848,7 +6850,6 @@ mod staking_interface { } mod staking_unchecked { - use frame_support::traits::InspectLockableCurrency; use sp_staking::{Stake, StakingInterface, StakingUnchecked}; use super::*; @@ -7546,7 +7547,6 @@ mod ledger { mod ledger_recovery { use super::*; - use frame_support::traits::InspectLockableCurrency; #[test] fn inspect_recovery_ledger_simple_works() {