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

Migration for over locked accounts in phgragmen elections #10649

Merged
merged 12 commits into from
Jan 18, 2022

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jan 13, 2022

This PR adds a migration that will fix any accounts affected by the bug addressed in #10646

pub fn migrate<T: Config>() -> Weight {
let mut weight = 0;

for (who, mut voter) in Voting::<T>::iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

could also use ::translate.

Copy link
Contributor Author

@emostov emostov Jan 13, 2022

Choose a reason for hiding this comment

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

From my understanding translate would be overkill here since it would re-write every map entry, while in this case we only want to write storage entries that were in an invalid state. Additionally it seems non-idiomatic since it looks like it targets use cases where the stored type is being changed

Copy link
Member

Choose a reason for hiding this comment

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

I think for the purposes of fixing this on our networks, it would be better to write a custom list of affected accounts, and directly address those versus iterating through all possible voters, as that kind of iteration migration is not really great for parachains or other runtimes where there is limited execution time.

Copy link
Member

Choose a reason for hiding this comment

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

and for that, we should wait until the original patch is live, then we will know a final list of affected users.

@emostov
Copy link
Contributor Author

emostov commented Jan 13, 2022

@shawntabrizi feel free to merge into your branch if you think it looks ok na

Base automatically changed from shawntabrizi-phragmen-free-balance to master January 14, 2022 01:43
@emostov emostov added C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited E1-runtimemigration labels Jan 14, 2022
@emostov emostov changed the title Migration for #10646 Migration for over locked accounts in phgragmen elections Jan 14, 2022
@emostov
Copy link
Contributor Author

emostov commented Jan 18, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7e87c08 into master Jan 18, 2022
@paritytech-processbot paritytech-processbot bot deleted the zeke-shawntabrizi-phragmen-free-balance branch January 18, 2022 03:06
@kianenigma
Copy link
Contributor

@emostov what is the plan here? I thought we are not going to merge this?

@emostov
Copy link
Contributor Author

emostov commented Jan 23, 2022

@kianenigma we merged this, but are waiting to apply it on polkadot/kusama until the fix is enacted. I'll reopen this once ready paritytech/polkadot#4710

@emostov
Copy link
Contributor Author

emostov commented Jan 23, 2022

Ah I see this made its way into the polkadot release notes - will try and take it out

@emostov emostov added B0-silent Changes should not be mentioned in any release notes and removed B7-runtimenoteworthy labels Jan 23, 2022
@viniul viniul added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 25, 2022
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…#10649)

* use free balance rather than total balance

* Docs

* Migration for over-locked phrag voters

* New line

* comment

* Update frame/elections-phragmen/src/migrations/v5.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Only set lock, don't remove it

* delete commented out

* docs

* Update migration to just take a set of accounts

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…#10649)

* use free balance rather than total balance

* Docs

* Migration for over-locked phrag voters

* New line

* comment

* Update frame/elections-phragmen/src/migrations/v5.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Only set lock, don't remove it

* delete commented out

* docs

* Update migration to just take a set of accounts

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants