Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Automatic withdraw_unbonded upon unbond #12582

Merged
merged 22 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9e569f5
Prevents max unbonding chunk slots from being filled when unbonding i…
gpestana Oct 29, 2022
a25ecaa
hardcode num_slashing to unlock chunks automatically
gpestana Nov 8, 2022
6e57960
refactor withdraw logic to do_withdraw; idiomatic rust improvements
gpestana Nov 8, 2022
e89a3cd
a
gpestana Nov 9, 2022
437ed30
callable unbond() to return a DispatchWithPostInfo to dynamically upd…
gpestana Nov 14, 2022
af00da5
refunds overpaid fees when unbond with withdraw
gpestana Nov 14, 2022
2fe20e4
fetches real slashing spans before withdrawal call
gpestana Nov 15, 2022
ffeb1b0
nits
gpestana Nov 15, 2022
d6b5adc
addresses PR comments
gpestana Nov 20, 2022
add919f
Adds more testing
gpestana Nov 21, 2022
90461ad
fixes doc comments
gpestana Nov 29, 2022
a12d351
Fixes weight refunding logic for fn unbond
gpestana Dec 1, 2022
8cc482a
generalizes to return used weight or dispatch error
gpestana Dec 2, 2022
a56794d
Update frame/staking/src/pallet/mod.rs
gpestana Dec 6, 2022
e209b7f
Update frame/staking/src/pallet/mod.rs
gpestana Dec 6, 2022
acaac84
Addresses PR comments
gpestana Dec 7, 2022
05d98f6
Add comment to speculative num spans
gpestana Dec 7, 2022
5222598
Merge branch 'master' into staking_interface-ensure_unbond
gpestana Dec 8, 2022
bbabc96
Merge branch 'master' into staking_interface-ensure_unbond
gpestana Dec 14, 2022
25317b5
adds missing add_slashing_spans in withdraw_unbonded_kill benchmarks
gpestana Dec 14, 2022
b376ad0
".git/.scripts/bench-bot.sh" pallet dev pallet_staking
Dec 14, 2022
b965691
fix publish
gpestana Dec 14, 2022
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
6 changes: 2 additions & 4 deletions frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1608,10 +1608,8 @@ pub mod pallet {
///
/// # Note
///
/// If there are too many unlocking chunks to unbond with the pool account,
/// [`Call::pool_withdraw_unbonded`] can be called to try and minimize unlocking chunks. If
/// there are too many unlocking chunks, the result of this call will likely be the
/// `NoMoreChunks` error from the staking system.
/// The [`StakingInterface::unbond`] will implicitly call [`Call::pool_withdraw_unbonded`]
gpestana marked this conversation as resolved.
Show resolved Hide resolved
/// if there are no left unlocking chunk slots available.
#[pallet::weight(T::WeightInfo::unbond())]
#[transactional]
pub fn unbond(
Expand Down
63 changes: 63 additions & 0 deletions frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,69 @@ impl<T: Config> Pallet<T> {
Self::slashable_balance_of_vote_weight(who, issuance)
}

pub(super) fn do_withdraw_unbonded(
controller: &T::AccountId,
num_slashing_spans: u32,
) -> DispatchResultWithPostInfo {
let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
let (stash, old_total) = (ledger.stash.clone(), ledger.total);
if let Some(current_era) = Self::current_era() {
ledger = ledger.consolidate_unlocked(current_era)
}

let mut dispatch_weight = Weight::zero();

let num_slashing_spans = {
// if using the speculative number of spans, get real number of slashing spans and
// calculate weight associated with fetching real number of slashing spans.
if num_slashing_spans == crate::SPECULATIVE_NUM_SPANS {
let real_num_slashing_spans = Self::slashing_spans(&controller).iter().count();
dispatch_weight = T::DbWeight::get()
.reads(1 as u64)
.saturating_mul(real_num_slashing_spans as u64);

real_num_slashing_spans as u32
} else {
num_slashing_spans
}
};

let post_info_weight = if ledger.unlocking.is_empty() &&
ledger.active < T::Currency::minimum_balance()
{
// This account must have called `unbond()` with some value that caused the active
// portion to fall below existential deposit + will have no more unlocking chunks
// left. We can now safely remove all staking-related information.
Self::kill_stash(&stash, num_slashing_spans)?;
// Remove the lock.
T::Currency::remove_lock(STAKING_ID, &stash);
// This is worst case scenario, so we use the full weight and return None
None
} else {
// This was the consequence of a partial unbond. just update the ledger and move on.
Self::update_ledger(&controller, &ledger);

// This is only an update, so we use less overall weight.
Some(
dispatch_weight
.saturating_add(T::WeightInfo::withdraw_unbonded_update(num_slashing_spans)),
)
};

// `old_total` should never be less than the new total because
// `consolidate_unlocked` strictly subtracts balance.
if ledger.total < old_total {
// Already checked that this won't overflow by entry condition.
let value = old_total - ledger.total;
Self::deposit_event(Event::<T>::Withdrawn { stash, amount: value });
}

Ok(post_info_weight
.map(|weight| weight.saturating_add(dispatch_weight))
.or_else(|| Some(dispatch_weight))
.into())
}

pub(super) fn do_payout_stakers(
validator_stash: T::AccountId,
era: EraIndex,
Expand Down
64 changes: 20 additions & 44 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use crate::{
};

const STAKING_ID: LockIdentifier = *b"staking ";
pub(crate) const SPECULATIVE_NUM_SPANS: u32 = 10_000;
gpestana marked this conversation as resolved.
Show resolved Hide resolved

#[frame_support::pallet]
pub mod pallet {
Expand Down Expand Up @@ -113,7 +114,6 @@ pub mod pallet {
// we only accept an election provider that has staking as data provider.
DataProvider = Pallet<Self>,
>;

/// Something that provides the election functionality at genesis.
type GenesisElectionProvider: frame_election_provider_support::ElectionProvider<
AccountId = Self::AccountId,
Expand Down Expand Up @@ -932,27 +932,36 @@ pub mod pallet {
/// the funds out of management ready for transfer.
///
/// No more than a limited number of unlocking chunks (see `MaxUnlockingChunks`)
/// can co-exists at the same time. In that case, [`Call::withdraw_unbonded`] need
/// to be called first to remove some of the chunks (if possible).
/// can co-exists at the same time. If there are no unlocking chunks slots available
/// [`Call::withdraw_unbonded`] is called to remove some of the chunks (if possible).
///
/// If a user encounters the `InsufficientBond` error when calling this extrinsic,
/// they should call `chill` first in order to free up their bonded funds.
///
/// Emits `Unbonded`.
///
/// See also [`Call::withdraw_unbonded`].
#[pallet::weight(T::WeightInfo::unbond())]
#[pallet::weight(
T::WeightInfo::withdraw_unbonded_kill(SPECULATIVE_NUM_SPANS).saturating_add(T::WeightInfo::unbond()))
]
pub fn unbond(
origin: OriginFor<T>,
#[pallet::compact] value: BalanceOf<T>,
) -> DispatchResult {
let controller = ensure_signed(origin)?;
let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
ensure!(
ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize,
Error::<T>::NoMoreChunks,
);
gpestana marked this conversation as resolved.
Show resolved Hide resolved
let controller = ensure_signed(origin.clone())?;
let unlocking = Self::ledger(&controller)
.map(|l| l.unlocking.len())
.ok_or(Error::<T>::NotController)?;

// ensure that there's chunk slots available by requesting the staking interface to
gpestana marked this conversation as resolved.
Show resolved Hide resolved
// withdraw chunks older than `BondingDuration`, if there are no more unlocking chunks
// slots available.
if unlocking == T::MaxUnlockingChunks::get() as usize {
Self::do_withdraw_unbonded(&controller, SPECULATIVE_NUM_SPANS)
.map_err(|with_post| with_post.error)?;
}

gpestana marked this conversation as resolved.
Show resolved Hide resolved
let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
gpestana marked this conversation as resolved.
Show resolved Hide resolved
let mut value = value.min(ledger.active);

if !value.is_zero() {
Expand Down Expand Up @@ -1026,40 +1035,7 @@ pub mod pallet {
num_slashing_spans: u32,
) -> DispatchResultWithPostInfo {
let controller = ensure_signed(origin)?;
let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
let (stash, old_total) = (ledger.stash.clone(), ledger.total);
if let Some(current_era) = Self::current_era() {
ledger = ledger.consolidate_unlocked(current_era)
}

let post_info_weight = if ledger.unlocking.is_empty() &&
ledger.active < T::Currency::minimum_balance()
{
// This account must have called `unbond()` with some value that caused the active
// portion to fall below existential deposit + will have no more unlocking chunks
// left. We can now safely remove all staking-related information.
Self::kill_stash(&stash, num_slashing_spans)?;
// Remove the lock.
T::Currency::remove_lock(STAKING_ID, &stash);
// This is worst case scenario, so we use the full weight and return None
None
} else {
// This was the consequence of a partial unbond. just update the ledger and move on.
Self::update_ledger(&controller, &ledger);

// This is only an update, so we use less overall weight.
Some(T::WeightInfo::withdraw_unbonded_update(num_slashing_spans))
};

// `old_total` should never be less than the new total because
// `consolidate_unlocked` strictly subtracts balance.
if ledger.total < old_total {
// Already checked that this won't overflow by entry condition.
let value = old_total - ledger.total;
Self::deposit_event(Event::<T>::Withdrawn { stash, amount: value });
}

Ok(post_info_weight.into())
Self::do_withdraw_unbonded(&controller, num_slashing_spans)
}

/// Declare the desire to validate for the origin controller.
Expand Down
35 changes: 19 additions & 16 deletions frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1350,12 +1350,14 @@ fn bond_extra_and_withdraw_unbonded_works() {
}

#[test]
fn too_many_unbond_calls_should_not_work() {
fn many_unbond_calls_should_work() {
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
ExtBuilder::default().build_and_execute(|| {
let mut current_era = 0;
// locked at era MaxUnlockingChunks - 1 until 3

for i in 0..<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() - 1 {
let max_unlocking_chunks = <<Test as Config>::MaxUnlockingChunks as Get<u32>>::get();

for i in 0..max_unlocking_chunks - 1 {
// There is only 1 chunk per era, so we need to be in a new era to create a chunk.
current_era = i as u32;
mock::start_active_era(current_era);
Expand All @@ -1365,27 +1367,28 @@ fn too_many_unbond_calls_should_not_work() {
current_era += 1;
mock::start_active_era(current_era);

// This chunk is locked at `current_era` through `current_era + 2` (because BondingDuration
// This chunk is locked at `current_era` through `current_era + 2` (because `BondingDuration`
// == 3).
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
assert_eq!(
Staking::ledger(&10).unwrap().unlocking.len(),
Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(),
<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() as usize
);
// can't do more.
assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::<Test>::NoMoreChunks);

current_era += 2;
mock::start_active_era(current_era);

assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::<Test>::NoMoreChunks);
// free up everything except the most recently added chunk.
assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(10), 0));
assert_eq!(Staking::ledger(&10).unwrap().unlocking.len(), 1);
// even though the number of unlocked chunks is the same as `MaxUnlockingChunks`,
// unbonding works as expected.
for i in current_era..(current_era + max_unlocking_chunks) - 1 {
// There is only 1 chunk per era, so we need to be in a new era to create a chunk.
current_era = i as u32;
mock::start_active_era(current_era);
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
}

// Can add again.
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
assert_eq!(Staking::ledger(&10).unwrap().unlocking.len(), 2);
// only slots of unbonds within last `BondingDuration` are filled.
assert_eq!(
Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(),
<<Test as Config>::BondingDuration>::get() as usize
);
})
}

Expand Down