Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow privileged virtual bond in Staking pallet #3889

Merged
merged 35 commits into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
dda12d4
add ability to super bond to staking
Ank4n Mar 29, 2024
802784c
some comments
Ank4n Mar 29, 2024
d2b680e
fix tests
Ank4n Mar 29, 2024
646c7f4
tests
Ank4n Mar 31, 2024
bc92a19
fix naming
Ank4n Mar 31, 2024
2528da7
fmt
Ank4n Mar 31, 2024
4ddcb77
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Mar 31, 2024
7104faf
move reward account check to caller
Ank4n Apr 3, 2024
aed693c
virtual staker cannot bond again
Ank4n Apr 3, 2024
989bc50
release all becomes migrate_to_virtual_staker
Ank4n Apr 3, 2024
6f05a52
add test
Ank4n Apr 3, 2024
264f71a
small refactors
Ank4n Apr 3, 2024
f9a52f1
fmt
Ank4n Apr 3, 2024
58b50c9
test for bonders cannot virtual bond
Ank4n Apr 4, 2024
b946c4a
virtual nominators receive rewards
Ank4n Apr 4, 2024
0aa9006
virtual nominator slashing test
Ank4n Apr 4, 2024
d242356
fmt
Ank4n Apr 4, 2024
8bd3bc3
fix clippy error
Ank4n Apr 4, 2024
fbccd0f
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Apr 9, 2024
e46b01b
prdoc
Ank4n Apr 9, 2024
0e41b01
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Apr 10, 2024
9079d82
add migrate to direct staker for testing
Ank4n Apr 11, 2024
616dfa7
doc comment
Ank4n Apr 11, 2024
afe8c87
pr feedback
Ank4n Apr 11, 2024
e7d65e2
add try state checks for virtual ledger
Ank4n Apr 11, 2024
474c1de
fmt
Ank4n Apr 11, 2024
6940f88
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Apr 17, 2024
a884846
safe maths
Ank4n Apr 17, 2024
d4633c2
fix docs based on feedback
Ank4n Apr 17, 2024
c013cde
rename StakingUnsafe to StakingUnchecked
Ank4n Apr 17, 2024
ac3cadf
use static mutate
Ank4n Apr 17, 2024
09d3359
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Apr 18, 2024
e31866e
inline locking
Ank4n Apr 19, 2024
078cf5f
optimize stash kill
Ank4n Apr 19, 2024
3790a40
Merge branch 'master' into ankan/01-prep-staking-for-delegation
Ank4n Apr 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions substrate/frame/nomination-pools/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand Down Expand Up @@ -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)]
Expand Down
38 changes: 26 additions & 12 deletions substrate/frame/staking/src/ledger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
VirtualStakers, STAKING_ID,
};

#[cfg(any(feature = "runtime-benchmarks", test))]
Expand Down Expand Up @@ -187,7 +188,9 @@ impl<T: Config> StakingLedger<T> {
return Err(Error::<T>::NotStash)
}

T::Currency::set_lock(STAKING_ID, &self.stash, self.total, WithdrawReasons::all());
// update lock on stash based on ledger.
Pallet::<T>::update_lock(&self.stash, self.total).map_err(|_| Error::<T>::BadState)?;
Ank4n marked this conversation as resolved.
Show resolved Hide resolved

Ledger::<T>::insert(
&self.controller().ok_or_else(|| {
defensive!("update called on a ledger that is not bonded.");
Expand All @@ -204,22 +207,32 @@ impl<T: Config> StakingLedger<T> {
/// It sets the reward preferences for the bonded stash.
pub(crate) fn bond(self, payee: RewardDestination<T::AccountId>) -> Result<(), Error<T>> {
if <Bonded<T>>::contains_key(&self.stash) {
Err(Error::<T>::AlreadyBonded)
} else {
<Payee<T>>::insert(&self.stash, payee);
<Bonded<T>>::insert(&self.stash, &self.stash);
self.update()
return Err(Error::<T>::AlreadyBonded)
}

// check if the payee is ok.
if Pallet::<T>::restrict_reward_destination(&self.stash, payee.clone()) {
return Err(Error::<T>::RewardDestinationRestricted);
}

<Payee<T>>::insert(&self.stash, payee);
<Bonded<T>>::insert(&self.stash, &self.stash);
self.update()
}

/// Sets the ledger Payee.
pub(crate) fn set_payee(self, payee: RewardDestination<T::AccountId>) -> Result<(), Error<T>> {
if !<Bonded<T>>::contains_key(&self.stash) {
Err(Error::<T>::NotStash)
} else {
<Payee<T>>::insert(&self.stash, payee);
Ok(())
return Err(Error::<T>::NotStash)
}

// check if the payee is ok.
if Pallet::<T>::restrict_reward_destination(&self.stash, payee.clone()) {
return Err(Error::<T>::RewardDestinationRestricted);
}

<Payee<T>>::insert(&self.stash, payee);
Ok(())
}

/// Sets the ledger controller to its stash.
Expand Down Expand Up @@ -257,6 +270,7 @@ impl<T: Config> StakingLedger<T> {

<Bonded<T>>::remove(&stash);
<Payee<T>>::remove(&stash);
<VirtualStakers<T>>::remove(&stash);
Ank4n marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
})?
Expand Down
130 changes: 128 additions & 2 deletions substrate/frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -149,6 +151,39 @@ impl<T: Config> Pallet<T> {
Self::slashable_balance_of_vote_weight(who, issuance)
}

pub(super) fn do_bond_extra(stash: &T::AccountId, additional: BalanceOf<T>) -> DispatchResult {
let mut ledger = Self::ledger(StakingAccount::Stash(stash.clone()))?;

// 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_staker(stash) {
additional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add the field saying that a ledger is virtual to the staking ledger, I'd add a debug_assert here to double checking that the ledger is in fact virtual.

} 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;
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
ledger.active += extra;
// Last check: the new active amount of ledger must be more than ED.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

for consistency

ensure!(ledger.active >= T::Currency::minimum_balance(), Error::<T>::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::<T>::Bonded { stash: stash.clone(), amount: extra });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit but I could see it being helpful in the future for traceability to distinguish ledger-related events that are virtual and non-virtual. Maybe we could add a field virtual: bool at very low cost.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue the distinction between virtual and non-virtual bonds should be made one level higher, as in an event from the entity doing the delegation should deposit a "virtual" event. At the staking pallet level, it probably doesn't matter whether the bonded funds are virtual or not, so long as they are bonded, but I don't feel too strongly about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree with @georgepisaltu more, but in general staking is already aware and leaking info about "virtual vs. not" in any case, so it is a bit of a lost cause :p


Ok(())
}

pub(super) fn do_withdraw_unbonded(
controller: &T::AccountId,
num_slashing_spans: u32,
Expand Down Expand Up @@ -1132,6 +1167,49 @@ impl<T: Config> Pallet<T> {
) -> Exposure<T::AccountId, BalanceOf<T>> {
EraInfo::<T>::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(
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
who: &T::AccountId,
reward_destination: RewardDestination<T::AccountId>,
) -> 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::<T>::contains_key(who)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated before, I think we could get rid of this extra storage map and add this info directly into the ledger. In any case, I'd remove this one-liner for readability.

}

/// Update the lock for a staker.
///
/// For virtual stakers, it is no-op.
pub(crate) fn update_lock(
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
who: &T::AccountId,
amount: BalanceOf<T>,
) -> 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<T: Config> Pallet<T> {
Expand Down Expand Up @@ -1748,6 +1826,15 @@ impl<T: Config> StakingInterface for Pallet<T> {
.map(|_| ())
}

fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult {
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Expand Down Expand Up @@ -1832,6 +1919,10 @@ impl<T: Config> StakingInterface for Pallet<T> {
}
}

fn slash_reward_fraction() -> Perbill {
SlashRewardFraction::<T>::get()
}

sp_staking::runtime_benchmarks_enabled! {
fn nominations(who: &Self::AccountId) -> Option<Vec<T::AccountId>> {
Nominators::<T>::get(who).map(|n| n.targets.into_inner())
Expand Down Expand Up @@ -1860,6 +1951,40 @@ impl<T: Config> StakingInterface for Pallet<T> {
}
}

impl<T: Config> sp_staking::StakingUnsafe for Pallet<T> {
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,
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
) -> DispatchResult {
if StakingLedger::<T>::is_bonded(StakingAccount::Stash(who.clone())) {
return Err(Error::<T>::AlreadyBonded.into())
}

// check if payee not same as who.
ensure!(who != payee, Error::<T>::RewardDestinationRestricted);

// mark this pallet as consumer of `who`.
frame_system::Pallet::<T>::inc_consumers(&who).map_err(|_| Error::<T>::BadState)?;

// mark who as a virtual staker.
VirtualStakers::<T>::insert(who, ());

Self::deposit_event(Event::<T>::Bonded { stash: who.clone(), amount: value });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Self::deposit_event(Event::<T>::Bonded { stash: who.clone(), amount: value });
Self::deposit_event(Event::<T>::Bonded { stash: who.clone(), amount: value, virtual: true });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is already a corresponding event in delegated staking pallet, I am leaning towards leaving it as it is. A dapp can get all information they need by reading other pallet events as well.

let ledger = StakingLedger::<T>::new(who.clone(), value);

// You're auto-bonded forever, here. We might improve this by only bonding when
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
// you actually validate/nominate and remove once you unbond __everything__.
ledger.bond(RewardDestination::Account(payee.clone()))?;

Ok(())
}
}

#[cfg(any(test, feature = "try-runtime"))]
impl<T: Config> Pallet<T> {
pub(crate) fn do_try_state(_: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
Expand Down Expand Up @@ -1984,6 +2109,7 @@ impl<T: Config> Pallet<T> {
/// * Staking ledger and bond are not corrupted.
fn check_ledgers() -> Result<(), TryRuntimeError> {
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
Bonded::<T>::iter()
.filter(|(stash, _)| !VirtualStakers::<T>::contains_key(stash))
.map(|(stash, ctrl)| {
// ensure locks consistency.
ensure!(
Expand Down
44 changes: 17 additions & 27 deletions substrate/frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -379,6 +379,16 @@ pub mod pallet {
pub type Nominators<T: Config> =
CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations<T>>;

/// 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
/// 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 VirtualStakers<T: Config> = CountedStorageMap<_, Twox64Concat, T::AccountId, ()>;
Ank4n marked this conversation as resolved.
Show resolved Hide resolved

/// The maximum nominator count before we stop allowing new validators to join.
///
/// When this value is not set, no limits are enforced.
Expand Down Expand Up @@ -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
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
NotEnoughFunds,
}

#[pallet::hooks]
Expand Down Expand Up @@ -985,29 +999,7 @@ pub mod pallet {
#[pallet::compact] max_additional: BalanceOf<T>,
) -> 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::<T>::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::<T>::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
Expand Down Expand Up @@ -1315,9 +1307,7 @@ pub mod pallet {
Error::<T>::ControllerDeprecated
);

let _ = ledger
.set_payee(payee)
.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?;
ledger.set_payee(payee)?;
Ank4n marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
}
Expand Down
Loading
Loading