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

Conversation

@kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Jan 6, 2022

Small part of #9864, one little step at a time at making pallet-staking "bounded". Also needed and extracted out of my branch for paritytech/polkadot-sdk#461.

Please note carefully the fact that this might, in the VERY rare case that we reduce MaxNomination on a live network, lead to corrupt state being created. Nonetheless, this non-decodable state is pretty harmless, and can be cleaned via the already existing chill_other call.

All in all though, this PR has ZERO logical changes. It is all a matter of abstraction.

Also, note that the scenario that we reduce MAX_NOMINATIONS can also happen now, and we actually don't handle it. So if for any reason we don't merge this, we need to fix this case separately.

Polkadot companion: paritytech/polkadot#4709

@kianenigma kianenigma added 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. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 6, 2022
@kianenigma kianenigma requested a review from andresilva as a code owner January 6, 2022 12:30

fn check_count() {
let nominator_count = Nominators::<Test>::iter().count() as u32;
let nominator_count = Nominators::<Test>::iter_keys().count() as u32;
Copy link
Contributor Author

@kianenigma kianenigma Jan 6, 2022

Choose a reason for hiding this comment

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

the reason for this is subtle and rather tricky. See if it is clear to you as a reviewer.

Copy link
Member

Choose a reason for hiding this comment

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

Because we can fail to decode?

Copy link
Contributor Author

@kianenigma kianenigma Jan 19, 2022

Choose a reason for hiding this comment

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

yes we might fail to decode the values (in the rare case that we suddenly reduce MaxNominators to e.g. 4), so iter().count() would return only the number of decodable ones, while iter_keys().count() is all of the nominators.

@kianenigma kianenigma requested review from emostov and gui1117 and removed request for andresilva January 6, 2022 12:45
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Generally looks good but my main concern is the changes to chill_other

See

@kianenigma kianenigma 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 7, 2022
@shawntabrizi
Copy link
Member

All in all though, this PR has ZERO logical changes. It is all a matter of abstraction.

There is a change in behavior to chill.

@kianenigma kianenigma added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Jan 20, 2022
@jakoblell jakoblell 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 24, 2022
@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Checks failed for 7fe85d2

@kianenigma
Copy link
Contributor Author

bot merge

1 similar comment
@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 9e9e4d3 into master Jan 25, 2022
@paritytech-processbot paritytech-processbot bot deleted the kiz-bound-nominator-votes branch January 25, 2022 14:44
@paritytech-processbot
Copy link

Error: Github API says #10601 is not mergeable

eskimor pushed a commit that referenced this pull request Jan 27, 2022
* Use proper bounded vector type for nominations

* add docs and tweak chill_other for cleanup purposes

* Fix the build

* remove TODO

* add a bit more doc

* even more docs
gushc

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

Co-authored-by: Zeke Mostov <[email protected]>

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

Co-authored-by: Zeke Mostov <[email protected]>

* Fix the nasty bug

* also bound the Snapshot type

* fix doc test

* document bounded_vec

* self-review

* remove unused

* Fix build

* frame-support: repetition overload for bounded_vec

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix

* remove the need to allocate into unbounded voters etc etc

* Don't expect

* unbreal the build again

* handle macro a bit better

Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Wizdave97 pushed a commit to ComposableFi/substrate that referenced this pull request Feb 4, 2022
* Use proper bounded vector type for nominations

* add docs and tweak chill_other for cleanup purposes

* Fix the build

* remove TODO

* add a bit more doc

* even more docs
gushc

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

Co-authored-by: Zeke Mostov <[email protected]>

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

Co-authored-by: Zeke Mostov <[email protected]>

* Fix the nasty bug

* also bound the Snapshot type

* fix doc test

* document bounded_vec

* self-review

* remove unused

* Fix build

* frame-support: repetition overload for bounded_vec

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix

* remove the need to allocate into unbounded voters etc etc

* Don't expect

* unbreal the build again

* handle macro a bit better

Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Use proper bounded vector type for nominations

* add docs and tweak chill_other for cleanup purposes

* Fix the build

* remove TODO

* add a bit more doc

* even more docs
gushc

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

Co-authored-by: Zeke Mostov <[email protected]>

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

Co-authored-by: Zeke Mostov <[email protected]>

* Fix the nasty bug

* also bound the Snapshot type

* fix doc test

* document bounded_vec

* self-review

* remove unused

* Fix build

* frame-support: repetition overload for bounded_vec

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix

* remove the need to allocate into unbounded voters etc etc

* Don't expect

* unbreal the build again

* handle macro a bit better

Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Use proper bounded vector type for nominations

* add docs and tweak chill_other for cleanup purposes

* Fix the build

* remove TODO

* add a bit more doc

* even more docs
gushc

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

Co-authored-by: Zeke Mostov <[email protected]>

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

Co-authored-by: Zeke Mostov <[email protected]>

* Fix the nasty bug

* also bound the Snapshot type

* fix doc test

* document bounded_vec

* self-review

* remove unused

* Fix build

* frame-support: repetition overload for bounded_vec

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix

* remove the need to allocate into unbounded voters etc etc

* Don't expect

* unbreal the build again

* handle macro a bit better

Co-authored-by: Zeke Mostov <[email protected]>
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

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.

6 participants