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 26 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
14 changes: 14 additions & 0 deletions prdoc/pr_3889.prdoc
Original file line number Diff line number Diff line change
@@ -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

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 @@ -131,6 +131,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 @@ -223,6 +227,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
28 changes: 16 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,22 @@ 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)
}

<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)
}

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

/// Sets the ledger controller to its stash.
Expand Down Expand Up @@ -257,6 +260,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
21 changes: 19 additions & 2 deletions substrate/frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,17 +249,22 @@ parameter_types! {
pub static LedgerSlashPerEra:
(BalanceOf<Test>, BTreeMap<EraIndex, BalanceOf<Test>>) =
(Zero::zero(), BTreeMap::new());
pub static SlashObserver: BTreeMap<AccountId, BalanceOf<Test>> = BTreeMap::new();
}

pub struct EventListenerMock;
impl OnStakingUpdate<AccountId, Balance> for EventListenerMock {
fn on_slash(
_pool_account: &AccountId,
pool_account: &AccountId,
slashed_bonded: Balance,
slashed_chunks: &BTreeMap<EraIndex, Balance>,
_total_slashed: Balance,
total_slashed: Balance,
) {
LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone()));
// update the observer.
let mut slash_observer = SlashObserver::get();
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
slash_observer.insert(*pool_account, total_slashed);
SlashObserver::set(slash_observer);
}
}

Expand Down Expand Up @@ -598,6 +603,18 @@ pub(crate) fn bond_nominator(who: AccountId, val: Balance, target: Vec<AccountId
assert_ok!(Staking::nominate(RuntimeOrigin::signed(who), target));
}

pub(crate) fn bond_virtual_nominator(
who: AccountId,
payee: AccountId,
val: Balance,
target: Vec<AccountId>,
) {
// who is provided by another pallet in a real scenario.
System::inc_providers(&who);
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
assert_ok!(<Staking as sp_staking::StakingUnsafe>::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
Expand Down
171 changes: 164 additions & 7 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.
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,31 @@ impl<T: Config> Pallet<T> {
) -> Exposure<T::AccountId, BalanceOf<T>> {
EraInfo::<T>::get_full_exposure(era, account)
}

/// 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 +1808,23 @@ 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 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::<T>::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(
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 +1909,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 +1941,54 @@ impl<T: Config> StakingInterface for Pallet<T> {
}
}

impl<T: Config> sp_staking::StakingUnsafe for Pallet<T> {
fn migrate_to_virtual_staker(who: &Self::AccountId) {
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
T::Currency::remove_lock(crate::STAKING_ID, who);
VirtualStakers::<T>::insert(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(feature = "runtime-benchmarks")]
fn migrate_to_direct_staker(who: &Self::AccountId) {
assert!(VirtualStakers::<T>::contains_key(who));
let ledger = StakingLedger::<T>::get(Stash(who.clone())).unwrap();
T::Currency::set_lock(
crate::STAKING_ID,
who,
ledger.total,
frame_support::traits::WithdrawReasons::all(),
);
VirtualStakers::<T>::remove(who);
}
}

#[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 @@ -1980,16 +2109,44 @@ impl<T: Config> Pallet<T> {
/// 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> {
Ank4n marked this conversation as resolved.
Show resolved Hide resolved
Bonded::<T>::iter()
.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::<T>::contains_key(stash.clone()) {
ensure!(
T::Currency::balance_locked(crate::STAKING_ID, &stash) == Zero::zero(),
"virtual stakers should not have any locked balance"
);
ensure!(
<Bonded<T>>::get(stash.clone()).unwrap() == stash.clone(),
"stash and controller should be same"
);
ensure!(
Ledger::<T>::get(stash.clone()).unwrap().stash == stash,
"ledger corrupted for virtual staker"
);
let reward_destination = <Payee<T>>::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)
Expand Down
Loading
Loading