Bound elections-phragmen pallet #12549

Szegoo · 2022-10-23T14:32:23Z

This PR replaces all the Vec types that are stored inside storage with BoundedVec so that we have an upper limit of items in all of the storage values. Also adapts the extrinsics to match these changes.

A new migration is introduced which will bound all the existing values inside the storage items and upgrade the elections pallet version to V6. The following storage will get updated when applying the storage migration:

Voting
Members
RunnersUp
Candidates

polkadot companion: paritytech/polkadot#6396

Polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

kianenigma

I think we already have a bound for MaxVoters that is a constant in the file. You should remove that, use the new config in the transaction where we set the voters.

You should also document that reducing MaxVoters in a runtime upgrade might need a migration.

Otherwise, looks good in general and should be feasible.

Szegoo · 2022-10-24T12:03:35Z

NOTE: this is still a draft PR, so it may be too early to review :)

bin/node/cli/src/chain_spec.rs

bin/node/runtime/src/lib.rs

kianenigma

Still has a few fundamental flaws, but certainly in the right direction! 🙇

kianenigma · 2022-10-29T09:19:28Z

NOTE: this is still a draft PR, so it may be too early to review :)

Indeed, I am just doing a quick review to make sure you are in the right path. Ping me when you think this is final.

Szegoo · 2022-11-01T20:09:24Z

@gpestana The PR is ready for review :)

command-bot · 2023-02-24T14:27:52Z

Here's a link to docs

kianenigma · 2023-02-24T14:28:11Z

@gpestana @kianenigma Could you please review this?

This should be reviewed after #13453.

gpestana

A few additional comments:

Could you add more info about what the PR does, the breaking changes and its migrations in the PR description? This helps reviewers and users and to keep track of changes pre and post-release.
It would be helpful if the branch build without errors/warnings when asking for a review, or state why it doesn't build if you need help/comments.
As @kianenigma mentioned, we should wait for Abstracts elections-phragmen pallet to use NposSolver (v2) #13453 to be merged and then rebase and apply these changes, since there are substancial changes on the election pallet (e.g. name change, etc). The comments above and my PR comments can be addressed before merging PR#13453 though.

frame/elections-phragmen/src/lib.rs

gpestana · 2023-02-27T10:40:37Z

frame/elections-phragmen/src/lib.rs

-							.collect::<Vec<_>>(),
+							.collect::<Vec<_>>()
+							.try_into()
+							.expect("number members will never exceed T::DesiredMembers. qed."),


I would prefer us to be defensive here, especially because this code runs on on_initialize. Changes in the logic/bounds may change the assumptions and number members will never exceed T::DesiredMembers. qed. may not hold true anymore.

I'd suggest to fail and return earlier here if the collect fails instead of using the expect.

This code is being executed inside a closure so I cannot return the weight from here, at least not without re-organizing the code. I took the approach of taking the first T::DesiredMembers members so that we can be sure there will never be more members.

Is this ok?

gpestana · 2023-02-27T10:40:48Z

frame/elections-phragmen/src/lib.rs

+							.try_into()
+							.expect(
+								"number runners up will never exceed T::DesiredRunnersUp. qed.",
+							),


Same as above.

Szegoo · 2023-02-27T12:26:55Z

Could you add more info about what the PR does, the breaking changes and its migrations in the PR description? This helps reviewers and users and to keep track of changes pre and post-release.

I have added a description now.

It would be helpful if the branch build without errors/warnings when asking for a review, or state why it doesn't build if you need help/comments.

The PR was passing the CI checks when I requested the review. After the comment from @kianenigma I merged master into this branch and the CI is not passing anymore, I will need to do some changes but I planned to do that once #13453 is merged. I will probably make this a draft PR for now.

stale · 2023-04-04T20:10:45Z