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

migration(elections-phragmen): unreserve deposits and clear locks #14218

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented May 24, 2023

Partial paritytech/polkadot-sdk#485

Overview

Adds a migration that unreserves all deposits and unlocks all stake held by the elections-phragmen pallet.

I've verified this works using try-runtime-cli and the latest Polkadot state (testing with Kusama state blocked by my last Q below).

Details

elections-phragmen contains 4 storage items containing information about balances to unreserve and unlock: Members, RunnerUps, Candidates, and Voters.

Every storage item stores a deposit to unreserve, and just Voters contains a stake to unlock.

pre_upgrade collects pre-migration data useful for validating the migration was successful, and also checks the integrity of deposited and reserved balances.

on_runtime_upgrade unreserves and unlocks the balances.

post_upgrade checks that all expected locks were removed, and reserved balances were reduced by the expected amount.

try-runtime-cli on-runtime-upgrade output

2023-06-13 16:51:34.375  INFO main runtime::democracy::migrations::unlock_and_unreserve_all_funds: Total accounts: 5364
2023-06-13 16:51:34.376  INFO main runtime::democracy::migrations::unlock_and_unreserve_all_funds: Total stake to unlock: 1179551847104532862
2023-06-13 16:51:34.376  INFO main runtime::democracy::migrations::unlock_and_unreserve_all_funds: Total deposit to unreserve: 25000000000000
2023-06-13 16:51:34.376  INFO main runtime::democracy::migrations::unlock_and_unreserve_all_funds: Bugged deposits: 1/21

Questions for reviewers / todos:

  • This currently assumes that the election_phragmen pallet is part of the runtime. We need to run it for Kusama where it has already been removed from the runtime. What's the cleanest way to go about this?
  • How do I calculate the weight of the remove_lock and unreserve calls?
  • remove_lock triggers some ERROR main runtime::system: Logic error: Unexpected underflow in reducing consumer error logs. I have verified that a non-zero lock always exists before remove_lock is called, even when it results in this log. Are they safe to ignore?

@liamaharon liamaharon added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 24, 2023
@liamaharon liamaharon requested review from a team May 24, 2023 14:14
@liamaharon liamaharon changed the title migration (elections-phragmen): unreserve all deposit and unlock all stake migration(elections-phragmen): unreserve all deposit and unlock all stake May 24, 2023
@liamaharon liamaharon added the B1-note_worthy Changes should be noted in the release notes label May 24, 2023
@liamaharon liamaharon changed the title migration(elections-phragmen): unreserve all deposit and unlock all stake migration(elections-phragmen): unreserve all deposits and unlock all stake May 24, 2023
@liamaharon liamaharon added the T1-runtime This PR/Issue is related to the topic “runtime”. label May 24, 2023
@liamaharon liamaharon changed the title migration(elections-phragmen): unreserve all deposits and unlock all stake migration(elections-phragmen): unreserve deposits and unlock stake May 25, 2023
@liamaharon liamaharon changed the title migration(elections-phragmen): unreserve deposits and unlock stake migration(elections-phragmen): unreserve deposits and clear locks May 25, 2023
@liamaharon
Copy link
Contributor Author

I just found a way to handle the Balances types cleaner. Switching this to a draft while I improve that, also going to add some unit tests.

@liamaharon liamaharon marked this pull request as draft May 26, 2023 11:32
@liamaharon liamaharon 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 May 26, 2023
…ections-phragmen-unlock-and-unreserve-funds-migration
.len()
.saturating_add(runner_ups.len())
.saturating_add(candidates.len())
.saturating_add(voters_len.saturating_mul(T::MaxVotesPerVoter::get() as usize))
Copy link
Contributor

@kianenigma kianenigma Jun 13, 2023

Choose a reason for hiding this comment

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

Not sure why you multiply T::MaxVotesPerVoter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each Voter contains a Vec called votes up to size MaxVotesPerVoter.

I guessed that the weight of the read scales with the size of the Vec, but maybe that's incorrect if it can all be loaded into memory in one read regardless?


// Get the total amount deposited (Members, RunnerUps, Candidates and Voters all can have
// deposits).
let account_deposited_sums: BTreeMap<T::AccountId, BalanceOf<T>> = members
Copy link
Contributor

Choose a reason for hiding this comment

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

to be pedantic about the variable name, we are voting here, not staking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's referred to as a stake in the pallet:

//! update the vote's stake ([`Voter::stake`]). After a round, votes are kept and might still be

/// The amount of stake placed on this vote.
pub stake: Balance,

Still happy to change it to locked though if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge this for now in the interests of trying to get the migrations ready for 1.0, but let me know if you'd still like me to change it and I will open another pr :)

liamaharon and others added 10 commits June 13, 2023 23:17
…gration' and 'liam-elections-phragmen-unlock-and-unreserve-funds-migration' of github.com:paritytech/substrate into liam-elections-phragmen-unlock-and-unreserve-funds-migration
…ections-phragmen-unlock-and-unreserve-funds-migration
@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Github API says #14218 is not mergeable

@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 319a7bd into master Jun 13, 2023
@paritytech-processbot paritytech-processbot bot deleted the liam-elections-phragmen-unlock-and-unreserve-funds-migration branch June 13, 2023 16:07
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ritytech#14218)

* pre_upgrade hook wip

* working pre_upgrade

* simplify code

* cleanup and document

* return reads from get_account_deposited_and_staked_sums

* improve comment

* on_runtime_upgrade comment

* post upgrade comment

* use saturating_add

* clippy

* clean up balances

* add tests

* fix comment

* oops

* actually fix comment

* fix std build

* address pr comments

* remove redundant comment

* update comment

* add comment

* oliver comments from tips pallet pr

* lint

* remove need for do_pre/do_post runtime functions

* generic dbweight

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

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

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

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

* pr comments

* remove useless check

* feature gate log target

* lint

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

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

* add log for unexpected amounts

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants