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

[Feature] Part 1: add TargetList for validator ranking #12034

Merged
merged 43 commits into from
Sep 18, 2022
Merged

[Feature] Part 1: add TargetList for validator ranking #12034

merged 43 commits into from
Sep 18, 2022

Conversation

ruseinov
Copy link
Contributor

@ruseinov ruseinov commented Aug 15, 2022 •
edited
Loading

This is the first of a series of PRs aimed at reviving #11013

@kianenigma some questions:

  1. Since we've introduced 2 instances of bagslist effectively overriding VoterList with an Instance1 - does that mean we have to migrate storage to make sure the list contains what it needs to contain?
  2. Gotta figure out where bag thresholds are being generated and make sure we generate balances? Or just reuse the same thresholds
  3. Why reverse?

This seems to introduce all the moving parts without actually touching any of the current business-logic.

polkadot companion: paritytech/polkadot#5930

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Aug 15, 2022
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1744862

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1744978

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1745014

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1746014

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1746316

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1746315

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1746433

@kianenigma
Copy link
Contributor

Since we've introduced 2 instances of bagslist effectively overriding VoterList with an Instance1 - does that mean we have to migrate storage to make sure the list contains what it needs to contain?

We sure do, but let's leave that for the end of the work -- for now don't bother with it.

Gotta figure out where bag thresholds are being generated and make sure we generate balances? Or just reuse the same thresholds

utils/frame/generate-bags

Why reverse?

This Compat type is there only to make the tests pass. If you remove it and just use type TargetList = TargetBagsList, you will see that a gzillion tests will fail. You can and should double check but I think this is only because the order of targets that are returned differs.

kianenigma
kianenigma reviewed Aug 16, 2022
View reviewed changes
frame/staking/src/lib.rs Outdated
@@ -685,6 +685,16 @@ pub struct Nominations<T: Config> {
pub suppressed: bool,
}

/// An unbounded version of `Nominations`, use for some really wacky hacks.
#[derive(PartialEqNoBound, EqNoBound, Clone, Encode, Decode, RuntimeDebugNoBound, TypeInfo)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed for this PR.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748008

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748045

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748044

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748082

gilescope
gilescope reviewed Sep 14, 2022
View reviewed changes
frame/staking/src/pallet/impls.rs

let mut targets_iter = T::TargetList::iter();
while all_targets.len() < max_allowed_len &&
targets_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * max_allowed_len as u32)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be worth doing .try_from rather than as and then .unwrap_or(u32::MAX)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea!

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 guess same as below - we will see what's possible in P2, the staking code is full of as, which is not necessarily an excuse, but if this is checked elsewhere - no need to overcomplicate the code just yet.

Thanks so much for the review and those nits, it was helpful for me to go over the codebase again to see if we can indeed assume that u32 never overflows here.

gilescope
gilescope reviewed Sep 14, 2022
View reviewed changes
frame/staking/src/pallet/impls.rs

Self::register_weight(T::WeightInfo::get_npos_targets(validator_count));
Self::register_weight(T::WeightInfo::get_npos_targets(all_targets.len() as u32));
Copy link
Contributor

Choose a reason for hiding this comment

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

again here maybe we want to saturate with a try_into?

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 a good spot, but at the moment it seems unnecessary to complicate things.
We will revisit this upon introduction of the P2.

gilescope
gilescope approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

The staking code is pretty new to me but aside from a couple of nits looks good.

@kianenigma kianenigma added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Sep 18, 2022
@ruseinov
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5930

@ruseinov ruseinov merged commit 03eb807 into master Sep 18, 2022
@ruseinov ruseinov deleted the ru/feature/p1-target-list branch September 18, 2022 09:28
ggwpez
ggwpez reviewed Sep 19, 2022
View reviewed changes
frame/staking/src/migrations.rs
for MigrateToV11<T, P, N>
{
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<(), &'static str> {
Copy link
Member

Choose a reason for hiding this comment

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

Should not compile.

@4meta5 4meta5 mentioned this pull request Oct 17, 2022
Closed
1 task
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Closed
@ruseinov ruseinov mentioned this pull request Nov 16, 2022
Open
@github-actions github-actions bot mentioned this pull request Nov 23, 2022
Closed
@hussein-aitlahcen hussein-aitlahcen mentioned this pull request Jan 16, 2023
Merged
7 tasks
vimukthi-git pushed a commit to ComposableFi/composable that referenced this pull request Jan 18, 2023
@hussein-aitlahcen @cor
[PICA-34] - Upgrade to polkadot v0.9.30 (#2913)
2ec221a 
#### Intro

Upgrade polkadot from v0.9.27 to v0.9.30 as a checkpoint, then to
v0.9.33 (latest version without workspace dependencies)

Require ComposableFi/composable-ibc#176 to be merged
first.

#### Migrations
- v0.9.28
    - [x] paritytech/polkadot#5582
        - Nomination not present in our runtimes
- v0.9.29
    - [x] paritytech/substrate#12095
        - Nomination not present in our runtimes
- v0.9.30
    - [x] paritytech/substrate#12034
        - BagList/Staking not present in our runtimes
    - [x] paritytech/polkadot#5930
        - Nomination/BagList/Staking not present in our runtimes
    - [x] paritytech/substrate#12230
        - Staking not present in our runtimes
    - [x] paritytech/polkadot#5996
        - Staking not present in our runtimes
    - [x] paritytech/substrate#12083
        - Contracts not present in our runtimes

Signed-off-by: cor <cor@pruijs.dev>
Co-authored-by: cor <cor@pruijs.dev>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@ruseinov @kianenigma
@gilescope @ark0f
[Feature] Part 1: add TargetList for validator ranking (paritytech#12034
3b6bdd4 
)

* [Feature] Part 1: add TargetList for validator ranking

* remove redundant todo

* remove typo

* cleanup

* implement score

* more fixes

* fix thresholds

* fmt

* Remove the stuff that has to come in the next PR, some fixes

* extended balance import

* Change all the references from VoteWeight to Self::Score

* Add a migration for VoterBagsList

* fix score

* add targetList to nomination-pools tests

* fix bench

* address review comments

* change get_npos_targets

* address more comments

* remove thresholds for the time being

* fix instance reference

* VoterBagsListInstance

* reus

* remove params that are not used yet

* Introduced pre/post upgrade try-runtime checks

* fix

* fixes

* fix migration

* fix migration

* fix post_upgrade

* change

* Fix

* eloquent PhantomData

* fix PD

* more fixes

* Update frame/staking/src/pallet/impls.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* is_nominator now works

* fix test-staking

* build fixes

* fix remote-tests

* Apply suggestions from code review

Co-authored-by: parity-processbot <>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@vovac12 vovac12 mentioned this pull request Feb 28, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez ggwpez left review comments

@gilescope gilescope gilescope approved these changes

@andresilva andresilva Awaiting requested review from andresilva

@shawntabrizi shawntabrizi Awaiting requested review from shawntabrizi

Assignees
No one assigned
Labels
C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
(Nominated) Proof of Stake
Status: ✅ Done
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ruseinov @paritytech-cicd-pr @kianenigma @gilescope @ggwpez