Skip to content

Conversation

@moliholy
Copy link
Contributor

@moliholy moliholy commented Apr 22, 2025

This PR continues the implementation of rossbulat and runcomet regarding RFC-0097 for fast unbonding.

After #8127 being merged, this PR continues the previous work started in #8232, moving the logic from pallet-statking to pallet-staking-async.


Description

This PR implements RFC-0097 and its modification, which introduces a variable unbonding time mechanism to improve the security-liquidity trade-off in the staking system.

The unbonding queue allows for faster unbonding of staked tokens while maintaining network security by keeping a minimum slashable share of stake locked for the full unbonding period.

Modified extrinsics:

  • unbond: Enhanced to work with the unbonding queue mechanism, calculating variable unbonding times based on stake distribution.
  • withdraw_unbonded: Modified to handle withdrawals from the unbonding queue with different unlock periods.
  • rebond: Updated to work with the queue-based unbonding system.

Storage items:

  • ElectableStashes: Added the stake backing up a given electable stash account.
  • UnbondingQueueParams: New storage item containing configuration parameters:
    • min_slashable_share: The minimum share of stake of the lowest backed validators that must remain slashable at any point in time.
    • lowest_ratio: The proportion of the lowest-backed validator set.
    • unbond_period_lower_bound: Minimum unbonding time in eras.
  • ErasLowestRatioTotalStake: Tracks the lowest stake proportion among validators in each era.
  • TotalUnbondInEra: Tracks the stake that started unbonding in a given era.
  • Ledger: Added previous_unbonded_stake to track the accumulated stake to be unbonded at the time a given unlock chunk was created.

View functions:

  • unbonding_duration: Used to obtain the list of funds that will be released at a given era.

Key features:

  1. Variable unbonding time: Unbonding time varies based on the amount of stake in the system.
  2. Security maintenance: Ensures a sufficient percentage of stake remains slashable at all times.
  3. Unbonding queue management: Implements an efficient queue system for managing unbonding requests.

Migration v18:

Includes multi-block migration LazyMigrationV17ToV18 that:

  • Updates StakingLedger structure to include the new previous_unbonded_stake field in UnlockChunk.
  • Migrates ElectableStashes from set-based to map-based storage (AccountId -> Balance mapping).
  • Performs era adjustment for existing unlock chunks by subtracting the bonding duration.
  • Uses stepped migration pattern to handle large datasets efficiently across multiple blocks.
  • Includes comprehensive pre/post-upgrade checks for data integrity.

@moliholy moliholy requested a review from a team as a code owner April 22, 2025 12:40
@moliholy moliholy marked this pull request as draft April 22, 2025 12:40
@moliholy moliholy changed the title Implement RFC-0097 for the staking-async pallet Implementation of RFC-0097: Unbonding Queue for pallet-staking-async Apr 22, 2025
@moliholy moliholy force-pushed the RFC-0097-port branch 6 times, most recently from e39998b to a65ced0 Compare April 23, 2025 13:51
@moliholy moliholy marked this pull request as ready for review April 29, 2025 15:19
@moliholy
Copy link
Contributor Author

/cmd prdoc

@github-actions
Copy link
Contributor

Command "prdoc" has failed ❌! See logs here

.and_then(|eras| eras.first())
.copied()
// if nothing in queue, use the active era.
.unwrap_or(active_era)
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not entirely sure about this logic (maybe I’m missing something so please add more context if you have).

The purpose of calculating the earliest withdrawal era is to ensure that if offences for an era haven’t been processed yet, withdrawals for that era are blocked.

Before this PR: We check the earliest era for which offences are still unprocessed. And if queue is completely empty, we fall back to active_era - 1 + bonding duration - implying anything unbonded before 27 days can be withdrawn.

New Change (I think):
if queue is empty -> allow any withdrawal for unbonding era > active era - 2
if queue not empty -> allow any withdrawal for unbonding era >= oldest_offence_era + 1 - 2
( + 1 to get the era for which all offences are processed.)

Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I simply changed logical comparison, including equality. But calculate_earliest_withdrawal_era was indeed not returning by itself the correct value. I just fixed it in e69da63.

@moliholy
Copy link
Contributor Author

@kianenigma @Ank4n we have wrapped up the work here from BlockDeep side and have addressed the review feedback provided so far. We are handing it over to you and the code owners for any future changes and merging the PR when possible. Thank you.

@kianenigma
Copy link
Contributor

@redzsina letting you know that I have done a round of review on this myself, and I have found more minor issues + need for some cleanup for better ergonomics. I suspect I will be done withing a week or two, and the audit can resume. This will make sure we won't need an audit for the second round of work. Sorry for the inconvenience!

@louismerlin louismerlin moved this from In progress to Scheduled in Security Audit (PRs) - SRLabs Nov 3, 2025
@kianenigma
Copy link
Contributor

kianenigma commented Nov 11, 2025

I want to leave a comment here indicating some of the pieces that are missing in this PR, giving an understanding to everyone that getting this passed the finish line is not merely a matter of clicking merge here.

I have spent about a full day reading this PR and making adjustments, and my work can be found in https://github.com/paritytech/polkadot-sdk/compare/kiz-rfc97?expand=1, specifically in this last commit: 4061e09

So far, I have identified the following:

  1. The way that the PR is initializing UnbondingQueueConfig is not ergonomic. This value is set as OptionQuery, and the code decides to do different things based on if it is Some(_) or None. Note that this was maybe a previous suggestion of me. After a bit of looking around, I have improved this to be as such: This value is always present, with its default being type DefaultUnbondingConfig which is set to values that will fallback to the previous 28 day fixed. A helper constructor is added fn from_fixed(days: u32) -> UnbondingQueueConfig that existing runtimes should use.
  2. UnbondingDuration/MaxUnbondingDuration is fully removed. UnbondingQueueConfig now has a min_time and max_time. The remaining MaxUnbondingDuration is only used as a bound for BoundedVec<_> and checking that we don't set min_time and max_time to too high values.
  3. The core unbonding logic was needlessly complicated, and I have re-written it to be an exact 1-1 map to the research spec. See fn can_release. Then, figuring out the current estimated release time of an unstake request becomes just a loop over the said function. Not necessarily performant, but much easier to read and argue about.
  4. Pruning of old eras need to be updated to remove the new data added in this PR as well.
  5. We need a strategy to retroactively change the ErasLowestRatioTotalStake in case the parameter changes.
  6. Many more test and edge cases not covered (listed as todo!() tests in the commit above).
  7. Much more elaboration and testing is needed on the transition period where this code is deployed, but the historical data we need about unbonding is not populated yet.
  8. I have not looked at the migration yet, and it is a complicated part of the PR
  9. Audit must happen, and there has already been one finding related to this PR (related to how we manage blocking of withdrawals while there are unprocessed slashes).

All in all, I think finishing this would require someone with more deep knowledge to spend at least 3-4 full weeks to further test the PR, and make it maintainable. Additionally, we need someone with PET knowledge (e.g. @rockbmb) to add a comprehensive PET test suite for the user behavior of this PR. Lastly, most wallets and UIs need to be adjusted to take this into account.

Again, I am mainly posting this here to clarify to the community that finishing this PR is not trivial. Blockdeep has done a lot of the heavy lifting, but since staking is a critical system of Polkadot, and the staking codebase requires a lot of domain specific knowledge, it is very hard to cover all corner cases.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/proposal-dynamic-allocation-pool-dap/15878/20

@redzsina redzsina moved this from Scheduled to In progress in Security Audit (PRs) - SRLabs Nov 17, 2025
@redzsina redzsina moved this from In progress to Scheduled in Security Audit (PRs) - SRLabs Nov 17, 2025
@redzsina redzsina moved this from Scheduled to Backlog in Security Audit (PRs) - SRLabs Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T2-pallets This PR/Issue is related to a particular pallet.

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

6 participants