Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add storage bounds for pallet
staking
and clean up deprecated non paged exposure storages #6445base: master
Are you sure you want to change the base?
Add storage bounds for pallet
staking
and clean up deprecated non paged exposure storages #6445Changes from 115 commits
6c808b6
1b18bb1
0ac60af
4edb8e1
65c7de6
35069c9
d4b15af
eb93a70
58cf45d
292c5d0
74f2055
63f2c5e
b249a67
362f505
4f21d53
e258baf
4b7d896
01e2233
243bec8
a599afc
9a25fbc
98daabc
aec5087
36204da
18e9bee
cbfd4a5
b5cbfec
9ca25a1
10e7295
b3186b1
336137a
c1911e1
2ac794e
583e46d
6347a8c
9e82b85
1db7f39
f996a97
e030506
3ab45d9
a980daa
827e898
c4d5142
df8e567
c066632
686b6b3
9cf2263
21ad9ea
ca881ac
e986264
ea59383
9a21b8f
47863bb
50ce1a1
40ccdec
c3365a6
584fe53
5333a7d
31a9300
898d469
32df05d
7bc9b98
9c4d874
536d5c5
608ec88
76469ea
fb1a4fa
181fe4d
b1f5f97
9c002c1
503a7d9
63a18b1
49059c0
efde7a7
8186680
3fb6ca7
66923a4
bec0544
3708085
73af22a
1139689
2bda8eb
fe3147b
cd8a0c3
5b15e24
9d055a8
07b021b
87e0e46
473e0ff
eb64ffc
74d6c7a
94f5b31
7b1fc5e
cf9d8c3
c265974
bb5229c
6588714
4754629
76a2d4b
67c41fa
bd28db5
043b7d6
68481f4
312904e
d508f8d
529fa0f
17eeef7
21a12d1
e2fa0ca
4bf3cd6
f7c1fe4
c881bb0
306cd41
2da736a
831f1b0
3702ce5
489fc9d
65fc95b
9c88d39
a5e98e6
7b82520
30520af
0d47dc5
4b58f0a
33e59b2
c889905
300b7f9
fc63dc7
720c9eb
a065a91
483eb03
7aeb8a5
c51d09b
1a9be6b
235a260
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall observation is that we had to add temporarily a lot of bounds to pallets just for benchmarks, but as staking has now proper bounds for all its storage, they are not needed ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message can be a bit more informative, hinting at which config should be tweaked if this
expect
is ever reached.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error message improved in 0d47dc5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the logs for this migration in this CI job:
https://github.com/paritytech/polkadot-sdk/actions/runs/12412281493/job/34651846644?pr=6445
there seems to be some error-ish logs in there:
💸 Migration failed for ClaimedRewards from v16 to v17.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved with
force_from(...)
forWeakBoundedVec
. I noticed that old individual validator exposures have up to 205 pages, while we would expect them to be at most 20 in the future @Ank4nWhat do you think is the best approach: keep 20 as a limit for
MaxRewardPagesPerValidator
for future items and force old items in aWeakBoundedVec
or increase theMaxRewardPagesPerValidator
limit to something like 250 to accomodate old items?EDIT: I can also see a third option: merge pages for old items until they are less than 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be included in the title, there might be other chain using the staking pallet,
It could also be nice to know if a migration is needed in case a chain didn't yet removed them, or they need to wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included in the title + killed deleted storage items (thanks!) in 21a12d1 + f7c1fe4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it lacks guideline in regards to the clean up of deprecated non paged exposure storage items.
The issue #433 state that 84 eras must pass for the migration to finish.
I think the prdoc should mention what are the consequences of this cleanup.
I would imagine a message like this:
I don't have the exact details of the consequences of not waiting for the 84 eras. But if you know what are the consequences then you can also write it down.
We should keep in mind that prdoc is used by other projects using the staking pallets not only polkadot/kusama.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other pallets like session, babe, grandpa and such also have a notion of
MaxValidators
(expressed asMaxAuthorities
) that should be equal to the max validators of this pallet.within this file, you can use one
pub const MaxValidators: u32 = 300
everywhere to unify it.Taking it a step further, you can expose this as a part of
trait SessionManager
. this trait can expose atrat SessionManager { type MaxAuthorities = <set-by-staking-pallet> }
. Then, within thepallet-session
, who consumesSessionManager
, you can do:This might be too much for this PR, but good for you to be familiar with the pattern.
Whenever they are multiple parameters within two pallets that have a logical dependency (they have to be equal, or one has to be larger than the other), you can remove the implicitness like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in a065a91
Opened issue #6972 to unify
MaxValidators
limit (I also see aMaxActiveValidators
)