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

Improving try state running time for staking pallet #13147

Closed
wants to merge 4 commits into from

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Jan 15, 2023

This should improve try state running time. We already check that VoterList::count == Nominators::count + Validators::count in fn check_count() which should give us sufficient safety as well as speed.

Should resolve paritytech/polkadot-sdk#234. It took around 6 second for try staking test to run locally on my computer with polkadot runtime and state.

Note: The try state checks will fail on kusama runtime because of a corrupted ledger. I am following up with the team whose account is affected to get it fixed via governance.

@Ank4n Ank4n requested a review from kianenigma as a code owner January 15, 2023 18:26
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 15, 2023
@Ank4n Ank4n added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jan 15, 2023
@Ank4n Ank4n changed the title Try state running time for staking pallet Imrpoving try state running time for staking pallet Jan 15, 2023
@Ank4n Ank4n requested review from ggwpez and gpestana January 15, 2023 23:54
@gpestana gpestana changed the title Imrpoving try state running time for staking pallet Improving try state running time for staking pallet Jan 16, 2023
@@ -1681,12 +1681,6 @@ impl<T: Config> StakingInterface for Pallet<T> {
#[cfg(any(test, feature = "try-runtime"))]
impl<T: Config> Pallet<T> {
pub(crate) fn do_try_state(_: BlockNumberFor<T>) -> Result<(), &'static str> {
ensure!(
T::VoterList::iter()
.all(|x| <Nominators<T>>::contains_key(&x) || <Validators<T>>::contains_key(&x)),
Copy link
Member

@ggwpez ggwpez Jan 18, 2023

Choose a reason for hiding this comment

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

Could you still try what difference it makes if you read the whole maps first and then iterate locally?
This is probably slow since it has quadratic storage complexity.
You can load the whole map with map::iter().collect().
Then we keep the tests security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this was totally wrong. I was not copying session keys which is why the try state was running fast for me locally.
The voterlist tests were still fast probably because second reads are from cache.

The test that is taking long time is the check_nominators. It has cubic time complexity (Total Nominators * Active Validator * Validator exposure list).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to draft till I find some way to speed up the above.

@Ank4n Ank4n added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 18, 2023
@Ank4n Ank4n marked this pull request as draft January 18, 2023 17:56
@Ank4n Ank4n closed this Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Staking try_state times out
3 participants