[NPoS] Paging reward payouts in order to scale rewardable nominators #1189
Conversation
Ank4n
added
A3-backport
substrate/frame/staking/src/lib.rs
Outdated
| /// Returns the number of pages of exposure a validator has for the given era. | ||
| /// | ||
| /// For eras where paged exposure does not exist, this returns 1 to keep backward compatibility. | ||
| pub(crate) fn get_page_count(era: EraIndex, validator: &T::AccountId) -> PageIndex { |
There was a problem hiding this comment.
This shouldn't return PageIndex, just usize
There was a problem hiding this comment.
Its a good point and I have thought about it too. But isn't page count always going to have the same type as PageIndex?
There was a problem hiding this comment.
Yeah it is, it works perfectly fine and will probably never change, it's just a minor naming thing. By reading PageIndex, you would expect you get back an actual page index, not the number of pages. Even though they are both numbers, it's better to be expressive with type names.
There was a problem hiding this comment.
Yeah agree about the readability but this is stored in storage and we can't use usize for that, so am keeping it as same type as PageIndex. It also is used to compare against PageIndex so it makes sense to have them as same type.
What do you think about renaming the type to just call it Page?
There was a problem hiding this comment.
You can still use u32, which is the type aliased to PageIndex, that'd be better in my opinion
| /// Wrapper struct for Era related information. It is not a pure encapsulation as these storage | ||
| /// items can be accessed directly but nevertheless, its recommended to use `EraInfo` where we | ||
| /// can and add more functions to it as needed. | ||
| pub(crate) struct EraInfo<T>(sp_std::marker::PhantomData<T>); |
There was a problem hiding this comment.
This could go on another file so as to not keep on bloating the lib.rs
Ank4n
added
T1-FRAME
polkadot/runtime/kusama/src/lib.rs
Outdated
| @@ -279,7 +279,7 @@ impl pallet_babe::Config for Runtime { | |||
| type WeightInfo = (); | |||
|
|
|||
| type MaxAuthorities = MaxAuthorities; | |||
| type MaxNominators = MaxNominatorRewardedPerValidator; | |||
| type MaxNominators = MaxExposurePageSize; | |||
There was a problem hiding this comment.
Isn't this a pretty bad name? I belive what pallet_babe wants to convey here is MaxNominatorsPerValidator?
There was a problem hiding this comment.
Also, indeed, the correct value for this should be something like max_pages * page_size, but I trust that this will come in a follow-up.
There was a problem hiding this comment.
Its terrible but its just more apparent now :(. MaxNominatorRewardedPerValidator was also wrong and exactly same thing.
There was a problem hiding this comment.
I will add a comment about this in the code. Not sure what else I can do in the scope of current PR.
| @@ -195,7 +195,7 @@ impl pallet_staking::Config for Test { | |||
| type SessionInterface = Self; | |||
| type UnixTime = pallet_timestamp::Pallet<Test>; | |||
| type EraPayout = pallet_staking::ConvertCurve<RewardCurve>; | |||
| type MaxNominatorRewardedPerValidator = ConstU32<64>; | |||
There was a problem hiding this comment.
can't wait to use derive_impl for staking as well, it is one of those pallet that is used in a couple of mocks.
…ech#1189)" (paritytech#1275) This reverts commit ad4299f.
…aritytech#1189)" (paritytech#1275)" (paritytech#1333) This reverts commit ffd25db.
…ech#1189)" (paritytech#1275) This reverts commit ad4299f.
…aritytech#1189)" (paritytech#1275)" (paritytech#1333) This reverts commit ffd25db.
…ech#1189)" (paritytech#1275) This reverts commit ad4299f.
…aritytech#1189)" (paritytech#1275)" (paritytech#1333) This reverts commit ffd25db.
…ech#1189)" (paritytech#1275) This reverts commit ad4299f.
…aritytech#1189)" (paritytech#1275)" (paritytech#1333) This reverts commit ffd25db.
…ech#1189)" (paritytech#1275) This reverts commit ad4299f.
…aritytech#1189)" (paritytech#1275)" (paritytech#1333) This reverts commit ffd25db.
closes #426. related to #1189. Would help offchain programs to query if there are unclaimed pages of rewards for a given era. The logic could look like below ```js // loop as long as all era pages are claimed. while (api.call.stakingApi.pendingRewards(era, validator_stash)) { api.tx.staking.payout_stakers(validator_stash, era) } ```
closes #426. related to #1189. Would help offchain programs to query if there are unclaimed pages of rewards for a given era. The logic could look like below ```js // loop as long as all era pages are claimed. while (api.call.stakingApi.pendingRewards(era, validator_stash)) { api.tx.staking.payout_stakers(validator_stash, era) } ```
helps #439.
closes #473.
PR link in the older substrate repository: paritytech/substrate#13498.
Context
Rewards payout is processed today in a single block and limited to
MaxNominatorRewardedPerValidator. This number is currently 512 on both Kusama and Polkadot.This PR tries to scale the nominators payout to an unlimited count in a multi-block fashion. Exposures are stored in pages, with each page capped to a certain number (
MaxExposurePageSize). Starting out, this number would be the same asMaxNominatorRewardedPerValidator, but eventually, this number can be lowered through new runtime upgrades to limit the rewardeable nominators per dispatched call instruction.The changes in the PR are backward compatible.
How payouts would work like after this change
Staking exposes two calls, 1) the existing
payout_stakersand 2)payout_stakers_by_page.payout_stakers
This remains backward compatible with no signature change. If for a given era a validator has multiple pages, they can call
payout_stakersmultiple times. The pages are executed in an ascending sequence and the runtime takes care of preventing double claims.payout_stakers_by_page
Very similar to
payout_stakersbut also accepts an extra parampage_index. An account can choose to payout rewards only for an explicitly passedpage_index.Lets look at an example scenario
Given an active validator on Kusama had 1100 nominators,
MaxExposurePageSizeset to 512 for Era e. In order to pay out rewards to all nominators, the caller would need to callpayout_stakers3 times.payout_stakers(origin, stash, e)=> will pay the first 512 nominators.payout_stakers(origin, stash, e)=> will pay the second set of 512 nominators.payout_stakers(origin, stash, e)=> will pay the last set of 76 nominators....
payout_stakers(origin, stash, e)=> calling it the 4th time would return an errorInvalidPage.The above calls can also be replaced by
payout_stakers_by_pageand passing apage_indexexplicitly.Commission note
Validator commission is paid out in chunks across all the pages where each commission chunk is proportional to the total stake of the current page. This implies higher the total stake of a page, higher will be the commission. If all the pages of a validator's single era are paid out, the sum of commission paid to the validator across all pages should be equal to what the commission would have been if we had a non-paged exposure.
Migration Note
Strictly speaking, we did not need to bump our storage version since there is no migration of storage in this PR. But it is still useful to mark a storage upgrade for the following reasons:
HistoryDeptheras, the exposure would be incrementally migrated to its corresponding paged storage item.HistoryDeptheras with current upgraded version (14) for the migration to complete. At some eraEsuch thatE > era_at_which_V14_gets_into_effect + HistoryDepth, we will upgrade to version X which will remove the deprecated storage items.In other words, it is a strict requirement that Ex - E14 >
HistoryDepth, whereEx = Era at which deprecated storages are removed from runtime,
E14 = Era at which runtime is upgraded to version 14.
Storage Changes
Added
Deprecated
The following can be cleaned up after 84 eras which is tracked here.
Config Changes
TODO
MaxNominatorRewardedPerValidator->MaxExposurePageSize.ErasStakersPaged.Followup issues
Runtime api for deprecated ErasStakers storage item